Skip to content

Bug 1704706: remove openshift-sdn and multus CNI configuration files on boot #685

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
Merged
smarterclayton merged 1 commit into from
May 3, 2019
Merged

Bug 1704706: remove openshift-sdn and multus CNI configuration files on boot #685

smarterclayton merged 1 commit into from
May 3, 2019

Conversation

@squeed
Copy link
Contributor

@squeed squeed commented Apr 30, 2019

Discovered in BZ1704706 that we weren't actually cleaning these files
up, leading to failed deletions.

Fixes: rhbz1704706

@squeed
templates: actually remove CNI configuration files on boot
ac1924d 
Discovered in BZ1704706 that we weren't actually cleaning these files
up, leading to failed deletions.
@openshift-ci-robot openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Apr 30, 2019
@squeed
Copy link
Contributor Author

squeed commented Apr 30, 2019

cc @fpan @dcbw - the question is whether or not we should be removing the multus configuration on boot. I argue yes.

dougbtv
dougbtv approved these changes Apr 30, 2019
View reviewed changes
Copy link
Contributor

@dougbtv dougbtv left a comment

Choose a reason for hiding this comment

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

LGTM, this should make Multus wait for openshift-sdn config. Instead of finding its own and starting right away.

@squeed
Copy link
Contributor Author

squeed commented Apr 30, 2019

/retest

@runcom
Copy link
Member

runcom commented Apr 30, 2019

@squeed can you change the PR title to what agreeed on for BZ? (email thread from Clayton)

@squeed squeed changed the title templates: actually remove CNI configuration files on boot Bug 1704706: templates: actually remove CNI configuration files on boot Apr 30, 2019
@squeed squeed changed the title Bug 1704706: templates: actually remove CNI configuration files on boot Bug 1704706: remove openshift-sdn and multus CNI configuration files on boot Apr 30, 2019
@squeed
Copy link
Contributor Author

squeed commented Apr 30, 2019

Ah yeah, good point. Updated.

@runcom
Copy link
Member

runcom commented Apr 30, 2019

/approve

@dcbw @fpan can you ack on this?

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

cgwalters commented Apr 30, 2019

/approve

@fepan
Copy link

fepan commented Apr 30, 2019

LGTM.

@squeed
Copy link
Contributor Author

squeed commented Apr 30, 2019

someone needs to actually do the /lgtm to make it count.

I don't understand what's up with that operator test failure. Surely it's a flake, right?

@squeed
Copy link
Contributor Author

squeed commented Apr 30, 2019

/retest

@runcom
Copy link
Member

runcom commented Apr 30, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 30, 2019
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Apr 30, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, dougbtv, runcom, squeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details 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

openshift-bot commented May 1, 2019

/retest

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

2 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented May 1, 2019

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented May 1, 2019

/retest

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

@runcom
Copy link
Member

runcom commented May 1, 2019

/retest

@openshift-bot
Copy link
Contributor

openshift-bot commented May 1, 2019

/retest

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

4 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented May 1, 2019

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented May 1, 2019

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented May 1, 2019

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented May 1, 2019

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented May 2, 2019

/retest

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

3 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented May 2, 2019

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented May 2, 2019

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented May 2, 2019

/retest

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

@squeed
Copy link
Contributor Author

squeed commented May 2, 2019

Is there a chance I somehow broke something? This is never passing.

@cgwalters
Copy link
Member

cgwalters commented May 2, 2019

It's possible that e2e-aws-op is failing for unrelated reasons; looks like it's timing out. However it also reboots, which I don't think e2e-aws does today...so it will actually be testing removing the files on boot. As of this moment e2e-aws-upgrade is also not actually testing OS updates either...I'd guess it's not rebooting either. But probably what would make a ton of sense is to inject #682 into that suite.

@cgwalters
Copy link
Member

cgwalters commented May 2, 2019

Did you try doing this change live on a running cluster and rebooting nodes? (Probably simplest way to do that is to create a dummy MC for the workers)

@openshift-bot
Copy link
Contributor

openshift-bot commented May 2, 2019

/retest

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

@squeed
Copy link
Contributor Author

squeed commented May 2, 2019

Did you try doing this change live on a running cluster and rebooting nodes? (Probably simplest way to do that is to create a dummy MC for the workers)

Basically, yes. I created the file manually, rather than through the MCO, but yes. What is the test doing that goes above and beyond e2e-aws-upgrade?

@runcom
Copy link
Member

runcom commented May 2, 2019

if something is broken in e2e-aws-op (as opposed as just being a legit timeout), you should see that in MCD logs in artifacts (probably have an error looping)

@openshift-bot
Copy link
Contributor

openshift-bot commented May 2, 2019

/retest

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

2 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented May 2, 2019

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented May 2, 2019

/retest

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

@kikisdeliveryservice
Copy link
Contributor

kikisdeliveryservice commented May 2, 2019

We are seeing these errors in e2e-aws-op across all prs now, so expect this to keep failing until we get a fix.

@openshift-bot
Copy link
Contributor

openshift-bot commented May 2, 2019

/retest

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

4 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented May 2, 2019

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented May 2, 2019

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented May 3, 2019

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented May 3, 2019

/retest

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

@knobunc
Copy link
Contributor

knobunc commented May 3, 2019

/retest

@openshift-bot
Copy link
Contributor

openshift-bot commented May 3, 2019

/retest

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

@kikisdeliveryservice
Copy link
Contributor

kikisdeliveryservice commented May 3, 2019

FTR: this PR has passed the now failing on all PRs e2e-aws-op job at least once on April 30th, at 9:40 PST:
https://openshift-gce-devel.appspot.com/build/origin-ci-test/pr-logs/pull/openshift_machine-config-operator/685/pull-ci-openshift-machine-config-operator-master-e2e-aws-op/1918

@smarterclayton
Copy link
Contributor

smarterclayton commented May 3, 2019

Force merging at team's request, test failures are unrelated and separate investigation stream is covering them.

@smarterclayton smarterclayton merged commit 8749349 into openshift:master May 3, 2019
@dcbw
Copy link
Contributor

dcbw commented May 3, 2019

@squeed FTR yes, we should be removing Multus's config too.

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented May 3, 2019

@squeed: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-aws-op ac1924d link /test e2e-aws-op

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.

Details

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.

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

Reviewers

@ashcrow ashcrow Awaiting requested review from ashcrow

@kikisdeliveryservice kikisdeliveryservice Awaiting requested review from kikisdeliveryservice

1 more reviewer

@dougbtv dougbtv dougbtv approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@runcom runcom

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/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.