Don't confict with nodeRunningUpdate same node (#773) (#775)

When "parallel" option is false if the NNCP status field nodeRunningUpdate is not empty it means that a node is progressing so Reconcile return a conflict, in the case of handler restarting before "unclaiming" the NNCP will be reconcile again and a conflict will be return. This change fix that by not returning conflict if the handler restarting is the one that has claim the NNCP. Signed-off-by: Quique Llorente <[email protected]> Co-authored-by: Quique Llorente <[email protected]> Co-authored-by: kubevirt-bot <[email protected]>
nmstate · Jun 14, 2021 · abd8f6f · abd8f6f
1 parent 6f1834e
commit abd8f6f
Show file tree

Hide file tree

Showing 2 changed files with 71 additions and 3 deletions.
diff --git a/controllers/nodenetworkconfigurationpolicy_controller.go b/controllers/nodenetworkconfigurationpolicy_controller.go
@@ -20,10 +20,11 @@ package controllers
 import (
 	"context"
 	"fmt"
-	"k8s.io/apimachinery/pkg/runtime/schema"
 	"reflect"
 	"time"
 
+	"k8s.io/apimachinery/pkg/runtime/schema"
+
 	"github.com/go-logr/logr"
 	"github.com/pkg/errors"
 	"k8s.io/client-go/util/retry"
@@ -171,7 +172,7 @@ func (r *NodeNetworkConfigurationPolicyReconciler) claimNodeRunningUpdate(policy
 	if err != nil {
 		return err
 	}
-	if policy.Status.NodeRunningUpdate != "" {
+	if policy.Status.NodeRunningUpdate != "" && policy.Status.NodeRunningUpdate != nodeName {
 		return apierrors.NewConflict(schema.GroupResource{Resource: "nodenetworkconfigurationpolicies"}, policy.Name, fmt.Errorf("Another node is working on configuration"))
 	}
 	policy.Status.NodeRunningUpdate = nodeName

diff --git a/controllers/nodenetworkconfigurationpolicy_controller_test.go b/controllers/nodenetworkconfigurationpolicy_controller_test.go
@@ -1,17 +1,26 @@
 package controllers
 
 import (
+	"context"
+
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/ginkgo/extensions/table"
 	. "github.com/onsi/gomega"
 
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/client-go/kubernetes/scheme"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 
+	"github.com/nmstate/kubernetes-nmstate/api/shared"
 	nmstatev1beta1 "github.com/nmstate/kubernetes-nmstate/api/v1beta1"
 )
 
-var _ = Describe("NodeNetworkConfigurationPolicy controller predicates", func() {
+var _ = Describe("NodeNetworkConfigurationPolicy controller", func() {
 	type predicateCase struct {
 		GenerationOld   int64
 		GenerationNew   int64
@@ -62,4 +71,62 @@ var _ = Describe("NodeNetworkConfigurationPolicy controller predicates", func()
 				ReconcileUpdate: true,
 			}),
 	)
+	type claimNodeRunningUpdateCase struct {
+		currentNodeRunningUpdate  string
+		expectedNodeRunningUpdate string
+		shouldConflict            bool
+	}
+	DescribeTable("when claimNodeRunningUpdate is called and",
+		func(c claimNodeRunningUpdateCase) {
+			reconciler := NodeNetworkConfigurationPolicyReconciler{}
+			s := scheme.Scheme
+			s.AddKnownTypes(nmstatev1beta1.GroupVersion,
+				&nmstatev1beta1.NodeNetworkConfigurationPolicy{},
+			)
+
+			nncp := nmstatev1beta1.NodeNetworkConfigurationPolicy{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "test",
+				},
+				Status: shared.NodeNetworkConfigurationPolicyStatus{
+					NodeRunningUpdate: c.currentNodeRunningUpdate,
+				},
+			}
+
+			objs := []runtime.Object{&nncp}
+
+			// Create a fake client to mock API calls.
+			cl := fake.NewFakeClientWithScheme(s, objs...)
+
+			reconciler.Client = cl
+			reconciler.Log = ctrl.Log.WithName("controllers").WithName("NodeNetworkConfigurationPolicy")
+
+			err := reconciler.claimNodeRunningUpdate(&nncp)
+			if c.shouldConflict {
+				Expect(err).Should(WithTransform(apierrors.IsConflict, BeTrue()), "should conflict")
+			} else {
+				Expect(err).ToNot(HaveOccurred())
+			}
+			obtainedNNCP := nmstatev1beta1.NodeNetworkConfigurationPolicy{}
+			cl.Get(context.TODO(), types.NamespacedName{Name: nncp.Name}, &obtainedNNCP)
+			Expect(obtainedNNCP.Status.NodeRunningUpdate).To(Equal(c.expectedNodeRunningUpdate))
+		},
+		Entry("there is no node configuring network, should update nodeRunnigUpdate field",
+			claimNodeRunningUpdateCase{
+				expectedNodeRunningUpdate: nodeName,
+				shouldConflict:            false,
+			}),
+		Entry("there is different node configuring network, should fail with conflict",
+			claimNodeRunningUpdateCase{
+				currentNodeRunningUpdate:  nodeName + "foo",
+				expectedNodeRunningUpdate: nodeName + "foo",
+				shouldConflict:            true,
+			}),
+		Entry("the node running the handler is configuring the network, should not conflict",
+			claimNodeRunningUpdateCase{
+				currentNodeRunningUpdate:  nodeName,
+				expectedNodeRunningUpdate: nodeName,
+				shouldConflict:            false,
+			}),
+	)
 })