diff --git a/cloud/scope/powervs_machine.go b/cloud/scope/powervs_machine.go index e119fe976..689f2374c 100644 --- a/cloud/scope/powervs_machine.go +++ b/cloud/scope/powervs_machine.go @@ -22,7 +22,6 @@ import ( "fmt" "strconv" "strings" - "sync" "github.com/IBM-Cloud/power-go-client/ibmpisession" "github.com/IBM-Cloud/power-go-client/power/client/p_cloud_p_vm_instances" @@ -52,9 +51,6 @@ import ( "sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/record" ) -// used to initialise the dhcpCacheStore. -var dhcpIPCacheOnce sync.Once - // PowerVSMachineScopeParams defines the input parameters used to create a new PowerVSMachineScope. type PowerVSMachineScopeParams struct { Logger logr.Logger @@ -65,6 +61,7 @@ type PowerVSMachineScopeParams struct { IBMPowerVSMachine *infrav1beta1.IBMPowerVSMachine IBMPowerVSImage *infrav1beta1.IBMPowerVSImage ServiceEndpoint []endpoints.ServiceEndpoint + DHCPIPCacheStore cache.Store } // PowerVSMachineScope defines a scope defined around a Power VS Machine. @@ -191,10 +188,7 @@ func NewPowerVSMachineScope(params PowerVSMachineScopeParams) (scope *PowerVSMac return } scope.IBMPowerVSClient = c - - dhcpIPCacheOnce.Do(func() { - scope.DHCPIPCacheStore = cache.NewTTLStore(options.CacheKeyFunc, options.CacheTTL) - }) + scope.DHCPIPCacheStore = params.DHCPIPCacheStore return scope, nil } @@ -455,10 +449,10 @@ func (m *PowerVSMachineScope) SetAddresses(instance *models.PVMInstance) { m.Error(err, "failed to fetch the DHCP IP address from cache store", "VM", *instance.ServerName) } if exists { - m.Info("found IP for VM from DHCP cache", "IP", obj.(options.VMip).IP, "VM", *instance.ServerName) + m.Info("found IP for VM from DHCP cache", "IP", obj.(powervs.VMip).IP, "VM", *instance.ServerName) addresses = append(addresses, corev1.NodeAddress{ Type: corev1.NodeInternalIP, - Address: obj.(options.VMip).IP, + Address: obj.(powervs.VMip).IP, }) m.IBMPowerVSMachine.Status.Addresses = addresses return @@ -482,7 +476,7 @@ func (m *PowerVSMachineScope) SetAddresses(instance *models.PVMInstance) { return } // Get all the DHCP servers - dhcpServer, err := m.IBMPowerVSClient.GetDHCPServers() + dhcpServer, err := m.IBMPowerVSClient.GetAllDHCPServers() if err != nil { m.Error(err, "failed to get DHCP server") return @@ -492,7 +486,7 @@ func (m *PowerVSMachineScope) SetAddresses(instance *models.PVMInstance) { for _, server := range dhcpServer { if *server.Network.ID == *networkID { m.Info("found DHCP server for network", "DHCP server ID", *server.ID, "network ID", *networkID) - dhcpServerDetails, err = m.IBMPowerVSClient.GetDHCPServerByID(*server.ID) + dhcpServerDetails, err = m.IBMPowerVSClient.GetDHCPServer(*server.ID) if err != nil { m.Error(err, "failed to get DHCP server details", "DHCP server ID", *server.ID) return @@ -527,7 +521,7 @@ func (m *PowerVSMachineScope) SetAddresses(instance *models.PVMInstance) { Address: *internalIP, }) // Update the cache with the ip and VM name - err = m.DHCPIPCacheStore.Add(options.VMip{ + err = m.DHCPIPCacheStore.Add(powervs.VMip{ Name: *instance.ServerName, IP: *internalIP, }) diff --git a/cloud/scope/powervs_machine_test.go b/cloud/scope/powervs_machine_test.go index 7a9957f75..b52d9f6de 100644 --- a/cloud/scope/powervs_machine_test.go +++ b/cloud/scope/powervs_machine_test.go @@ -26,18 +26,19 @@ import ( "github.com/IBM/go-sdk-core/v5/core" "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/scheme" "k8s.io/client-go/tools/cache" "k8s.io/klog/v2/klogr" "k8s.io/utils/pointer" - "sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/options" capiv1beta1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" infrav1beta1 "sigs.k8s.io/cluster-api-provider-ibmcloud/api/v1beta1" + "sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/cloud/services/powervs" "sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/cloud/services/powervs/mock" . "github.com/onsi/ginkgo" @@ -93,7 +94,7 @@ func setupPowerVSMachineScope(clusterName string, machineName string, imageID *s Machine: machine, IBMPowerVSCluster: powervsCluster, IBMPowerVSMachine: powervsMachine, - DHCPIPCacheStore: cache.NewTTLStore(options.CacheKeyFunc, options.CacheTTL), + DHCPIPCacheStore: cache.NewTTLStore(powervs.CacheKeyFunc, powervs.CacheTTL), } } @@ -408,7 +409,7 @@ func TestSetAddresses(t *testing.T) { } defaultDhcpCacheStoreFunc := func() cache.Store { - return cache.NewTTLStore(options.CacheKeyFunc, options.CacheTTL) + return cache.NewTTLStore(powervs.CacheKeyFunc, powervs.CacheTTL) } testCases := []struct { @@ -534,7 +535,7 @@ func TestSetAddresses(t *testing.T) { testcase: "error while getting DHCP servers", powerVSClientFunc: func(ctrl *gomock.Controller) *mock.MockPowerVS { mockPowerVSClient := mock.NewMockPowerVS(ctrl) - mockPowerVSClient.EXPECT().GetDHCPServers().Return(nil, fmt.Errorf("intentional error")) + mockPowerVSClient.EXPECT().GetAllDHCPServers().Return(nil, fmt.Errorf("intentional error")) return mockPowerVSClient }, pvmInstance: newPowerVSInstance(instanceName, networkID, instanceMac), @@ -546,7 +547,7 @@ func TestSetAddresses(t *testing.T) { testcase: "dhcp server details not found associated to network id", powerVSClientFunc: func(ctrl *gomock.Controller) *mock.MockPowerVS { mockPowerVSClient := mock.NewMockPowerVS(ctrl) - mockPowerVSClient.EXPECT().GetDHCPServers().Return(newDHCPServer(dhcpServerID, "test-network"), nil) + mockPowerVSClient.EXPECT().GetAllDHCPServers().Return(newDHCPServer(dhcpServerID, "test-network"), nil) return mockPowerVSClient }, pvmInstance: newPowerVSInstance(instanceName, networkID, instanceMac), @@ -558,8 +559,8 @@ func TestSetAddresses(t *testing.T) { testcase: "error on getting DHCP server details", powerVSClientFunc: func(ctrl *gomock.Controller) *mock.MockPowerVS { mockPowerVSClient := mock.NewMockPowerVS(ctrl) - mockPowerVSClient.EXPECT().GetDHCPServers().Return(newDHCPServer(dhcpServerID, networkID), nil) - mockPowerVSClient.EXPECT().GetDHCPServerByID(dhcpServerID).Return(nil, fmt.Errorf("intentnional error")) + mockPowerVSClient.EXPECT().GetAllDHCPServers().Return(newDHCPServer(dhcpServerID, networkID), nil) + mockPowerVSClient.EXPECT().GetDHCPServer(dhcpServerID).Return(nil, fmt.Errorf("intentnional error")) return mockPowerVSClient }, pvmInstance: newPowerVSInstance(instanceName, networkID, instanceMac), @@ -571,8 +572,8 @@ func TestSetAddresses(t *testing.T) { testcase: "dhcp server lease does not have lease for instance", powerVSClientFunc: func(ctrl *gomock.Controller) *mock.MockPowerVS { mockPowerVSClient := mock.NewMockPowerVS(ctrl) - mockPowerVSClient.EXPECT().GetDHCPServers().Return(newDHCPServer(dhcpServerID, networkID), nil) - mockPowerVSClient.EXPECT().GetDHCPServerByID(dhcpServerID).Return(newDHCPServerDetails(dhcpServerID, leaseIP, "ff:11:33:dd:00:33"), nil) + mockPowerVSClient.EXPECT().GetAllDHCPServers().Return(newDHCPServer(dhcpServerID, networkID), nil) + mockPowerVSClient.EXPECT().GetDHCPServer(dhcpServerID).Return(newDHCPServerDetails(dhcpServerID, leaseIP, "ff:11:33:dd:00:33"), nil) return mockPowerVSClient }, pvmInstance: newPowerVSInstance(instanceName, networkID, instanceMac), @@ -584,8 +585,8 @@ func TestSetAddresses(t *testing.T) { testcase: "success in getting ip address from dhcp server", powerVSClientFunc: func(ctrl *gomock.Controller) *mock.MockPowerVS { mockPowerVSClient := mock.NewMockPowerVS(ctrl) - mockPowerVSClient.EXPECT().GetDHCPServers().Return(newDHCPServer(dhcpServerID, networkID), nil) - mockPowerVSClient.EXPECT().GetDHCPServerByID(dhcpServerID).Return(newDHCPServerDetails(dhcpServerID, leaseIP, instanceMac), nil) + mockPowerVSClient.EXPECT().GetAllDHCPServers().Return(newDHCPServer(dhcpServerID, networkID), nil) + mockPowerVSClient.EXPECT().GetDHCPServer(dhcpServerID).Return(newDHCPServerDetails(dhcpServerID, leaseIP, instanceMac), nil) return mockPowerVSClient }, pvmInstance: newPowerVSInstance(instanceName, networkID, instanceMac), @@ -600,8 +601,8 @@ func TestSetAddresses(t *testing.T) { testcase: "ip stored in cache expired, fetch from dhcp server", powerVSClientFunc: func(ctrl *gomock.Controller) *mock.MockPowerVS { mockPowerVSClient := mock.NewMockPowerVS(ctrl) - mockPowerVSClient.EXPECT().GetDHCPServers().Return(newDHCPServer(dhcpServerID, networkID), nil) - mockPowerVSClient.EXPECT().GetDHCPServerByID(dhcpServerID).Return(newDHCPServerDetails(dhcpServerID, leaseIP, instanceMac), nil) + mockPowerVSClient.EXPECT().GetAllDHCPServers().Return(newDHCPServer(dhcpServerID, networkID), nil) + mockPowerVSClient.EXPECT().GetDHCPServer(dhcpServerID).Return(newDHCPServerDetails(dhcpServerID, leaseIP, instanceMac), nil) return mockPowerVSClient }, pvmInstance: newPowerVSInstance(instanceName, networkID, instanceMac), @@ -610,8 +611,8 @@ func TestSetAddresses(t *testing.T) { Address: leaseIP, }), dhcpCacheStoreFunc: func() cache.Store { - cacheStore := cache.NewTTLStore(options.CacheKeyFunc, time.Millisecond) - _ = cacheStore.Add(options.VMip{ + cacheStore := cache.NewTTLStore(powervs.CacheKeyFunc, time.Millisecond) + _ = cacheStore.Add(powervs.VMip{ Name: instanceName, IP: "192.168.99.98", }) @@ -632,8 +633,8 @@ func TestSetAddresses(t *testing.T) { Address: leaseIP, }), dhcpCacheStoreFunc: func() cache.Store { - cacheStore := cache.NewTTLStore(options.CacheKeyFunc, options.CacheTTL) - _ = cacheStore.Add(options.VMip{ + cacheStore := cache.NewTTLStore(powervs.CacheKeyFunc, powervs.CacheTTL) + _ = cacheStore.Add(powervs.VMip{ Name: instanceName, IP: leaseIP, }) diff --git a/controllers/ibmpowervsmachine_controller.go b/controllers/ibmpowervsmachine_controller.go index dc3102bd8..243df695d 100644 --- a/controllers/ibmpowervsmachine_controller.go +++ b/controllers/ibmpowervsmachine_controller.go @@ -25,6 +25,7 @@ import ( "github.com/pkg/errors" apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/record" capiv1beta1 "sigs.k8s.io/cluster-api/api/v1beta1" capierrors "sigs.k8s.io/cluster-api/errors" @@ -36,8 +37,8 @@ import ( infrav1beta1 "sigs.k8s.io/cluster-api-provider-ibmcloud/api/v1beta1" "sigs.k8s.io/cluster-api-provider-ibmcloud/cloud/scope" + "sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/cloud/services/powervs" "sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/endpoints" - "sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/options" capibmrecord "sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/record" ) @@ -50,6 +51,13 @@ type IBMPowerVSMachineReconciler struct { Scheme *runtime.Scheme } +// dhcpCacheStore is a cache store to hold the Power VS VM DHCP IP. +var dhcpCacheStore cache.Store + +func init() { + dhcpCacheStore = powervs.InitialiseDHCPCacheStore() +} + // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=ibmpowervsmachines,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=infrastructure.cluster.x-k8s.io,resources=ibmpowervsmachines/status,verbs=get;update;patch @@ -118,6 +126,7 @@ func (r *IBMPowerVSMachineReconciler) Reconcile(ctx context.Context, req ctrl.Re IBMPowerVSMachine: ibmPowerVSMachine, IBMPowerVSImage: ibmPowerVSImage, ServiceEndpoint: r.ServiceEndpoint, + DHCPIPCacheStore: dhcpCacheStore, }) if err != nil { return ctrl.Result{}, errors.Errorf("failed to create scope: %+v", err) @@ -165,7 +174,7 @@ func (r *IBMPowerVSMachineReconciler) reconcileDelete(scope *scope.PowerVSMachin return ctrl.Result{}, errors.Wrapf(err, "error deleting IBMPowerVSMachine %s/%s", scope.IBMPowerVSMachine.Namespace, scope.IBMPowerVSMachine.Name) } // Remove the cached VM IP - err := scope.DHCPIPCacheStore.Delete(options.VMip{Name: scope.IBMPowerVSMachine.Name}) + err := scope.DHCPIPCacheStore.Delete(powervs.VMip{Name: scope.IBMPowerVSMachine.Name}) if err != nil { scope.Error(err, "failed to delete the VM entry from DHCP cache store", "VM", scope.IBMPowerVSMachine.Name) } diff --git a/controllers/ibmpowervsmachine_controller_test.go b/controllers/ibmpowervsmachine_controller_test.go index 5d2ed96a7..f267710d5 100644 --- a/controllers/ibmpowervsmachine_controller_test.go +++ b/controllers/ibmpowervsmachine_controller_test.go @@ -32,7 +32,6 @@ import ( "k8s.io/client-go/tools/record" "k8s.io/klog/v2/klogr" "k8s.io/utils/pointer" - "sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/options" capiv1beta1 "sigs.k8s.io/cluster-api/api/v1beta1" "sigs.k8s.io/cluster-api/util" "sigs.k8s.io/cluster-api/util/conditions" @@ -42,6 +41,7 @@ import ( infrav1beta1 "sigs.k8s.io/cluster-api-provider-ibmcloud/api/v1beta1" "sigs.k8s.io/cluster-api-provider-ibmcloud/cloud/scope" + "sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/cloud/services/powervs" "sigs.k8s.io/cluster-api-provider-ibmcloud/pkg/cloud/services/powervs/mock" . "github.com/onsi/gomega" @@ -301,7 +301,7 @@ func TestIBMPowerVSMachineReconciler_Delete(t *testing.T) { InstanceID: "powervs-instance-id", }, }, - DHCPIPCacheStore: cache.NewTTLStore(options.CacheKeyFunc, options.CacheTTL), + DHCPIPCacheStore: cache.NewTTLStore(powervs.CacheKeyFunc, powervs.CacheTTL), } mockpowervs.EXPECT().DeleteInstance(machineScope.IBMPowerVSMachine.Status.InstanceID).Return(nil) _, err := reconciler.reconcileDelete(machineScope) @@ -480,7 +480,7 @@ func TestIBMPowerVSMachineReconciler_ReconcileOperations(t *testing.T) { }, }, IBMPowerVSClient: mockpowervs, - DHCPIPCacheStore: cache.NewTTLStore(options.CacheKeyFunc, options.CacheTTL), + DHCPIPCacheStore: cache.NewTTLStore(powervs.CacheKeyFunc, powervs.CacheTTL), } instanceReferences := &models.PVMInstances{ diff --git a/pkg/options/caching.go b/pkg/cloud/services/powervs/caching.go similarity index 84% rename from pkg/options/caching.go rename to pkg/cloud/services/powervs/caching.go index 8f643e3df..9184abbe8 100644 --- a/pkg/options/caching.go +++ b/pkg/cloud/services/powervs/caching.go @@ -14,9 +14,13 @@ See the License for the specific language governing permissions and limitations under the License. */ -package options +package powervs -import "time" +import ( + "time" + + "k8s.io/client-go/tools/cache" +) // CacheTTL is duration of time to store the vm ip in cache // Currently the default sync period is 10 minutes that means every 10 minutes @@ -34,3 +38,8 @@ type VMip struct { func CacheKeyFunc(obj interface{}) (string, error) { return obj.(VMip).Name, nil } + +// InitialiseDHCPCacheStore returns a new cache store. +func InitialiseDHCPCacheStore() cache.Store { + return cache.NewTTLStore(CacheKeyFunc, CacheTTL) +} diff --git a/pkg/cloud/services/powervs/mock/powervs_generated.go b/pkg/cloud/services/powervs/mock/powervs_generated.go index 2690c791c..2970cf27f 100644 --- a/pkg/cloud/services/powervs/mock/powervs_generated.go +++ b/pkg/cloud/services/powervs/mock/powervs_generated.go @@ -121,6 +121,21 @@ func (mr *MockPowerVSMockRecorder) DeleteJob(id interface{}) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteJob", reflect.TypeOf((*MockPowerVS)(nil).DeleteJob), id) } +// GetAllDHCPServers mocks base method. +func (m *MockPowerVS) GetAllDHCPServers() (models.DHCPServers, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetAllDHCPServers") + ret0, _ := ret[0].(models.DHCPServers) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetAllDHCPServers indicates an expected call of GetAllDHCPServers. +func (mr *MockPowerVSMockRecorder) GetAllDHCPServers() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetAllDHCPServers", reflect.TypeOf((*MockPowerVS)(nil).GetAllDHCPServers)) +} + // GetAllImage mocks base method. func (m *MockPowerVS) GetAllImage() (*models.Images, error) { m.ctrl.T.Helper() @@ -181,34 +196,19 @@ func (mr *MockPowerVSMockRecorder) GetCosImages(id interface{}) *gomock.Call { return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetCosImages", reflect.TypeOf((*MockPowerVS)(nil).GetCosImages), id) } -// GetDHCPServerByID mocks base method. -func (m *MockPowerVS) GetDHCPServerByID(id string) (*models.DHCPServerDetail, error) { +// GetDHCPServer mocks base method. +func (m *MockPowerVS) GetDHCPServer(id string) (*models.DHCPServerDetail, error) { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetDHCPServerByID", id) + ret := m.ctrl.Call(m, "GetDHCPServer", id) ret0, _ := ret[0].(*models.DHCPServerDetail) ret1, _ := ret[1].(error) return ret0, ret1 } -// GetDHCPServerByID indicates an expected call of GetDHCPServerByID. -func (mr *MockPowerVSMockRecorder) GetDHCPServerByID(id interface{}) *gomock.Call { - mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetDHCPServerByID", reflect.TypeOf((*MockPowerVS)(nil).GetDHCPServerByID), id) -} - -// GetDHCPServers mocks base method. -func (m *MockPowerVS) GetDHCPServers() (models.DHCPServers, error) { - m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "GetDHCPServers") - ret0, _ := ret[0].(models.DHCPServers) - ret1, _ := ret[1].(error) - return ret0, ret1 -} - -// GetDHCPServers indicates an expected call of GetDHCPServers. -func (mr *MockPowerVSMockRecorder) GetDHCPServers() *gomock.Call { +// GetDHCPServer indicates an expected call of GetDHCPServer. +func (mr *MockPowerVSMockRecorder) GetDHCPServer(id interface{}) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetDHCPServers", reflect.TypeOf((*MockPowerVS)(nil).GetDHCPServers)) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetDHCPServer", reflect.TypeOf((*MockPowerVS)(nil).GetDHCPServer), id) } // GetImage mocks base method. diff --git a/pkg/cloud/services/powervs/powervs.go b/pkg/cloud/services/powervs/powervs.go index f8bf86854..c236159d3 100644 --- a/pkg/cloud/services/powervs/powervs.go +++ b/pkg/cloud/services/powervs/powervs.go @@ -37,6 +37,6 @@ type PowerVS interface { GetCosImages(id string) (*models.Job, error) GetJob(id string) (*models.Job, error) DeleteJob(id string) error - GetDHCPServers() (models.DHCPServers, error) - GetDHCPServerByID(id string) (*models.DHCPServerDetail, error) + GetAllDHCPServers() (models.DHCPServers, error) + GetDHCPServer(id string) (*models.DHCPServerDetail, error) } diff --git a/pkg/cloud/services/powervs/service.go b/pkg/cloud/services/powervs/service.go index 5092a38d6..78fe3a3dd 100644 --- a/pkg/cloud/services/powervs/service.go +++ b/pkg/cloud/services/powervs/service.go @@ -111,13 +111,13 @@ func (s *Service) GetAllNetwork() (*models.Networks, error) { return s.networkClient.GetAll() } -// GetDHCPServers returns all the DHCP servers in the Power VS service instance. -func (s *Service) GetDHCPServers() (models.DHCPServers, error) { +// GetAllDHCPServers returns all the DHCP servers in the Power VS service instance. +func (s *Service) GetAllDHCPServers() (models.DHCPServers, error) { return s.dhcpClient.GetAll() } -// GetDHCPServerByID returns the details for DHCP server associated with id. -func (s *Service) GetDHCPServerByID(id string) (*models.DHCPServerDetail, error) { +// GetDHCPServer returns the details for DHCP server associated with id. +func (s *Service) GetDHCPServer(id string) (*models.DHCPServerDetail, error) { return s.dhcpClient.Get(id) }