controller/bootstrap: use files with multiple yaml documents #577

abhinavdahiya · 2019-03-26T00:40:03Z

- What I did
Users can push manifests during bootstrap that of the form:

---

Especially for the installer: setting authorizes_keys 1 and setting hyperthreading 2 will push a manifest that includes multiple machineconfig objects for
control-plane (master) and compute (worker) roles.

Single file with multiple k8s objects separated by --- is also a supported structure for oc create|apply ie. there is a high chance that users trying to push machineconfigs at
install time might create such files.

This commit allows bootstrap controller to read all k8s objects, even ones described above to find all the machineconfiguration.openshift.io Objects.

- How to verify it

Added a unit test.

- Description for the changelog

Allow bootstrap controller to read multi object yaml files

Users can push manifests during bootstrap that of the form: ```yaml --- ``` Especially for the installer: setting authorizes_keys [1] and setting hyperthreading [2] will push a manifest that includes multiple machineconfig objects for control-plane (master) and compute (worker) roles. Single file with multiple k8s objects separated by `---` is also a supported structure for `oc create|apply` ie. there is a high chance that users trying to push machineconfigs at install time might create such files. This commit allows bootstrap controller to read all k8s objects, even ones described above to find all the `machineconfiguration.openshift.io` Objects. [1]: openshift/installer#1150 [2]: openshift/installer#1392

runcom · 2019-03-26T10:37:01Z

/approve
/retest

/assign @cgwalters

cgwalters

One minor nit, otherwise LGTM

cgwalters · 2019-03-26T12:53:59Z

pkg/controller/bootstrap/bootstrap.go

+			obji, err := runtime.Decode(scheme.Codecs.UniversalDecoder(v1.SchemeGroupVersion), m.Raw)
+			if err != nil {
+				glog.V(4).Infof("skipping path %q [%d] manifest because of error: %v", file.Name(), idx+1, err)
+				// don't care


I know this isn't a problem introduced in this patch, but I think silently ignoring errors is going to lead someone to a lot of debugging pain at some point.

pushed df19bd7 that makes decoding of invalid objects in mco apigroup fatal.

cgwalters · 2019-03-26T12:54:47Z

pkg/controller/bootstrap/bootstrap.go

+			case *v1.ControllerConfig:
+				cconfig = obj
+			default:
+				glog.Infof("skipping %q [%d] manifest because %T", file.Name(), idx+1, obji)


s/because %T/of unhandled type %T/ ?

When decoding a manifest we get 2 kinds of errors: - this manifest does not belong to mco apigroup and hence we do not care about these failures and we need to skip. - the manifest does belong to mco apigroup, but it cannot be decoded into any of the known types, this needs to be a failure. Using IsNotRegistreredError [1] allows aus to differentiate between the 2 errors. And therefore, later kind of error is now failure. [1]: https://godoc.org/k8s.io/apimachinery/pkg/runtime#IsNotRegisteredError

cgwalters · 2019-03-26T18:46:43Z

/approve
/lgtm

openshift-ci-robot · 2019-03-26T18:46:50Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [cgwalters,runcom]

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

openshift-bot · 2019-03-26T19:43:37Z

/retest