Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

✨ Add NamePrefix field to OpenStackMachine.Spec #2035

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

✨ Add NamePrefix field to OpenStackMachine.Spec #2035

wants to merge 1 commit into from

Conversation

lentzi90
Copy link
Contributor

@lentzi90 lentzi90 commented Apr 24, 2024 •
edited

What this PR does / why we need it:

This adds a new field to the spec of OpenStackMachines: namePrefix. It is an optional field that can be specified as a way to influence how OpenStack resources are named. If it is not specified, the OpenStackMachine name will be used (current behavior).

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 #2039, related to kubernetes-sigs/cluster-api#10463.

Special notes for your reviewer:

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

TODOs:

  • squashed commits
  • if necessary:
    • includes documentation
    • adds unit tests

/hold

@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. labels Apr 24, 2024
@k8s-ci-robot
Copy link
Contributor

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

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 24, 2024
@k8s-ci-robot
Copy link
Contributor

[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 ask for approval from lentzi90. 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:

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

@netlify Netlify
Copy link

netlify bot commented Apr 24, 2024 •
edited

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

Name Link
🔨 Latest commit 8ce8116
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/662ba33330a00f00085446a7
😎 Deploy Preview https://deploy-preview-2035--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.

@mdbooth
Copy link
Contributor

mdbooth commented Apr 24, 2024

Unfortunately I don't have time to go into this deeply today, but my first impression is that I'm not keen.

It's worth noting that I'm generally supportive of the new CAPI default. It has always been a trip hazard that the Machine and InfraMachine have different names, and making them the same should be simpler for most people.

However, as you point out in the CAPI issue, it has been this way long enough that I think it's reasonable to treat it like a breaking change in practise, regardless of whether it was documented. I hope we can work with the CAPI folks to sort that out.

That said, this change would introduce that same trip hazard in CAPO, just moved one level down. The idea that cloud resources are named after the OpenStackMachine which owns them is simple and elegant. I'm not keen to break it, especially now that we've released v1beta1 and would be left supporting it indefinitely.

My strong preference is to work with CAPI to enable backwards-compatibility for the inadvertent breakage. I took a brief look and the code to support the previous behaviour is still there. I think this can and should be implemented by adding a backwards-compatibility flag (optional, defaults to the new behaviour) to CAPI.

@lentzi90
Copy link
Contributor Author

Thank you for the comment @mdbooth ! I was reluctant to push this to begin with. Now I have a better understanding of the issue and the use-case. Based on that I have created #2039.
It is unfortunately a feature that we really need. We just didn't realize until the breaking change in CAPI.

Could you please take a look at the issue and see if this makes sense to you? I'm very open to suggestions for how exactly we should implement this!

@lentzi90 lentzi90 force-pushed the lentzi90/instance-name-prefix branch from 1b55b32 to 1782d5a Compare April 26, 2024 10:30
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Apr 26, 2024
@lentzi90 lentzi90 marked this pull request as ready for review April 26, 2024 10:31
@k8s-ci-robot 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 Apr 26, 2024
lentzi90
lentzi90 commented Apr 26, 2024
View reviewed changes
api/v1beta1/openstackmachine_types.go
// NamePrefix is an optional string that will be used as a prefix for the name of OpenStack
// resources created based on this OpenStackMachine. A random suffix will be added to the prefix.
// If omitted, the name of the OpenStackMachine will be used as name.
// +kubebuilder:validation:MaxLength:=255
Copy link
Contributor Author

@lentzi90 lentzi90 Apr 26, 2024 •
edited

Choose a reason for hiding this comment

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

Can we do better here? Are there any specific requirements on names in OpenStack that we could include?

@lentzi90 lentzi90 force-pushed the lentzi90/instance-name-prefix branch from 1782d5a to 6b61809 Compare April 26, 2024 10:46
@lentzi90
Add NamePrefix field to OpenStackMachine.Spec
8ce8116 
This adds a new field to the spec of OpenStackMachines: `namePrefix`. It
is an optional field that can be specified as a way to influence how
OpenStack resources are named. If it is not specified, the
OpenStackMachine name will be used (current behavior).

Signed-off-by: Lennart Jern <[email protected]>
@lentzi90 lentzi90 force-pushed the lentzi90/instance-name-prefix branch from 6b61809 to 8ce8116 Compare April 26, 2024 12:50
@mdbooth
Copy link
Contributor

mdbooth commented Apr 26, 2024

It looks like you're making good progress in kubernetes-sigs/cluster-api#10511. I think this PR would be redundant if we're successful getting this fixed in CAPI.

Can we hold fire on this to see how that goes? I'd very much prefer to see this fixed in CAPI rather than carry our own custom workaround.

@lentzi90
Copy link
Contributor Author

Sure, there is no hurry with this. However, kubernetes-sigs/cluster-api#10511 will only provide a temporary solution. It is targeting only the release branch and will be gone by CAPI v1.8

@mdbooth mdbooth mentioned this pull request Apr 29, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jichenjc jichenjc Awaiting requested review from jichenjc

@mdbooth mdbooth Awaiting requested review from mdbooth

@EmilienM EmilienM Awaiting requested review from EmilienM

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
CAPO Roadmap
Status: Inbox
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Name prefix for OpenStackMachine related resources
3 participants
@lentzi90 @k8s-ci-robot @mdbooth