✨ Use Out-of-service taint in Node remediation in place of deletion #1808

clobrano · 2024-06-27T12:23:28Z

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

metal3-io-bot · 2024-06-27T12:23:39Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

tuminoid · 2024-06-28T06:27:43Z

/ok-to-test

tuminoid · 2024-06-28T06:28:02Z

/cc @kashifest @Rozzii @mboukhalfa

clobrano · 2024-06-28T06:44:42Z

@clobrano: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:
Test name Commit Details Required Rerun command
unit 60009dc link true /test unit

Full PR test history. Your PR dashboard.

I must have forgot one latest change. I'll fix it right away

adilGhaffarDev · 2024-07-02T09:47:05Z

/test ?

metal3-io-bot · 2024-07-02T09:47:10Z

@adilGhaffarDev: The following commands are available to trigger required jobs:

/test build
/test generate
/test gomod
/test manifestlint
/test markdownlint
/test metal3-centos-e2e-integration-test-main
/test metal3-ubuntu-e2e-integration-test-main
/test shellcheck
/test test
/test unit

The following commands are available to trigger optional jobs:

/test metal3-centos-e2e-basic-test-main
/test metal3-centos-e2e-feature-test-main
/test metal3-e2e-1-26-1-27-upgrade-test-main
/test metal3-e2e-1-27-1-28-upgrade-test-main
/test metal3-e2e-1-28-1-29-upgrade-test-main
/test metal3-e2e-clusterctl-upgrade-test-main
/test metal3-ubuntu-e2e-basic-test-main
/test metal3-ubuntu-e2e-feature-test-main

Use /test all to run the following jobs that were automatically triggered:

build
generate
gomod
manifestlint
unit

In response to this:

/test ?

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.

adilGhaffarDev · 2024-07-02T09:47:32Z

/test metal3-ubuntu-e2e-feature-test-main

kashifest

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.

kashifest · 2024-07-01T18:15:04Z

baremetal/metal3remediation_manager.go

+func (r *RemediationManager) RemoveOutOfServiceTaint(ctx context.Context, clusterClient v1.CoreV1Interface, node *corev1.Node) error {
+	newTaints := []corev1.Taint{}
+
+	var IsPopOutOfServiceTaintServiceTaint bool


variable name seems to use ServiceTaint twice is that intentional or shall we just keep it simpler like outOfServiceTaintDropped ?

This is definitely a typo, thanks!

kashifest · 2024-07-01T18:54:13Z

controllers/metal3remediation_controller.go

 }

+// +kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch;update;delete;deletecollection


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?

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?

You both are correct. I need to delete some verbs.
I will test if get is also necessary

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)

kashifest · 2024-07-01T18:54:28Z

controllers/metal3remediation_controller.go

 }

+// +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


same question, do we need all the verbs?

I see List on L561, but nothing else being used. Same question, where do we need them?

kashifest · 2024-07-02T10:47:37Z

/cc @zaneb @dtantsur @lentzi90 @tuminoid @mboukhalfa @honza it would be good to have your feedback on this

metal3-io-bot · 2024-07-02T10:47:47Z

@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:

/cc @zaneb @dtantsur @lentzi90 @tuminoid @mboukhalfa @honza it would be good top have your feedback on 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.

tuminoid

RBAC scope questions.

tuminoid · 2024-07-02T11:40:31Z

controllers/metal3remediation_controller.go

 }

+// +kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch;update;delete;deletecollection


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?

tuminoid · 2024-07-02T11:42:06Z

controllers/metal3remediation_controller.go

 }

+// +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


I see List on L561, but nothing else being used. Same question, where do we need them?

zaneb · 2024-07-04T00:17:36Z

I was going to tag @clobrano as the expert on this subject, but I see that he is already the author of the PR 🙂

metal3-io-bot · 2024-07-05T09:04:00Z

@clobrano: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
metal3-ubuntu-e2e-feature-test-main	`a76c074`	link	false	`/test metal3-ubuntu-e2e-feature-test-main`

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.

Rozzii · 2024-07-08T11:37:49Z

@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

clobrano · 2024-07-08T13:44:19Z

@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>

Rozzii · 2024-07-09T08:48:42Z

/test metal3-centos-e2e-integration-test-main
/test metal3-ubuntu-e2e-integration-test-main

Rozzii

/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!!!

Rozzii · 2024-07-09T11:55:59Z

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.

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.

clobrano · 2024-07-09T12:24:43Z

/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!!!

correct :)

adilGhaffarDev · 2024-07-25T15:38:56Z

/approve
Thank you for working on this.

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.
/test metal3-ubuntu-e2e-feature-test-main

metal3-io-bot · 2024-07-25T15:39:00Z

@adilGhaffarDev: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test build
/test generate
/test gomod
/test manifestlint
/test markdownlint
/test metal3-centos-e2e-integration-test-main
/test metal3-ubuntu-e2e-integration-test-main
/test shellcheck
/test test
/test unit

The following commands are available to trigger optional jobs:

/test metal3-centos-e2e-basic-test-main
/test metal3-centos-e2e-feature-test-main-features
/test metal3-centos-e2e-feature-test-main-pivoting
/test metal3-centos-e2e-feature-test-main-remediation
/test metal3-e2e-1-26-1-27-upgrade-test-main
/test metal3-e2e-1-27-1-28-upgrade-test-main
/test metal3-e2e-1-28-1-29-upgrade-test-main
/test metal3-e2e-clusterctl-upgrade-test-main
/test metal3-ubuntu-e2e-basic-test-main
/test metal3-ubuntu-e2e-feature-test-main-features
/test metal3-ubuntu-e2e-feature-test-main-pivoting
/test metal3-ubuntu-e2e-feature-test-main-remediation

Use /test all to run the following jobs that were automatically triggered:

build
generate
gomod
manifestlint
unit

In response to this:

/approve
Thank you for working on this.

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.
/test metal3-ubuntu-e2e-feature-test-main

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.

metal3-io-bot · 2024-07-25T15:39:04Z

[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:

~~OWNERS~~ [adilGhaffarDev]

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

adilGhaffarDev · 2024-07-25T15:39:30Z

/test metal3-centos-e2e-feature-test-main-remediation

adilGhaffarDev · 2024-07-25T15:40:01Z

/hold
lets wait for test to pass

clobrano · 2024-07-25T16:19:49Z

/approve Thank you for working on this.

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. /test metal3-ubuntu-e2e-feature-test-main

sure no problem

🤞 for the last tests

adilGhaffarDev · 2024-07-26T06:52:05Z

/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>

metal3-io-bot requested review from adilGhaffarDev and smoshiur1237 June 27, 2024 12:23

metal3-io-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 27, 2024

metal3-io-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 27, 2024

metal3-io-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 28, 2024

metal3-io-bot requested review from kashifest, mboukhalfa and Rozzii June 28, 2024 06:28

clobrano force-pushed the feature/out-of-service-taint-0 branch 3 times, most recently from f0ca462 to 97888f7 Compare June 28, 2024 11:12

Rozzii added this to the 1.8.0 milestone Jun 28, 2024

metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Jun 28, 2024

clobrano force-pushed the feature/out-of-service-taint-0 branch from 97888f7 to a76c074 Compare July 1, 2024 15:16

metal3-io-bot removed the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2024

kashifest reviewed Jul 2, 2024

View reviewed changes

metal3-io-bot requested review from honza, zaneb, lentzi90, dtantsur and tuminoid July 2, 2024 10:47

tuminoid reviewed Jul 2, 2024

View reviewed changes

clobrano force-pushed the feature/out-of-service-taint-0 branch from a76c074 to 11a74e9 Compare July 3, 2024 13:47

clobrano force-pushed the feature/out-of-service-taint-0 branch from 11a74e9 to 608174e Compare July 5, 2024 09:02

clobrano force-pushed the feature/out-of-service-taint-0 branch from 8be7da9 to c6cd725 Compare July 8, 2024 13:48

Rozzii reviewed Jul 9, 2024

View reviewed changes

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2024

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 25, 2024

metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 25, 2024

metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 26, 2024

metal3-io-bot merged commit a599911 into metal3-io:main Jul 26, 2024
17 checks passed

clobrano deleted the feature/out-of-service-taint-0 branch July 26, 2024 07:11

clobrano added a commit to clobrano/metal3-docs that referenced this pull request Jul 29, 2024

Update Metal3 Remediation section

49a2ef0

Update the Metal3 Remediation section with the new step Out-of-Service taint replacing node deletion. See metal3-io/cluster-api-provider-metal3#1808

clobrano mentioned this pull request Jul 29, 2024

Update Metal3 Remediation section metal3-io/metal3-docs#462

Merged

		}

		// +kubebuilder:rbac:groups=core,resources=pods,verbs=get;list;watch;update;delete;deletecollection

✨ 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

clobrano commented Jun 27, 2024

metal3-io-bot commented Jun 27, 2024

tuminoid commented Jun 28, 2024

tuminoid commented Jun 28, 2024

clobrano commented Jun 28, 2024

adilGhaffarDev commented Jul 2, 2024

metal3-io-bot commented Jul 2, 2024

adilGhaffarDev commented Jul 2, 2024

kashifest left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kashifest commented Jul 2, 2024 • edited Loading

metal3-io-bot commented Jul 2, 2024

tuminoid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaneb commented Jul 4, 2024

metal3-io-bot commented Jul 5, 2024 • edited Loading

Rozzii commented Jul 8, 2024

clobrano commented Jul 8, 2024

Rozzii commented Jul 9, 2024

Rozzii left a comment • edited Loading

Choose a reason for hiding this comment

Rozzii commented Jul 9, 2024 • edited Loading

clobrano commented Jul 9, 2024

adilGhaffarDev commented Jul 25, 2024

metal3-io-bot commented Jul 25, 2024

metal3-io-bot commented Jul 25, 2024

adilGhaffarDev commented Jul 25, 2024

adilGhaffarDev commented Jul 25, 2024

clobrano commented Jul 25, 2024

adilGhaffarDev commented Jul 26, 2024

kashifest commented Jul 2, 2024 •

edited

Loading

metal3-io-bot commented Jul 5, 2024 •

edited

Loading

Rozzii left a comment •

edited

Loading

Rozzii commented Jul 9, 2024 •

edited

Loading