From 1504b02314471fbd9933c7bd3682ec147213ebd9 Mon Sep 17 00:00:00 2001 From: chen hui Date: Thu, 20 Jan 2022 08:39:48 +0000 Subject: [PATCH 1/4] Add tests for issue-290 Signed-off-by: chen hui --- .../infrastructure/byomachine_controller.go | 38 +++++++++---------- .../byomachine_controller_test.go | 27 +++++++++++++ 2 files changed, 45 insertions(+), 20 deletions(-) diff --git a/controllers/infrastructure/byomachine_controller.go b/controllers/infrastructure/byomachine_controller.go index 4cb3429d7..4a5601294 100644 --- a/controllers/infrastructure/byomachine_controller.go +++ b/controllers/infrastructure/byomachine_controller.go @@ -42,7 +42,7 @@ import ( const ( ProviderIDPrefix = "byoh://" - providerIDSuffixLength = 6 + ProviderIDSuffixLength = 6 RequeueForbyohost = 10 * time.Second ) @@ -259,27 +259,25 @@ func (r *ByoMachineReconciler) reconcileNormal(ctx context.Context, machineScope r.Recorder.Eventf(machineScope.ByoMachine, corev1.EventTypeNormal, "ByoHostAttachSucceeded", "Attached ByoHost %s", machineScope.ByoHost.Name) } - if machineScope.ByoMachine.Spec.ProviderID == "" { - logger.Info("Updating Node with ProviderID") - remoteClient, err := r.getRemoteClient(ctx, machineScope.ByoMachine) - if err != nil { - logger.Error(err, "failed to get remote client") - return ctrl.Result{}, err - } - - providerID, err := r.setNodeProviderID(ctx, remoteClient, machineScope.ByoHost) - if err != nil { - logger.Error(err, "failed to set node providerID") - r.Recorder.Eventf(machineScope.ByoMachine, corev1.EventTypeWarning, "SetNodeProviderFailed", "Node %s does not exist", machineScope.ByoHost.Name) - return ctrl.Result{}, err - } + logger.Info("Updating Node with ProviderID") + remoteClient, err := r.getRemoteClient(ctx, machineScope.ByoMachine) + if err != nil { + logger.Error(err, "failed to get remote client") + return ctrl.Result{}, err + } - machineScope.ByoMachine.Spec.ProviderID = providerID - machineScope.ByoMachine.Status.Ready = true - conditions.MarkTrue(machineScope.ByoMachine, infrav1.BYOHostReady) - r.Recorder.Eventf(machineScope.ByoMachine, corev1.EventTypeNormal, "NodeProvisionedSucceeded", "Provisioned Node %s", machineScope.ByoHost.Name) + providerID, err := r.setNodeProviderID(ctx, remoteClient, machineScope.ByoHost) + if err != nil { + logger.Error(err, "failed to set node providerID") + r.Recorder.Eventf(machineScope.ByoMachine, corev1.EventTypeWarning, "SetNodeProviderFailed", "Node %s does not exist", machineScope.ByoHost.Name) + return ctrl.Result{}, err } + machineScope.ByoMachine.Spec.ProviderID = providerID + machineScope.ByoMachine.Status.Ready = true + conditions.MarkTrue(machineScope.ByoMachine, infrav1.BYOHostReady) + r.Recorder.Eventf(machineScope.ByoMachine, corev1.EventTypeNormal, "NodeProvisionedSucceeded", "Provisioned Node %s", machineScope.ByoHost.Name) + return ctrl.Result{}, nil } @@ -375,7 +373,7 @@ func (r *ByoMachineReconciler) setNodeProviderID(ctx context.Context, remoteClie return "", err } - node.Spec.ProviderID = fmt.Sprintf("%s%s/%s", ProviderIDPrefix, host.Name, util.RandomString(providerIDSuffixLength)) + node.Spec.ProviderID = fmt.Sprintf("%s%s/%s", ProviderIDPrefix, host.Name, util.RandomString(ProviderIDSuffixLength)) return node.Spec.ProviderID, helper.Patch(ctx, node) } diff --git a/controllers/infrastructure/byomachine_controller_test.go b/controllers/infrastructure/byomachine_controller_test.go index 348e8f380..94868a530 100644 --- a/controllers/infrastructure/byomachine_controller_test.go +++ b/controllers/infrastructure/byomachine_controller_test.go @@ -18,6 +18,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/kubernetes/scheme" clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1" + "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/annotations" "sigs.k8s.io/cluster-api/util/conditions" "sigs.k8s.io/cluster-api/util/patch" @@ -117,6 +118,32 @@ var _ = Describe("Controllers/ByomachineController", func() { Expect(err).To(MatchError("nodes \"" + byoHost.Name + "\" not found")) }) + Context("When node.Spec.ProviderID is already set", func() { + + BeforeEach(func() { + byoHost = builder.ByoHost(defaultNamespace, "test-node-providerid-host").Build() + Expect(k8sClientUncached.Create(ctx, byoHost)).Should(Succeed()) + }) + + It("should not return error when node.Spec.ProviderID is with correct value", func() { + node := builder.Node(defaultNamespace, byoHost.Name).Build() + node.Spec.ProviderID = fmt.Sprintf("%s%s/%s", controllers.ProviderIDPrefix, byoHost.Name, util.RandomString(controllers.ProviderIDSuffixLength)) + Expect(clientFake.Create(ctx, node)).Should(Succeed()) + WaitForObjectsToBePopulatedInCache(byoHost) + _, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: byoMachineLookupKey}) + Expect(err).ToNot(HaveOccurred()) + }) + + It("should return error when node.Spec.ProviderID is without correct value", func() { + node := builder.Node(defaultNamespace, byoHost.Name).Build() + node.Spec.ProviderID = "invalid_format" + Expect(clientFake.Create(ctx, node)).Should(Succeed()) + WaitForObjectsToBePopulatedInCache(byoHost) + _, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: byoMachineLookupKey}) + Expect(err).To(MatchError("invalid format for node.Spec.ProviderID")) + }) + }) + Context("When BYO Hosts are not available", func() { It("should mark BYOHostReady as False", func() { _, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: byoMachineLookupKey}) From a6bb985313b589da0a81dbc690071cbe7bca5539 Mon Sep 17 00:00:00 2001 From: huchen2021 <85480625+huchen2021@users.noreply.github.com> Date: Mon, 24 Jan 2022 15:45:12 +0800 Subject: [PATCH 2/4] Update controllers/infrastructure/byomachine_controller_test.go Co-authored-by: Anusha Hegde --- controllers/infrastructure/byomachine_controller_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controllers/infrastructure/byomachine_controller_test.go b/controllers/infrastructure/byomachine_controller_test.go index 94868a530..dc18441d0 100644 --- a/controllers/infrastructure/byomachine_controller_test.go +++ b/controllers/infrastructure/byomachine_controller_test.go @@ -134,7 +134,7 @@ var _ = Describe("Controllers/ByomachineController", func() { Expect(err).ToNot(HaveOccurred()) }) - It("should return error when node.Spec.ProviderID is without correct value", func() { + It("should return error when node.Spec.ProviderID has stale value", func() { node := builder.Node(defaultNamespace, byoHost.Name).Build() node.Spec.ProviderID = "invalid_format" Expect(clientFake.Create(ctx, node)).Should(Succeed()) From 3a74da94716ddcc41b88c01260890875d245f6a2 Mon Sep 17 00:00:00 2001 From: chen hui Date: Mon, 24 Jan 2022 09:15:11 +0000 Subject: [PATCH 3/4] comment from anshua Signed-off-by: chen hui --- .../byomachine_controller_test.go | 17 +++++++++++------ test/builder/builders.go | 15 ++++++++++++--- 2 files changed, 23 insertions(+), 9 deletions(-) diff --git a/controllers/infrastructure/byomachine_controller_test.go b/controllers/infrastructure/byomachine_controller_test.go index dc18441d0..d73c77721 100644 --- a/controllers/infrastructure/byomachine_controller_test.go +++ b/controllers/infrastructure/byomachine_controller_test.go @@ -123,22 +123,27 @@ var _ = Describe("Controllers/ByomachineController", func() { BeforeEach(func() { byoHost = builder.ByoHost(defaultNamespace, "test-node-providerid-host").Build() Expect(k8sClientUncached.Create(ctx, byoHost)).Should(Succeed()) + WaitForObjectsToBePopulatedInCache(byoHost) + }) + + AfterEach(func() { + Expect(k8sClientUncached.Delete(ctx, byoHost)).ToNot(HaveOccurred()) }) It("should not return error when node.Spec.ProviderID is with correct value", func() { - node := builder.Node(defaultNamespace, byoHost.Name).Build() - node.Spec.ProviderID = fmt.Sprintf("%s%s/%s", controllers.ProviderIDPrefix, byoHost.Name, util.RandomString(controllers.ProviderIDSuffixLength)) + node := builder.Node(defaultNamespace, byoHost.Name). + WithProviderID(fmt.Sprintf("%s%s/%s", controllers.ProviderIDPrefix, byoHost.Name, util.RandomString(controllers.ProviderIDSuffixLength))). + Build() Expect(clientFake.Create(ctx, node)).Should(Succeed()) - WaitForObjectsToBePopulatedInCache(byoHost) _, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: byoMachineLookupKey}) Expect(err).ToNot(HaveOccurred()) }) It("should return error when node.Spec.ProviderID has stale value", func() { - node := builder.Node(defaultNamespace, byoHost.Name).Build() - node.Spec.ProviderID = "invalid_format" + node := builder.Node(defaultNamespace, byoHost.Name). + WithProviderID(fmt.Sprintf("%sanother-host/%s", controllers.ProviderIDPrefix, util.RandomString(controllers.ProviderIDSuffixLength))). + Build() Expect(clientFake.Create(ctx, node)).Should(Succeed()) - WaitForObjectsToBePopulatedInCache(byoHost) _, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: byoMachineLookupKey}) Expect(err).To(MatchError("invalid format for node.Spec.ProviderID")) }) diff --git a/test/builder/builders.go b/test/builder/builders.go index b5c55dc2b..2910c267a 100644 --- a/test/builder/builders.go +++ b/test/builder/builders.go @@ -332,8 +332,9 @@ func (s *SecretBuilder) Build() *corev1.Secret { // NodeBuilder holds the variables and objects required to build a corev1.Node type NodeBuilder struct { - namespace string - name string + namespace string + name string + providerID string } // Node returns a NodeBuilder with the given name and namespace @@ -344,6 +345,12 @@ func Node(namespace, name string) *NodeBuilder { } } +// WithProviderID adds the passed providerID to the ByoHostBuilder +func (n *NodeBuilder) WithProviderID(providerID string) *NodeBuilder { + n.providerID = providerID + return n +} + // Build returns a Node with the attributes added to the NodeBuilder func (n *NodeBuilder) Build() *corev1.Node { node := &corev1.Node{ @@ -355,7 +362,9 @@ func (n *NodeBuilder) Build() *corev1.Node { Name: n.name, Namespace: n.namespace, }, - Spec: corev1.NodeSpec{}, + Spec: corev1.NodeSpec{ + ProviderID: n.providerID, + }, Status: corev1.NodeStatus{}, } From 4dbc10211659646046cc1d82f38d4d544559ddbc Mon Sep 17 00:00:00 2001 From: huchen2021 <85480625+huchen2021@users.noreply.github.com> Date: Tue, 25 Jan 2022 13:37:28 +0800 Subject: [PATCH 4/4] Update test/builder/builders.go Co-authored-by: Anusha Hegde --- test/builder/builders.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/builder/builders.go b/test/builder/builders.go index 2910c267a..a92b521a2 100644 --- a/test/builder/builders.go +++ b/test/builder/builders.go @@ -345,7 +345,7 @@ func Node(namespace, name string) *NodeBuilder { } } -// WithProviderID adds the passed providerID to the ByoHostBuilder +// WithProviderID adds the passed providerID to the NodeBuilder func (n *NodeBuilder) WithProviderID(providerID string) *NodeBuilder { n.providerID = providerID return n