✨ Add OpenStackServerGroup CRD and Controller #1912

dalees · 2024-02-28T01:41:53Z

What this PR does / why we need it:

Implements new CRD for OpenstackServerGroup in v1beta1 to allow managed Server Groups with standard policies, and adds ServerGroupRef to OpenstackMachine that references the new CRD and uses it for VM creation.

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 #1256

Special notes for your reviewer:

This implements comment #1256 (comment)

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

TODOs:

squashed commits
includes documentation
adds unit tests
Rebased onto v1beta1 commit (removes v1alpha8)

k8s-ci-robot · 2024-02-28T01:42:03Z

Hi @dalees. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

netlify · 2024-02-28T01:42:12Z

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

Name	Link
🔨 Latest commit	`d8850da`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/66a045aeb8177600088ceb97
😎 Deploy Preview	https://deploy-preview-1912--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.

dulek

This looks pretty good, some remarks inline.

dulek · 2024-02-28T16:42:39Z

api/v1alpha8/openstackservergroup_types.go

+	// The name of the cloud to use from the clouds secret
+	// +optional
+	CloudName string `json:"cloudName"`


This seems a bit weird, we should probably have a reference to an OpenStackCluster instead?

Thank you for the feedback! Yeah, this allows the resource to be reconciled alone, as it's self contained.

However that isn't in any of the use cases, it doesn't seem a limitation to be tied to an existing OpenStackCluster even if the OpenStackServerGroup was only used for workers. It would remove duplication of these creds.

I'll make this change, once the CRD approach is agreed.

Hm, okay, that's a fair point. The use case to keep all the workers from different clusters in a single ServerGroup makes sense, I see your point.

In v1beta1 CloudName has moved into IdentifyRef, but this review thread is still relevant.

For now, I've chosen not to tie the OpenStackServerGroup to an OpenStackCluster reference and instead hold it's own secret reference. This matches how OpenStackMachine's are designed, and feels reasonable for this resource and avoids circular dependency.

I don't think there's much of a use case for two clusters using a single OpenStackServerGroup, that's not the design intention (but is possible with the current implementation).

api/v1alpha8/types.go

dulek · 2024-02-28T16:50:37Z

controllers/openstackmachine_controller.go

 	err = compute.ResolveReferencedMachineResources(scope, &openStackMachine.Spec, &openStackMachine.Status.ReferencedResources)
 	if err != nil {
 		return reconcile.Result{}, err
 	}

+	// Resolve referenced resources CAPO resources, using the K8s client
+	err = resolveReferencedClientResources(ctx, r.Client, openStackMachine)


It feels like it's still a Machine resource. Couldn't we put that into ResolveReferencedMachineResources directly? Even if we need to change the arguments of the function.

Hmm, I did start by doing this; I changed to this separation as what they're fetching from is distinct (OpenStack resource vs Kubernetes resource) and the client objects used are different. The OpenStack compute package just doesn't feel like the right place to be looking up K8s resources. It also makes test cases clearer to mock each function.

However, I agree the naming isn't clear. I wonder if renaming ResolveReferencedMachineResources to ResolveReferencedOpenStackResources may help to this end.

I'm open to changing this, but wanted to provide my reasoning first.

I see that, sure. Let's see what other reviewers will say here, especially @mdbooth as ResolveReferencedMachineResources() is an idea of his.

While rebased onto v1beta I have removed resolveReferencedClientResources and moved the code into the new resolveMachineResources.

controllers/openstackservergroup_controller.go

dulek · 2024-02-28T16:54:09Z

controllers/openstackservergroup_controller.go

+
+	serverGroupName := openStackServerGroup.Name
+
+	serverGroup, err := computeService.GetServerGroupByName(serverGroupName, false)


Again, we should probably lookup by ID first in case we have duplicate names.

agree
instead of GetServerGroupByName raising error in case of multiple server groups with the same name, i suggest to check openStackServerGroup.Status.ID, if it is nil, create a new server group

dalees · 2024-03-01T02:58:13Z

controllers/openstackmachine_controller.go

 	err = compute.ResolveReferencedMachineResources(scope, &openStackMachine.Spec, &openStackMachine.Status.ReferencedResources)
 	if err != nil {
 		return reconcile.Result{}, err
 	}

+	// Resolve referenced resources CAPO resources, using the K8s client
+	err = resolveReferencedClientResources(ctx, r.Client, openStackMachine)


Hmm, I did start by doing this; I changed to this separation as what they're fetching from is distinct (OpenStack resource vs Kubernetes resource) and the client objects used are different. The OpenStack compute package just doesn't feel like the right place to be looking up K8s resources. It also makes test cases clearer to mock each function.

However, I agree the naming isn't clear. I wonder if renaming ResolveReferencedMachineResources to ResolveReferencedOpenStackResources may help to this end.

I'm open to changing this, but wanted to provide my reasoning first.

pkg/cloud/services/compute/referenced_resources.go

controllers/openstackservergroup_controller.go

api/v1alpha8/types.go

jichenjc · 2024-03-04T08:05:22Z

/ok-to-test

mdbooth · 2024-03-08T18:17:28Z

@pierreprinetti We agreed this in principal this week. Pinging you because it's similar to something ORC would do.

chess-knight · 2024-05-22T11:15:13Z

Hi, at @SovereignCloudStack we are very interested in this feature. What is the progress here @dalees?

dalees · 2024-05-23T00:19:28Z

Hi, at @SovereignCloudStack we are very interested in this feature. What is the progress here @dalees?

Hello - pleased to hear of the interest! I'm keen to get this in, and I'm scheduled to revisit this in the next few weeks to get it back into a reviewable state.

k8s-ci-robot · 2024-06-18T01:10:31Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign vincepri 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

dalees · 2024-07-09T03:35:50Z

/unhold

dalees · 2024-07-15T07:56:31Z

This change is ready for review, when reviewers have the time :) cc @mdbooth

Implements new CRD for OpenstackServerGroup in v1beta1 to allow managed Server Groups with standard policies, and adds ServerGroupRef to OpenstackMachine that references the new CRD and uses it for VM creation. Closes: kubernetes-sigs#1256

chess-knight · 2024-08-08T11:52:18Z

Hi @dalees, thank you for pushing this PR. I am also kindly asking you and others about checking the OCCM host-id labelling issue kubernetes/cloud-provider-openstack#2579, what do you think about it? These two features can be nicely combined then, E.g. one can create an anti-affinity server group, and then check the host-id label of k8s nodes to ensure that nodes are distributed on different underlying hypervisors.

mnaser · 2024-08-14T00:05:18Z

@mdbooth this seems like a really good candidate for an ORC-style approach too?

k8s-ci-robot · 2024-08-14T00:05:26Z

PR needs rebase.

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.

okozachenko1203 · 2024-08-14T02:45:04Z

controllers/openstackserver_controller.go

+
+		// Store the resolved UUID, once it's ready and set.
+		if servergroup.Status.Ready && servergroup.Status.ID != "" {
+			resolved.ServerGroupID = servergroup.Status.ID


Suggested change

resolved.ServerGroupID = servergroup.Status.ID

resolved.ServerGroupID = servergroup.Status.ID

openStackServer.Status.Resolved = resolved

i think this is missing

okozachenko1203 · 2024-08-14T03:24:58Z

controllers/openstackservergroup_controller.go

+
+	serverGroupName := openStackServerGroup.Name
+
+	serverGroup, err := computeService.GetServerGroupByName(serverGroupName, false)


agree
instead of GetServerGroupByName raising error in case of multiple server groups with the same name, i suggest to check openStackServerGroup.Status.ID, if it is nil, create a new server group

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 Feb 28, 2024

k8s-ci-robot requested review from EmilienM and jichenjc February 28, 2024 01:41

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Feb 28, 2024

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 28, 2024

dalees mentioned this pull request Feb 28, 2024

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

Closed

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

dulek suggested changes Feb 28, 2024

View reviewed changes

dalees commented Mar 1, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 4, 2024

chess-knight mentioned this pull request May 22, 2024

Create v2 of node distribution standard (issues/#494) SovereignCloudStack/standards#524

Merged

robincron mentioned this pull request May 23, 2024

Make use of servergroups for cluster nodes vexxhost/magnum-cluster-api#375

Open

dalees force-pushed the crd_openstackservergroup branch from 65a96b7 to 1dd66f9 Compare June 18, 2024 01:10

dalees force-pushed the crd_openstackservergroup branch 3 times, most recently from 2519eb2 to 2df0503 Compare June 18, 2024 02:39

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 18, 2024

dalees force-pushed the crd_openstackservergroup branch from 2df0503 to db35504 Compare July 8, 2024 03:52

k8s-ci-robot removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 8, 2024

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 8, 2024

dalees force-pushed the crd_openstackservergroup branch from db35504 to 6ad654a Compare July 8, 2024 04:49

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

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

dalees force-pushed the crd_openstackservergroup branch from 6ad654a to 142a7c0 Compare July 23, 2024 23:47

Add OpenStackServerGroup CRD and Controller

d8850da

Implements new CRD for OpenstackServerGroup in v1beta1 to allow managed Server Groups with standard policies, and adds ServerGroupRef to OpenstackMachine that references the new CRD and uses it for VM creation. Closes: kubernetes-sigs#1256

dalees force-pushed the crd_openstackservergroup branch from 142a7c0 to d8850da Compare July 24, 2024 00:07

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

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

okozachenko1203 reviewed Aug 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add OpenStackServerGroup CRD and Controller #1912

✨ Add OpenStackServerGroup CRD and Controller #1912

dalees commented Feb 28, 2024 •

edited

Loading

k8s-ci-robot commented Feb 28, 2024

netlify bot commented Feb 28, 2024 •

edited

Loading

dulek left a comment

dulek Feb 28, 2024

dalees Feb 28, 2024 •

edited

Loading

dulek Feb 29, 2024

dalees Jun 18, 2024

dulek Feb 28, 2024

dalees Mar 1, 2024

dulek Mar 1, 2024

dalees Jun 18, 2024

dulek Feb 28, 2024

okozachenko1203 Aug 14, 2024

dalees Mar 1, 2024

jichenjc commented Mar 4, 2024

mdbooth commented Mar 8, 2024

chess-knight commented May 22, 2024

dalees commented May 23, 2024

k8s-ci-robot commented Jun 18, 2024

dalees commented Jul 9, 2024

dalees commented Jul 15, 2024

chess-knight commented Aug 8, 2024

mnaser commented Aug 14, 2024

k8s-ci-robot commented Aug 14, 2024

okozachenko1203 Aug 14, 2024

okozachenko1203 Aug 14, 2024

okozachenko1203 Aug 14, 2024


		serverGroupName := openStackServerGroup.Name

		serverGroup, err := computeService.GetServerGroupByName(serverGroupName, false)

	resolved.ServerGroupID = servergroup.Status.ID
	resolved.ServerGroupID = servergroup.Status.ID
	openStackServer.Status.Resolved = resolved

✨ Add OpenStackServerGroup CRD and Controller #1912

Are you sure you want to change the base?

✨ Add OpenStackServerGroup CRD and Controller #1912

Conversation

dalees commented Feb 28, 2024 • edited Loading

k8s-ci-robot commented Feb 28, 2024

netlify bot commented Feb 28, 2024 • edited Loading

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

dulek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalees Feb 28, 2024 • edited Loading

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

jichenjc commented Mar 4, 2024

mdbooth commented Mar 8, 2024

chess-knight commented May 22, 2024

dalees commented May 23, 2024

k8s-ci-robot commented Jun 18, 2024

dalees commented Jul 9, 2024

dalees commented Jul 15, 2024

chess-knight commented Aug 8, 2024

mnaser commented Aug 14, 2024

k8s-ci-robot commented Aug 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dalees commented Feb 28, 2024 •

edited

Loading

netlify bot commented Feb 28, 2024 •

edited

Loading

dalees Feb 28, 2024 •

edited

Loading