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

Only start the SDN controller if an openshift network plugin is configured #16060

Merged
merged 1 commit into from
Sep 5, 2017
Merged

Only start the SDN controller if an openshift network plugin is configured #16060

merged 1 commit into from
Sep 5, 2017

Conversation

liggitt
Copy link
Contributor

@liggitt liggitt commented Aug 30, 2017 •
edited
Loading

Returning an error causes a process exit. This logs and skips instead.

@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 30, 2017
@liggitt liggitt assigned danwinship and deads2k and unassigned fabianofranz and 0xmichalis Aug 30, 2017
@deads2k
Copy link
Contributor

deads2k commented Aug 30, 2017

/lgtm
/retest-not-required

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

liggitt commented Aug 30, 2017

"FAILURE: PROVISION TEST CLUSTER"
/retest

@smarterclayton
Copy link
Contributor

Yikes. Wish this was easy to test automatically.

danwinship
danwinship reviewed Aug 30, 2017
View reviewed changes
pkg/cmd/server/origin/controller/network_unsupported.go Outdated
@@ -13,5 +13,6 @@ type SDNControllerConfig struct {
}

func (c *SDNControllerConfig) RunController(ctx ControllerContext) (bool, error) {
return false, fmt.Errorf("SDN not supported on this platform")
glog.Errorf("SDN not supported on this platform")
Copy link
Contributor

Choose a reason for hiding this comment

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

Oops, I was thinking this only even got called if the config specified networkPluginName: redhat/openshift-ovs-multitenant, etc. But that's checked elsewhere.

I think this should do:

if sdn.IsOpenShiftNetworkPlugin(c.NetworkConfig.NetworkPluginName) {
        return false, fmt.Errorf("SDN not supported on this platform")
} else {
        return false, nil
}

(where sdn is "github.com/openshift/origin/pkg/sdn")

Then if master-config.yaml says to use the SDN plugin, it will fail to start. But otherwise it will just log Skipping "openshift.io/sdn"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the linux controller doesn't check sdn.IsOpenShiftNetworkPlugin... should it?

@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 31, 2017
@openshift-merge-robot openshift-merge-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2017
@liggitt
Copy link
Contributor Author

liggitt commented Aug 31, 2017

turns out, the linux controller wasn't checking if the configured network plugin required the sdn controller, so this is now a bugfix. @danwinship PTAL

@liggitt liggitt changed the title Allow openshift start master to work on non-linux platforms Only start the SDN controller if an openshift network plugin is configured Aug 31, 2017
danwinship
danwinship approved these changes Aug 31, 2017
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.

turns out, the linux controller wasn't checking if the configured network plugin required the sdn controller

yeah, sdnmaster.Start() checks that itself. But this is better since the "return false" will make it log that it's skipping that controller

@danwinship
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 31, 2017
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2017
@smarterclayton
Copy link
Contributor

neeeeeds..... reeeeebaaaaasssseeeeee

@openshift-merge-robot openshift-merge-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 1, 2017
@danwinship
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 1, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, deads2k, liggitt

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@smarterclayton
Copy link
Contributor

smarterclayton commented Sep 1, 2017 via email

@smarterclayton
Copy link
Contributor

smarterclayton commented Sep 2, 2017 via email

@openshift-bot
Copy link
Contributor

/retest

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

16 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.

@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-merge-robot
Copy link
Contributor

Automatic merge from submit-queue

@openshift-merge-robot openshift-merge-robot merged commit 047c984 into openshift:master Sep 5, 2017
@liggitt liggitt deleted the sdn-darwin branch September 5, 2017 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danwinship danwinship danwinship approved these changes

Assignees

@danwinship danwinship

@deads2k deads2k

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@liggitt @deads2k @smarterclayton @danwinship @openshift-merge-robot @openshift-bot @fabianofranz @0xmichalis @openshift-ci-robot