Allow a user specifiable node draining timeout

[ajwdev]

As a user/operator, I would like the option to specify a timeout for how long the machine controller will wait for a node to drain before forcibly removing the machine.

Today, if a user has specified a pod disruption budget that cannot be fulfilled, CAPI could wait forever on a machine/node that is running a pod whose budget could not be met and will never successfully drain. I would like the option to say ...

1. Wait X minutes for a node to drain.
2. If the node does not drain within that amount of time, continue on with the deletion anyways.

This would prevent CAPI from getting suck in an infinite reconcile loop during certain cluster conditions.

/kind feature

[vincepri]

/help
/milestone v0.3.0
/priority backlog

[k8s-ci-robot]

@vincepri:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help
/milestone v0.3.0
/priority backlog

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.

[SataQiu]

/cc

[vincepri]

/milestone Next

[ncdc]

@williamsandrew does #2165 solve this for you?

[ncdc]

Oh wait, that PR is only for unreachable/unready nodes

[ajwdev]

Correct. My use case would include nodes that are accessible but are still unable to drain within a certain period of time.

[wfernandes]

/assign
Investigating what needs to be done here.

[wfernandes]

Just wanted to get some clarification on expected behavior.

In the Machine Controller, when we are draining the nodes, the default behavior is to Evict the pods before deleting them.

In this scenario of evicting the pods and then deleting them we are waiting forever for the pods to be deleted. The timeout property is set to MaxInt64 in evictPods().

cluster-api/third_party/kubernetes-drain/drain.go

Lines 261 to 273 in 5f0a49e

	params := waitForDeleteParams{
	ctx: ctx,
	pods: []corev1.Pod{pod},
	interval: 1 * time.Second,
	timeout: time.Duration(math.MaxInt64),
	usingEviction: true,
	getPodFn: getPodFn,
	onDoneFn: d.OnPodDeletedOrEvicted,
	globalTimeout: globalTimeout,
	skipWaitForDeleteTimeoutSeconds: d.SkipWaitForDeleteTimeoutSeconds,
	out: d.Out,
	}
	_, err := waitForDelete(params)

However if we just delete the pods directly by calling deletePods() we only wait for 20s to delete.
Here timeout: globalTimeout which is 20s by default.

cluster-api/third_party/kubernetes-drain/drain.go

Lines 315 to 327 in 5f0a49e

	params := waitForDeleteParams{
	ctx: ctx,
	pods: pods,
	interval: 1 * time.Second,
	timeout: globalTimeout,
	usingEviction: false,
	getPodFn: getPodFn,
	onDoneFn: d.OnPodDeletedOrEvicted,
	globalTimeout: globalTimeout,
	skipWaitForDeleteTimeoutSeconds: d.SkipWaitForDeleteTimeoutSeconds,
	out: d.Out,
	}
	_, err := waitForDelete(params)

It seems that the MachineController always calls drainNode with DisableEviction: false (default value for kubedrain.Helper)

cluster-api/controllers/machine_controller.go

Lines 389 to 426 in 5f0a49e

	drainer := &kubedrain.Helper{
	Client: kubeClient,
	Force: true,
	IgnoreAllDaemonSets: true,
	DeleteLocalData: true,
	GracePeriodSeconds: -1,
	// If a pod is not evicted in 20 seconds, retry the eviction next time the
	// machine gets reconciled again (to allow other machines to be reconciled).
	Timeout: 20 * time.Second,
	OnPodDeletedOrEvicted: func(pod *corev1.Pod, usingEviction bool) {
	verbStr := "Deleted"
	if usingEviction {
	verbStr = "Evicted"
	}
	logger.Info(fmt.Sprintf("%s pod from Node", verbStr),
	"pod", fmt.Sprintf("%s/%s", pod.Name, pod.Namespace))
	},
	Out: writer{klog.Info},
	ErrOut: writer{klog.Error},
	DryRun: false,
	}
	if noderefutil.IsNodeUnreachable(node) {
	// When the node is unreachable and some pods are not evicted for as long as this timeout, we ignore them.
	drainer.SkipWaitForDeleteTimeoutSeconds = 60 * 5 // 5 minutes
	}
	if err := kubedrain.RunCordonOrUncordon(drainer, node, true); err != nil {
	// Machine will be re-reconciled after a cordon failure.
	logger.Error(err, "Cordon failed")
	return errors.Errorf("unable to cordon node %s: %v", node.Name, err)
	}
	if err := kubedrain.RunNodeDrain(drainer, node.Name); err != nil {
	// Machine will be re-reconciled after a drain failure.
	logger.Error(err, "Drain failed")
	return &capierrors.RequeueAfterError{RequeueAfter: 20 * time.Second}
	}

So we are going to wait forever when trying to delete pods after eviction. My question is Why are we waiting forever in one case but only 20s in the other?

Feel free to reach out on slack to discuss more. 🙂

Other thoughts:

• Can we use the timeout field and make that configurable instead of adding yet another NodeDrainTimeout to the kubedrain helper?
• Where did we get Timeout: 20s from?

[vincepri]

Thanks for the great write-up @wfernandes, let's sync up at the community meeting. +1 to using a different default where we're using MaxInt64, seems rather a large timeout. What would be a good default we can get in v0.3.7 @detiber ?

[wfernandes]

@vincepri I added a topic under the General Questions section for the July 8th meeting.

[detiber]

If I had to hazard a guess, the idea was to try to err on the side of workload safety, since pod deletion should be subject to any pod disruption budgets that are configured if we force early deletion we could be affecting configured pod disruption budgets.

It does seem that MaxInt64 is a tad big excessive, but I would think that 20s is probably a bit short if we want to avoid evicting or deleting user workloads that could violate disruption budgets.

That said, if we want to respect PDBs, then we need to also ensure we are calling the eviction APIs by default as well: https://kubernetes.io/docs/concepts/workloads/pods/disruptions/#how-disruption-budgets-work

[namnx228]

Hi all, is there anyone who is working with this issue? If not, I'd like to take care of it. My plan is:

• First, add a NodeDrainTimeout parameter to KCP and machinedeployment, and these objects then pass the parameter to the corresponding machines.
• Next, set the time that the machine drains for the first time to its status/conditions.
• Finally, if the NodeDrainTimeout is over, forcefully delete the machine.
  How do you think about this solution?

[wfernandes]

Hey @namnx228 This issue is all yours. Feel free to /assign and mark it as /lifecycle active.

I believe this issue was specifically for worker machines and not the control plane nodes. However, if there is a use case for the control plane then I think this can be done all at once.

I don't have much context for this specifically but maybe @detiber and/or @vincepri can offer some insight. Else bring it up as a topic during this week's CAPI meeting.

/unassign

[namnx228]

/assign
/lifecycle active
@wfernandes that is great, Thanks.
Yes, there is an use case that we need to have workload running on control plane in baremetal, so it would be good to take care of KCP as well.
I will bring ask on Slack or in the meeting if I need more explanations.

[ncdc]

@namnx228 thanks for working on this!

First, add a NodeDrainTimeout parameter to KCP and machinedeployment, and these objects then pass the parameter to the corresponding machines.

I think it makes sense to add this to the KCP spec, but the MachineDeployment already has the MachineSpec, so I don't know that you'd need to add anything to the MachineDeployment (since you'll be adding a field to MachineSpec). WDYT?

[namnx228]

@ncdc yes, that is a good idea to make use of the machineSpec instead of making change to the machinedeployment. Thanks for your suggestion.