✨ New API for Server Groups #1779

stephenfin · 2023-12-07T15:27:28Z

What this PR does / why we need it:

Allow users to (a) specify server groups by name or (b) defer management of server groups to CAPO.

Note

This is a proposal to allow me test out ideas before writing a KEP. It's
intentionally incomplete so we can focus on the API first.

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

n/a

Special notes for your reviewer:

n/a

TODOs:

Implement non-API code
Run make generate
Propose KEP?

/hold

k8s-ci-robot · 2023-12-07T15:27:31Z

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-12-07T15:27:34Z

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

Name	Link
🔨 Latest commit	`32369b6`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/659be64885ff190008ccb5ab
😎 Deploy Preview	https://deploy-preview-1779--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 configuration.

api/v1alpha7/openstackmachine_types.go

api/v1alpha7/types.go

api/v1alpha7/managed_server_groups.go

EmilienM · 2023-12-07T18:27:31Z

/cc EmilienM

jichenjc · 2023-12-13T07:12:33Z

pkg/cloud/services/compute/instance.go

@@ -562,8 +568,18 @@ func applyServerGroupID(opts servers.CreateOptsBuilder, serverGroupID string) se
 	return opts
 }

-// Helper function for getting image id from name.
-func (s *Service) getImageIDFromName(imageName string) (string, error) {
+// Helper function for getting image ID from name or ID.


Helper function for getting image ID from name or ID.

this seems confusing

I agree 😄 However, despite what GitHub says I didn't write this. This is being picked up as a moved line when in fact I simply renamed the function from getImageID to GetImageID and folded getImageIDFromName into this. It's also technically true.

jichenjc · 2023-12-13T07:17:54Z

pkg/cloud/services/compute/instance.go

+		if err != nil {
+			return "", err
+		}
+		if serverGroupFilter.Name != "" && serverGroupFilter.Name != serverGroup.Name {


should we really have this check? or just safely ignore the Name ? I think the logic here want to make sure they are consistent..

So while I think this is silly check and I think providing both a name and ID is even sillier, but if someone does it then I think we should respect their request fully. To not do so would be confusing.

I'm with @jichenjc on this one. Can we not just add an admission validation that if you specify ID you can't specify anything else?

If the machine creation fails because the specified ID was invalid that should be reported in the machine status.

mdbooth

I think this is mostly there. I'd like to:

Rebase on to v1alpha8
Consequently, take the opportunity the remove the ServerGroupID field
Remove changes to ImageID in status unless I'm missing some reason it's required for Server Groups (it's good but I'd prefer a separate PR)
Remove additional validation of SecurityGroup specified by ID

mdbooth · 2023-12-13T17:33:48Z

api/v1alpha7/openstackmachine_types.go

+	// ImageID contains the ID of the image this machine was created with.
+	ImageID string `json:"imageID,omitempty"`


Why are we also adding ImageID here? It seems like a good addition, but does it need to happen in this PR?

mdbooth · 2023-12-13T17:39:16Z

pkg/cloud/services/compute/instance.go

+		if err != nil {
+			return "", err
+		}
+		if serverGroupFilter.Name != "" && serverGroupFilter.Name != serverGroup.Name {


I'm with @jichenjc on this one. Can we not just add an admission validation that if you specify ID you can't specify anything else?

If the machine creation fails because the specified ID was invalid that should be reported in the machine status.

dulek · 2023-12-19T10:24:09Z

I tried to implement Matt's remarks based on his own version of this code. I think I'm still failing to get the conversions or the conversions tests right, will continue on that.

EmilienM · 2023-12-19T14:07:34Z

api/v1alpha8/openstackmachine_types.go

-	ServerGroupID string `json:"serverGroupID,omitempty"`
+	// The server group to assign the machine to.
+	// +optional
+	ServerGroup *ServerGroupFilter `json:"serverGroup,omitempty"`


The openstackmachine controller tests need an update, see https://github.com/shiftstack/cluster-api-provider-openstack/blob/3eb70ea4ad246bc5bacf388c18287015fd15e915/controllers/openstackmachine_controller_test.go#L94

EmilienM

beside a few nits which can be fixed later, I think this is good to go (feel free to do it in this PR though)

/lgtm

EmilienM · 2023-12-22T14:54:25Z

/hold cancel

stephenfin · 2024-01-03T15:57:13Z

docs/book/src/topics/crd-changes/v1alpha7-to-v1alpha8.md

+
+### `OpenStackMachine`
+
+#### Change to `serverGroupID`


Suggested change

#### Change to `serverGroupID`

#### ⚠️ Change to `serverGroupID`

docs/book/src/topics/crd-changes/v1alpha7-to-v1alpha8.md

mdbooth

There's something wrong with the API conversions, but the rest of it lgtm.

pkg/cloud/services/compute/referenced_resources_test.go

pkg/cloud/services/compute/servergroup_test.go

mdbooth · 2024-01-03T16:12:43Z

pkg/cloud/services/compute/referenced_resources_test.go

@@ -0,0 +1,99 @@
+/*
+Copyright 2018 The Kubernetes Authors.


Suggested change

Copyright 2018 The Kubernetes Authors.

Copyright 2024 The Kubernetes Authors.

mdbooth · 2024-01-03T16:21:20Z

api/v1alpha7/conversion.go

+	if in.ServerGroupID != "" {
+		out.ServerGroup = &infrav1.ServerGroupFilter{ID: in.ServerGroupID}
+	} else {
+		out.ServerGroup = &infrav1.ServerGroupFilter{}


This isn't necessary. You don't need to set ServerGroup in this case: it can safely be nil.

Somehow tests fail when this is set. I have hard time debugging these, I could use pairing with someone who would teach me how to debug conversion tests failures.

mdbooth · 2024-01-03T16:25:24Z

api/v1alpha7/conversion_test.go

+			func(v1alpha8OpenStackMachine *infrav1.OpenStackMachine, c fuzz.Continue) {
+				c.FuzzNoCustom(v1alpha8OpenStackMachine)
+
+				// None of the following fields have ever been set in v1alpha7


This is v1alpha8, though, and they are set in v1alpha8.

mdbooth · 2024-01-03T16:26:43Z

api/v1alpha7/conversion_test.go

+
+			func(v1alpha8OpenStackMachine *infrav1.OpenStackMachine, c fuzz.Continue) {
+				c.FuzzNoCustom(v1alpha8OpenStackMachine)
+
+				// None of the following fields have ever been set in v1alpha7
+				v1alpha8OpenStackMachine.Status.ReferencedResources = infrav1.ReferencedMachineResources{}
+			},
+
+			func(v1alpha8OpenStackCluster *infrav1.OpenStackCluster, c fuzz.Continue) {
+				c.FuzzNoCustom(v1alpha8OpenStackCluster)
+
+				// None of the following fields have ever been set in v1alpha7
+				if v1alpha8OpenStackCluster.Status.Bastion != nil {
+					v1alpha8OpenStackCluster.Status.Bastion.ReferencedResources = infrav1.ReferencedMachineResources{}
+				}
+			},
+
+			func(v1alpha8ServerGroupFilter *infrav1.ServerGroupFilter, c fuzz.Continue) {
+				c.FuzzNoCustom(v1alpha8ServerGroupFilter)
+
+				// None of the following fields have ever been set in v1alpha7
+				v1alpha8ServerGroupFilter.Name = ""
+			},


This doesn't look right to me. We should support round-trip conversion of all of these new fields without constraining the fuzzer.

For reference, @mdbooth found a problem in the conversion functions: RestoreField must be provided to a HashedFieldRestorer. AFIK he's working on a patch now to address this so we can apply that in ongoing PRs where I wrote these unwanted fuzzer functions.

dulek · 2024-01-04T12:54:23Z

I addressed the docs and comments issue. I'm not sure how to debug the conversions though.

EmilienM · 2024-01-05T14:53:22Z

I've addressed the conversion stuff.

EmilienM · 2024-01-05T16:40:47Z

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

mdbooth · 2024-01-08T09:46:24Z

api/v1alpha7/conversion.go

+	} else {
+		out.ServerGroup = nil
+	}


nit: This can be omitted.

mdbooth · 2024-01-08T09:47:21Z

api/v1alpha7/conversion.go

+}
+
+func Convert_v1alpha8_OpenStackMachineStatus_To_v1alpha7_OpenStackMachineStatus(in *infrav1.OpenStackMachineStatus, out *OpenStackMachineStatus, s apiconversion.Scope) error {
+	// ReferencedResources have no equivalent in v1alpha7


Suggested change

// ReferencedResources have no equivalent in v1alpha7

// ReferencedResources has no equivalent in v1alpha7

mdbooth · 2024-01-08T09:47:33Z

api/v1alpha7/conversion.go

+}
+
+func Convert_v1alpha8_BastionStatus_To_v1alpha7_BastionStatus(in *infrav1.BastionStatus, out *BastionStatus, s apiconversion.Scope) error {
+	// ReferencedResources have no equivalent in v1alpha7


Suggested change

// ReferencedResources have no equivalent in v1alpha7

// ReferencedResources has no equivalent in v1alpha7

mdbooth · 2024-01-08T09:47:54Z

api/v1alpha7/conversion.go

+	} else {
+		out.Instance.ServerGroup = nil
+	}


nit: This can be omitted.

mdbooth · 2024-01-08T10:59:27Z

api/v1alpha7/conversion_test.go

+
+			func(v1alpha8OpenStackMachine *infrav1.OpenStackMachine, c fuzz.Continue) {
+				c.FuzzNoCustom(v1alpha8OpenStackMachine)
+
+				v1alpha8OpenStackMachine.Status.ReferencedResources = infrav1.ReferencedMachineResources{}
+			},
+
+			func(v1alpha8OpenStackCluster *infrav1.OpenStackCluster, c fuzz.Continue) {
+				c.FuzzNoCustom(v1alpha8OpenStackCluster)
+
+				// None of the following fields have ever been set in v1alpha7
+				if v1alpha8OpenStackCluster.Status.Bastion != nil {
+					v1alpha8OpenStackCluster.Status.Bastion.ReferencedResources = infrav1.ReferencedMachineResources{}
+				}
+			},
+
+			func(v1alpha8ServerGroupFilter *infrav1.ServerGroupFilter, c fuzz.Continue) {
+				c.FuzzNoCustom(v1alpha8ServerGroupFilter)
+
+				// None of the following fields have ever been set in v1alpha7
+				v1alpha8ServerGroupFilter.Name = ""
+			},


None of these should be required. I'll debug.

Allow users to request Server Groups by either name or ID. This aligns us with the pattern of filters used for other resources such as Security Groups and Ports. Additionally `ReferencedResources` field is added to `BastionStatus` and `OpenStackMachineStatus` as a container to hold all the IDs of resources computed by the controllers. `ServerGroupID` is the first field added there. Co-Authored-By: Emilien Macchi <emilien@redhat.com> Co-Authored-By: Michał Dulko <mdulko@redhat.com> Co-Authored-By: Stephen Finucane <stephenfin@redhat.com>

mdbooth · 2024-01-08T12:17:14Z

I've pushed a new version which makes changes only to the conversion code. It:

Removes all the fuzzer test workarounds in v1alpha7
Adds a missing restorer for ReferencedMachineResources in v1alpha7 (which was why the workarounds were required)
Updates the way we store ServerGroupID in v1alpha6

The last one is interesting. In v1alpha6 Instance had an unused ServerGroupID field which was removed in v1alpha7. We were storing the ServerGroupID in there. This works well for ServerGroup, but I think we're about to add more fields to this struct. As this trick is only likely to work for ServerGroupID I think it'll end up being more confusing than it's worth, so I've switched it to just doing a regular restore.

EmilienM

/lgtm

mdbooth · 2024-01-08T13:41:53Z

/approve

k8s-ci-robot · 2024-01-08T13:42:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: EmilienM, mdbooth, stephenfin

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

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

k8s-ci-robot requested review from mdbooth and seanschneeweiss December 7, 2023 15:27

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 7, 2023

mdbooth reviewed Dec 7, 2023

View reviewed changes

api/v1alpha7/openstackmachine_types.go Outdated Show resolved Hide resolved

api/v1alpha7/openstackmachine_types.go Outdated Show resolved Hide resolved

api/v1alpha7/types.go Outdated Show resolved Hide resolved

api/v1alpha7/managed_server_groups.go Outdated Show resolved Hide resolved

stephenfin force-pushed the better-server-groups branch from 7e58141 to 24cda28 Compare December 7, 2023 17:13

k8s-ci-robot requested a review from EmilienM December 7, 2023 18:27

stephenfin force-pushed the better-server-groups branch from 24cda28 to 4041278 Compare December 8, 2023 14:17

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 Dec 8, 2023

stephenfin force-pushed the better-server-groups branch 3 times, most recently from 3155371 to f6d6a88 Compare December 8, 2023 16:59

stephenfin marked this pull request as ready for review December 8, 2023 17:31

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 Dec 8, 2023

k8s-ci-robot requested a review from mdbooth December 8, 2023 17:31

jichenjc reviewed Dec 13, 2023

View reviewed changes

mdbooth reviewed Dec 13, 2023

View reviewed changes

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

dulek force-pushed the better-server-groups branch from f6d6a88 to 3eb70ea Compare December 19, 2023 10:21

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 19, 2023

EmilienM reviewed Dec 19, 2023

View reviewed changes

EmilienM approved these changes Dec 22, 2023

View reviewed changes

k8s-ci-robot assigned EmilienM Dec 22, 2023

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

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

stephenfin commented Jan 3, 2024

View reviewed changes

mdbooth reviewed Jan 3, 2024

View reviewed changes

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

mdbooth mentioned this pull request Jan 4, 2024

🌱 Improve method signatures of conversion functions #1813

Merged

EmilienM force-pushed the better-server-groups branch from 9252f66 to f74831f Compare January 5, 2024 14:42

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

EmilienM force-pushed the better-server-groups branch from f74831f to 7e0ee45 Compare January 5, 2024 14:51

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

EmilienM force-pushed the better-server-groups branch from 7e0ee45 to 9b4858e Compare January 5, 2024 15:51

mdbooth reviewed Jan 8, 2024

View reviewed changes

mdbooth and others added 2 commits January 8, 2024 11:50

conversion: Allow getField to return nil in UnconditionalFieldRestorer

7bd42ab

mdbooth force-pushed the better-server-groups branch from 9b4858e to 32369b6 Compare January 8, 2024 12:10

EmilienM approved these changes Jan 8, 2024

View reviewed changes

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

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 8, 2024

k8s-ci-robot merged commit 5f306c3 into kubernetes-sigs:main Jan 8, 2024
9 checks passed

EmilienM deleted the better-server-groups branch January 8, 2024 13:44

dalees mentioned this pull request Feb 28, 2024

Use a server group to ensure anti-affinity for control plane nodes #1256

Open

		// ImageID contains the ID of the image this machine was created with.
		ImageID string `json:"imageID,omitempty"`

	#### Change to `serverGroupID`
	#### ⚠️ Change to `serverGroupID`

	Copyright 2018 The Kubernetes Authors.
	Copyright 2024 The Kubernetes Authors.

	// ReferencedResources have no equivalent in v1alpha7
	// ReferencedResources has no equivalent in v1alpha7

✨ New API for Server Groups #1779

✨ New API for Server Groups #1779

Conversation

stephenfin commented Dec 7, 2023 • edited

k8s-ci-robot commented Dec 7, 2023

netlify bot commented Dec 7, 2023 • edited

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

EmilienM commented Dec 7, 2023

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

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

dulek commented Dec 19, 2023

EmilienM Dec 19, 2023 • edited

Choose a reason for hiding this comment

EmilienM left a comment

Choose a reason for hiding this comment

EmilienM commented Dec 22, 2023

Choose a reason for hiding this comment

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

dulek commented Jan 4, 2024

EmilienM commented Jan 5, 2024

EmilienM commented Jan 5, 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

mdbooth commented Jan 8, 2024

EmilienM left a comment

Choose a reason for hiding this comment

mdbooth commented Jan 8, 2024

k8s-ci-robot commented Jan 8, 2024

stephenfin commented Dec 7, 2023 •

edited

netlify bot commented Dec 7, 2023 •

edited

EmilienM Dec 19, 2023 •

edited