From a40918b6f6374f540b5d999afc91f0c822f9d46d Mon Sep 17 00:00:00 2001 From: Karthik Bhat Date: Tue, 5 Nov 2024 20:40:40 +0530 Subject: [PATCH] Modify provider id set logic to explicity return error --- cloud/scope/powervs_machine.go | 38 +++++++++++++------ controllers/ibmpowervsmachine_controller.go | 4 +- .../ibmpowervsmachine_controller_test.go | 21 ++++++++-- 3 files changed, 48 insertions(+), 15 deletions(-) diff --git a/cloud/scope/powervs_machine.go b/cloud/scope/powervs_machine.go index fc96bfd89..1c135d199 100644 --- a/cloud/scope/powervs_machine.go +++ b/cloud/scope/powervs_machine.go @@ -929,29 +929,45 @@ func (m *PowerVSMachineScope) GetZone() string { } // GetServiceInstanceID returns the service instance id. -func (m *PowerVSMachineScope) GetServiceInstanceID() string { +func (m *PowerVSMachineScope) GetServiceInstanceID() (string, error) { if m.IBMPowerVSCluster.Status.ServiceInstance != nil && m.IBMPowerVSCluster.Status.ServiceInstance.ID != nil { - return *m.IBMPowerVSCluster.Status.ServiceInstance.ID + return *m.IBMPowerVSCluster.Status.ServiceInstance.ID, nil } if m.IBMPowerVSCluster.Spec.ServiceInstanceID != "" { - return m.IBMPowerVSCluster.Spec.ServiceInstanceID + return m.IBMPowerVSCluster.Spec.ServiceInstanceID, nil } if m.IBMPowerVSCluster.Spec.ServiceInstance != nil && m.IBMPowerVSCluster.Spec.ServiceInstance.ID != nil { - return *m.IBMPowerVSCluster.Spec.ServiceInstance.ID + return *m.IBMPowerVSCluster.Spec.ServiceInstance.ID, nil } - return "" + // If we are not able to find service instance id, derive it from name if defined. + if m.IBMPowerVSCluster.Spec.ServiceInstance != nil && m.IBMPowerVSCluster.Spec.ServiceInstance.Name == nil { + return "", fmt.Errorf("failed to find service instance id as both name and id are not set") + } + serviceInstance, err := m.ResourceClient.GetServiceInstance("", *m.IBMPowerVSCluster.Spec.ServiceInstance.Name, ptr.To(m.GetZone())) + if err != nil { + m.Error(err, "failed to get Power VS service instance id", "serviceInstanceName", *m.IBMPowerVSCluster.Spec.ServiceInstance.Name) + return "", err + } + // It's safe to directly dereference GUID as its already done in NewPowerVSMachineScope + return *serviceInstance.GUID, nil } // SetProviderID will set the provider id for the machine. -func (m *PowerVSMachineScope) SetProviderID(id *string) { +func (m *PowerVSMachineScope) SetProviderID(instanceID string) error { // Based on the ProviderIDFormat version the providerID format will be decided. - if options.ProviderIDFormatType(options.ProviderIDFormat) == options.ProviderIDFormatV2 { - if id != nil { - m.IBMPowerVSMachine.Spec.ProviderID = ptr.To(fmt.Sprintf("ibmpowervs://%s/%s/%s/%s", m.GetRegion(), m.GetZone(), m.GetServiceInstanceID(), *id)) - } - } else { + if options.ProviderIDFormatType(options.ProviderIDFormat) == options.ProviderIDFormatV1 { m.IBMPowerVSMachine.Spec.ProviderID = ptr.To(fmt.Sprintf("ibmpowervs://%s/%s", m.Machine.Spec.ClusterName, m.IBMPowerVSMachine.Name)) + return nil + } + m.V(3).Info("setting provider id in v2 format") + + serviceInstanceID, err := m.GetServiceInstanceID() + if err != nil { + m.Error(err, "failed to get service instance ID") + return err } + m.IBMPowerVSMachine.Spec.ProviderID = ptr.To(fmt.Sprintf("ibmpowervs://%s/%s/%s/%s", m.GetRegion(), m.GetZone(), serviceInstanceID, instanceID)) + return nil } // GetMachineInternalIP returns the machine's internal IP. diff --git a/controllers/ibmpowervsmachine_controller.go b/controllers/ibmpowervsmachine_controller.go index 69ff1e4f0..81d5059af 100644 --- a/controllers/ibmpowervsmachine_controller.go +++ b/controllers/ibmpowervsmachine_controller.go @@ -248,7 +248,9 @@ func (r *IBMPowerVSMachineReconciler) reconcileNormal(machineScope *scope.PowerV if err != nil { return ctrl.Result{}, err } - machineScope.SetProviderID(instance.PvmInstanceID) + if err := machineScope.SetProviderID(*ins.PvmInstanceID); err != nil { + return ctrl.Result{}, errors.Wrapf(err, "failed to set provider id") + } machineScope.SetInstanceID(instance.PvmInstanceID) machineScope.SetAddresses(instance) machineScope.SetHealth(instance.Health) diff --git a/controllers/ibmpowervsmachine_controller_test.go b/controllers/ibmpowervsmachine_controller_test.go index 84cb2fc70..d6d999943 100644 --- a/controllers/ibmpowervsmachine_controller_test.go +++ b/controllers/ibmpowervsmachine_controller_test.go @@ -512,6 +512,9 @@ func TestIBMPowerVSMachineReconciler_ReconcileOperations(t *testing.T) { }, }, Spec: infrav1beta2.IBMPowerVSClusterSpec{ + ServiceInstance: &infrav1beta2.IBMPowerVSResourceReference{ + ID: ptr.To("serviceInstanceID"), + }, VPC: &infrav1beta2.VPCResourceReference{ Region: ptr.To("us-south"), }, @@ -606,6 +609,9 @@ func TestIBMPowerVSMachineReconciler_ReconcileOperations(t *testing.T) { }, }, Spec: infrav1beta2.IBMPowerVSClusterSpec{ + ServiceInstance: &infrav1beta2.IBMPowerVSResourceReference{ + ID: ptr.To("serviceInstanceID"), + }, VPC: &infrav1beta2.VPCResourceReference{ Region: ptr.To("us-south"), }, @@ -708,9 +714,15 @@ func TestIBMPowerVSMachineReconciler_ReconcileOperations(t *testing.T) { Ready: true, }, }, - IBMPowerVSClient: mockpowervs, - DHCPIPCacheStore: cache.NewTTLStore(powervs.CacheKeyFunc, powervs.CacheTTL), - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{}, + IBMPowerVSClient: mockpowervs, + DHCPIPCacheStore: cache.NewTTLStore(powervs.CacheKeyFunc, powervs.CacheTTL), + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{ + ServiceInstance: &infrav1beta2.IBMPowerVSResourceReference{ + ID: ptr.To("serviceInstanceID"), + }, + }, + }, } instanceReferences := &models.PVMInstances{ @@ -822,6 +834,9 @@ func TestIBMPowerVSMachineReconciler_ReconcileOperations(t *testing.T) { }, }, Spec: infrav1beta2.IBMPowerVSClusterSpec{ + ServiceInstance: &infrav1beta2.IBMPowerVSResourceReference{ + ID: ptr.To("serviceInstanceID"), + }, LoadBalancers: []infrav1beta2.VPCLoadBalancerSpec{ { Name: "capi-test-lb",