✨ Implement MachineDrainRules #11353
Conversation
k8s-ci-robot
added
cncf-cla: yes
k8s-ci-robot
added
the
size/XXL
sbueringer
added
the
area/machine
|
/assign @fabriziopandini /assign @JoelSpeed @JoelSpeed If you have time, would be great if you can review the new API (api/v1beta1/machinedrainrules_types.go). No changes compared to the proposal (as far as I can tell :)). |
|
/test pull-cluster-api-e2e-main |
|
@enxebre @JoelSpeed Thx for the quick reviews. Findings either fixed or answered to the conversations (I'll look into the e2e test failure, needs a minor fix in the in-memory provider) |
|
/test pull-cluster-api-e2e-main |
test/infrastructure/inmemory/internal/controllers/inmemorymachine_controller.go
Outdated
Show resolved
Hide resolved
sbueringer
added
the
tide/merge-method-squash
|
@JoelSpeed I think everything either fixed or answered |
sbueringer
force-pushed
the
pr-machine-drain-rules
branch
from
b8a7e87 to
05cae45
Compare
|
/test pull-cluster-api-e2e-main |
|
/test pull-cluster-api-e2e-main |
|
/test pull-cluster-api-e2e-main |
| // +listType=atomic | ||
| // +kubebuilder:validation:MinItems=1 | ||
| // +kubebuilder:validation:MaxItems=32 | ||
| // +kubebuilder:validation:XValidation:rule="self.all(x, self.exists_one(y, x == y))",message="entries in pods must be unique" |
There was a problem hiding this comment.
one note on these, what's the minimum cluster version that these would work in?
There was a problem hiding this comment.
1.28. We cannot use CEL, missed that. CI on main would have caught it eventually..
There was a problem hiding this comment.
Where did you get 1.28 from?
I think this validation would work from 1.26 clusters onwards
Edit: CEL validations have been on by default from 1.25, forced on from 1.29
There was a problem hiding this comment.
Sorry. Misread the comment from Vince. v1.28 is the miminum Kubernetes mgmt cluster version that Cluster API v1.9 will support (source: https://main.cluster-api.sigs.k8s.io/reference/versions#core-provider-cluster-api-controller)
According to the Kubernetes website validation rules have become stable with Kubernetes 1.29: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#validation-rules
There was a problem hiding this comment.
Not sure what that means for our Kubernetes version support. I think currently we just state that we support >= 1.28.
We didn't specify details like: "We support 1.28, but only if you don't disable CEL"
There was a problem hiding this comment.
Hmm, good point. I would wager most people don't disable beta, enabled by default Kube features, but, we could in theory break someone by adding this.
What release is this going into? Could a release note be added for this release saying we support 1.28 but you must not have disabled CEL?
There was a problem hiding this comment.
This goes into CAPI v1.9 in December.
Could a release note be added for this release saying we support 1.28 but you must not have disabled CEL?
Not sure if it's worth it, just to get 2 CEL validations in there :)
Random example:
Starting with 1.30 kubernetes version and 1.27 LTS versions the beta APIs will be disabled by default when you upgrade to them.
https://learn.microsoft.com/en-us/azure/aks/upgrade-aks-cluster?tabs=azure-cli#before-you-begin
(But this is not about CEL, it's only for >= 1.30 and I don't know if what they do with beta APIs is comparable to beta feature gates. In general I don't know what policies folks follow for beta features)
There was a problem hiding this comment.
Damn you AKS 😂 Do we risk it?
Or do we implement the same CEL validations as webhooks for now, and leave the markers there (disable them somehow?) with a todo to reimplement them and delete the code once the minimum kube version is bumped to 1.29 for CAPI?
There was a problem hiding this comment.
I would suggest to keep it simple and do the following:
- Implement the validation in the webhook for now
- Open a follow-up issue to track the API validation discussion in general and also add a specific sub-task for this validation
Context: current plan is that CAPI v1.10 supports mgmt clusters >= v1.29 (which means that once we branched away release-1.9 in 3 weeks this is not a blocker anymore anyway).
|
/test pull-cluster-api-e2e-mink8s-main Update: Looks like it works because CEL is enabled per default in 1.28 (xref: #11353 (comment)). |
|
/test pull-cluster-api-e2e-mink8s-main |
|
@fabriziopandini @JoelSpeed @vincepri PTAL :) |
|
lgtm |
|
@enxebre PTAL, thx :) |
|
/test pull-cluster-api-e2e-mink8s-main |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: e2de3e9859256144dca80d0e87a62675aed8dc4c
|
|
Great work /hold feel free to cancel anytime |
k8s-ci-robot
added
the
do-not-merge/hold
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
Thx everyone! /hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
|
Flake is independent of this PR and will be fixed in a separate PR |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Part of #11240