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.

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

Removes configuration invalidation #50

Merged

Conversation

dougbtv
Copy link
Member

@dougbtv dougbtv commented Feb 24, 2020

Experimental commit, do not merge. Hacks in method for not removing config invalidation.

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 24, 2020
@smarterclayton
Copy link

/test all

@smarterclayton
Copy link

/retest

2 similar comments
@smarterclayton
Copy link

/retest

@smarterclayton
Copy link

/retest

@smarterclayton
Copy link

/retest

There's a very good chance that this fixed the openshift-apiserver going unavailable during upgrades (https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/pr-logs/pull/openshift_multus-cni/50/pull-ci-openshift-multus-cni-master-e2e-aws-upgrade/77) - since that test has consistently failed since it was introduced, but it's passed here. Which is excellent news - @deads2k were there any other changes landed in the last few days that could have made this start passing, or can we infer causation from this change?

@smarterclayton
Copy link

/retest

@smarterclayton
Copy link

This time the api was down for only 11s, which is a big improvement.

I think this passes the bar for me "value in coasting" > "slightly complexity increase of cleaning up".

So I'm supportive of this going into 4.5, us assessing it, and then back porting after a few days/week of improvement.

@smarterclayton
Copy link

/retest

@smarterclayton
Copy link

It looks like the upgrade is broken at 79% and multus container is crash looping:

/entrypoint.sh: line 17: syntax error near unexpected token `}'

@dougbtv
Copy link
Member Author

dougbtv commented Feb 26, 2020

Thanks for the eyes on the shell script error. I've got a new commit up that should address that.

If this look OK with this one, I'll put together a final product that can go both upstream/downstream.

@smarterclayton
Copy link

I'm going to run an upgrade job in the reverse order (from this PR to master) to see that it fixes the disruption (since the current path won't because we upgrade from not this code to having this code and the check is on teardown).

@smarterclayton
Copy link

/test images

2 similar comments
@smarterclayton
Copy link

/test images

@smarterclayton
Copy link

/test images

@smarterclayton
Copy link

Ok, https://storage.googleapis.com/origin-ci-test/logs/release-openshift-origin-installer-e2e-aws-upgrade/19234/build-log.txt (in https://prow.svc.ci.openshift.org/view/gcs/origin-ci-test/logs/release-openshift-origin-installer-e2e-aws-upgrade/19234) looks like it successfully met the criteria (node didn't go notready during multus upgrade).

This improves the outcome of upgrade by reducing unrelated failures, so lgtm in terms of approach (still needs code review).

Copy link
Contributor

@s1061123 s1061123 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 Feb 28, 2020
@s1061123
Copy link
Contributor

/retest

@smarterclayton
Copy link

Do we need to change this code? Can't we just set MULTUS_CLEANUP_CONFIG_ON_EXIT=false on the multus pod via the CNO?

@smarterclayton
Copy link

/hold

for the question

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 28, 2020
@s1061123
Copy link
Contributor

As far as I look the code, following line does not have "if $MULTUS_CLEANUP_CONFIG_ON_EXIT" so need to modify diffs to keep the flag.

https://github.com/openshift/multus-cni/pull/50/files#diff-5b59de2786d0584351695a727ff60b59L389-L392

@dougbtv
Copy link
Member Author

dougbtv commented Mar 2, 2020

Two parts to the cleanup, invalidating the config (which this commit removes) and the other -- regenerating the config after an sdn upgrade (which this does not remove) so... I recommend we move forward with what's here (ok to merge but we'll need to sync with upstream later)

@dougbtv dougbtv force-pushed the test-no-config-invalidation branch from 6911259 to 50da638 Compare March 2, 2020 15:10
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 2, 2020
@dougbtv
Copy link
Member Author

dougbtv commented Mar 2, 2020

/title Removes configuration invalidation

@s1061123
Copy link
Contributor

s1061123 commented Mar 2, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 2, 2020
@dougbtv
Copy link
Member Author

dougbtv commented Mar 2, 2020

/retitle Removes configuration invalidation

@openshift-ci-robot openshift-ci-robot changed the title [WIP] Removes configuration invalidation Removes configuration invalidation Mar 2, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 2, 2020
@dougbtv
Copy link
Member Author

dougbtv commented Mar 2, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 2, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dougbtv, s1061123

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-bot
Copy link
Contributor

/retest

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

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

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

@dougbtv
Copy link
Member Author

dougbtv commented Mar 2, 2020

/retest

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. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants