✨ Add nodeVolumeDetachTimeout property to Machine #6413

furkatgofurov7 · 2022-04-13T09:04:14Z

What this PR does / why we need it:

introduces a new timeout called nodeVolumeDetachTimeout to the machine and other CRDs, which can be used to wait for volume detachment to happen within the timeout limit
introduces a new annotation called ExcludeWaitForNodeVolumeDetachAnnotation to be able to skip shouldWaitForNodeVolumes check entirely if needed
moves out shouldWaitForNodeVolumes from node drain logic (isNodeDrainAllowed) and adds new check only for volume related checks called isNodeVolumeDetachingAllowed to to keep a cleaner separation for API
adds unit tests
documents introduced new timeout nodeVolumeDetachTimeout and annotation ExcludeWaitForNodeVolumeDetachAnnotation as a breaking changes in the provider's migration v1.2 to v1.3 document
Renames VolumeDetachSucceededCondition to VolumeDetachFinishedCondition so it is clearer for the user, because we have to mark VolumeDetachSucceededCondition as True when volumeDetachTimeoutExceeded condition met even though in fact volumes are not detached. For that reason, I renamed it to VolumeDetachFinishedCondition (open to suggestions on this) to make it a bit generic and less confusing for the user.
~~Adds VolumeDetachTimedOutCondition to indicate volume detaching timeout exceeded~~
~~Adds WaitingForVolumeDetachTimeoutReason to indicate the reason for detach timeout~~

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #6285

k8s-ci-robot · 2022-04-13T09:04:22Z

Hi @furkatgofurov7. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

api/v1beta1/machine_types.go

fabriziopandini · 2022-04-13T14:54:13Z

/hold
while discussing requirements and solution on the issue

k8s-triage-robot · 2022-07-12T15:23:15Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

furkatgofurov7 · 2022-07-13T08:14:48Z

/remove-lifecycle stale

fabriziopandini

Q: for node draining, we have the machine.cluster.x-k8s.io/exclude-node-draining annotation that allows us to skip the process entirely. Should we have something similar for volumeDetach?

internal/controllers/machine/machine_controller.go

fabriziopandini · 2022-08-04T17:36:19Z

/hold cancel
@furkatgofurov7 sorry for getting late at this PR, it would be great if we can get this moving

fabriziopandini · 2022-08-04T17:36:38Z

/ok-to-test

furkatgofurov7 · 2022-08-10T07:41:09Z

Q: for node draining, we have the machine.cluster.x-k8s.io/exclude-node-draining annotation that allows us to skip the process entirely. Should we have something similar for volumeDetach?

@fabriziopandini great idea, I was not aware of that annotation, since it could simplify the logic a lot and not affect the current behavior. I will make changes accordingly hopefully soon (sorry for the late reply btw, I am on vacation now)

furkatgofurov7 · 2022-08-15T14:16:58Z

Q: for node draining, we have the machine.cluster.x-k8s.io/exclude-node-draining annotation that allows us to skip the process entirely. Should we have something similar for volumeDetach?

@fabriziopandini I have updated the PR accordingly to add exclude volumeDetach annotation, PTAL. I am also investigating verify main test failure

furkatgofurov7 · 2022-08-16T10:08:34Z

I can not reproduce this test failure locally (make verify-gen is not throwing any error? tried both 1.17 and v1.18 Go versions )

/test pull-cluster-api-verify-main

furkatgofurov7 · 2022-09-16T20:27:05Z

Created #7233 for tracking related work on Cluster.Topology

Thank you, most probably you will be working on it, but in case not, I am also happy to work on this issue. This looks very much like 83036e1 and 67080e1

ykakarap · 2022-09-16T22:28:41Z

I can take care of doing the work in Cluster.Topology.
I am assigned to the issue. I will get started on that once this PR is merged.

fabriziopandini · 2022-09-19T08:51:11Z

/lgtm

furkatgofurov7 · 2022-09-19T09:03:34Z

/cc @sbueringer @enxebre hi folks, if you could take another look and help to merge this, thanks!

sbueringer · 2022-09-19T09:28:08Z

/lgtm

Giving Alberto some time to take a look as well

furkatgofurov7 · 2022-09-19T10:29:27Z

/lgtm

Giving Alberto some time to take a look as well

Sure, anyways I had to rebase it after bac8e27 and it got removed again 😀

sbueringer · 2022-09-19T11:26:15Z

/lgtm

fabriziopandini · 2022-09-19T14:15:03Z

Oh, I just noticed that commits are not squashed 😅

furkatgofurov7 · 2022-09-19T14:19:57Z

Oh, I just noticed that commits are not squashed sweat_smile

@fabriziopandini done 😃

enxebre · 2022-09-20T08:56:54Z

/lgtm

sbueringer · 2022-09-20T09:00:38Z

/approve

@furkatgofurov7 Thx for all your work and patience on this!

k8s-ci-robot · 2022-09-20T09:00:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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~~ [sbueringer]

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

schrej · 2023-02-13T16:34:02Z

internal/controllers/machine/machine_controller.go

+		// After node draining is completed, and if isNodeVolumeDetachingAllowed returns True, make sure all
+		// volumes are detached before proceeding to delete the Node.
+		if r.isNodeVolumeDetachingAllowed(m) {
+			log.Info("Waiting for node volumes to be detached", "Node", klog.KRef("", m.Status.NodeRef.Name))


This log message duplicates the one shown in line 374. Would be good to remove one of them. If we keep this one, we should add a second log entry when volumes are detached. Otherwise it makes debugging harder - caused a lot of confusion for us just now.
I can move this to an issue if necessary.

Would be good if you can either move it to an issue or open a PR if you want to

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Apr 13, 2022

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 13, 2022

k8s-ci-robot requested review from JoelSpeed and killianmuldoon April 13, 2022 09:04

furkatgofurov7 mentioned this pull request Apr 13, 2022

CAPI waiting forever for the volume to be detached #6285

Closed

JoelSpeed reviewed Apr 13, 2022

View reviewed changes

api/v1beta1/machine_types.go Outdated Show resolved Hide resolved

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 13, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 12, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 13, 2022

furkatgofurov7 force-pushed the feature/add-volume-detach-timeout-property branch from 6a80f0b to bf36f49 Compare July 18, 2022 18:41

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2022

fabriziopandini reviewed Aug 4, 2022

View reviewed changes

internal/controllers/machine/machine_controller.go Outdated Show resolved Hide resolved

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 4, 2022

k8s-ci-robot 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 Aug 4, 2022

fabriziopandini mentioned this pull request Aug 5, 2022

Improve API around machine deletion strategy #7021

Open

furkatgofurov7 force-pushed the feature/add-volume-detach-timeout-property branch from bf36f49 to 1a64aeb Compare August 15, 2022 12:10

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 15, 2022

furkatgofurov7 force-pushed the feature/add-volume-detach-timeout-property branch from 1a0f416 to fd85fbe Compare August 15, 2022 14:01

adilGhaffarDev force-pushed the feature/add-volume-detach-timeout-property branch from fd85fbe to 02d905c Compare August 16, 2022 12:08

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 16, 2022

furkatgofurov7 force-pushed the feature/add-volume-detach-timeout-property branch from 566ba4e to 60b3ef4 Compare September 16, 2022 19:53

ykakarap mentioned this pull request Sep 16, 2022

✨ add NodeVolumeDetachTimeout support for Cluster Topology #7235

Merged

furkatgofurov7 requested a review from fabriziopandini September 18, 2022 12:00

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2022

furkatgofurov7 force-pushed the feature/add-volume-detach-timeout-property branch from a543dd6 to 3514f1b Compare September 19, 2022 10:27

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2022

✨ Add nodeVolumeDetachTimeout property to Machine

a27f71c

furkatgofurov7 force-pushed the feature/add-volume-detach-timeout-property branch from 3514f1b to a27f71c Compare September 19, 2022 14:19

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 19, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 20, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 20, 2022

k8s-ci-robot merged commit 1a56bc5 into kubernetes-sigs:main Sep 20, 2022

k8s-ci-robot added this to the v1.3 milestone Sep 20, 2022

furkatgofurov7 deleted the feature/add-volume-detach-timeout-property branch October 17, 2022 14:18

schrej reviewed Feb 13, 2023

View reviewed changes

schrej mentioned this pull request Feb 14, 2023

🌱 improve logging during Machine deletion #8108

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add nodeVolumeDetachTimeout property to Machine #6413

✨ Add nodeVolumeDetachTimeout property to Machine #6413

furkatgofurov7 commented Apr 13, 2022 •

edited

Loading

k8s-ci-robot commented Apr 13, 2022

fabriziopandini commented Apr 13, 2022

k8s-triage-robot commented Jul 12, 2022

furkatgofurov7 commented Jul 13, 2022

fabriziopandini left a comment

fabriziopandini commented Aug 4, 2022

fabriziopandini commented Aug 4, 2022

furkatgofurov7 commented Aug 10, 2022

furkatgofurov7 commented Aug 15, 2022 •

edited

Loading

furkatgofurov7 commented Aug 16, 2022 •

edited

Loading

furkatgofurov7 commented Sep 16, 2022

ykakarap commented Sep 16, 2022

fabriziopandini commented Sep 19, 2022

furkatgofurov7 commented Sep 19, 2022

sbueringer commented Sep 19, 2022

furkatgofurov7 commented Sep 19, 2022

sbueringer commented Sep 19, 2022

fabriziopandini commented Sep 19, 2022

furkatgofurov7 commented Sep 19, 2022

enxebre commented Sep 20, 2022

sbueringer commented Sep 20, 2022

k8s-ci-robot commented Sep 20, 2022

schrej Feb 13, 2023

sbueringer Feb 13, 2023

✨ Add nodeVolumeDetachTimeout property to Machine #6413

✨ Add nodeVolumeDetachTimeout property to Machine #6413

Conversation

furkatgofurov7 commented Apr 13, 2022 • edited Loading

k8s-ci-robot commented Apr 13, 2022

fabriziopandini commented Apr 13, 2022

k8s-triage-robot commented Jul 12, 2022

furkatgofurov7 commented Jul 13, 2022

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini commented Aug 4, 2022

fabriziopandini commented Aug 4, 2022

furkatgofurov7 commented Aug 10, 2022

furkatgofurov7 commented Aug 15, 2022 • edited Loading

furkatgofurov7 commented Aug 16, 2022 • edited Loading

furkatgofurov7 commented Sep 16, 2022

ykakarap commented Sep 16, 2022

fabriziopandini commented Sep 19, 2022

furkatgofurov7 commented Sep 19, 2022

sbueringer commented Sep 19, 2022

furkatgofurov7 commented Sep 19, 2022

sbueringer commented Sep 19, 2022

fabriziopandini commented Sep 19, 2022

furkatgofurov7 commented Sep 19, 2022

enxebre commented Sep 20, 2022

sbueringer commented Sep 20, 2022

k8s-ci-robot commented Sep 20, 2022

schrej Feb 13, 2023

Choose a reason for hiding this comment

sbueringer Feb 13, 2023

Choose a reason for hiding this comment

furkatgofurov7 commented Apr 13, 2022 •

edited

Loading

furkatgofurov7 commented Aug 15, 2022 •

edited

Loading

furkatgofurov7 commented Aug 16, 2022 •

edited

Loading