From 22012f88b93d605659b95144c0f4e7e0288416a8 Mon Sep 17 00:00:00 2001 From: Dharaneeshwaran Ravichandran Date: Tue, 5 Nov 2024 10:14:55 +0530 Subject: [PATCH] =?UTF-8?q?Refactor=20ReconcileNetwork=20-=20If=20only=20I?= =?UTF-8?q?BMPowerVSCluster.Spec.Network=20is=20set,=20network=20would=20b?= =?UTF-8?q?e=20validated=20and=20if=20exists=20already=20will=20get=20used?= =?UTF-8?q?=20as=20cluster=E2=80=99s=20network=20or=20a=20new=20network=20?= =?UTF-8?q?will=20be=20created=20via=20DHCP=20service.=20-=20If=20only=20I?= =?UTF-8?q?BMPowerVSCluster.Spec.DHCPServer=20is=20set,=20DHCP=20server=20?= =?UTF-8?q?would=20be=20validated=20and=20if=20exists=20already,=20will=20?= =?UTF-8?q?use=20DHCP=20server=E2=80=99s=20network=20as=20cluster=20networ?= =?UTF-8?q?k.=20If=20not=20a=20new=20DHCP=20service=20will=20be=20created?= =?UTF-8?q?=20and=20it=E2=80=99s=20network=20will=20be=20used.=20-=20If=20?= =?UTF-8?q?both=20IBMPowerVSCluster.Spec.Network=20&=20IBMPowerVSCluster.S?= =?UTF-8?q?pec.DHCPServer=20is=20set,=20network=20and=20DHCP=20server=20wo?= =?UTF-8?q?uld=20be=20validated=20and=20if=20both=20exists=20already=20the?= =?UTF-8?q?n=20network=20is=20belongs=20to=20given=20DHCP=20server=20or=20?= =?UTF-8?q?not=20would=20be=20validated.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- cloud/scope/powervs_cluster.go | 120 ++++++++-- cloud/scope/powervs_cluster_test.go | 223 ++++++++++++++++-- .../ibmpowervscluster_controller_test.go | 29 +-- 3 files changed, 310 insertions(+), 62 deletions(-) diff --git a/cloud/scope/powervs_cluster.go b/cloud/scope/powervs_cluster.go index fd9fb7034..062cc7242 100644 --- a/cloud/scope/powervs_cluster.go +++ b/cloud/scope/powervs_cluster.go @@ -120,6 +120,10 @@ func getTGPowerVSConnectionName(tgName string) string { return fmt.Sprintf("%s-p func getTGVPCConnectionName(tgName string) string { return fmt.Sprintf("%s-vpc-con", tgName) } +func dhcpNetworkName(dhcpServerName string) string { + return fmt.Sprintf("DHCPSERVER%s_Private", dhcpServerName) +} + // NewPowerVSClusterScope creates a new PowerVSClusterScope from the supplied parameters. func NewPowerVSClusterScope(params PowerVSClusterScopeParams) (*PowerVSClusterScope, error) { if params.Client == nil { @@ -459,6 +463,14 @@ func (s *PowerVSClusterScope) SetStatus(resourceType infrav1beta2.ResourceType, } } +// GetNetworkID returns the Network id from status of IBMPowerVSCluster object. If it doesn't exist, returns nil. +func (s *PowerVSClusterScope) GetNetworkID() *string { + if s.IBMPowerVSCluster.Status.Network != nil { + return s.IBMPowerVSCluster.Status.Network.ID + } + return nil +} + // Network returns the cluster Network. func (s *PowerVSClusterScope) Network() *infrav1beta2.IBMPowerVSResourceReference { return &s.IBMPowerVSCluster.Spec.Network @@ -849,16 +861,27 @@ func (s *PowerVSClusterScope) createServiceInstance() (*resourcecontrollerv2.Res // ReconcileNetwork reconciles network. func (s *PowerVSClusterScope) ReconcileNetwork() (bool, error) { - if s.GetDHCPServerID() != nil { + if s.GetNetworkID() != nil { + // Check the network exists + if _, err := s.IBMPowerVSClient.GetNetworkByID(*s.GetNetworkID()); err != nil { + return false, err + } + + if s.GetDHCPServerID() == nil { + // If only network is set, return once network is validated to be ok + return true, nil + } + s.V(3).Info("DHCP server ID is set, fetching details", "dhcpServerID", s.GetDHCPServerID()) active, err := s.isDHCPServerActive() if err != nil { return false, err } - // if dhcp server exist and in active state, its assumed that dhcp network exist - // TODO(Phase 2): Verify that dhcp network is exist. - return active, nil - // TODO(karthik-k-n): If needed set dhcp status here + // DHCP server still not active, skip checking network for now + if !active { + return false, nil + } + return true, nil } // check network exist in cloud networkID, err := s.checkNetwork() @@ -868,10 +891,20 @@ func (s *PowerVSClusterScope) ReconcileNetwork() (bool, error) { if networkID != nil { s.V(3).Info("Found PowerVS network in IBM Cloud", "networkID", networkID) s.SetStatus(infrav1beta2.ResourceTypeNetwork, infrav1beta2.ResourceReference{ID: networkID, ControllerCreated: ptr.To(false)}) + } + dhcpServerID, err := s.checkDHCPServer() + if err != nil { + return false, err + } + if dhcpServerID != nil { + s.V(3).Info("Found DHCP server in IBM Cloud", "DHCP Server ID", dhcpServerID) + s.SetStatus(infrav1beta2.ResourceTypeDHCPServer, infrav1beta2.ResourceReference{ID: dhcpServerID, ControllerCreated: ptr.To(false)}) + } + if s.GetNetworkID() != nil { return true, nil } - dhcpServerID, err := s.createDHCPServer() + dhcpServerID, err = s.createDHCPServer() if err != nil { s.Error(err, "Error creating DHCP server") return false, err @@ -882,37 +915,80 @@ func (s *PowerVSClusterScope) ReconcileNetwork() (bool, error) { return false, nil } -// checkNetwork checks if network exists in cloud with given name or ID mentioned in spec. -func (s *PowerVSClusterScope) checkNetwork() (*string, error) { - if s.IBMPowerVSCluster.Spec.Network.ID != nil { - s.V(3).Info("Checking if PowerVS network exists in IBM Cloud with ID", "networkID", *s.IBMPowerVSCluster.Spec.Network.ID) - network, err := s.IBMPowerVSClient.GetNetworkByID(*s.IBMPowerVSCluster.Spec.Network.ID) +// checkDHCPServer checks if DHCP server exists in cloud with given DHCPServer's ID or name mentioned in spec. +// If exists and s.IBMPowerVSCluster.Status.Network is not populated will set DHCP server's network as cluster's network. +// If exists and s.IBMPowerVSCluster.Status.Network is populated already will validate the DHCP server's network and cluster networks are matching, if not will throw an error. +func (s *PowerVSClusterScope) checkDHCPServer() (*string, error) { + if s.DHCPServer() != nil && s.DHCPServer().ID != nil { + dhcpServer, err := s.IBMPowerVSClient.GetDHCPServer(*s.DHCPServer().ID) if err != nil { return nil, err } - return network.NetworkID, nil + if s.GetNetworkID() == nil { + if _, err := s.IBMPowerVSClient.GetNetworkByID(*dhcpServer.Network.ID); err != nil { + return nil, err + } + s.SetStatus(infrav1beta2.ResourceTypeNetwork, infrav1beta2.ResourceReference{ID: dhcpServer.Network.ID, ControllerCreated: ptr.To(false)}) + } else if *dhcpServer.Network.ID != *s.GetNetworkID() { + return nil, fmt.Errorf("error network set via spec and DHCP server's networkID are not matching") + } + return dhcpServer.ID, nil } - // if the user has provided the already existing dhcp server name then there might exist network name - // with format DHCPSERVER_Private , try fetching that + // if user provides DHCP server name then we can use network name to match the existing DHCP server var networkName string if s.DHCPServer() != nil && s.DHCPServer().Name != nil { - networkName = fmt.Sprintf("DHCPSERVER%s_Private", *s.DHCPServer().Name) + networkName = dhcpNetworkName(*s.DHCPServer().Name) } else { - networkName = *s.GetServiceName(infrav1beta2.ResourceTypeNetwork) + networkName = dhcpNetworkName(s.InfraCluster()) } s.V(3).Info("Checking if PowerVS network exists in IBM Cloud with name", "name", networkName) - // fetch the network associated with name - network, err := s.IBMPowerVSClient.GetNetworkByName(networkName) + dhcpServers, err := s.IBMPowerVSClient.GetAllDHCPServers() if err != nil { return nil, err } - if network == nil || network.NetworkID == nil { - s.V(3).Info("Unable to find PowerVS network in IBM Cloud", "network", s.IBMPowerVSCluster.Spec.Network) - return nil, nil + for _, dhcpServer := range dhcpServers { + if *dhcpServer.Network.Name == networkName { + if s.GetNetworkID() == nil { + if _, err := s.IBMPowerVSClient.GetNetworkByID(*dhcpServer.Network.ID); err != nil { + return nil, err + } + s.SetStatus(infrav1beta2.ResourceTypeNetwork, infrav1beta2.ResourceReference{ID: dhcpServer.Network.ID, ControllerCreated: ptr.To(false)}) + } else if *dhcpServer.Network.ID != *s.GetNetworkID() { + return nil, fmt.Errorf("error network set via spec and DHCP server's networkID are not matching") + } + return dhcpServer.ID, nil + } } - return network.NetworkID, nil + + return nil, nil +} + +// checkNetwork checks if network exists in cloud with given network's ID or name mentioned in spec. +func (s *PowerVSClusterScope) checkNetwork() (*string, error) { + if s.Network().ID != nil { + s.V(3).Info("Checking if PowerVS network exists in IBM Cloud with ID", "networkID", *s.Network().ID) + network, err := s.IBMPowerVSClient.GetNetworkByID(*s.Network().ID) + if err != nil { + return nil, err + } + return network.NetworkID, nil + } + + if s.Network().Name != nil { + s.V(3).Info("Checking if PowerVS network exists in IBM Cloud with name") + network, err := s.IBMPowerVSClient.GetNetworkByName(*s.Network().Name) + if err != nil { + return nil, err + } + if network == nil || network.NetworkID == nil { + s.V(3).Info("Unable to find PowerVS network in IBM Cloud", "network", s.IBMPowerVSCluster.Spec.Network) + return nil, nil + } + return network.NetworkID, nil + } + return nil, nil } // isDHCPServerActive checks if the DHCP server status is active. diff --git a/cloud/scope/powervs_cluster_test.go b/cloud/scope/powervs_cluster_test.go index b8eba5de2..523efd01e 100644 --- a/cloud/scope/powervs_cluster_test.go +++ b/cloud/scope/powervs_cluster_test.go @@ -3229,7 +3229,8 @@ func TestReconcileNetwork(t *testing.T) { mockPowerVS *mockP.MockPowerVS mockCtrl *gomock.Controller ) - + const netID = "netID" + const dhcpID = "dhcpID" setup := func(t *testing.T) { t.Helper() mockCtrl = gomock.NewController(t) @@ -3238,39 +3239,77 @@ func TestReconcileNetwork(t *testing.T) { teardown := func() { mockCtrl.Finish() } - t.Run("When GetDHCPServer returns error", func(t *testing.T) { + + t.Run("When network is available in cloud during status reconciliation", func(t *testing.T) { g := NewWithT(t) setup(t) t.Cleanup(teardown) clusterScope := PowerVSClusterScope{ IBMPowerVSClient: mockPowerVS, - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{Status: infrav1beta2.IBMPowerVSClusterStatus{DHCPServer: &infrav1beta2.ResourceReference{ID: ptr.To("dhcpID")}}}, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{Status: infrav1beta2.IBMPowerVSClusterStatus{Network: &infrav1beta2.ResourceReference{ID: ptr.To("netID")}}}, } - mockPowerVS.EXPECT().GetDHCPServer(gomock.Any()).Return(nil, fmt.Errorf("GetDHCPServer error")) - requeue, err := clusterScope.ReconcileNetwork() + network := &models.Network{NetworkID: ptr.To("netID")} + mockPowerVS.EXPECT().GetNetworkByID(gomock.Any()).Return(network, nil) + + isNetworkAvailable, err := clusterScope.ReconcileNetwork() + g.Expect(err).To(BeNil()) + g.Expect(isNetworkAvailable).To(BeTrue()) + }) + t.Run("When GetNetworkByID returns error during status reconciliation", func(t *testing.T) { + g := NewWithT(t) + setup(t) + t.Cleanup(teardown) + + clusterScope := PowerVSClusterScope{ + IBMPowerVSClient: mockPowerVS, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{Status: infrav1beta2.IBMPowerVSClusterStatus{Network: &infrav1beta2.ResourceReference{ID: ptr.To("netID")}}}, + } + mockPowerVS.EXPECT().GetNetworkByID(gomock.Any()).Return(nil, fmt.Errorf("GetNetworkByID error")) + + isNetworkAvailable, err := clusterScope.ReconcileNetwork() g.Expect(err).ToNot(BeNil()) - g.Expect(requeue).To(BeFalse()) + g.Expect(isNetworkAvailable).To(BeFalse()) }) - t.Run("When DHCPServer exists and is active", func(t *testing.T) { + t.Run("When both network and DHCP server is available in cloud during status reconciliation", func(t *testing.T) { g := NewWithT(t) setup(t) t.Cleanup(teardown) clusterScope := PowerVSClusterScope{ IBMPowerVSClient: mockPowerVS, - IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{Status: infrav1beta2.IBMPowerVSClusterStatus{DHCPServer: &infrav1beta2.ResourceReference{ID: ptr.To("dhcpID")}}}, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{Status: infrav1beta2.IBMPowerVSClusterStatus{DHCPServer: &infrav1beta2.ResourceReference{ID: ptr.To("dhcpID")}, Network: &infrav1beta2.ResourceReference{ID: ptr.To("netID")}}}, } dhcpServer := &models.DHCPServerDetail{ID: ptr.To("dhcpID"), Status: ptr.To(string(infrav1beta2.DHCPServerStateActive))} mockPowerVS.EXPECT().GetDHCPServer(gomock.Any()).Return(dhcpServer, nil) + network := &models.Network{NetworkID: ptr.To("netID")} + mockPowerVS.EXPECT().GetNetworkByID(gomock.Any()).Return(network, nil) - requeue, err := clusterScope.ReconcileNetwork() + isNetworkAvailable, err := clusterScope.ReconcileNetwork() g.Expect(err).To(BeNil()) - g.Expect(requeue).To(BeTrue()) + g.Expect(isNetworkAvailable).To(BeTrue()) + }) + t.Run("When network is available in cloud but DHCP server is not available during status reconciliation", func(t *testing.T) { + g := NewWithT(t) + setup(t) + t.Cleanup(teardown) + + clusterScope := PowerVSClusterScope{ + IBMPowerVSClient: mockPowerVS, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{Status: infrav1beta2.IBMPowerVSClusterStatus{DHCPServer: &infrav1beta2.ResourceReference{ID: ptr.To("dhcpID")}, Network: &infrav1beta2.ResourceReference{ID: ptr.To("netID")}}}, + } + + mockPowerVS.EXPECT().GetDHCPServer(gomock.Any()).Return(nil, fmt.Errorf("GetDHCPServer error")) + network := &models.Network{NetworkID: ptr.To("netID")} + mockPowerVS.EXPECT().GetNetworkByID(gomock.Any()).Return(network, nil) + + isNetworkAvailable, err := clusterScope.ReconcileNetwork() + g.Expect(err).ToNot(BeNil()) + g.Expect(isNetworkAvailable).To(BeFalse()) }) - t.Run("When DHCPID is empty and GetNetworkByID returns error ", func(t *testing.T) { + t.Run("When networkID is set via spec and GetNetworkByID returns error ", func(t *testing.T) { g := NewWithT(t) setup(t) t.Cleanup(teardown) @@ -3283,25 +3322,163 @@ func TestReconcileNetwork(t *testing.T) { network := &models.Network{} mockPowerVS.EXPECT().GetNetworkByID(gomock.Any()).Return(network, fmt.Errorf("GetNetworkByID error")) - requeue, err := clusterScope.ReconcileNetwork() + isNetworkAvailable, err := clusterScope.ReconcileNetwork() g.Expect(err).ToNot(BeNil()) - g.Expect(requeue).To(BeFalse()) + g.Expect(isNetworkAvailable).To(BeFalse()) }) - t.Run("When DHCPID is empty and networkID is not empty", func(t *testing.T) { + t.Run("When networkID is set via spec and exists in IBM cloud", func(t *testing.T) { g := NewWithT(t) setup(t) t.Cleanup(teardown) - network := &models.Network{NetworkID: ptr.To("networkID")} + + network := &models.Network{NetworkID: ptr.To(netID)} clusterScope := PowerVSClusterScope{ IBMPowerVSClient: mockPowerVS, IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{Spec: infrav1beta2.IBMPowerVSClusterSpec{ - Network: infrav1beta2.IBMPowerVSResourceReference{ID: ptr.To("networkID")}}}, + Network: infrav1beta2.IBMPowerVSResourceReference{ID: ptr.To(netID)}}}, } mockPowerVS.EXPECT().GetNetworkByID(gomock.Any()).Return(network, nil) - requeue, err := clusterScope.ReconcileNetwork() + mockPowerVS.EXPECT().GetAllDHCPServers().Return(nil, nil) + isNetworkAvailable, err := clusterScope.ReconcileNetwork() g.Expect(clusterScope.IBMPowerVSCluster.Status.Network.ID).To(Equal(network.NetworkID)) g.Expect(err).To(BeNil()) - g.Expect(requeue).To(BeTrue()) + g.Expect(isNetworkAvailable).To(BeTrue()) + }) + t.Run("When network name is set and exists in IBM cloud", func(t *testing.T) { + g := NewWithT(t) + setup(t) + t.Cleanup(teardown) + + netName := "networkName" + network := &models.NetworkReference{Name: ptr.To(netName), NetworkID: ptr.To(netID)} + clusterScope := PowerVSClusterScope{ + IBMPowerVSClient: mockPowerVS, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{Spec: infrav1beta2.IBMPowerVSClusterSpec{ + Network: infrav1beta2.IBMPowerVSResourceReference{Name: ptr.To(netName)}}}, + } + mockPowerVS.EXPECT().GetAllDHCPServers().Return(nil, nil) + mockPowerVS.EXPECT().GetNetworkByName(gomock.Any()).Return(network, nil) + isNetworkAvailable, err := clusterScope.ReconcileNetwork() + g.Expect(*clusterScope.IBMPowerVSCluster.Status.Network.ID).To(Equal(netID)) + g.Expect(err).To(BeNil()) + g.Expect(isNetworkAvailable).To(BeTrue()) + }) + t.Run("When network and DHCP server ID is set via spec and exists in IBM cloud", func(t *testing.T) { + g := NewWithT(t) + setup(t) + t.Cleanup(teardown) + + network := &models.Network{NetworkID: ptr.To(netID)} + dhcpServer := &models.DHCPServerDetail{ID: ptr.To(dhcpID), Network: &models.DHCPServerNetwork{ID: ptr.To(netID)}} + clusterScope := PowerVSClusterScope{ + IBMPowerVSClient: mockPowerVS, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{Network: infrav1beta2.IBMPowerVSResourceReference{ID: ptr.To(netID)}, DHCPServer: &infrav1beta2.DHCPServer{ID: ptr.To(dhcpID)}}, + }, + } + mockPowerVS.EXPECT().GetNetworkByID(gomock.Any()).Return(network, nil) + mockPowerVS.EXPECT().GetDHCPServer(gomock.Any()).Return(dhcpServer, nil) + isNetworkAvailable, err := clusterScope.ReconcileNetwork() + g.Expect(*clusterScope.IBMPowerVSCluster.Status.Network.ID).To(Equal(netID)) + g.Expect(*clusterScope.IBMPowerVSCluster.Status.DHCPServer.ID).To(Equal(dhcpID)) + g.Expect(err).To(BeNil()) + g.Expect(isNetworkAvailable).To(BeTrue()) + }) + t.Run("When network and DHCP server ID is set via spec but network is not belong to given dhcp server", func(t *testing.T) { + g := NewWithT(t) + setup(t) + t.Cleanup(teardown) + + network := &models.Network{NetworkID: ptr.To(netID)} + dhcpServer := &models.DHCPServerDetail{ID: ptr.To(dhcpID), Network: &models.DHCPServerNetwork{ID: ptr.To("netID2")}} + clusterScope := PowerVSClusterScope{ + IBMPowerVSClient: mockPowerVS, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ + Spec: infrav1beta2.IBMPowerVSClusterSpec{Network: infrav1beta2.IBMPowerVSResourceReference{ID: ptr.To(netID)}, DHCPServer: &infrav1beta2.DHCPServer{ID: ptr.To(dhcpID)}}, + }, + } + mockPowerVS.EXPECT().GetNetworkByID(gomock.Any()).Return(network, nil) + mockPowerVS.EXPECT().GetDHCPServer(gomock.Any()).Return(dhcpServer, nil) + isNetworkAvailable, err := clusterScope.ReconcileNetwork() + g.Expect(err).ToNot(BeNil()) + g.Expect(isNetworkAvailable).To(BeFalse()) + }) + t.Run("When only DHCP server ID is set via spec and exists in IBM cloud", func(t *testing.T) { + g := NewWithT(t) + setup(t) + t.Cleanup(teardown) + + network := &models.Network{NetworkID: ptr.To(netID)} + dhcpServer := &models.DHCPServerDetail{ID: ptr.To(dhcpID), Network: &models.DHCPServerNetwork{ID: ptr.To(netID)}} + clusterScope := PowerVSClusterScope{ + IBMPowerVSClient: mockPowerVS, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{Spec: infrav1beta2.IBMPowerVSClusterSpec{ + DHCPServer: &infrav1beta2.DHCPServer{ID: ptr.To(dhcpID)}}}, + } + mockPowerVS.EXPECT().GetDHCPServer(gomock.Any()).Return(dhcpServer, nil) + mockPowerVS.EXPECT().GetNetworkByID(gomock.Any()).Return(network, nil) + isNetworkAvailable, err := clusterScope.ReconcileNetwork() + g.Expect(*clusterScope.IBMPowerVSCluster.Status.Network.ID).To(Equal(netID)) + g.Expect(*clusterScope.IBMPowerVSCluster.Status.DHCPServer.ID).To(Equal(dhcpID)) + g.Expect(err).To(BeNil()) + g.Expect(isNetworkAvailable).To(BeTrue()) + }) + t.Run("When only DHCP server ID is set but not exists in IBM cloud", func(t *testing.T) { + g := NewWithT(t) + setup(t) + t.Cleanup(teardown) + + clusterScope := PowerVSClusterScope{ + IBMPowerVSClient: mockPowerVS, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{Spec: infrav1beta2.IBMPowerVSClusterSpec{ + DHCPServer: &infrav1beta2.DHCPServer{ID: ptr.To("dhcpID")}}}, + } + mockPowerVS.EXPECT().GetDHCPServer(gomock.Any()).Return(nil, fmt.Errorf("dhcp server by ID not found")) + isNetworkAvailable, err := clusterScope.ReconcileNetwork() + g.Expect(err).ToNot(BeNil()) + g.Expect(isNetworkAvailable).To(BeFalse()) + }) + t.Run("When DHCP server name is set and exists in IBM cloud", func(t *testing.T) { + g := NewWithT(t) + setup(t) + t.Cleanup(teardown) + + dhcpServerName := "dhcpServerName" + netName := dhcpNetworkName(dhcpServerName) + network := &models.Network{NetworkID: ptr.To(netID)} + dhcpServers := models.DHCPServers{&models.DHCPServer{ID: ptr.To(dhcpID), Network: &models.DHCPServerNetwork{ID: ptr.To(netID), Name: ptr.To(netName)}}} + clusterScope := PowerVSClusterScope{ + IBMPowerVSClient: mockPowerVS, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{Spec: infrav1beta2.IBMPowerVSClusterSpec{ + DHCPServer: &infrav1beta2.DHCPServer{Name: ptr.To(dhcpServerName)}}}, + } + mockPowerVS.EXPECT().GetNetworkByID(gomock.Any()).Return(network, nil) + mockPowerVS.EXPECT().GetAllDHCPServers().Return(dhcpServers, nil) + isNetworkAvailable, err := clusterScope.ReconcileNetwork() + g.Expect(*clusterScope.IBMPowerVSCluster.Status.Network.ID).To(Equal(netID)) + g.Expect(*clusterScope.IBMPowerVSCluster.Status.DHCPServer.ID).To(Equal(dhcpID)) + g.Expect(err).To(BeNil()) + g.Expect(isNetworkAvailable).To(BeTrue()) + }) + t.Run("When no network details set via spec but network exist with cluster name", func(t *testing.T) { + g := NewWithT(t) + setup(t) + t.Cleanup(teardown) + + clusterName := "clusterName" + netName := dhcpNetworkName(clusterName) + network := &models.Network{NetworkID: ptr.To(netID)} + dhcpServers := models.DHCPServers{&models.DHCPServer{ID: ptr.To(dhcpID), Network: &models.DHCPServerNetwork{ID: ptr.To(netID), Name: ptr.To(netName)}}} + clusterScope := PowerVSClusterScope{ + IBMPowerVSClient: mockPowerVS, + IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{ObjectMeta: metav1.ObjectMeta{Name: clusterName}}, + } + mockPowerVS.EXPECT().GetAllDHCPServers().Return(dhcpServers, nil) + mockPowerVS.EXPECT().GetNetworkByID(gomock.Any()).Return(network, nil) + isNetworkAvailable, err := clusterScope.ReconcileNetwork() + g.Expect(*clusterScope.IBMPowerVSCluster.Status.Network.ID).To(Equal(netID)) + g.Expect(err).To(BeNil()) + g.Expect(isNetworkAvailable).To(BeTrue()) }) t.Run("When network name is set in spec and DHCP server is created successfully", func(t *testing.T) { g := NewWithT(t) @@ -3315,13 +3492,14 @@ func TestReconcileNetwork(t *testing.T) { IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{Spec: infrav1beta2.IBMPowerVSClusterSpec{ Network: infrav1beta2.IBMPowerVSResourceReference{Name: ptr.To("networkName")}}}, } + mockPowerVS.EXPECT().GetAllDHCPServers().Return(nil, nil) mockPowerVS.EXPECT().GetNetworkByName(gomock.Any()).Return(nil, nil) mockPowerVS.EXPECT().CreateDHCPServer(gomock.Any()).Return(dhcpServer, nil) - requeue, err := clusterScope.ReconcileNetwork() + isNetworkAvailable, err := clusterScope.ReconcileNetwork() g.Expect(clusterScope.IBMPowerVSCluster.Status.DHCPServer.ID).To(Equal(dhcpServer.ID)) g.Expect(clusterScope.IBMPowerVSCluster.Status.Network.ID).To(Equal(dhcpNetwork.ID)) g.Expect(err).To(BeNil()) - g.Expect(requeue).To(BeFalse()) + g.Expect(isNetworkAvailable).To(BeFalse()) }) t.Run("When network name is set in spec and createDHCPServer returns error", func(t *testing.T) { g := NewWithT(t) @@ -3333,11 +3511,12 @@ func TestReconcileNetwork(t *testing.T) { IBMPowerVSCluster: &infrav1beta2.IBMPowerVSCluster{Spec: infrav1beta2.IBMPowerVSClusterSpec{ Network: infrav1beta2.IBMPowerVSResourceReference{Name: ptr.To("networkName")}}}, } + mockPowerVS.EXPECT().GetAllDHCPServers().Return(nil, nil) mockPowerVS.EXPECT().GetNetworkByName(gomock.Any()).Return(nil, nil) mockPowerVS.EXPECT().CreateDHCPServer(gomock.Any()).Return(nil, fmt.Errorf("CreateDHCPServer error")) - requeue, err := clusterScope.ReconcileNetwork() + isNetworkAvailable, err := clusterScope.ReconcileNetwork() g.Expect(err).ToNot(BeNil()) - g.Expect(requeue).To(BeFalse()) + g.Expect(isNetworkAvailable).To(BeFalse()) }) } diff --git a/controllers/ibmpowervscluster_controller_test.go b/controllers/ibmpowervscluster_controller_test.go index 4b5711487..e8f3ad9cd 100644 --- a/controllers/ibmpowervscluster_controller_test.go +++ b/controllers/ibmpowervscluster_controller_test.go @@ -635,10 +635,7 @@ func TestIBMPowerVSClusterReconciler_reconcile(t *testing.T) { } mockPowerVS := powervsmock.NewMockPowerVS(gomock.NewController(t)) mockPowerVS.EXPECT().GetDatacenterCapabilities(gomock.Any()).Return(map[string]bool{"power-edge-router": true}, nil) - mockPowerVS.EXPECT().GetDHCPServer(gomock.Any()).Return(&models.DHCPServerDetail{ - ID: ptr.To("dhcpID"), - Status: ptr.To(string(infrav1beta2.DHCPServerStateBuild)), - }, nil) + mockPowerVS.EXPECT().GetNetworkByID(gomock.Any()).Return(nil, fmt.Errorf("error get networkByID")) mockPowerVS.EXPECT().WithClients(gomock.Any()) clusterScope.IBMPowerVSClient = mockPowerVS @@ -1643,14 +1640,13 @@ func TestReconcilePowerVSResources(t *testing.T) { ServiceInstanceID: "serviceInstanceID", }, Status: infrav1beta2.IBMPowerVSClusterStatus{ - DHCPServer: &infrav1beta2.ResourceReference{ID: ptr.To("DHCPServerID")}, + Network: &infrav1beta2.ResourceReference{ID: ptr.To("NetworkID")}, ServiceInstance: &infrav1beta2.ResourceReference{ID: ptr.To("serviceInstanceID")}, }, }, } mockPowerVS := powervsmock.NewMockPowerVS(gomock.NewController(t)) - dhcpServer := &models.DHCPServerDetail{ID: ptr.To("dhcpID"), Status: ptr.To(string(infrav1beta2.DHCPServerStateError))} - mockPowerVS.EXPECT().GetDHCPServer(gomock.Any()).Return(dhcpServer, nil) + mockPowerVS.EXPECT().GetNetworkByID(gomock.Any()).Return(nil, fmt.Errorf("error getting network")) mockPowerVS.EXPECT().WithClients(gomock.Any()) mockResourceController := resourceclientmock.NewMockResourceController(gomock.NewController(t)) mockResourceController.EXPECT().GetResourceInstance(gomock.Any()).Return(&resourcecontrollerv2.ResourceInstance{State: ptr.To(string(infrav1beta2.ServiceInstanceStateActive)), Name: ptr.To("serviceInstanceName")}, nil, nil) @@ -1659,7 +1655,7 @@ func TestReconcilePowerVSResources(t *testing.T) { return clusterScope }, reconcileResult: reconcileResult{ - error: errors.New("DHCP server creation failed and is in error state"), + error: errors.New("error getting network"), }, conditions: capiv1beta1.Conditions{ capiv1beta1.Condition{ @@ -1668,7 +1664,7 @@ func TestReconcilePowerVSResources(t *testing.T) { Severity: capiv1beta1.ConditionSeverityError, LastTransitionTime: metav1.Time{}, Reason: infrav1beta2.NetworkReconciliationFailedReason, - Message: "DHCP server creation failed and is in error state", + Message: "error getting network", }, getServiceInstanceReadyCondition(), }, @@ -1682,14 +1678,13 @@ func TestReconcilePowerVSResources(t *testing.T) { ServiceInstanceID: "serviceInstanceID", }, Status: infrav1beta2.IBMPowerVSClusterStatus{ - DHCPServer: &infrav1beta2.ResourceReference{ID: ptr.To("DHCPServerID")}, + Network: &infrav1beta2.ResourceReference{ID: ptr.To("netID")}, ServiceInstance: &infrav1beta2.ResourceReference{ID: ptr.To("serviceInstanceID")}, }, }, } mockPowerVS := powervsmock.NewMockPowerVS(gomock.NewController(t)) - dhcpServer := &models.DHCPServerDetail{ID: ptr.To("dhcpID"), Status: ptr.To(string(infrav1beta2.DHCPServerStateActive))} - mockPowerVS.EXPECT().GetDHCPServer(gomock.Any()).Return(dhcpServer, nil) + mockPowerVS.EXPECT().GetNetworkByID(gomock.Any()).Return(&models.Network{NetworkID: ptr.To("netID")}, nil) mockPowerVS.EXPECT().WithClients(gomock.Any()) mockResourceController := resourceclientmock.NewMockResourceController(gomock.NewController(t)) mockResourceController.EXPECT().GetResourceInstance(gomock.Any()).Return(&resourcecontrollerv2.ResourceInstance{State: ptr.To(string(infrav1beta2.ServiceInstanceStateActive)), Name: ptr.To("serviceInstanceName")}, nil, nil) @@ -1793,8 +1788,8 @@ func getPowerVSClusterWithSpecAndStatus() *infrav1beta2.IBMPowerVSCluster { ServiceInstance: &infrav1beta2.ResourceReference{ ID: ptr.To("serviceInstanceID"), }, - DHCPServer: &infrav1beta2.ResourceReference{ - ID: ptr.To("DHCPServerID"), + Network: &infrav1beta2.ResourceReference{ + ID: ptr.To("NetworkID"), }, VPC: &infrav1beta2.ResourceReference{ ID: ptr.To("vpcID"), @@ -1810,10 +1805,8 @@ func getMockPowerVS(t *testing.T) *powervsmock.MockPowerVS { t.Helper() mockPowerVS := powervsmock.NewMockPowerVS(gomock.NewController(t)) mockPowerVS.EXPECT().GetDatacenterCapabilities(gomock.Any()).Return(map[string]bool{"power-edge-router": true}, nil) - mockPowerVS.EXPECT().GetDHCPServer(gomock.Any()).Return(&models.DHCPServerDetail{ - ID: ptr.To("dhcpID"), - Status: ptr.To(string(infrav1beta2.DHCPServerStateActive)), - }, nil) + network := &models.Network{NetworkID: ptr.To("netID")} + mockPowerVS.EXPECT().GetNetworkByID(gomock.Any()).Return(network, nil) mockPowerVS.EXPECT().WithClients(gomock.Any()) return mockPowerVS }