Skip to content

Bug 1868536: Watching the networkType in the status of Network.config.openshift.io#2015

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
pliurh:networkType
Aug 26, 2020
Merged

Bug 1868536: Watching the networkType in the status of Network.config.openshift.io#2015
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
pliurh:networkType

Conversation

@pliurh
Copy link
Contributor

@pliurh pliurh commented Aug 20, 2020

- What I did
It prevents the user from breaking the cluster network when configuring an invalid value.

- How to verify it

  1. oc patch networks.config.openshift.io cluster -p {"spec":{"networkType":"bad"}} --type=merge

Expect no reboot triggered by MCO

- Description for the changelog
Watching the networkType in the status of Network.config.openshift.io if it
is not empty, otherwise use the value in spec.

@openshift-ci-robot openshift-ci-robot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Aug 20, 2020
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Aug 20, 2020

@pliurh: This pull request references Bugzilla bug 1868536, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.6.0) matches configured target release for branch (4.6.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)
Details

In response to this:

Bug 1868536: Watching the networkType in the status of Network.config.openshift.io

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-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 Aug 20, 2020
@pliurh
Copy link
Contributor Author

pliurh commented Aug 20, 2020

@danwinship @squeed PTAL

danwinship
danwinship approved these changes Aug 20, 2020
View reviewed changes
Copy link
Contributor

@danwinship danwinship left a comment

Choose a reason for hiding this comment

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

/lgtm

The issue here is that CNO will reject some configuration changes (eg, trying to change the network type in an already-deployed cluster... except in certain special circumstances). So the MCO should not assume the network is changing just because the spec changed, it needs to wait until CNO updates the status.

Except, we don't want MCO to have to wait for CNO to be started up at install time, so in that case (when the status is still unset), it should set the initial state from the spec instead.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 20, 2020
@pliurh
Copy link
Contributor Author

pliurh commented Aug 21, 2020

/retest

kikisdeliveryservice
kikisdeliveryservice reviewed Aug 21, 2020
View reviewed changes
Copy link
Contributor

@kikisdeliveryservice kikisdeliveryservice left a comment

Choose a reason for hiding this comment

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

One request

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2020
@pliurh
Watching the networkType in the status of Network.config.openshift.io
94de592 
Watching the networkType in the status of  Network.config.openshift.io if it
is not empty, otherwise use the value in spec. It prevents user from breaking
the cluster network when configuring an invalid value.
@danwinship
Copy link
Contributor

danwinship commented Aug 24, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2020
@pliurh
Copy link
Contributor Author

pliurh commented Aug 25, 2020

/retest

1 similar comment
@pliurh
Copy link
Contributor Author

pliurh commented Aug 25, 2020

/retest

@pliurh pliurh mentioned this pull request Aug 25, 2020
Merged
@pliurh
Copy link
Contributor Author

pliurh commented Aug 26, 2020

/retest

@pliurh
Copy link
Contributor Author

pliurh commented Aug 26, 2020

/test e2e-ovn-step-registry
/test okd-e2e-aws

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Aug 26, 2020

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

Test name Commit Details Rerun command
ci/prow/e2e-ovn-step-registry 94de592 link /test e2e-ovn-step-registry
ci/prow/okd-e2e-aws 94de592 link /test okd-e2e-aws

Full PR test history. Your PR dashboard.

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.

@cgwalters
Copy link
Member

cgwalters commented Aug 26, 2020

/approve

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Aug 26, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, danwinship, pliurh

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-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 26, 2020
@openshift-bot
Copy link
Contributor

openshift-bot commented Aug 26, 2020

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit 16d243c into openshift:master Aug 26, 2020
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Aug 26, 2020

@pliurh: All pull requests linked via external trackers have merged:

Bugzilla bug 1868536 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1868536: Watching the networkType in the status of Network.config.openshift.io

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.

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

Reviewers

@runcom runcom Awaiting requested review from runcom

2 more reviewers

@kikisdeliveryservice kikisdeliveryservice kikisdeliveryservice left review comments

@danwinship danwinship danwinship approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@danwinship danwinship

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. 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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@pliurh @openshift-ci-robot @danwinship @cgwalters @openshift-bot @kikisdeliveryservice @openshift-merge-robot