Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Increase termination grace period to 45s to allow for flows to get saved #511

Merged
merged 1 commit into from Mar 5, 2020
Merged

Increase termination grace period to 45s to allow for flows to get saved #511

merged 1 commit into from Mar 5, 2020

Conversation

abhat
Copy link
Contributor

@abhat abhat commented Mar 3, 2020 •
edited

This pull request changes the termination grace period to 45s from 10s to allow for saving off flows prior to the pod getting a SIGKILL.

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 3, 2020
@abhat
Copy link
Contributor Author

abhat commented Mar 3, 2020

/assign @dcbw @smarterclayton

@juanluisvaladas
Copy link
Contributor

/retest
/lgtm
Until we have ovs as a system daemon, seems like a reasonable thing to do.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2020
@danwinship
Copy link
Contributor

Do we have some reason/evidence for thinking that 10s is not enough time for it to save the flows and 45s is?

@abhat
Copy link
Contributor Author

abhat commented Mar 4, 2020

Do we have some reason/evidence for thinking that 10s is not enough time for it to save the flows and 45s is?

Yes. For instance here: https://storage.googleapis.com/origin-ci-test/logs/release-openshift-origin-installer-e2e-aws-upgrade/19835/artifacts/e2e-aws-upgrade/pods/openshift-sdn_ovs-92p4b_openvswitch_previous.log, we see that the flows are being saved, and before we see a debug line saying "info: Saved flows", we see that the pod gets SIGKILL'd because of the terminationGracePeriodSeconds expiring.

@abhat
Copy link
Contributor Author

abhat commented Mar 4, 2020

/retest

@abhat
Copy link
Contributor Author

abhat commented Mar 4, 2020

/test e2e-gcp-ovn

1 similar comment
@abhat
Copy link
Contributor Author

abhat commented Mar 4, 2020

/test e2e-gcp-ovn

@danwinship
Copy link
Contributor

Yes. For instance here

Thank you. PLEASE put information like this in PRs so that people trying to understand the code in the future can see why we changed things

/lgtm

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhat, danwinship, juanluisvaladas

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 Mar 4, 2020
@smarterclayton
Copy link
Contributor

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@smarterclayton
Copy link
Contributor

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

7 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

9 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@smarterclayton
Copy link
Contributor

I’m merging this to give us more soak in master on upgrades. OVN failures are unrelated (and a bug in OVN if so).

@smarterclayton smarterclayton merged commit 3ebb4d2 into openshift:master Mar 5, 2020
@openshift-ci-robot
Copy link
Contributor

@abhat: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-gcp-ovn a47ecd3 link /test e2e-gcp-ovn
ci/prow/e2e-gcp a47ecd3 link /test e2e-gcp

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@abhat abhat deleted the save_fix branch March 6, 2020 19:40
@abhat
Copy link
Contributor Author

abhat commented Oct 26, 2020

/cherry-pick release-4.4

@openshift-cherrypick-robot
Copy link

@abhat: #511 failed to apply on top of branch "release-4.4":

Applying: Increase termination grace period to 45s to allow for flows to get saved
Using index info to reconstruct a base tree...
M	bindata/network/openshift-sdn/sdn-ovs.yaml
Falling back to patching base and 3-way merge...
Auto-merging bindata/network/openshift-sdn/sdn-ovs.yaml
CONFLICT (content): Merge conflict in bindata/network/openshift-sdn/sdn-ovs.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Increase termination grace period to 45s to allow for flows to get saved
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

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

@abhat abhat mentioned this pull request Oct 26, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JacobTanenbaum JacobTanenbaum Awaiting requested review from JacobTanenbaum

@juanluisvaladas juanluisvaladas Awaiting requested review from juanluisvaladas

Assignees

@danwinship danwinship

@smarterclayton smarterclayton

@juanluisvaladas juanluisvaladas

@dcbw dcbw

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@abhat @juanluisvaladas @danwinship @openshift-ci-robot @smarterclayton @openshift-bot @openshift-cherrypick-robot @dcbw