[OCPCLOUD-1586] Reintroduce and update ControlPlaneMachineSet API #1216
Conversation
This reverts c9a0b0b which removed the API from the 4.11 release. We will be shipping this feature in 4.12 so now is the time to reintroduce the API.
We've learnt since this was initially implemented that discriminants should be required in unions. I tested this manually and if failure domains are omitted completely, this works, if failuredomains are present at all, the value is required as expected.
Mostly nits and adding full stops, but found a few bits along the way to update
The discriminant values should be PascalCase by convention, this was missed in the previous review
|
Hello @JoelSpeed! Some important instructions when contributing to openshift/api: For merging purposes, this repository follows the no-Feature-Freeze process which means that in addition to the standard
OR
Who should apply these qe/docs/px labels?
|
JoelSpeed
changed the title
damdo
left a comment
damdo
left a comment
There was a problem hiding this comment.
/lgtm
Thanks for adding the new UpdatedReadyReplicas field!
| // created by the ControlPlaneMachineSet controller that have the desired | ||
| // provider spec and are ready. | ||
| // +optional | ||
| UpdatedReadyReplicas int32 `json:"updatedReadyReplicas,omitempty"` |
There was a problem hiding this comment.
I spoke with @JoelSpeed about it on slack, the feedback I gave him is that this field seems unnecessary, since comparing controlplanemachineset with deployments and how it drives its rollout it should be possible to set UpdatedReplicas=0 when a new rollout starts, and update ReadyReplicas whenever the new machine is fully operational. There is no need to differentiate between UpdatedReadyReplicas and ReadyReplicas. It's important to note that during rollout ReadyReplicas can have values bigger than .spec.Replicas which is caused by the current surge of 1, and that should be call out explicitly in the field's description.
There was a problem hiding this comment.
The suggestion of adding this field was originally from @damdo. By adding it, we make it very explicit to users the number of machines with the current desired spec and of those, how many are ready. However, as this design is based heavily on deployments, I can see the arguments for keeping it more similar to the deployment API.
We ran through a bunch of examples on a call and in each scenario I believe you can infer from the combination of Ready, Updated and Replicas how many replicas are updated or not. So we should be fine without this for now.
It does create some confusion within the codebase because it's hard to define updated as "updated and ready" in the API, when the rest of the code it means something else, but we can deal with that
|
/label qe-approved This project is not a part of the no-ff process, therefore these labels shouldn't block us |
openshift-ci
bot
added
qe-approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexander-demichev, damdo, JoelSpeed, soltysh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/retest |
|
@JoelSpeed: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
4 participants
This reintroduces the ControlPlaneMachineSet API originally introduced in 4.11 via #1112, but then later reverted.
As we have updated the conventions over time and been working on the implementation, there are a few changes I've made which I've tried to make atomic:
My suggestion would be to review this commit wise as the first commit is large and is simply a revert of the revert to pull the API out of 4.11.