Skip to content

Conversation

@flaper87
Copy link
Contributor

@flaper87 flaper87 commented Nov 5, 2018

We need to pass the cloud's config data to the MCO in order to be able
to configure the cloud provider openstack. This commit passes such data
as part of the platform configuration since MCO consumes install-config
to get the cluster conifguration.

As part of this commit, we're now also validating the user's input to
verify whether the cloud name passed is correct.

This PR depends on #588

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 5, 2018
@flaper87 flaper87 force-pushed the cloud-config-install-config branch from c8c561e to ade1e96 Compare November 5, 2018 13:03
@tomassedovic
Copy link
Contributor

@flaper87 when I try this patch and log into the bootstrap node, I see that the MCO container keeps crashing with this error:

# podman logs b9e97aebba47
I1105 16:08:19.572754       1 bootstrap.go:52] Version: 3.11.0-203-g8176c556-dirty
F1105 16:08:19.576038       1 bootstrap.go:89] error rendering bootstrap manifests: error discovering MCOConfig from "/assets/manifests/cluster-config.yaml": error unmarshaling JSON: json: cannot unmarshal object into Go struct field OpenStackPlatform.cloud of type string

Looks like we'll need to do a corresponding change in there? Just flagging it here, because this prevents other containers and the master nodes from coming up and I didn't see you mention it in the message.

@flaper87
Copy link
Contributor Author

flaper87 commented Nov 5, 2018

@tomassedovic correct! There's no way to make this change without breaking MCO :( I have a WIP commit that fixes this in MCO already: https://github.com/openshift/machine-config-operator/compare/master...flaper87:cloud-provider-conf?expand=1#diff-36b9f3e22f35e4ae1cfd1ff70a12ca71R64

Thanks for flagging it, I should have mentioned it in the commit :)

@tomassedovic
Copy link
Contributor

tomassedovic commented Nov 5, 2018

Right, and we can't fix it in MCO first, because it needs to vendor the changes in this commit, right?

In that case, better do this sooner (as in now in this PR) rather than later, I'd say.

@flaper87 flaper87 force-pushed the cloud-config-install-config branch from ade1e96 to 3c603ac Compare November 14, 2018 10:32
@openshift-ci-robot openshift-ci-robot removed the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 14, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flaper87

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 size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 14, 2018
@wking wking changed the title Cloud config install config openstack: Cloud config install config Nov 15, 2018
@flaper87 flaper87 force-pushed the cloud-config-install-config branch 3 times, most recently from 1ed7643 to 1c5d731 Compare November 15, 2018 14:08
@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 Nov 15, 2018
We need to pass the cloud's config data to the MCO in order to be able
to configure the cloud provider openstack. This commit passes such data
as part of the platform configuration since MCO consumes install-config
to get the cluster conifguration.

As part of this commit, we're now also validating the user's input to
verify whether the cloud name passed is correct.
@flaper87 flaper87 force-pushed the cloud-config-install-config branch from 1c5d731 to 995d8ee Compare November 19, 2018 13:43
@flaper87
Copy link
Contributor Author

@abhinavdahiya @sallyom I've added the "ProviderConfig" data to the kube-cloud-config.yaml secret. I'll take a look at the MCO and teach it how to use the secret.

@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 3, 2018
@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
Copy link
Contributor

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

Test name Commit Details Rerun command
ci/prow/e2e-aws 995d8ee link /test e2e-aws
ci/prow/rhel-images 995d8ee link /test rhel-images

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
Copy link
Contributor Author

flaper87 commented Mar 5, 2019

we'll be addressing this differently.
/close

@openshift-ci-robot
Copy link
Contributor

@flaper87: Closed this PR.

Details

In response to this:

we'll be addressing this differently.
/close

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. platform/openstack size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants