-
Notifications
You must be signed in to change notification settings - Fork 87
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.
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
✨ Use Out-of-service taint in Node remediation in place of deletion #1808
✨ Use Out-of-service taint in Node remediation in place of deletion #1808
Conversation
Hi @clobrano. Thanks for your PR. I'm waiting for a metal3-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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/test-infra repository. |
/ok-to-test |
f0ca462
to
97888f7
Compare
97888f7
to
a76c074
Compare
/test ? |
@adilGhaffarDev: The following commands are available to trigger required jobs:
The following commands are available to trigger optional jobs:
Use
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 kubernetes/test-infra repository. |
/test metal3-ubuntu-e2e-feature-test-main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the feature. I am in a double mind thinking if this feature should come from CAPI side or from infra provider's side, specially since it is talking about k8s node behavior. I will probably pose it as an open question for now. I dont see any harm on doing it from infra provider's side to be honest, but its just that we dont usually do node operations like taint nodes or check for it to drain from CAPM3. Anyways, a couple of comments inline for now.
func (r *RemediationManager) RemoveOutOfServiceTaint(ctx context.Context, clusterClient v1.CoreV1Interface, node *corev1.Node) error { | ||
newTaints := []corev1.Taint{} | ||
|
||
var IsPopOutOfServiceTaintServiceTaint bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
variable name seems to use ServiceTaint
twice is that intentional or shall we just keep it simpler like outOfServiceTaintDropped
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely a typo, thanks!
} | ||
|
||
// +kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch;update;delete;deletecollection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need all the verbs like update, delete, deletecollection
? As far as I can see in the code list
and watch
should suffice or may be only list
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see pod list on L545, annotation get on L170, but fail to see where client would need any of the other verbs. Can you elaborate why they'd be needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You both are correct. I need to delete some verbs.
I will test if get
is also necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that listing VA does need the get
verbs
pkg/mod/k8s.io/client-go@v0.29.5/tools/cache/reflector.go:229: Failed to watch *v1.VolumeAttachment: unknown (get volumeattachments.storage.k8s.io)
} | ||
|
||
// +kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch;update;delete;deletecollection | ||
// +kubebuilder:rbac:groups=storage.k8s.io,resources=volumeattachments,verbs=get;list;watch;update;delete;deletecollection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question, do we need all the verbs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see List on L561, but nothing else being used. Same question, where do we need them?
@kashifest: GitHub didn't allow me to request PR reviews from the following users: it, would, be, your, on, good, top, have, this. Note that only metal3-io members and repo collaborators can review this PR, and authors cannot review their own PRs. 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 kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RBAC scope questions.
} | ||
|
||
// +kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch;update;delete;deletecollection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see pod list on L545, annotation get on L170, but fail to see where client would need any of the other verbs. Can you elaborate why they'd be needed?
} | ||
|
||
// +kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch;update;delete;deletecollection | ||
// +kubebuilder:rbac:groups=storage.k8s.io,resources=volumeattachments,verbs=get;list;watch;update;delete;deletecollection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see List on L561, but nothing else being used. Same question, where do we need them?
a76c074
to
11a74e9
Compare
I was going to tag @clobrano as the expert on this subject, but I see that he is already the author of the PR 🙂 |
11a74e9
to
608174e
Compare
@clobrano: The following test failed, say
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. |
@clobrano could you please squash the commits and sign the resulting commit off , to be compliant with the DCO enforcement policy https://github.com/metal3-io/cluster-api-provider-metal3/pull/1808/checks?check_run_id=27076588793 |
sure, I'll do it right now |
If the Out of service taint (OOST) is supported (k8s server version >= 1.28), enable Metal3RemediationController to set the OOST taint on the target node instead than delete it. Ensure the target node is drained (no stateful pod running) before moving on to waiting state, and host power on again. When the host is powered on, remove the OOST from the target node. Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>
8be7da9
to
c6cd725
Compare
/test metal3-centos-e2e-integration-test-main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Just writing down my understanding, because I was unsure first about this topic:
The logic of detecting the "success" of the draining is a bit messy IMO, but it is a kubernetes thing. AFAIK there is no clear indication whether draining was done for a node because as far as I understand there is no such process as "draining" from K8s API perspective, instead what is happening is just pod deletion and/or pod eviction on a node scale thus each eviction+deletion is done in the context of a pod and not in the context of the node but done on all pods of a node, and because this situation the only way to detect the "status of the draining" is to check whether any Pod (of the draining Node) is still around with a deletion timestamp. Please correct me if I am wrong!!!
after a bit of contemplation I came to the conclusion that IMO CAPM3 is a acceptable location for this feature because it only enhances the existing remediation process and the remediation is the responsibility of the infra provider. |
correct :) |
/approve Let's also update our book: https://github.com/metal3-io/metal3-docs/blob/main/docs/user-guide/src/capm3/remediaton.md#metal3-remediation Can you open a PR for that too? I am running the e2e feature again to be on the safe side. |
@adilGhaffarDev: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
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 kubernetes-sigs/prow repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adilGhaffarDev 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 |
/test metal3-centos-e2e-feature-test-main-remediation |
/hold |
sure no problem 🤞 for the last tests |
/hold cancel |
Update the Metal3 Remediation section with the new step Out-of-Service taint replacing node deletion. See metal3-io/cluster-api-provider-metal3#1808
Update the Metal3 Remediation section with the new step Out-of-Service taint replacing node deletion. See metal3-io/cluster-api-provider-metal3#1808 Signed-off-by: Carlo Lobrano <c.lobrano@gmail.com>
What this PR does / why we need it:
Currently, Metal3Remediation deletes the Node object to speed up the remediation, however, starting from Kubernetes 1.28 (GA) the new out-of-service taint is available, and CAPM3 can use it in place of deleting the node.
Which issue(s) this PR fixes:
Fixes #1725