Refactor ReconcileNetwork #2040

dharaneeshvrd · 2024-11-07T06:09:13Z

What this PR does / why we need it:

If only IBMPowerVSCluster.Spec.Network is set, network would be validated and if exists already will get used as cluster’s network or a new network will be created via DHCP service.
If only IBMPowerVSCluster.Spec.DHCPServer is set, DHCP server would be validated and if exists already, will use DHCP server’s network as cluster network. If not a new DHCP service will be created and it’s network will be used.
If both IBMPowerVSCluster.Spec.Network & IBMPowerVSCluster.Spec.DHCPServer is set, network and DHCP server would be validated and if both exists already then network is belongs to given DHCP server or not would be validated.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1933

Special notes for your reviewer:

/area provider/ibmcloud

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Refactor ReconcileNetwork

k8s-ci-robot · 2024-11-07T06:09:19Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: dharaneeshvrd
Once this PR has been reviewed and has the lgtm label, please assign prajyot-parab for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

netlify · 2024-11-07T06:09:30Z

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

Name	Link
🔨 Latest commit	`205a6e2`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-ibmcloud/deploys/67471c45a7a4910008ff5e0c
😎 Deploy Preview	https://deploy-preview-2040--kubernetes-sigs-cluster-api-ibmcloud.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

dharaneeshvrd · 2024-11-07T06:09:33Z

/cc @Amulyam24 @Karthik-K-N

Karthik-K-N · 2024-11-07T07:17:34Z

cloud/scope/powervs_cluster.go

 	}
-	return network.NetworkID, nil
+
+	if nw != nil && !nw.DhcpManaged {


Do you think nw.DhcpManaged is required? We are getting network id from status and we set network id to status only when we create DHCP server.

Ok I can remove this check.

Karthik-K-N · 2024-11-07T07:25:57Z

cloud/scope/powervs_cluster.go

 		if err != nil {
 			return nil, err
 		}
+		if err := s.verifyDHCPServerRespectToNetwork(*networkID); err != nil {


I think this forces the user to set the dhcp network only. so we should update the api spec to mention it.
Also never tried what happens if we pass public network id here, Not relaying on DHCP.

Yes. I think we should use DHCP server only, during boot, CoreOS expects the ip from a DHCP server. I hope the case is same for vanilla capi cluster as well.
Shall I modify the API spec as part of this PR?

We use Centos images in normal capi flow as well.
@mkumatag whats your thought. Should we make this restriction or do you foresee any other use cases.

nope, we should allow both dhcp based and regular public networks.

Ok, current code designed to handle only DHCP network. @Karthik-K-N We can create a story for this to handle later I believe. Wdyt?

Yeah, That can be done.

Amulyam24 · 2024-11-09T06:15:14Z

cloud/scope/powervs_cluster.go

@@ -459,6 +463,14 @@ func (s *PowerVSClusterScope) SetStatus(resourceType infrav1beta2.ResourceType,
 	}
 }

+// GetNetworkID returns the Networkf id from status of IBMPowerVSCluster object. If it doesn't exist, returns nil.


Suggested change

// GetNetworkID returns the Networkf id from status of IBMPowerVSCluster object. If it doesn't exist, returns nil.

// GetNetworkID returns the Network id from status of IBMPowerVSCluster object. If it doesn't exist, returns nil.

Amulyam24 · 2024-11-09T07:03:56Z

cloud/scope/powervs_cluster.go

 	// if the user has provided the already existing dhcp server name then there might exist network name
 	// with format DHCPSERVER<DHCPServer.Name>_Private , try fetching that
 	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)


is this required? as we are already checking this condition above

Can you please give me the ref? I think this is the only place where I am checking with cluster name.

Thanks for clarifying, I understand it is required to preserve the order.

Sure, addressed your other comments. ptal.

Amulyam24 · 2024-11-09T07:16:11Z

cloud/scope/powervs_cluster.go

-		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)
+// checkNetwork checks if network exists in cloud with given network's ID or name or dhcp server's ID or name mentioned in spec.
+// Priority order: Network ID - Network name - DHCP server ID - DHCP server name - Network with cluster name.


Suggested change

// Priority order: Network ID - Network name - DHCP server ID - DHCP server name - Network with cluster name.

// Priority order: Network ID > Network name > DHCP server ID > DHCP server name > Network with cluster name.

dharaneeshvrd · 2024-11-19T05:21:45Z

@Karthik-K-N @Amulyam24 I have refactored this PR to handle plain network and DHCP network separately. PTAL.

Amulyam24

Overall flow looks neat and good, couple of questions

Should we update this var name dhcpServerActive since we check for both network and dhcp?
In a case where only network name or ID is set in spec, we do not check for associated dhcp servers right, is this to accommodate public network type?

Amulyam24 · 2024-11-21T10:29:19Z

cloud/scope/powervs_cluster.go

+				return nil, err
+			}
+			s.SetStatus(infrav1beta2.ResourceTypeNetwork, infrav1beta2.ResourceReference{ID: dhcpServer.Network.ID, ControllerCreated: ptr.To(false)})
+		} else if *dhcpServer.Network.ID != *s.GetNetworkID() {


can there occur a scenario where dhcpServer.Network is nil?

I don't think so, but have added a nil check for dhcpServer.Network anyway, ptal.

dharaneeshvrd · 2024-11-25T03:55:23Z

Should we update this var name dhcpServerActive since we check for both network and dhcp?

Agree, We mainly check network's active status, so kept networkActive

In a case where only network name or ID is set in spec, we do not check for associated dhcp servers right, is this to accommodate public network type?

Yes that has become the goal of this PR, earlier I was assuming that network should belong to a DHCP server. Now it can be a separate entity, if user provides any field on spec.IBMPowerVSCluster.DHCPServer then only I am considering its a DHCP server's network. Otherwise default flow remains same where no existing details provided, it would create a DHCP server to setup the network.

Amulyam24

overall LGTM.

one minor suggestion, in UT, can we also verify the ControllerCreated value wherever applicable?

/cc @Karthik-K-N

Karthik-K-N

ReconcileNetwork is very compilcated logic, Thank you for taking this up.

Few comments otherwise LGTM. Can we also mention in high level as comments over ReconcileNetwork() function what we are doing.

Karthik-K-N · 2024-11-26T12:10:13Z

cloud/scope/powervs_cluster.go

+		return false, err
+	}
+	if dhcpServerID != nil {
+		s.V(3).Info("Found DHCP server in IBM Cloud", "DHCP Server ID", dhcpServerID)


To keep uniform and improve our logging, lets use , dhcpServerID instead of "DHCP Server ID"

Karthik-K-N · 2024-11-26T12:25:06Z

cloud/scope/powervs_cluster.go

 		if err != nil {
 			return nil, err
 		}
-		return network.NetworkID, nil
+		if s.GetNetworkID() == nil {
+			if _, err := s.IBMPowerVSClient.GetNetworkByID(*dhcpServer.Network.ID); err != nil {


Is it safe to say that dhcpServer will be active by this time? If not the network may be nil.
I am just concerned about *dhcpServer.Network.ID

Here we are validating existing DHCP server, new DHCP server creation won't reach this block, but anyway I have handled this via nil check, PTAL.

Karthik-K-N · 2024-11-26T12:30:41Z

cloud/scope/powervs_cluster.go

 	} 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)


This log line should be updated to state that, we are trying to look for network from DHCPServer's network list

Karthik-K-N · 2024-11-26T12:35:59Z

cloud/scope/powervs_cluster.go

+	}
+
+	if s.Network().Name != nil {
+		s.V(3).Info("Checking if PowerVS network exists in IBM Cloud with name")


Lets log network name also.

- If only IBMPowerVSCluster.Spec.Network is set, network would be validated and if exists already will get used as cluster’s network or a new network will be created via DHCP service. - If only IBMPowerVSCluster.Spec.DHCPServer is set, DHCP server would be validated and if exists already, will use DHCP server’s network as cluster network. If not a new DHCP service will be created and it’s network will be used. - If both IBMPowerVSCluster.Spec.Network & IBMPowerVSCluster.Spec.DHCPServer is set, network and DHCP server would be validated and if both exists already then network is belongs to given DHCP server or not would be validated.

dharaneeshvrd · 2024-11-27T13:20:21Z

@Karthik-K-N @Amulyam24 I have tried to address your comments, PTAL.

k8s-ci-robot · 2024-11-27T13:36:17Z

@dharaneeshvrd: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-ibmcloud-test	`205a6e2`	link	true	`/test pull-cluster-api-provider-ibmcloud-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Karthik-K-N

LGTM lets fix the failing test. Thank you.

Karthik-K-N · 2024-11-27T14:18:38Z

cloud/scope/powervs_cluster.go

@@ -847,18 +859,32 @@ func (s *PowerVSClusterScope) createServiceInstance() (*resourcecontrollerv2.Res
 	return serviceInstance, nil
 }

-// ReconcileNetwork reconciles network.
+// ReconcileNetwork reconciles network
+// If only IBMPowerVSCluster.Spec.Network is set, network would be validated and if exists already will get used as cluster’s network or a new network will be created via DHCP service.


Can you also mention what happens when both network and dhcpserver is not set, Currently that configuration is used the most.

k8s-ci-robot added the area/provider/ibmcloud Issues or PRs related to ibmcloud provider label Nov 7, 2024

k8s-ci-robot requested review from mkumatag and Prajyot-Parab November 7, 2024 06:09

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 7, 2024

k8s-ci-robot requested review from Amulyam24 and Karthik-K-N November 7, 2024 06:09

dharaneeshvrd force-pushed the validate-existing-dhcp branch 2 times, most recently from e39f7b8 to bbef168 Compare November 7, 2024 07:14

Karthik-K-N reviewed Nov 7, 2024

View reviewed changes

dharaneeshvrd force-pushed the validate-existing-dhcp branch 3 times, most recently from fb3990c to fac8446 Compare November 8, 2024 05:24

Amulyam24 reviewed Nov 9, 2024

View reviewed changes

dharaneeshvrd force-pushed the validate-existing-dhcp branch 2 times, most recently from 3cc2ab9 to 8d6fe0c Compare November 11, 2024 10:10

dharaneeshvrd force-pushed the validate-existing-dhcp branch 2 times, most recently from 41a3a74 to 22012f8 Compare November 19, 2024 04:59

dharaneeshvrd changed the title ~~Validate DHCP server and network status when using existing network~~ Refactor ReconcileNetwork Nov 19, 2024

Amulyam24 reviewed Nov 21, 2024

View reviewed changes

dharaneeshvrd force-pushed the validate-existing-dhcp branch from 22012f8 to ee79e44 Compare November 25, 2024 03:52

Amulyam24 reviewed Nov 26, 2024

View reviewed changes

k8s-ci-robot requested a review from Karthik-K-N November 26, 2024 11:55

Karthik-K-N reviewed Nov 26, 2024

View reviewed changes

dharaneeshvrd force-pushed the validate-existing-dhcp branch from ee79e44 to 205a6e2 Compare November 27, 2024 13:19

Karthik-K-N reviewed Nov 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ReconcileNetwork #2040

Refactor ReconcileNetwork #2040

dharaneeshvrd commented Nov 7, 2024 •

edited

Loading

k8s-ci-robot commented Nov 7, 2024

netlify bot commented Nov 7, 2024 •

edited

Loading

dharaneeshvrd commented Nov 7, 2024

Karthik-K-N Nov 7, 2024

dharaneeshvrd Nov 7, 2024

Karthik-K-N Nov 7, 2024

dharaneeshvrd Nov 7, 2024

Karthik-K-N Nov 11, 2024

mkumatag Nov 11, 2024

dharaneeshvrd Nov 11, 2024

Karthik-K-N Nov 11, 2024

Amulyam24 Nov 9, 2024

Amulyam24 Nov 9, 2024

dharaneeshvrd Nov 11, 2024

Amulyam24 Nov 11, 2024

dharaneeshvrd Nov 11, 2024

Amulyam24 Nov 9, 2024

dharaneeshvrd commented Nov 19, 2024

Amulyam24 left a comment •

edited

Loading

Amulyam24 Nov 21, 2024

dharaneeshvrd Nov 25, 2024

dharaneeshvrd commented Nov 25, 2024 •

edited

Loading

Amulyam24 left a comment

Karthik-K-N left a comment

Karthik-K-N Nov 26, 2024

Karthik-K-N Nov 26, 2024

dharaneeshvrd Nov 27, 2024

Karthik-K-N Nov 26, 2024

Karthik-K-N Nov 26, 2024

dharaneeshvrd commented Nov 27, 2024

k8s-ci-robot commented Nov 27, 2024

Karthik-K-N left a comment

Karthik-K-N Nov 27, 2024

	// GetNetworkID returns the Networkf id from status of IBMPowerVSCluster object. If it doesn't exist, returns nil.
	// GetNetworkID returns the Network id from status of IBMPowerVSCluster object. If it doesn't exist, returns nil.

	// Priority order: Network ID - Network name - DHCP server ID - DHCP server name - Network with cluster name.
	// Priority order: Network ID > Network name > DHCP server ID > DHCP server name > Network with cluster name.

Refactor ReconcileNetwork #2040

Are you sure you want to change the base?

Refactor ReconcileNetwork #2040

Conversation

dharaneeshvrd commented Nov 7, 2024 • edited Loading

k8s-ci-robot commented Nov 7, 2024

netlify bot commented Nov 7, 2024 • edited Loading

✅ Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

dharaneeshvrd commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dharaneeshvrd commented Nov 19, 2024

Amulyam24 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dharaneeshvrd commented Nov 25, 2024 • edited Loading

Amulyam24 left a comment

Choose a reason for hiding this comment

Karthik-K-N left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dharaneeshvrd commented Nov 27, 2024

k8s-ci-robot commented Nov 27, 2024

Karthik-K-N left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dharaneeshvrd commented Nov 7, 2024 •

edited

Loading

netlify bot commented Nov 7, 2024 •

edited

Loading

Amulyam24 left a comment •

edited

Loading

dharaneeshvrd commented Nov 25, 2024 •

edited

Loading