Add Eviction Timeout Field for Maintenance Time #122
Conversation
Configurable drain/deletion timeout for the user. ATM it was set to 30s
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: razo7 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 4.15-openshift-e2e |
| @@ -229,7 +229,7 @@ func (r *NodeMaintenanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ | |||
| } | |||
|
|
|||
| // createDrainer creates a drain.Helper struct for external cordon and drain API | |||
| func createDrainer(ctx context.Context, mgrConfig *rest.Config) (*drain.Helper, error) { | |||
| func createDrainer(ctx context.Context, evicitonTimeout time.Duration, mgrConfig *rest.Config) (*drain.Helper, error) { | |||
| //The length of time to wait before giving up, zero means infinite | ||
| drainer.Timeout = DrainerTimeout | ||
| drainer.Timeout = evicitonTimeout |
There was a problem hiding this comment.
what's going on with the old const now?
There was a problem hiding this comment.
Redundant and can be removed. The old value (30s) is the default value of the evicitonTimeout field.
Using 50s evicitonTimeout to demonstrate a normal maintenance that won't reach eviction timeout and 10s evicitonTimeout for maintenance that will reach the eviction timeout
|
/test 4.15-openshift-e2e |
|
/test 4.15-openshift-e2e |
|
@razo7: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
| @@ -58,7 +58,7 @@ var _ = Describe("Starting Maintenance", func() { | |||
| if controPlaneMaintenance == nil { | |||
| // do this once only | |||
| controlPlaneNode = controlPlaneNodes[0] | |||
| controPlaneMaintenance = getNodeMaintenance(fmt.Sprintf("test-1st-control-plane-%s", controlPlaneNode), controlPlaneNode) | |||
| controPlaneMaintenance = getNodeMaintenance(fmt.Sprintf("test-1st-control-plane-%s", controlPlaneNode), controlPlaneNode, evicitonTimeout) | |||
There was a problem hiding this comment.
typo (multiple occurrences) evicitonTimeout -> evictionTimeout
| DeferCleanup(Client.Delete, context.TODO(), nodeMaintenance) | ||
| }) | ||
|
|
||
| When("evction timeout is low", func() { |
| // check operator log showing it reconciled with fixed duration because of drain timeout | ||
| // it should be caused by the test deployment's termination graceperiod > drain timeout | ||
| Expect(getOperatorLogs()).To(ContainSubstring(nodemaintenance.FixedDurationReconcileLog)) | ||
| Expect(getOperatorLogs()).NotTo(ContainSubstring(nodemaintenance.FixedDurationReconcileLog)) |
| By("verify printed log") | ||
| // check operator log showing it reconciled with fixed duration because of drain timeout | ||
| // it should be caused by the test deployment's termination graceperiod > drain timeout | ||
| Expect(getOperatorLogs()).To(ContainSubstring(nodemaintenance.FixedDurationReconcileLog)) |
There was a problem hiding this comment.
- comment doesn't match because you removed the termination graceperiod
- how do you ensure the pod isn't evicted within the drain timeout without termination graceperiod?
There was a problem hiding this comment.
comment doesn't match because you removed the termination graceperiod
There is a default value of 30 seconds for the graceperiod when it isn't set
|
Closing this PR as its goal is minor, it introduces a possible longer reconciliation (while we strive for a short reconcile), an API change, and the gain doesn't worth the effort |
What this PR does / why we need it:
Adds a new Spec field for the pod's eviction timeout. Configurable pod's eviction drain/deletion timeout for the user, while previously it was set to 30s (the default value).
Changes made
Which issue(s) this PR fixes:
ECOPROJECT-1668
Test plan
Add e2e test for different
evicitonTimeoutvalues - high (50s) vs. low (10s)