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

mco: If the platform is unrecognized, treat the platform as 'none' #340

Merged
merged 1 commit into from
Jan 28, 2019

Conversation

smarterclayton
Copy link
Contributor

In order to incrementally roll new platforms out across the project
we must be able to provision platforms before all support is added.
Components should treat an unrecognized platform as "None", performing
the same function as they would in that scenario, and print a warning
to the logs.

Formalizing this requirement as we assess how future providers will
be added in openshift/installer#1112 (found in openshift/installer#1109)

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 23, 2019
@openshift-ci-robot openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 23, 2019
@smarterclayton
Copy link
Contributor Author

/assign @abhinavdahiya

Spitballed this a bit on slack today, I want to lay some foundations as I go and incrementally get components ready for this.

@abhinavdahiya
Copy link
Contributor

/approve

@smarterclayton
Copy link
Contributor Author

/retest

@cgwalters
Copy link
Member

I think this needs something like:

diff --git a/pkg/controller/template/render.go b/pkg/controller/template/render.go
index 05758bb..71cac20 100644
--- a/pkg/controller/template/render.go
+++ b/pkg/controller/template/render.go
@@ -49,12 +49,15 @@ const (
 //                                    /files/hostname.tmpl
 //
 func generateMachineConfigs(config *RenderConfig, templateDir string) ([]*mcfgv1.MachineConfig, error) {
-	if config.Platform == "" {
-		return nil, fmt.Errorf("cannot generateMachineConfigs with an empty Platform")
-	}
+	platform := config.Platform
 
-	if config.Platform == "_base" {
+	if platform == "" {
+		return nil, fmt.Errorf("cannot generateMachineConfigs with an empty Platform")
+	} else if platform == "_base" {
 		return nil, fmt.Errorf("platform _base unsupported")
+	} else if platform == "none" {
+		// none is the supported name for our internal "_base" to
+		// be used during bringup of new platforms.
 	}
 
 	infos, err := ioutil.ReadDir(templateDir)
@@ -125,8 +128,12 @@ func GenerateMachineConfigsForRole(config *RenderConfig, role string, path strin
 }
 
 func generateMachineConfigForName(config *RenderConfig, role, name, path string) (*mcfgv1.MachineConfig, error) {
+	expectedPlatformDirs := []string{"_base"}
+	if config.Platform != "base" {
+		expectedPlatformDirs = append(expectedPlatformDirs, config.Platform)
+	}
 	platformDirs := []string{}
-	for _, dir := range []string{"_base", config.Platform} {
+	for _, dir := range expectedPlatformDirs {
 		platformPath := filepath.Join(path, dir)
 		exists, err := existsDir(platformPath)
 		if err != nil {

too

@smarterclayton
Copy link
Contributor Author

/retest

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 23, 2019
@smarterclayton
Copy link
Contributor Author

I kept a similar structure and fixed the test.

This highlighted something that the MCO API needs before GA - we need to be using consistent constants (the constants in https://github.com/openshift/api/blob/master/config/v1/types_infrastructure.go#L41 need to be used everywhere) @abhinavdahiya also in install config

@smarterclayton
Copy link
Contributor Author

/retest

@kikisdeliveryservice
Copy link
Contributor

@smarterclayton I opened an issue to make constants consistent in MCO: #342

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 23, 2019
@smarterclayton
Copy link
Contributor Author

I believe I found a bug, if Platform was empty on ControllerConfig before we would have looked at the root template dir incorrectly when trying to find templates (we weren't guarding some of the config generation with an empty string check, which this PR fixes).

@smarterclayton
Copy link
Contributor Author

/retest

1 similar comment
@ashcrow
Copy link
Member

ashcrow commented Jan 24, 2019

/retest

@smarterclayton
Copy link
Contributor Author

Any other comments?

In order to incrementally roll new platforms out across the project
we must be able to provision platforms before all support is added.
Components should treat an unrecognized platform as "None", performing
the same function as they would in that scenario, and print a warning
to the logs.

This commit changes the generation so that a ControllerConfig with an
empty platform returns an error instead of silently running.

Ensure we test config on multiple platforms.
@smarterclayton
Copy link
Contributor Author

/retest

@smarterclayton
Copy link
Contributor Author

Gentle ping ;)

@abhinavdahiya
Copy link
Contributor

Gentle ping ;)

/approve

@abhinavdahiya
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, smarterclayton

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:
  • OWNERS [abhinavdahiya,smarterclayton]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kikisdeliveryservice
Copy link
Contributor

/test e2e-aws

@kikisdeliveryservice
Copy link
Contributor

Error in e2e-aws was ye olde:

level=info msg="Waiting up to 30m0s for the bootstrap-complete event..."
ERROR: logging before flag.Parse: E0128 22:02:35.173080      30 streamwatcher.go:109] Unable to decode an event from the watch stream: http2: server sent GOAWAY and closed the connection; LastStreamID=3, ErrCode=NO_ERROR, debug=""
level=warning msg="RetryWatcher - getting event failed! Re-creating the watcher. Last RV: 3250

@openshift-merge-robot openshift-merge-robot merged commit 2875046 into openshift:master Jan 28, 2019
osherdp pushed a commit to osherdp/machine-config-operator that referenced this pull request Apr 13, 2021
Bug 1898745: actually set imagestreams in progress to false in imagestream event path
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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants