⚠️ Update NetworkStatus ready for dual stack #1577

mdbooth · 2023-06-01T15:53:36Z

What this PR does / why we need it:
This is a breaking API change to OpenStackClusterStatus. It makes 2 changes to status.Network. These are done in a single PR as they're somewhat related and can't be merged separately without conflicts. I have intentionally kept them in multiple commits.

The first change moves apiServerLoadBalancer and router out of status.network. These are not properties of the cluster network and may evolve separately in the future. Separating them now before v1beta1 reduces the scope of future churn.

The second change transforms network.subnet into a slice of subnets. This is in anticipation of dual stack support, which we hope to add to a future release based on v1beta1.

Special notes for your reviewer:
The multiple commits are deliberate. I don't intend to squash them. I recommend reviewing this PR by commit, as the changes are logically separate.

Where it was simpler to do so I have updated code to handle multiple subnets, but not in all places. This change doesn't intend to support multiple subnets. It simply allows the API to represent multiple subnets.

This does not represent all API changes required for dual stack support, only one of them.

/hold

k8s-ci-robot · 2023-06-01T15:53:38Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

netlify · 2023-06-01T15:53:40Z

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

Name	Link
🔨 Latest commit	`802654e`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/6490a43af970b6000885c2a1
😎 Deploy Preview	https://deploy-preview-1577--kubernetes-sigs-cluster-api-openstack.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 settings.

k8s-ci-robot · 2023-06-01T15:53:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

mdbooth · 2023-06-01T16:03:39Z

/cc @dulek @MaysaMacedo

k8s-ci-robot · 2023-06-01T16:03:42Z

@mdbooth: GitHub didn't allow me to request PR reviews from the following users: maysamacedo.

Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @dulek @MaysaMacedo

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/test-infra repository.

seanschneeweiss · 2023-06-04T08:10:56Z

api/v1alpha7/openstackcluster_types.go

 	// externalNetwork contains information about the external network used for default ingress and egress traffic.
 	ExternalNetwork *NetworkStatus `json:"externalNetwork,omitempty"`

+	// router describes the default cluster router
+	Router *Router `json:"router,omitempty"`
+
+	// apiServerLoadBalancer describes the api server load balancer if one exists


Comment case should match variable case Router, ExternalNetwork, APIServerLoadBalancer and end with a dot.

The dot linter would be great but is unfortunately not reliable enough.

Interesting. I have no strong personal preference, but as our API astronauts in OpenShift always insist on using the json tag in API godocs I was assuming it was a documented convention. And it is, but the only reference I can find is for OpenShift specifically: https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#use-json-field-names-in-godoc

The reasoning is that users see this output in kubectl explain, and only the json tags make sense in that context.

But... this isn't documented in kubernetes 🤔 The API Conventions don't have anything to say on the matter, and the coding conventions just point to the Go conventions, which obviously say to use the Go field. I'm genuinely surprised by that. I was kinda hoping to do a big API cleanup and change it everywhere so it's consistent, but now I'm wondering.

Thoughts?

I think we don't even have to start with the object name itself. If we take a look at kubectl explain we can see that the name will be printed anyway.

Example

❯ k explain osc.spec | grep -A1 "^ network" network <Object> If NodeCIDR cannot be set this can be used to detect an existing network.

If we want to keep the name in the comment I'd suggest to use the go notation and not the json tag.
But tbh, I don't really have a hard opinion on this.

On that there is a convention: https://www.kubernetes.dev/docs/guide/coding-convention/ points to https://go.dev/blog/godoc, which says:

Notice this comment is a complete sentence that begins with the name of the element it describes. This important convention allows us to generate documentation in a variety of formats, from plain text to HTML to UNIX man pages, and makes it read better when tools truncate it for brevity, such as when they extract the first line or sentence.

api/v1alpha6/conversion.go

pkg/cloud/services/compute/instance.go

seanschneeweiss · 2023-06-04T08:29:13Z

api/v1alpha7/openstackcluster_types.go

@@ -157,7 +157,7 @@ type OpenStackClusterStatus struct {

 	// Network contains all information about the created OpenStack Network.
 	// It includes Subnets and Router.


Does it still include Router?

Nope! I need to update that.

seanschneeweiss · 2023-06-04T08:30:29Z

~~Consider setting label tide/merge-method-squash for keeping the commits.~~

tobiasgiese · 2023-06-04T09:35:56Z

Consider setting label tide/merge-method-squash for keeping the commits.

@seanschneeweiss it‘s the other way around. Setting the squash label would squash the commits. The default merge behavior of Prow is a merge commit :)

mdbooth

Thanks, Sean!

api/v1alpha6/conversion.go

pkg/cloud/services/compute/instance.go

mdbooth · 2023-06-07T10:43:52Z

api/v1alpha7/openstackcluster_types.go

@@ -157,7 +157,7 @@ type OpenStackClusterStatus struct {

 	// Network contains all information about the created OpenStack Network.
 	// It includes Subnets and Router.


Nope! I need to update that.

tobiasgiese · 2023-06-08T17:10:17Z

config/crd/bases/infrastructure.cluster.x-k8s.io_openstackclusters.yaml

+                description: apiServerLoadBalancer describes the api server load balancer
+                  if one exists
+                properties:
+                  allowedCIDRs:


nit: it's maybe not part of this PR, but we should add a short description to all properties itself. kubectl explain is not very meaningful if we don't add descriptions.

❯ k explain osc.status.network.apiServerLoadBalancer GROUP: infrastructure.cluster.x-k8s.io KIND: OpenStackCluster VERSION: v1alpha5 FIELD: apiServerLoadBalancer <Object> DESCRIPTION: Be careful when using APIServerLoadBalancer, because this field is optional and therefore not set in all cases FIELDS: id <string> -required- <no description> internalIP <string> -required- <no description> ip <string> -required- <no description> name <string> -required- <no description>

For consistency I'm going to stick with Go type case for now. I thought we were obviously flouting a definite convention, which it seems we're not. As the cleanup is uncertain I'll stick with how most of them are currently defined.

mdbooth · 2023-06-19T16:32:58Z

This update rebases on to the latest main and updates some godocs as discussed above.

Renames the Network type to NetworkStatusWithSubnets, and embeds NetworkStatus. This is a pure Go change with no implications for the CRDs.

lentzi90

Looks good to me!
Leaving the hold for now so others have time to review
/lgtm

seanschneeweiss

/lgtm

seanschneeweiss · 2023-06-27T07:00:45Z

api/v1alpha7/openstackcluster_types.go

-	// It includes Subnets and Router.
-	Network *Network `json:"network,omitempty"`
+	// Network contains information about the created OpenStack Network.
+	Network *NetworkStatusWithSubnets `json:"network,omitempty"`

 	// externalNetwork contains information about the external network used for default ingress and egress traffic.


Not really part of your PR, would you mind changing that too?

Suggested change

// externalNetwork contains information about the external network used for default ingress and egress traffic.

// ExternalNetwork contains information about the external network used for default ingress and egress traffic.

I think we need a general godoc cleanup, tbh.

mdbooth · 2023-06-27T09:57:13Z

/hold cancel

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 1, 2023

k8s-ci-robot requested review from jichenjc and seanschneeweiss June 1, 2023 15:53

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 1, 2023

mdbooth marked this pull request as ready for review June 1, 2023 16:00

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 Jun 1, 2023

k8s-ci-robot requested a review from tobiasgiese June 1, 2023 16:00

k8s-ci-robot requested a review from dulek June 1, 2023 16:03

seanschneeweiss reviewed Jun 4, 2023

View reviewed changes

mdbooth commented Jun 7, 2023

View reviewed changes

tobiasgiese reviewed Jun 8, 2023

View reviewed changes

mdbooth force-pushed the networkstatus branch from a7afd9a to ec14e76 Compare June 19, 2023 16:32

mdbooth added 5 commits June 19, 2023 19:44

Move APIServerLoadBalancer and Router out of Network status

79cf28d

Document move of router and apiServerLoadBalancer

1011563

Rename Network type and embed NetworkStatus

d961508

Renames the Network type to NetworkStatusWithSubnets, and embeds NetworkStatus. This is a pure Go change with no implications for the CRDs.

Allow multiple subnets in network status

6912766

Document multiple subnets in status.network

802654e

mdbooth force-pushed the networkstatus branch from ec14e76 to 802654e Compare June 19, 2023 18:53

lentzi90 reviewed Jun 26, 2023

View reviewed changes

k8s-ci-robot assigned lentzi90 Jun 26, 2023

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

seanschneeweiss reviewed Jun 27, 2023

View reviewed changes

k8s-ci-robot assigned seanschneeweiss Jun 27, 2023

seanschneeweiss reviewed Jun 27, 2023

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 Jun 27, 2023

k8s-ci-robot merged commit f968786 into kubernetes-sigs:main Jun 27, 2023

matofeder mentioned this pull request Jul 7, 2023

Add ability to specify service and pod CIDRs SovereignCloudStack/k8s-cluster-api-provider#454

Merged

mdbooth mentioned this pull request Jul 13, 2023

kubectl describe reports empty subnet in v1alpha7 #1608

Closed

		@@ -157,7 +157,7 @@ type OpenStackClusterStatus struct {

		// Network contains all information about the created OpenStack Network.
		// It includes Subnets and Router.

	// externalNetwork contains information about the external network used for default ingress and egress traffic.
	// ExternalNetwork contains information about the external network used for default ingress and egress traffic.

⚠️ Update NetworkStatus ready for dual stack #1577

⚠️ Update NetworkStatus ready for dual stack #1577

Uh oh!

Conversation

mdbooth commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Jun 1, 2023

Uh oh!

netlify bot commented Jun 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

Uh oh!

k8s-ci-robot commented Jun 1, 2023

Uh oh!

mdbooth commented Jun 1, 2023

Uh oh!

k8s-ci-robot commented Jun 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanschneeweiss commented Jun 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tobiasgiese commented Jun 4, 2023

Uh oh!

mdbooth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdbooth commented Jun 19, 2023

Uh oh!

lentzi90 left a comment

Choose a reason for hiding this comment

Uh oh!

seanschneeweiss left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mdbooth commented Jun 27, 2023

Uh oh!

Uh oh!

mdbooth commented Jun 1, 2023 •

edited

Loading

netlify bot commented Jun 1, 2023 •

edited

Loading

seanschneeweiss commented Jun 4, 2023 •

edited

Loading