Skip to content

Conversation

@flaper87
Copy link
Contributor

In order to start OpenShift with the OpenStack provider enabled, there ought to be a config file in the server. The Openshift deployment on OpenStack is currently failing because such file is not present.

This patch, combined with openshift/installer#611 aims to create the config file in the destination node using the cloud provided by the operator during the dpeloyment.

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 15, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: flaper87
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: wking

If they are not already assigned, you can assign the PR to them by writing /assign @wking in a comment when ready.

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

@flaper87
Copy link
Contributor Author

/cc @tomassedovic @hardys

@ashcrow
Copy link
Member

ashcrow commented Nov 15, 2018

Looks like "github.com/gophercloud/utils/openstack/clientconfig" hasn't been vendored. The golang tool dep is used for this project.

vendor/github.com/openshift/installer/pkg/types/installconfig.go:6:2: cannot find package "github.com/gophercloud/utils/openstack/clientconfig" in any of:
	/go/src/github.com/openshift/machine-config-operator/vendor/github.com/gophercloud/utils/openstack/clientconfig (vendor tree)
	/usr/local/go/src/github.com/gophercloud/utils/openstack/clientconfig (from $GOROOT)
	/go/src/github.com/gophercloud/utils/openstack/clientconfig (from $GOPATH)

Copy link
Contributor

Choose a reason for hiding this comment

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

Should there be a default in this switch statement?

Copy link
Contributor

Choose a reason for hiding this comment

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

you can return error

@abhinavdahiya
Copy link
Contributor

@flaper87 the cloud provider config is usually a list of options.
I am open to MCO / MCC understanding a configmap source for getting the cloud provider config contents rather than the marshal/unmarshal in this PR.

@sjenning
Copy link
Contributor

i just hit this issue just this morning trying to do an install. glad to see someone is already on it 👍

@flaper87
Copy link
Contributor Author

Thakn y'all for the review!

@abhinavdahiya a config map should also work. Should the config map be created by the installer or by MCO directly? One problem we have on the openstack provider side is that this file has to exist in the filesystem (at least until I manage to remove this constraint).

Would it be ok for us to start with this approach and iterate towards using configmaps?

@flaper87 flaper87 force-pushed the cloud-provider-conf branch from 3f4c7e2 to 1c87bef Compare November 16, 2018 10:16
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 16, 2018
@flaper87
Copy link
Contributor Author

This won't work until openshift/installer#611 lands but I'd like to get some consensus on this one before merging the installer patch.

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 16, 2018
@abhinavdahiya
Copy link
Contributor

Thakn y'all for the review!

@abhinavdahiya a config map should also work. Should the config map be created by the installer or by MCO directly? One problem we have on the openstack provider side is that this file has to exist in the filesystem (at least until I manage to remove this constraint).

Would it be ok for us to start with this approach and iterate towards using configmaps?

I am pretty sure all clouds will eventually have need for a cloud config file, like azure...
So I think config map approach is going to be the best way forward. I'll try to work something up today to see how much effort that will be.

@flaper87
Copy link
Contributor Author

@abhinavdahiya awesome, Thanks!

So, lemme recap and see if I understood correctly. The installer will create a configmap that MCO will use and from which (at least in this case) we'll be able to create the required file on the node FS. correct?

@abhinavdahiya
Copy link
Contributor

installer has https://github.com/openshift/installer/blob/master/data/data/manifests/bootkube/kube-cloud-config.yaml which is empty right now. i think we should populate that config map and then teach MC* to use it.

@flaper87
Copy link
Contributor Author

@abhinavdahiya sounds good to me, I'll give it a go!

@derekwaynecarr
Copy link
Member

The config map should be editable by end users on bring your own infra environments as noted for azure, gce, and vsphere. We do need this PR to update so kubelet is passed the flag once the final source is available.

@flaper87
Copy link
Contributor Author

@abhinavdahiya @derekwaynecarr @sjenning hey folks, I'd really like to move this PR forward as we're kinda blocked on not being able to start the kubelet due to the missing cloud operator config. What can I do to help move this forward? Any indication on what I should do next?

@sjenning
Copy link
Contributor

need to rebase and run go test ./pkg/controller/template/... -u to update the test_data (why the ci/prow/unit job is failing)

@sjenning
Copy link
Contributor

If it can be made to pass e2e-aws, then it seems fine to me. I'll see if I can test in on my Openstack cluster soon.

@flaper87 flaper87 force-pushed the cloud-provider-conf branch from 1c87bef to 7995451 Compare November 27, 2018 08:08
@flaper87
Copy link
Contributor Author

@sjenning done! Thanks for the feedback :)

@flaper87 flaper87 force-pushed the cloud-provider-conf branch from 7995451 to cd9da15 Compare November 27, 2018 10:51
inline: |
{{with cloudProviderConfig .}}
[Global]
username={{.Cloud.AuthInfo.Username}}
Copy link
Member

Choose a reason for hiding this comment

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

will this make the username and pw generally visible from a machineconfig?

if so, this seems like a problem since the ignition configs are not generally protected.

we need to determine a way to deliver secret data to nodes securely.

@rphillips
Copy link
Contributor

Looks like a number of these files need a gofmt.

@openshift-ci-robot
Copy link
Contributor

@flaper87: PR needs rebase.

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.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 7, 2018
@frobware
Copy link

/cc @frobware

@kikisdeliveryservice
Copy link
Contributor

how does #591 affect this PR?

@openshift-ci-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
ci/prow/e2e-aws cd9da15 link /test e2e-aws
ci/prow/images cd9da15 link /test images
ci/prow/rhel-images cd9da15 link /test rhel-images
ci/prow/e2e-aws-op cd9da15 link /test e2e-aws-op
ci/prow/verify cd9da15 link /test verify
ci/prow/e2e-aws-upgrade cd9da15 link /test e2e-aws-upgrade

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.

@flaper87 flaper87 closed this Jul 9, 2019
osherdp pushed a commit to osherdp/machine-config-operator that referenced this pull request Apr 13, 2021
…try-override

Bug 1744071: allow override of jenkins* imagestream registries for disconnected/mi…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants