cluster-api icon indicating copy to clipboard operation
cluster-api copied to clipboard

🐛 Fix nodeDrainTimeout for ControlPlane on Topology managed clusters

Open killianmuldoon opened this issue 2 years ago • 6 comments

Signed-off-by: killianmuldoon [email protected]

This changes adds setting nodeDrainTimout field to the ControlPlane and MachineDeploymentTopology in our ClusterClass quick start test and adds a fix which unquotes the string before setting it in the patch.

On reviewing #https://github.com/kubernetes-sigs/cluster-api/pull/7031 it looks like we're not able to reconcile NodeDrainTimeout (added in https://github.com/kubernetes-sigs/cluster-api/issues/6278) on the control plane in a topology managed cluster.

killianmuldoon avatar Aug 10 '22 15:08 killianmuldoon

@killianmuldoon: This issue is currently awaiting triage.

If CAPI contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-ci-robot avatar Aug 10 '22 15:08 k8s-ci-robot

@abhijit-dev82 this is where I'm figuring out a fix for https://github.com/kubernetes-sigs/cluster-api/pull/7031

killianmuldoon avatar Aug 10 '22 15:08 killianmuldoon

Failed run before fix here: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_cluster-api/7047/pull-cluster-api-e2e-informing-main/1557400014039289856

killianmuldoon avatar Aug 10 '22 16:08 killianmuldoon

We should probably cherry pick this to 1.2 give it's a broken feature that was including in that release.

/cherry-pick release-1.2

killianmuldoon avatar Aug 10 '22 19:08 killianmuldoon

@killianmuldoon: once the present PR merges, I will cherry-pick it on top of release-1.2 in a new PR and assign it to you.

In response to this:

We should probably cherry pick this to 1.2 give it's a broken feature that was including in that release.

/cherry-pick release-1.2

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.

Nice catch, thx!

sbueringer avatar Aug 17 '22 11:08 sbueringer

/hold

sbueringer avatar Aug 17 '22 11:08 sbueringer

Sorry forgot my own nits...

/approve cancel /hold cancel

sbueringer avatar Aug 17 '22 11:08 sbueringer

Thx!

/lgtm /approve

Agree with the cherry-pick.

(cc @fabriziopandini just fyi, setting the nodeDrainTimeout on cp will only work in the upcoming v1.2.x release)

sbueringer avatar Aug 17 '22 12:08 sbueringer

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

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

k8s-ci-robot avatar Aug 17 '22 12:08 k8s-ci-robot

@killianmuldoon: new pull request created: #7072

In response to this:

We should probably cherry pick this to 1.2 give it's a broken feature that was including in that release.

/cherry-pick release-1.2

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.