Skip to content

Conversation

@runcom
Copy link
Member

@runcom runcom commented Oct 3, 2019

Honoring the drain's library termination timeout can cause unnecessary
timeout on delete.
The MCO is wronly setting a 600s termination timeout for every pod which
is wrong.
Fix MCO and any other user who might have set an hard timeout exceeding
the pod's termination timeout.

Signed-off-by: Antonio Murdaca runcom@linux.com

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 3, 2019
@runcom runcom force-pushed the drain-fix-timeout branch from 0248315 to f7081a8 Compare October 3, 2019 21:07
@openshift-ci-robot openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Oct 3, 2019
@runcom runcom changed the title pkg/drain: always honor pod termination timeout Bug 1758343: pkg/drain: always honor pod termination timeout Oct 3, 2019
@openshift-ci-robot openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Oct 3, 2019
@openshift-ci-robot
Copy link

@runcom: This pull request references Bugzilla bug 1758343, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1758343: pkg/drain: always honor pod termination timeout

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.

@runcom
Copy link
Member Author

runcom commented Oct 3, 2019

@enxebre @michaelgugino ptal

@runcom runcom force-pushed the drain-fix-timeout branch from f7081a8 to 3afe7e1 Compare October 3, 2019 21:21
Copy link

@michaelgugino michaelgugino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. Let's add a BZ link for tracking.

@runcom
Copy link
Member Author

runcom commented Oct 3, 2019

This looks good to me. Let's add a BZ link for tracking.

it's linked in the title here and #130 (comment) - do you want it in the commit message as well?

Copy link

@michaelgugino michaelgugino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 3, 2019
@runcom runcom force-pushed the drain-fix-timeout branch from 3afe7e1 to 412f4a9 Compare October 3, 2019 22:19
@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 3, 2019
@wking
Copy link
Member

wking commented Oct 3, 2019

Document the pod/library interaction on the option type? (if we keep the option; I'm fine dropping it).

@runcom
Copy link
Member Author

runcom commented Oct 3, 2019

Document the pod/library interaction on the option type?

I'll hold that on #130 (comment)

… timeout

Honoring the drain's library termination timeout can cause unnecessary
timeout on delete.
The MCO is wronly setting a 600s termination timeout for every pod which
is wrong.
Fix MCO and any other user who might have set an hard timeout exceeding
the pod's termination timeout.

Signed-off-by: Antonio Murdaca <runcom@linux.com>
@runcom runcom force-pushed the drain-fix-timeout branch from 412f4a9 to b891cff Compare October 4, 2019 07:23
@runcom
Copy link
Member Author

runcom commented Oct 4, 2019

Document the pod/library interaction on the option type?

ok, fixed, I believe it's good to go now since there's quite a bit of a rush to get this in and get the 4.2 blocker fixed in MCO

@runcom
Copy link
Member Author

runcom commented Oct 4, 2019

/retest

@openshift-cherrypick-robot

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

In response to this:

/cherrypick release-4.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.

@enxebre
Copy link
Member

enxebre commented Oct 4, 2019

/approve
thanks @runcom! is this being reported upstream? if so can you post the link here? We have already reported other fixes included here kubernetes/kubernetes#81333

We are in the process of migrating to release-4.X branches. Meanwhile, for porting
4.2 openshift-4.2-cluster-api-0.1.0
4.1 openshift-4.1-cluster-api-0.0.0-alpha.4

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

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

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 4, 2019
@runcom
Copy link
Member Author

runcom commented Oct 4, 2019

/cherrypick openshift-4.2-cluster-api-0.1.0

@openshift-cherrypick-robot

@runcom: once the present PR merges, I will cherry-pick it on top of openshift-4.2-cluster-api-0.1.0 in a new PR and assign it to you.

In response to this:

/cherrypick openshift-4.2-cluster-api-0.1.0

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.

@enxebre
Copy link
Member

enxebre commented Oct 4, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 4, 2019
@runcom
Copy link
Member Author

runcom commented Oct 4, 2019

/cherrypick openshift-4.1-cluster-api-0.0.0-alpha.4

@openshift-cherrypick-robot

@runcom: once the present PR merges, I will cherry-pick it on top of openshift-4.1-cluster-api-0.0.0-alpha.4 in a new PR and assign it to you.

In response to this:

/cherrypick openshift-4.1-cluster-api-0.0.0-alpha.4

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.

@openshift-merge-robot openshift-merge-robot merged commit 83f32d3 into openshift:master Oct 4, 2019
@openshift-ci-robot
Copy link

@runcom: All pull requests linked via external trackers have merged. Bugzilla bug 1758343 has been moved to the MODIFIED state.

In response to this:

Bug 1758343: pkg/drain: always honor pod termination timeout

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.

@openshift-cherrypick-robot

@runcom: new pull request created: #131

In response to this:

/cherrypick openshift-4.2-cluster-api-0.1.0

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.

@openshift-cherrypick-robot

@runcom: new pull request created: #132

In response to this:

/cherrypick openshift-4.1-cluster-api-0.0.0-alpha.4

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.

@wking
Copy link
Member

wking commented Oct 4, 2019

Just to make sure, the local consumer has always set the timeout -1, so all good there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants