Skip to content

✨ New CRD + controller for OpenStackServer (v1alpha1)#2067

Merged
k8s-ci-robot merged 1 commit intokubernetes-sigs:mainfrom
shiftstack:OpenStackServer
Jul 18, 2024
Merged

✨ New CRD + controller for OpenStackServer (v1alpha1)#2067
k8s-ci-robot merged 1 commit intokubernetes-sigs:mainfrom
shiftstack:OpenStackServer

Conversation

@EmilienM
Copy link
Contributor

@EmilienM EmilienM commented May 7, 2024

What this PR does / why we need it:

A new CRD and controller which will manage the OpenStack servers in the cluster.

#2020
#2021

TODOs:

  • OpenStackServer API && make generate
  • openstackserver webook (MVP)
  • openstackserver_controller: create/delete/adopt instances & ports (MVP)
  • Migrating Bastion to use the new controller (MVP)
  • Migrating OpenStackMachine to the new controller (MVP)
  • Stop storing the bastion spec hash into the OpenStackCluster annotations and watch for changes in Bastion spec, where any change would trigger a bastionDelete.
  • Switch UserData as a ref
  • adds unit tests
  • includes documentation
  • squashed commits

Future work that can happen outside this PR:

  • Stop populating server related status fields for both OpenStackCluster and OpenStackMachine objects, since they now live in OpenStackServer. The fields will be removed for a potential v1.
  • Optimize SetupWithManager when watching for OpenStackServer, since we are only interested in certain state transition: when the server is deleted, and when the server is ready. For that we probably want to write Predicate functions.

@k8s-ci-robot 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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 7, 2024
@k8s-ci-robot k8s-ci-robot requested review from dulek and lentzi90 May 7, 2024 18:53
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 7, 2024
@netlify
Copy link

netlify bot commented May 7, 2024

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

Name Link
🔨 Latest commit fb3779c
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/66990e7fdf2bc80008aef505
😎 Deploy Preview https://deploy-preview-2067--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@EmilienM EmilienM force-pushed the OpenStackServer branch 2 times, most recently from 721f487 to 8c090fe Compare May 8, 2024 12:27
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 8, 2024
@EmilienM EmilienM force-pushed the OpenStackServer branch 2 times, most recently from ccbf0b6 to f081701 Compare May 8, 2024 20:44
@k8s-ci-robot k8s-ci-robot requested review from mdbooth and removed request for dulek May 8, 2024 20:44
@EmilienM EmilienM force-pushed the OpenStackServer branch 5 times, most recently from b80f256 to c608240 Compare May 9, 2024 20:11
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 9, 2024
@EmilienM EmilienM force-pushed the OpenStackServer branch 3 times, most recently from b5c3f50 to cd77e8a Compare May 15, 2024 17:01
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 15, 2024
@EmilienM EmilienM force-pushed the OpenStackServer branch 2 times, most recently from 18a83a6 to 0354264 Compare May 15, 2024 19:36
@EmilienM EmilienM force-pushed the OpenStackServer branch 3 times, most recently from 9f8ea24 to 2356928 Compare May 30, 2024 19:33
wantRemoveFinalizer bool
clusterNotReady bool
}{
{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I translated all these tests into openstackserver_controller.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any validation we can't do with CEL? If not, lets delete this.

Copy link
Contributor Author

@EmilienM EmilienM Jun 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the only validation we make here is to ensure immutability. I don't know enough CEL to know if it can check that. However I'll investigate. However I'm not sure if CEL recognize TopologyDryRunAnnotation (link).
I checked other providers and they do that via the webhook.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets put all of these in the apivalidations suite instead. That way they will still work when we implement them with CEL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll put that into a separate commit for now as I'm not sure whether we want to keep the same validations in OpenStackMachine (?).

@EmilienM
Copy link
Contributor Author

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

e2e jobs passed, I want to see if they pass again a few times to ensure I didn't miss any race.

@EmilienM
Copy link
Contributor Author

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

@EmilienM
Copy link
Contributor Author

Two successful CI jobs in a row. Let's try a third and stop the rechecks, letting others to review.

@EmilienM
Copy link
Contributor Author

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

2 similar comments
@EmilienM
Copy link
Contributor Author

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

@EmilienM
Copy link
Contributor Author

EmilienM commented Jul 2, 2024

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

Copy link
Contributor

@mdbooth mdbooth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've looked through OS Server and OS Machine.

OS Server looks fine except for the unused IPAM field in the spec. I think there are a few things to address in OS Machine.

2 high level things to consider:

  • Should OS Server support IPAM?
  • Should OS Machine pass fully resolved resources to OS Server?

My personal take on these is:

OS Server should support IPAM. i.e. Leave FloatingIPPoolRef in OS Server spec and add support for it. Remove that support from OS Machine and instead just copy the field from OS Machine spec to OS Server spec.

OS Machine should not fully resolve resources, otherwise we're just duplicating this behaviour between OS Server and OS Machine. i.e. Remove everything from OS Machine that populates or reads resolved or adopted resources.

@EmilienM
Copy link
Contributor Author

EmilienM commented Jul 2, 2024

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

3 similar comments
@EmilienM
Copy link
Contributor Author

EmilienM commented Jul 3, 2024

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

@EmilienM
Copy link
Contributor Author

EmilienM commented Jul 3, 2024

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

@EmilienM
Copy link
Contributor Author

EmilienM commented Jul 3, 2024

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

@EmilienM
Copy link
Contributor Author

Just a (clean) rebase on main.

@EmilienM
Copy link
Contributor Author

/test pull-cluster-api-provider-openstack-e2e-full-test pull-cluster-api-provider-openstack-e2e-test
for sanity, this is ready to merge

@mdbooth
Copy link
Contributor

mdbooth commented Jul 18, 2024

/lgtm
/approve

Let's do this!

@k8s-ci-robot
Copy link
Contributor

[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

Details Needs approval from an approver in each of these files:

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants