OSDOCS-14472: CPMS custom machine name prefixes #93119
Conversation
|
@skopacz1: This pull request references OSDOCS-14472 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
size/M
|
🤖 Mon May 19 17:17:22 - Prow CI generated the docs preview: |
|
@skopacz1: This pull request references OSDOCS-14472 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
I added a really generic (and perhaps not very useful) intro to the new section below. Please feel free to suggest any other wording.
modules/cpmso-config-options.adoc
Outdated
|
|
||
| . Save your changes to the `ControlPlaneMachineSet` CR. | ||
|
|
||
| . If you are using an `OnDelete` update strategy for your control plane machine set: Manually apply the prefix to the name of any existing machines. |
There was a problem hiding this comment.
If there are verification steps to add, please let me know.
For example, what command to run to check machine names, and example output of that command
modules/cpmso-config-options.adoc
Outdated
| ** The name contains no more than 253 characters | ||
| ** The name contains only lowercase alphanumeric characters, `-`, or `.` | ||
| ** The name starts and ends with an alphanumeric character | ||
|
|
There was a problem hiding this comment.
The validation needs some modification. Please have a look at https://github.com/openshift/api/blob/7318813e48da641a8b2e068018a3723a5cc34d5e/machine/v1/types_controlplanemachineset.go#L45-L61 details.
// It must be a lowercase RFC 1123 subdomain, consisting of lowercase
// alphanumeric characters, hyphens ('-'), and periods ('.').
// Each block, separated by periods, must start and end with an alphanumeric character.
// Hyphens are not allowed at the start or end of a block, and consecutive periods are not permitted.
// The prefix must be between 1 and 245 characters in length.There was a problem hiding this comment.
I will rewrite this to basically say exactly what this text says is a requirement
modules/cpmso-config-options.adoc
Outdated
|
|
||
| . Save your changes to the `ControlPlaneMachineSet` CR. | ||
|
|
||
| . If you are using an `OnDelete` update strategy for your control plane machine set: Manually apply the prefix to the name of any existing machines. |
There was a problem hiding this comment.
Manually apply the prefix to the name of any existing machines.
I don't think there is any different way to apply the prefix for OnDelete or RollingUpdate update strategy. In both the cases, we need to modify .spec.machineNamePrefix field.
- For
OnDelete: We need to delete an existing machine; then only a new replacement machine will be created with the prefixed name. - For
RollingUpdate: We need to force a rollout, maybe by adding some labels to the machine template metadata.
/cc @huali9, could you please help with some user-facing commands and outputs which can be added as an example? TiA!
There was a problem hiding this comment.
For both OnDelete and RollingUpdate, we can delete an existing machine, then a new replacement machine will be created with the prefixed name.
For RollingUpdate, we can also add some labels to the master machine template metadata to force a rollout. for example, under a master machine's .spec.providerSpec.value.metadata, add a label like
metadata:
labels:
old: old
then a new replacement machine will be created with the prefixed name.
There was a problem hiding this comment.
Do we want to add this extra step to add labels, specifically for people using RollingUpdate? Or should we just tell users to delete their old machines no matter which strategy they're using, and assume that people who use RollingUpdate should already be familiar with forcing rollouts?
I'm happy to document whichever option you guys prefer!
There was a problem hiding this comment.
Do we want to add this extra step to add labels, specifically for people using
RollingUpdate? Or should we just tell users to delete their old machines no matter which strategy they're using, and assume that people who useRollingUpdateshould already be familiar with forcing rollouts?
I prefer the latter. Because even for adding labels when people using RollingUpdate, it's the same work as deleting old machines, people have to do that one by one manually, and have to wait until a new master machine get Running and the old master machine disappeared, and then do the same operation on the next old master machine.
There was a problem hiding this comment.
In the case of RollingUpdate strategy, any trigger (for example, adding a label in the providerSpec of ControlPlaneMachineSet) should replace the old machines one by one without manually removing them.
However, in the case of OnDelete strategy, replacement machines will only be created in the event of the deletion of the existing machine.
There was a problem hiding this comment.
@huali9 and @chiragkyal I would vote that we use the same construct I have elsewhere. We typically don't specifically tell users to delete a machine if they don't have to.
My reasoning here is that I think after you save the config, you should be prepared to see one of the following behaviors:
RollingUpdate(default): CMPS goes off and updates the machines. By saying this, users aren't surprised to see it doing something if they didn't know/forgot how CPMS is configured on the cluster.OnDelete: Nothing happens w/o your action. Again, no surprises for the user, and tells them how to make it change the machines.
It might also be that the user wants to make some other changes, so they might do a few things and THEN delete machines to kick it off. So again I don't wanna say "you must do this right now" since that's not true as far as I understand.
There was a problem hiding this comment.
Hi @jeana-redhat Your suggestion basically LGTM.
But it should be noted that even though it is RollingUpdate, only changing machineNamePrefix will not trigger an update. Other changes are required to trigger an update. Like @chiragkyal mentioned adding a label in the providerSpec of ControlPlaneMachineSet should replace the old machines one by one without manually removing them. But if users don't want to change other values in the ControlPlaneMachineSet except machineNamePrefix, then they need to delete the old master machines one by one or add labels to the old master machines one by one to trigger the replacement.
Refer https://issues.redhat.com/browse/OAPE-22?focusedId=26363185&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-26363185
modules/cpmso-config-options.adoc
Outdated
| -n openshift-machine-api | ||
| ---- | ||
|
|
||
| . Add the following snippet to the `ControlPlaneMachineSet` CR: |
There was a problem hiding this comment.
Is this a snippet? I am not sure if this the right wording here. The intention would be to edit .spec.machineNamePrefix field of ControlPlaneMachineSet CR.
There was a problem hiding this comment.
Will the .spec.machineNamePrefix field already be specified in all ControlPlaneMachineSet CRs for 4.19+ clusters, and users are free to change the value from the default?
Or do users need to add the field to the CR the first time they are trying to customize the prefix?
For now I will change the wording to match what you are suggesting. But if they need to add this field the first time, maybe it should be worded like "add the .spec.machineNamePrefix field to the ControlPlaneMachineSet CR."
There was a problem hiding this comment.
Will the
.spec.machineNamePrefixfield already be specified in allControlPlaneMachineSetCRs for 4.19+ clusters, and users are free to change the value from the default?
No, there is no .spec.machineNamePrefix field in ControlPlaneMachineSet CRs by default.
Or do users need to add the field to the CR the first time they are trying to customize the prefix?
So right, users need to add the field to the CR the first time they are trying to customize the prefix.
There was a problem hiding this comment.
Technically, .spec.machineNamePrefix field is added in the API by default for 4.19+ (oc explain command can help here). Since the type of the field is string; it's defaulted to empty string and not showing up in the yaml.
So I am fine using whichever wording makes sense: add or edit
modules/cpmso-config-options.adoc
Outdated
| = Adding a custom prefix to control plane machine names | ||
|
|
||
| You can customize the prefix of machine names created by a control plane machine set. | ||
| This can be done by editing a control plane machine set custom resource (CR) |
There was a problem hiding this comment.
| This can be done by editing a control plane machine set custom resource (CR) | |
| This can be done by editing the control plane machine set custom resource (CR) |
modules/cpmso-config-options.adoc
Outdated
| [id="cpmso-config-prefix_{context}"] | ||
| = Adding a custom prefix to control plane machine names | ||
|
|
||
| You can customize the prefix of machine names created by a control plane machine set. |
There was a problem hiding this comment.
| You can customize the prefix of machine names created by a control plane machine set. | |
| You can customize the prefix of machine names created by the control plane machine set. |
modules/cpmso-config-options.adoc
Outdated
|
|
||
| // Saw the step below in another procedure, let me know if it's good to keep it in here. | ||
|
|
||
| . Edit your control plane machine set CR by running the following command: |
There was a problem hiding this comment.
Which one is the right way to write this?
- control plane machine set
- control plane machine set CR
- control plane machine set custom resource (CR)
ControlPlaneMachineSetCR
I am seeing all these are being used interchangeably.
There was a problem hiding this comment.
I am covering this feature for @jeana-redhat so I will let them weigh in (or tag anyone else who might weigh in). I am happy to go through this PR and change any variations in my added docs to any single term.
There was a problem hiding this comment.
Please cross-verify within your team for the right usage here.
There was a problem hiding this comment.
The "control plane machine set" is defined by a "control plane machine set custom resource (CR)" which has the kind value of ControlPlaneMachineSet. So, in answer to the list above:
- control plane machine set
- this is valid, but here we are talking about editing the YAML CR so I would probably lean towards one of the others.
- control plane machine set CR
- this is valid and works here. In the repo, I see it 36 times.
- control plane machine set custom resource (CR)
- this is just how you are always supposed to write "CR" on first appearance. Once you define an abbreviation, you don't keep doing so. This is correct for line 10 above where it appears, afterwards it should be CR.
ControlPlaneMachineSetCR- this is valid and works here. In the repo, I see it 32 times
So, apparently I have been pretty evenly inconsistent on using "control plane machine set" and "ControlPlaneMachineSet" in front of custom resource/CR. For tangentially related docs reasons, I think we should prefer to write it as ControlPlaneMachineSet to avoid confusion with a "compute machine set" which is just MachineSet. I'll leave comments on this PR with my recommendations.
I will sweep the repo to change "control plane machine set CR" to "ControlPlaneMachineSet CR" at some point.
modules/cpmso-config-options.adoc
Outdated
|
|
||
| .Procedure | ||
|
|
||
| // Saw the step below in another procedure, let me know if it's good to keep it in here. |
There was a problem hiding this comment.
If you think the step below is valid to keep (which I am guessing is the case), then I'll go ahead and delete this comment.
|
/cc @huali9 |
modules/cpmso-config-options.adoc
Outdated
|
|
||
| // Saw the step below in another procedure, let me know if it's good to keep it in here. | ||
|
|
||
| . Edit your control plane machine set CR by running the following command: |
There was a problem hiding this comment.
The "control plane machine set" is defined by a "control plane machine set custom resource (CR)" which has the kind value of ControlPlaneMachineSet. So, in answer to the list above:
- control plane machine set
- this is valid, but here we are talking about editing the YAML CR so I would probably lean towards one of the others.
- control plane machine set CR
- this is valid and works here. In the repo, I see it 36 times.
- control plane machine set custom resource (CR)
- this is just how you are always supposed to write "CR" on first appearance. Once you define an abbreviation, you don't keep doing so. This is correct for line 10 above where it appears, afterwards it should be CR.
ControlPlaneMachineSetCR- this is valid and works here. In the repo, I see it 32 times
So, apparently I have been pretty evenly inconsistent on using "control plane machine set" and "ControlPlaneMachineSet" in front of custom resource/CR. For tangentially related docs reasons, I think we should prefer to write it as ControlPlaneMachineSet to avoid confusion with a "compute machine set" which is just MachineSet. I'll leave comments on this PR with my recommendations.
I will sweep the repo to change "control plane machine set CR" to "ControlPlaneMachineSet CR" at some point.
| spec: | ||
| machineNamePrefix: <machine_prefix> |
There was a problem hiding this comment.
Missing some Required fields here and ... to show there's more unwritten:
| spec: | |
| machineNamePrefix: <machine_prefix> | |
| apiVersion: machine.openshift.io/v1 | |
| kind: ControlPlaneMachineSet | |
| metadata: | |
| name: cluster | |
| namespace: openshift-machine-api | |
| spec: | |
| machineNamePrefix: <machine_prefix> | |
| # ... |
modules/cpmso-config-options.adoc
Outdated
| . Save your changes to the `ControlPlaneMachineSet` CR. | ||
|
|
||
| . Delete your existing machines and wait for replacement machines with the new prefix name to be created. | ||
|
|
||
| // For now I am documenting this generic step above, but I can also add a step for RollingUpdate users to force rollouts. |
There was a problem hiding this comment.
I've used this before, probably just copy it for consistency:
| . Save your changes to the `ControlPlaneMachineSet` CR. | |
| . Delete your existing machines and wait for replacement machines with the new prefix name to be created. | |
| // For now I am documenting this generic step above, but I can also add a step for RollingUpdate users to force rollouts. | |
| . Save your changes. | |
| .Next steps | |
| * For clusters that use the default `RollingUpdate` update strategy, the control plane machine set propagates changes to your control plane configuration automatically. | |
| * For clusters that are configured to use the `OnDelete` update strategy, you must replace your control plane machines manually. |
|
@skopacz1: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
OSDOCS-14472
Version(s): 4.19+
This PR adds a feature to add custom prefixes to machine names in control plane machine sets.
QE review:
Preview: Control plane machine set configuration options