From 91949550fe4472e60ee18ddf4eda81ae54923fd0 Mon Sep 17 00:00:00 2001 From: Rahul Sharma Date: Thu, 7 Nov 2024 15:00:34 +0000 Subject: [PATCH 1/3] handle case where instance already exists --- .../linodemachine_controller_helpers.go | 21 ++++++ controller/linodemachine_controller_test.go | 75 +++++++++++++++++++ 2 files changed, 96 insertions(+) diff --git a/controller/linodemachine_controller_helpers.go b/controller/linodemachine_controller_helpers.go index 995400c1..2167f859 100644 --- a/controller/linodemachine_controller_helpers.go +++ b/controller/linodemachine_controller_helpers.go @@ -722,6 +722,27 @@ func createInstance(ctx context.Context, logger logr.Logger, machineScope *scope machineScope.LinodeClient.OnAfterResponse(ctr.ApiResponseRatelimitCounter) inst, err := machineScope.LinodeClient.CreateInstance(ctx, *createOpts) + + // if instance already exists, we get 400 response. get respective instance and return + if linodego.ErrHasStatus(err, http.StatusBadRequest) { + logger.Error(err, "Failed to create instance, received [400 BadRequest] response.") + + // check if instance already exists + listFilter := util.Filter{Label: createOpts.Label} + filter, errFilter := listFilter.String() + if errFilter != nil { + logger.Error(err, "Failed to create filter to list instance") + return nil, ctr.RetryAfter(), err + } + instances, listErr := machineScope.LinodeClient.ListInstances(ctx, linodego.NewListOptions(1, filter)) + if listErr != nil { + return nil, ctr.RetryAfter(), listErr + } + if len(instances) > 0 { + return &instances[0], ctr.RetryAfter(), nil + } + } + return inst, ctr.RetryAfter(), err } diff --git a/controller/linodemachine_controller_test.go b/controller/linodemachine_controller_test.go index 9c99bc31..d986fff5 100644 --- a/controller/linodemachine_controller_test.go +++ b/controller/linodemachine_controller_test.go @@ -1410,6 +1410,81 @@ var _ = Describe("machine-lifecycle", Ordered, Label("machine", "machine-lifecyc Expect(rutil.ConditionTrue(linodeMachine, ConditionPreflightBootTriggered)).To(BeTrue()) Expect(rutil.ConditionTrue(linodeMachine, ConditionPreflightReady)).To(BeTrue()) + Expect(*linodeMachine.Status.InstanceState).To(Equal(linodego.InstanceOffline)) + Expect(*linodeMachine.Spec.ProviderID).To(Equal("linode://123")) + Expect(linodeMachine.Status.Addresses).To(Equal([]clusterv1.MachineAddress{ + {Type: clusterv1.MachineExternalIP, Address: "172.0.0.2"}, + {Type: clusterv1.MachineExternalIP, Address: "fd00::"}, + {Type: clusterv1.MachineInternalIP, Address: "192.168.0.2"}, + })) + })), + Path(Result("uses worker machine without disk if it already exists", func(ctx context.Context, mck Mock) { + linodeMachine = &infrav1alpha2.LinodeMachine{ + ObjectMeta: metadata, + Spec: infrav1alpha2.LinodeMachineSpec{ + Type: "g6-nanode-1", + Image: rutil.DefaultMachineControllerLinodeImage, + Configuration: nil, + }, + Status: infrav1alpha2.LinodeMachineStatus{}, + } + getRegion := mck.LinodeClient.EXPECT(). + GetRegion(ctx, gomock.Any()). + Return(&linodego.Region{Capabilities: []string{"Metadata"}}, nil) + getImage := mck.LinodeClient.EXPECT(). + GetImage(ctx, gomock.Any()). + After(getRegion). + Return(&linodego.Image{Capabilities: []string{"cloud-init"}}, nil) + createInst := mck.LinodeClient.EXPECT(). + CreateInstance(ctx, gomock.Any()). + After(getImage). + Return(nil, &linodego.Error{Code: http.StatusBadRequest}) + listInst := mck.LinodeClient.EXPECT(). + ListInstances(ctx, gomock.Any()). + After(createInst). + Return([]linodego.Instance{{ + ID: 123, + IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))}, + IPv6: "fd00::", + Status: linodego.InstanceOffline, + }}, nil) + mck.LinodeClient.EXPECT(). + OnAfterResponse(gomock.Any()). + Return() + bootInst := mck.LinodeClient.EXPECT(). + BootInstance(ctx, 123, 0). + After(listInst). + Return(nil) + getAddrs := mck.LinodeClient.EXPECT(). + GetInstanceIPAddresses(ctx, 123). + After(bootInst). + Return(&linodego.InstanceIPAddressResponse{ + IPv4: &linodego.InstanceIPv4Response{ + Private: []*linodego.InstanceIP{{Address: "192.168.0.2"}}, + Public: []*linodego.InstanceIP{{Address: "172.0.0.2"}}, + }, + IPv6: &linodego.InstanceIPv6Response{ + SLAAC: &linodego.InstanceIP{ + Address: "fd00::", + }, + }, + }, nil) + mck.LinodeClient.EXPECT(). + ListInstanceConfigs(ctx, 123, gomock.Any()). + After(getAddrs). + Return([]linodego.InstanceConfig{{ + Devices: &linodego.InstanceConfigDeviceMap{ + SDA: &linodego.InstanceConfigDevice{DiskID: 100}, + }, + }}, nil) + _, err := reconciler.reconcile(ctx, mck.Logger(), mScope) + Expect(err).NotTo(HaveOccurred()) + + Expect(rutil.ConditionTrue(linodeMachine, ConditionPreflightCreated)).To(BeTrue()) + Expect(rutil.ConditionTrue(linodeMachine, ConditionPreflightConfigured)).To(BeTrue()) + Expect(rutil.ConditionTrue(linodeMachine, ConditionPreflightBootTriggered)).To(BeTrue()) + Expect(rutil.ConditionTrue(linodeMachine, ConditionPreflightReady)).To(BeTrue()) + Expect(*linodeMachine.Status.InstanceState).To(Equal(linodego.InstanceOffline)) Expect(*linodeMachine.Spec.ProviderID).To(Equal("linode://123")) Expect(linodeMachine.Status.Addresses).To(Equal([]clusterv1.MachineAddress{ From 02e6e67fcabdbbcfe8d917ddc3d85c32289fb585 Mon Sep 17 00:00:00 2001 From: Rahul Sharma Date: Thu, 7 Nov 2024 17:07:06 +0000 Subject: [PATCH 2/3] check if error is due to label --- controller/linodemachine_controller_helpers.go | 3 ++- controller/linodemachine_controller_test.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/controller/linodemachine_controller_helpers.go b/controller/linodemachine_controller_helpers.go index 2167f859..35c25562 100644 --- a/controller/linodemachine_controller_helpers.go +++ b/controller/linodemachine_controller_helpers.go @@ -24,6 +24,7 @@ import ( "net/http" "net/netip" "slices" + "strings" "time" "github.com/go-logr/logr" @@ -724,7 +725,7 @@ func createInstance(ctx context.Context, logger logr.Logger, machineScope *scope inst, err := machineScope.LinodeClient.CreateInstance(ctx, *createOpts) // if instance already exists, we get 400 response. get respective instance and return - if linodego.ErrHasStatus(err, http.StatusBadRequest) { + if linodego.ErrHasStatus(err, http.StatusBadRequest) && strings.Contains(err.Error(), "Label must be unique") { logger.Error(err, "Failed to create instance, received [400 BadRequest] response.") // check if instance already exists diff --git a/controller/linodemachine_controller_test.go b/controller/linodemachine_controller_test.go index d986fff5..025306bb 100644 --- a/controller/linodemachine_controller_test.go +++ b/controller/linodemachine_controller_test.go @@ -1438,7 +1438,7 @@ var _ = Describe("machine-lifecycle", Ordered, Label("machine", "machine-lifecyc createInst := mck.LinodeClient.EXPECT(). CreateInstance(ctx, gomock.Any()). After(getImage). - Return(nil, &linodego.Error{Code: http.StatusBadRequest}) + Return(nil, &linodego.Error{Code: http.StatusBadRequest, Message: "[400] [label] Label must be unique among your linodes"}) listInst := mck.LinodeClient.EXPECT(). ListInstances(ctx, gomock.Any()). After(createInst). From 4944970584b06e238b6b55d7df35705791c22a12 Mon Sep 17 00:00:00 2001 From: Rahul Sharma Date: Fri, 8 Nov 2024 00:06:56 +0000 Subject: [PATCH 3/3] fix test case --- controller/linodemachine_controller_test.go | 161 +++++++++++--------- 1 file changed, 86 insertions(+), 75 deletions(-) diff --git a/controller/linodemachine_controller_test.go b/controller/linodemachine_controller_test.go index 025306bb..7e3a47ec 100644 --- a/controller/linodemachine_controller_test.go +++ b/controller/linodemachine_controller_test.go @@ -498,6 +498,92 @@ var _ = Describe("create", Label("machine", "create"), func() { Expect(testLogs.String()).NotTo(ContainSubstring("Failed to add instance to Node Balancer backend")) }) + It("adopts a worker instance which already exists", func(ctx SpecContext) { + mockLinodeClient := mock.NewMockLinodeClient(mockCtrl) + getRegion := mockLinodeClient.EXPECT(). + GetRegion(ctx, gomock.Any()). + Return(&linodego.Region{Capabilities: []string{linodego.CapabilityMetadata, linodego.CapabilityDiskEncryption}}, nil) + getImage := mockLinodeClient.EXPECT(). + GetImage(ctx, gomock.Any()). + After(getRegion). + Return(&linodego.Image{Capabilities: []string{"cloud-init"}}, nil) + createInst := mockLinodeClient.EXPECT(). + CreateInstance(ctx, gomock.Any()). + After(getImage). + Return(nil, &linodego.Error{Code: http.StatusBadRequest, Message: "[400] [label] Label must be unique among your linodes"}) + listInst := mockLinodeClient.EXPECT(). + ListInstances(ctx, gomock.Any()). + After(createInst). + Return([]linodego.Instance{{ + ID: 123, + IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))}, + IPv6: "fd00::", + Status: linodego.InstanceOffline, + }}, nil) + mockLinodeClient.EXPECT(). + OnAfterResponse(gomock.Any()). + Return() + bootInst := mockLinodeClient.EXPECT(). + BootInstance(ctx, 123, 0). + After(listInst). + Return(nil) + mockLinodeClient.EXPECT(). + GetInstanceIPAddresses(ctx, 123). + After(bootInst). + Return(&linodego.InstanceIPAddressResponse{ + IPv4: &linodego.InstanceIPv4Response{ + Private: []*linodego.InstanceIP{{Address: "192.168.0.2"}}, + Public: []*linodego.InstanceIP{{Address: "172.0.0.2"}}, + }, + IPv6: &linodego.InstanceIPv6Response{ + SLAAC: &linodego.InstanceIP{ + Address: "fd00::", + }, + }, + }, nil) + + mScope := scope.MachineScope{ + Client: k8sClient, + LinodeClient: mockLinodeClient, + Cluster: &cluster, + Machine: &machine, + LinodeCluster: &linodeCluster, + LinodeMachine: &linodeMachine, + } + + patchHelper, err := patch.NewHelper(mScope.LinodeMachine, k8sClient) + Expect(err).NotTo(HaveOccurred()) + mScope.PatchHelper = patchHelper + + _, err = reconciler.reconcileCreate(ctx, logger, &mScope) + Expect(err).NotTo(HaveOccurred()) + _, err = reconciler.reconcileCreate(ctx, logger, &mScope) + Expect(err).NotTo(HaveOccurred()) + + Expect(rutil.ConditionTrue(&linodeMachine, ConditionPreflightMetadataSupportConfigured)).To(BeTrue()) + Expect(rutil.ConditionTrue(&linodeMachine, ConditionPreflightCreated)).To(BeTrue()) + Expect(rutil.ConditionTrue(&linodeMachine, ConditionPreflightConfigured)).To(BeTrue()) + Expect(rutil.ConditionTrue(&linodeMachine, ConditionPreflightBootTriggered)).To(BeTrue()) + Expect(rutil.ConditionTrue(&linodeMachine, ConditionPreflightReady)).To(BeTrue()) + + Expect(*linodeMachine.Status.InstanceState).To(Equal(linodego.InstanceOffline)) + Expect(*linodeMachine.Spec.ProviderID).To(Equal("linode://123")) + Expect(linodeMachine.Status.Addresses).To(Equal([]clusterv1.MachineAddress{ + {Type: clusterv1.MachineExternalIP, Address: "172.0.0.2"}, + {Type: clusterv1.MachineExternalIP, Address: "fd00::"}, + {Type: clusterv1.MachineInternalIP, Address: "192.168.0.2"}, + })) + + Expect(testLogs.String()).To(ContainSubstring("creating machine")) + Expect(testLogs.String()).NotTo(ContainSubstring("Failed to list Linode machine instance")) + Expect(testLogs.String()).NotTo(ContainSubstring("Linode instance already exists")) + Expect(testLogs.String()).NotTo(ContainSubstring("Failed to create Linode machine InstanceCreateOptions")) + Expect(testLogs.String()).NotTo(ContainSubstring("Failed to create Linode machine instance")) + Expect(testLogs.String()).NotTo(ContainSubstring("Failed to boot instance")) + Expect(testLogs.String()).NotTo(ContainSubstring("multiple instances found")) + Expect(testLogs.String()).NotTo(ContainSubstring("Failed to add instance to Node Balancer backend")) + }) + Context("fails when a preflight condition is stale", func() { It("can't create an instance in time", func(ctx SpecContext) { mockLinodeClient := mock.NewMockLinodeClient(mockCtrl) @@ -1410,81 +1496,6 @@ var _ = Describe("machine-lifecycle", Ordered, Label("machine", "machine-lifecyc Expect(rutil.ConditionTrue(linodeMachine, ConditionPreflightBootTriggered)).To(BeTrue()) Expect(rutil.ConditionTrue(linodeMachine, ConditionPreflightReady)).To(BeTrue()) - Expect(*linodeMachine.Status.InstanceState).To(Equal(linodego.InstanceOffline)) - Expect(*linodeMachine.Spec.ProviderID).To(Equal("linode://123")) - Expect(linodeMachine.Status.Addresses).To(Equal([]clusterv1.MachineAddress{ - {Type: clusterv1.MachineExternalIP, Address: "172.0.0.2"}, - {Type: clusterv1.MachineExternalIP, Address: "fd00::"}, - {Type: clusterv1.MachineInternalIP, Address: "192.168.0.2"}, - })) - })), - Path(Result("uses worker machine without disk if it already exists", func(ctx context.Context, mck Mock) { - linodeMachine = &infrav1alpha2.LinodeMachine{ - ObjectMeta: metadata, - Spec: infrav1alpha2.LinodeMachineSpec{ - Type: "g6-nanode-1", - Image: rutil.DefaultMachineControllerLinodeImage, - Configuration: nil, - }, - Status: infrav1alpha2.LinodeMachineStatus{}, - } - getRegion := mck.LinodeClient.EXPECT(). - GetRegion(ctx, gomock.Any()). - Return(&linodego.Region{Capabilities: []string{"Metadata"}}, nil) - getImage := mck.LinodeClient.EXPECT(). - GetImage(ctx, gomock.Any()). - After(getRegion). - Return(&linodego.Image{Capabilities: []string{"cloud-init"}}, nil) - createInst := mck.LinodeClient.EXPECT(). - CreateInstance(ctx, gomock.Any()). - After(getImage). - Return(nil, &linodego.Error{Code: http.StatusBadRequest, Message: "[400] [label] Label must be unique among your linodes"}) - listInst := mck.LinodeClient.EXPECT(). - ListInstances(ctx, gomock.Any()). - After(createInst). - Return([]linodego.Instance{{ - ID: 123, - IPv4: []*net.IP{ptr.To(net.IPv4(192, 168, 0, 2))}, - IPv6: "fd00::", - Status: linodego.InstanceOffline, - }}, nil) - mck.LinodeClient.EXPECT(). - OnAfterResponse(gomock.Any()). - Return() - bootInst := mck.LinodeClient.EXPECT(). - BootInstance(ctx, 123, 0). - After(listInst). - Return(nil) - getAddrs := mck.LinodeClient.EXPECT(). - GetInstanceIPAddresses(ctx, 123). - After(bootInst). - Return(&linodego.InstanceIPAddressResponse{ - IPv4: &linodego.InstanceIPv4Response{ - Private: []*linodego.InstanceIP{{Address: "192.168.0.2"}}, - Public: []*linodego.InstanceIP{{Address: "172.0.0.2"}}, - }, - IPv6: &linodego.InstanceIPv6Response{ - SLAAC: &linodego.InstanceIP{ - Address: "fd00::", - }, - }, - }, nil) - mck.LinodeClient.EXPECT(). - ListInstanceConfigs(ctx, 123, gomock.Any()). - After(getAddrs). - Return([]linodego.InstanceConfig{{ - Devices: &linodego.InstanceConfigDeviceMap{ - SDA: &linodego.InstanceConfigDevice{DiskID: 100}, - }, - }}, nil) - _, err := reconciler.reconcile(ctx, mck.Logger(), mScope) - Expect(err).NotTo(HaveOccurred()) - - Expect(rutil.ConditionTrue(linodeMachine, ConditionPreflightCreated)).To(BeTrue()) - Expect(rutil.ConditionTrue(linodeMachine, ConditionPreflightConfigured)).To(BeTrue()) - Expect(rutil.ConditionTrue(linodeMachine, ConditionPreflightBootTriggered)).To(BeTrue()) - Expect(rutil.ConditionTrue(linodeMachine, ConditionPreflightReady)).To(BeTrue()) - Expect(*linodeMachine.Status.InstanceState).To(Equal(linodego.InstanceOffline)) Expect(*linodeMachine.Spec.ProviderID).To(Equal("linode://123")) Expect(linodeMachine.Status.Addresses).To(Equal([]clusterv1.MachineAddress{