✨ Modify OpenStackCluster.Spec.Network API #1836

MaysaMacedo · 2024-01-19T20:36:17Z

For the BYO scenario, when the OpenStackCluster.Spec.Network
is not specified the query to OpenStack will return all the
Networks available in the cloud and fail the reconciliation.
To avoid this, if any Subnets were specified under
OpenStackCluster.Spec.Subnets this can used to identify
which Network to use.

Note, this pattern to infer a field based on another field has already been used by the
OpenStackCluster.Spec.Subnets and also when no Network is defined in a Port.

TODOs:

includes documentation
adds unit tests

/hold

netlify · 2024-01-19T20:36:24Z

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

Name	Link
🔨 Latest commit	`2e4ca73`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/65d62bd380815e0008207d05
😎 Deploy Preview	https://deploy-preview-1836--kubernetes-sigs-cluster-api-openstack.netlify.app/print
📱 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.

pkg/cloud/services/networking/network.go

MaysaMacedo · 2024-01-26T17:19:56Z

/test pull-cluster-api-provider-openstack-e2e-test

EmilienM · 2024-01-26T19:11:15Z

/test pull-cluster-api-provider-openstack-e2e-full-test

EmilienM · 2024-01-26T19:53:31Z

/lgtm

jichenjc · 2024-01-29T06:08:08Z

pkg/cloud/services/networking/network.go

+// It returns an error in case it failed to retrieve the network.
+func (s *Service) PopulateCAPONetworkFromSubnet(subnets []subnets.Subnet, openStackCluster *infrav1.OpenStackCluster) error {
+	if openStackCluster.Status.Network.ID == "" && len(subnets) > 0 {
+		if len(subnets) > 1 && subnets[0].NetworkID != subnets[1].NetworkID {


just curious why this has subnet[0] and [1] ? some case we have 2 subnets and only 2 subnet?

In case a dual-stack cluster is desired, one IPv4 and IPv6 subnet needs to be specified. The API already enforces the max number of subnets to be 2.

MaysaMacedo · 2024-01-29T17:19:04Z

/hold cancel

mdbooth

We're doing a very similar dance in normalizePortTarget: we are getting a network a subnets from a NetworkFilter and []SubnetFilter and we infer the network from the subnets if it's not specified. Is there any opportunity to refactor that code to be used in both places?

mdbooth · 2024-01-30T16:18:08Z

controllers/openstackcluster_controller.go

 		if err != nil {
 			return err
 		}

+		var subnets []infrav1.Subnet
+		for subnet := range filteredSubnets {
+			subnets = networking.ConvertOpenStackSubnetToCAPOSubnet(subnets, &filteredSubnets[subnet])


Please could you rename this function to something like AppendOpenStackSubnetToCAPOSubnets. I had to double-take this in review because without checking the implementation it looks like it overwrites previous subnets on each iteration.

mdbooth · 2024-01-30T16:45:47Z

controllers/openstackcluster_controller.go

-		if len(networkList) > 1 {
-			handleUpdateOSCError(openStackCluster, fmt.Errorf("found multiple networks (result: %v)", networkList))
-			return fmt.Errorf("found multiple networks (result: %v)", networkList)
-		}
 		if openStackCluster.Status.Network == nil {


A potential issue here is that we're now telling people they can omit the network section, but we're still executing an empty list operation above. This is going to list all networks. I know how long this takes on PSI, and we should probably avoid it.

I wonder if we should not execute the network query if the network spec is empty.

ack, I added a condition checking if the filter is empty.

ack, I added a condition checking if the filter is empty.

Where?

this? https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/1836/files#diff-39b99bc838c9f76967790a10572cc44f0c41ccacddcd6e1eff934828d4b050a3R738

@EmilienM No, here.

mdbooth · 2024-01-30T16:53:56Z

pkg/cloud/services/networking/network.go

Please can you move all of these functions, including the existing ConvertOpenStackSubnetToCAPOSubnet, to unexported functions in the cluster controller. They're not really service methods and with them having side-effects (they modify their arguments) they're not really independent library functions. Having them in a different file just breaks up readability.

If any have only a single caller you might consider inlining them at the point of use, tbh.

MaysaMacedo · 2024-02-01T14:19:58Z

We're doing a very similar dance in normalizePortTarget: we are getting a network a subnets from a NetworkFilter and []SubnetFilter and we infer the network from the subnets if it's not specified. Is there any opportunity to refactor that code to be used in both places?

The scenario I'm handling is a bit different because I need the hole Network and not just the ID in order to populate the OpenStackCluster.status. Also, the way we filter the subnets is also a bit different because we need to account for when there is no subnets or various subnets. So the end goal for Ports and Network is the same, but there are restrictions that IMHO make the refactor not feasible.

EmilienM · 2024-02-05T18:30:32Z

controllers/openstackcluster_controller.go

+func populateCAPONetworkFromSubnet(networkingService *networking.Service, subnets []subnets.Subnet, openStackCluster *infrav1.OpenStackCluster) error {
+	if openStackCluster.Status.Network.ID == "" && len(subnets) > 0 {
+		if len(subnets) > 1 && subnets[0].NetworkID != subnets[1].NetworkID {
+			return fmt.Errorf("unable to identify the Network to use. NetworkID from Subnets differ: %s, %s", subnets[0].NetworkID, subnets[1].NetworkID)


Can we improve the logging maybe? Here is an example (can be improved):

Suggested change

return fmt.Errorf("unable to identify the Network to use. NetworkID from Subnets differ: %s, %s", subnets[0].NetworkID, subnets[1].NetworkID)

return fmt.Errorf("unable to identify the Network to use. NetworkID %s from subnet %s does not match NetworkID %s from subnet %s", subnets[0].NetworkID, subnets[0].ID, subnets[1].NetworkID, subnets[1].ID)

EmilienM · 2024-02-05T18:37:59Z

controllers/openstackcluster_controller.go

+	return nil
+}
+
+// getCAPONetwork gets a network based on a filter.


nit: add a bit more doc into this function. Yes indeed, we get a network based on the filter and return errors if not found. But if found, we will update the status (by calling convertOpenStackNetworkToCAPONetwork). Also we return no error if Spec.Network is empty because we now try to find the Network specs by the Subnets field.

EmilienM · 2024-02-05T18:38:34Z

controllers/openstackcluster_controller.go

+			handleUpdateOSCError(openStackCluster, fmt.Errorf("failed to find any network"))
+			return fmt.Errorf("failed to find any network")
+		}
+		if len(networkList) == 1 {


I'll like to see a switch probably, and return an error if more than one network is found maybe?

My proposal is intentional. If multiple networks are found, we won't fail and will give another chance to look up the network by the subnet later on on populateCAPONetworkFromSubnet.

EmilienM · 2024-02-05T18:40:51Z

controllers/openstackcluster_controller.go

+	return filteredSubnets, nil
+}
+
+// convertOpenStackNetworkToCAPONetwork converts an OpenStack network to a capo network.


more of an code architecture question (ping @mdbooth / @lentzi90), I would have put these functions into network.go instead, as I've seen it elsewhere. However I'm not sure if we have a clear line on that.

I'm still convinced that we want to move these new functions into network.go.

This is pure API manipulation. I think it's best done here, but it's not a hill I would die on.

EmilienM · 2024-02-09T20:01:58Z

controllers/openstackcluster_controller.go

+func populateCAPONetworkFromSubnet(networkingService *networking.Service, subnets []subnets.Subnet, openStackCluster *infrav1.OpenStackCluster) error {
+	if openStackCluster.Status.Network.ID == "" && len(subnets) > 0 {
+		if len(subnets) > 1 && subnets[0].NetworkID != subnets[1].NetworkID {
+			return fmt.Errorf("unable to identify the Network to use. NetworkID %s from subnet %s does not match NetworkID %s from subnet %s", subnets[0].NetworkID, subnets[0].ID, subnets[1].NetworkID, subnets[1].ID)


the error message can be improved a little bit, because we also check for the length of subnets.
also please s/Network/network/ to be consistent with subnet.

mdbooth · 2024-02-16T16:14:19Z

controllers/openstackcluster_controller.go

+	return filteredSubnets, nil
+}
+
+// convertOpenStackNetworkToCAPONetwork converts an OpenStack network to a capo network.


This is pure API manipulation. I think it's best done here, but it's not a hill I would die on.

mdbooth · 2024-02-21T15:24:52Z

controllers/openstackcluster_controller_test.go

@@ -544,6 +546,45 @@ var _ = Describe("OpenStackCluster controller", func() {
 		Expect(err).To(BeNil())
 		Expect(len(testCluster.Status.Network.Subnets)).To(Equal(2))
 	})
+
+	It("should allow fetch network by subnet", func() {


Great test, thanks.

k8s-ci-robot · 2024-02-21T15:27:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MaysaMacedo, mdbooth

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mdbooth]

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

For the BYO scenario, when the `OpenStackCluster.Spec.Network` is not specified the query to OpenStack would return all the Networks available in the cloud and fail the reconciliation. To avoid this, if any Subnets were specified under `OpenStackCluster.Spec.Subnets` this can be used to identify which Network to use.

EmilienM · 2024-02-21T17:06:25Z

/test pull-cluster-api-provider-openstack-e2e-full-test
/lgtm
/hold for CI

MaysaMacedo · 2024-02-21T19:08:52Z

/hold cancel
CI passed

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 19, 2024

k8s-ci-robot requested review from jichenjc and mdbooth January 19, 2024 20:36

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 19, 2024

MaysaMacedo changed the title ~~✨ Modify OpenStackCluster.Spec.Network API~~ ✨ WIP Modify OpenStackCluster.Spec.Network API Jan 19, 2024

MaysaMacedo marked this pull request as draft January 19, 2024 22:07

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 19, 2024

MaysaMacedo force-pushed the find-network-by-subnet branch from b347263 to 43d8f89 Compare January 22, 2024 19:42

MaysaMacedo changed the title ~~✨ WIP Modify OpenStackCluster.Spec.Network API~~ ✨ Modify OpenStackCluster.Spec.Network API Jan 22, 2024

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 22, 2024

MaysaMacedo marked this pull request as ready for review January 22, 2024 19:42

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 22, 2024

k8s-ci-robot requested a review from lentzi90 January 22, 2024 19:42

MaysaMacedo force-pushed the find-network-by-subnet branch from 43d8f89 to c48fa08 Compare January 23, 2024 13:30

EmilienM reviewed Jan 24, 2024

View reviewed changes

pkg/cloud/services/networking/network.go Outdated Show resolved Hide resolved

MaysaMacedo force-pushed the find-network-by-subnet branch from c48fa08 to 380f8d2 Compare January 26, 2024 12:54

k8s-ci-robot assigned EmilienM Jan 26, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 26, 2024

jichenjc reviewed Jan 29, 2024

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 29, 2024

mdbooth reviewed Jan 30, 2024

View reviewed changes

MaysaMacedo force-pushed the find-network-by-subnet branch from 380f8d2 to 14ecbda Compare February 1, 2024 14:24

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2024

EmilienM reviewed Feb 5, 2024

View reviewed changes

MaysaMacedo force-pushed the find-network-by-subnet branch from 14ecbda to 9709a58 Compare February 9, 2024 18:31

EmilienM reviewed Feb 9, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2024

MaysaMacedo force-pushed the find-network-by-subnet branch from 9709a58 to 0efefe8 Compare February 15, 2024 18:54

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 15, 2024

mdbooth approved these changes Feb 21, 2024

View reviewed changes

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 21, 2024

MaysaMacedo force-pushed the find-network-by-subnet branch from 0efefe8 to 2e4ca73 Compare February 21, 2024 16:58

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2024

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 21, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 21, 2024

k8s-ci-robot merged commit 029d643 into kubernetes-sigs:main Feb 21, 2024
10 checks passed

MaysaMacedo deleted the find-network-by-subnet branch February 21, 2024 19:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Modify OpenStackCluster.Spec.Network API #1836

✨ Modify OpenStackCluster.Spec.Network API #1836

MaysaMacedo commented Jan 19, 2024 •

edited

Loading

netlify bot commented Jan 19, 2024 •

edited

Loading

MaysaMacedo commented Jan 26, 2024

EmilienM commented Jan 26, 2024

EmilienM commented Jan 26, 2024

jichenjc Jan 29, 2024

MaysaMacedo Jan 29, 2024

MaysaMacedo commented Jan 29, 2024

mdbooth left a comment

mdbooth Jan 30, 2024

mdbooth Jan 30, 2024

MaysaMacedo Feb 1, 2024

EmilienM Feb 2, 2024

EmilienM Feb 5, 2024

MaysaMacedo Feb 5, 2024

mdbooth Jan 30, 2024

MaysaMacedo commented Feb 1, 2024

EmilienM Feb 5, 2024

EmilienM Feb 5, 2024

EmilienM Feb 5, 2024

MaysaMacedo Feb 9, 2024

EmilienM Feb 9, 2024

EmilienM Feb 5, 2024

EmilienM Feb 9, 2024

mdbooth Feb 16, 2024

EmilienM Feb 9, 2024

mdbooth Feb 16, 2024

mdbooth Feb 21, 2024

k8s-ci-robot commented Feb 21, 2024

EmilienM commented Feb 21, 2024

MaysaMacedo commented Feb 21, 2024

	return fmt.Errorf("unable to identify the Network to use. NetworkID from Subnets differ: %s, %s", subnets[0].NetworkID, subnets[1].NetworkID)
	return fmt.Errorf("unable to identify the Network to use. NetworkID %s from subnet %s does not match NetworkID %s from subnet %s", subnets[0].NetworkID, subnets[0].ID, subnets[1].NetworkID, subnets[1].ID)

✨ Modify OpenStackCluster.Spec.Network API #1836

✨ Modify OpenStackCluster.Spec.Network API #1836

Conversation

MaysaMacedo commented Jan 19, 2024 • edited Loading

netlify bot commented Jan 19, 2024 • edited Loading

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

MaysaMacedo commented Jan 26, 2024

EmilienM commented Jan 26, 2024

EmilienM commented Jan 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaysaMacedo commented Jan 29, 2024

mdbooth 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MaysaMacedo commented Feb 1, 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

k8s-ci-robot commented Feb 21, 2024

EmilienM commented Feb 21, 2024

MaysaMacedo commented Feb 21, 2024

MaysaMacedo commented Jan 19, 2024 •

edited

Loading

netlify bot commented Jan 19, 2024 •

edited

Loading