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

render: generate cloud config when infrastructure and configmap are provided #140

Merged
merged 1 commit into from Jul 28, 2020
Merged

render: generate cloud config when infrastructure and configmap are provided #140

merged 1 commit into from Jul 28, 2020

Conversation

jhixson74
Copy link
Member

This relies on #133 and is included for now. When it can be shown that it works, I can remove those commits from this PR. This is very much a WIP, so please point out anywhere that I am going about things completely wrong ;-)

https://issues.redhat.com/browse/CORS-1458

@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 19, 2020
abhinavdahiya
abhinavdahiya reviewed Jun 22, 2020
View reviewed changes
pkg/cmd/render/render.go Outdated
Comment on lines 187 to 216
// Determine the platform type
var platformName configv1.PlatformType
if pstatus := clusterInfrastructure.Status.PlatformStatus; pstatus != nil {
platformName = pstatus.Type
}
if len(platformName) == 0 {
platformName = clusterInfrastructure.Status.Platform
}

// Determine the platform specific transformer method to use
cloudConfigTransformers := kubecloudconfig.CloudConfigTransformers()
cloudConfigTransformerFn, ok := cloudConfigTransformers[platformName]
if !ok {
cloudConfigTransformerFn = kubecloudconfig.DefaultCloudConfigTransformer()
}
target, err := cloudConfigTransformerFn(&cloudProviderConfigInput, clusterInfrastructure.Spec.CloudConfig.Key, &clusterInfrastructure)
if err != nil {
return err
}

targetCloudConfigMap := kubecloudconfig.TargetConfigName()

target.Name = kubecloudconfig.TargetConfigName()
target.Namespace = operatorclient.GlobalMachineSpecifiedConfigNamespace
/* ApplyConfigMap() */

targetCloudConfigMapData, err := yaml.Marshal(target)
if err != nil {
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

all this should be part of the kube_cloud_config package.

we can keep the load inputs in here, but how/when to generate the new cloud config file should be controlled by kube_cloud_config package. the sync also needs something like that so maybe we can re-use one implementation...

Copy link
Member Author

Choose a reason for hiding this comment

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

I've moved the code to kube_cloud_config. I still need to look at sync() and perhaps generalize the transformer functions also.

@jhixson74 jhixson74 force-pushed the master_cloud_configmap_bootstrap branch from 47b92c2 to f3417d3 Compare June 23, 2020 01:20
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2020
sttts
sttts reviewed Jun 23, 2020
View reviewed changes
go.mod Outdated Show resolved Hide resolved
sttts
sttts reviewed Jun 23, 2020
View reviewed changes
pkg/cmd/render/render.go Outdated Show resolved Hide resolved
sttts
sttts reviewed Jun 23, 2020
View reviewed changes
pkg/cmd/render/render.go Outdated Show resolved Hide resolved
sttts
sttts reviewed Jun 23, 2020
View reviewed changes
pkg/cmd/render/render.go Outdated Show resolved Hide resolved
sttts
sttts reviewed Jun 23, 2020
View reviewed changes
pkg/cmd/render/render.go Outdated Show resolved Hide resolved
sttts
sttts reviewed Jun 23, 2020
View reviewed changes
pkg/cmd/render/render.go Outdated Show resolved Hide resolved
@jhixson74 jhixson74 force-pushed the master_cloud_configmap_bootstrap branch from f3417d3 to d250cd1 Compare June 29, 2020 23:51
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 29, 2020
@jhixson74 jhixson74 force-pushed the master_cloud_configmap_bootstrap branch from d250cd1 to edb88fb Compare June 30, 2020 00:48
abhinavdahiya
abhinavdahiya reviewed Jun 30, 2020
View reviewed changes
pkg/cmd/render/render.go Show resolved Hide resolved
pkg/operator/kube_cloud_config/transform.go Outdated Show resolved Hide resolved
pkg/operator/kube_cloud_config/transform.go Outdated Show resolved Hide resolved
pkg/operator/kube_cloud_config/transform.go Outdated Show resolved Hide resolved
pkg/operator/kube_cloud_config/transform.go Outdated Show resolved Hide resolved
pkg/operator/kube_cloud_config/transform.go Outdated Show resolved Hide resolved
@jhixson74 jhixson74 force-pushed the master_cloud_configmap_bootstrap branch 2 times, most recently from 29bfd4e to d23b37f Compare June 30, 2020 21:49
abhinavdahiya
abhinavdahiya reviewed Jul 1, 2020
View reviewed changes
pkg/cmd/render/render.go Outdated Show resolved Hide resolved
pkg/operator/kube_cloud_config/bootstrap.go Outdated Show resolved Hide resolved
pkg/operator/kube_cloud_config/bootstrap.go Outdated Show resolved Hide resolved
@abhinavdahiya
Copy link
Contributor

abhinavdahiya commented Jul 1, 2020 •
edited

@jhixson74 can you open a PR on the installer showing how we will use this on the bootstrap host.

you can then test these together using cluster-bot in slack
Option A:

  1. /msg @cluster-bot launch 4.6.0-0.ci,openshift/installer#<pr number>,openshift/cluster-config-operator#140 aws
  2. /msg @cluster-bot launch 4.6.0-0.ci,openshift/installer#<pr number>,openshift/cluster-config-operator#140 azure

Option B:

  1. /msg @cluster-bot build 4.6.0-0.ci,openshift/installer#<pr number>,openshift/cluster-config-operator#140
  2. Get the release image.
  3. Ensure you have latest pull secret from api.openshift.com
    3.a run login command
    3.b oc registry login
    3.c make sure you add that pulll secret to the installer install-config.yaml
  4. oc extract --command=openshift-install <release image>
  5. Use the installer to create a cluster as you like

@jhixson74 jhixson74 force-pushed the master_cloud_configmap_bootstrap branch from d23b37f to f6b5e2b Compare July 2, 2020 04:06
@patrickdillon patrickdillon mentioned this pull request Jul 2, 2020
Merged
@jhixson74 jhixson74 force-pushed the master_cloud_configmap_bootstrap branch from f6b5e2b to 6f540a1 Compare July 6, 2020 18:54
@abhinavdahiya
Copy link
Contributor

LGTM

/assign @deads2k @sttts
Can you help review / approve this?

sttts
sttts reviewed Jul 21, 2020
View reviewed changes
pkg/cmd/render/render.go Outdated Show resolved Hide resolved
sttts
sttts reviewed Jul 21, 2020
View reviewed changes
pkg/cmd/render/render.go Outdated Show resolved Hide resolved
sttts
sttts reviewed Jul 21, 2020
View reviewed changes
pkg/cmd/render/render.go Outdated Show resolved Hide resolved
@abhinavdahiya
Copy link
Contributor

/test e2e-azure

@abhinavdahiya
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2020
@wking
Copy link
Member

wking commented Jul 27, 2020 •
edited

/retitle render: generate cloud config when infrastructure and configmap are provided

To match e87cce2, because the current PR title doesn't talk about which ConfigMap.

@openshift-ci-robot openshift-ci-robot changed the title generate configmap on bootstrap node render: generate cloud config when infrastructure and configmap are provided Jul 27, 2020
@sttts
Copy link
Contributor

sttts commented Jul 28, 2020

/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, jhixson74, sttts

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:

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 28, 2020
@openshift-bot
Copy link
Contributor

/retest

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

14 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-ci-robot
Copy link

openshift-ci-robot commented Jul 28, 2020 •
edited

@jhixson74: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-azure e87cce2 link /test e2e-azure

Full PR test history. Your PR dashboard.

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.

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit 17c4507 into openshift:master Jul 28, 2020
@abhinavdahiya abhinavdahiya mentioned this pull request Jul 31, 2020
Merged
abhinavdahiya added a commit to abhinavdahiya/machine-config-operator that referenced this pull request Aug 3, 2020
@abhinavdahiya
operator/bootstrap: use correct key for generated cloud conf
344fb9e 
Previously the cloud provider config map was sourced from the `.spec.cloudConf` of the infrastructure object.
but to enable user input and stitching explained in [1] and [2], we added a controller to generate a cloud configuraration using the spec
for use by kubelet and kube cloud controller manager. The new API defined in [3] creates a config map `openshift-config-managed/kube-cloud-config` with
the configuration in `cloud.conf` key. We updated the in-cluster MCO to use the new API [4] but the MCO on bootstrap host was left untouched because the controller
could not generated on the bootstrap host yet.

But [5] and [6] intend to provide the same generated config map on the bootstrap host for MCO, and therefore we need the bootstrap MCO to read the key defined by
the generated API but fallback to he spec key (old behavior) for backward compatibility.

[1]: https://github.com/openshift/enhancements/blob/master/enhancements/installer/aws-custom-region-and-endpoints.md
[2]: https://github.com/openshift/enhancements/blob/master/enhancements/installer/azure-support-known-cloud-environments.md
[3]: https://github.com/openshift/api/blob/e21882127f24772e3b5388fe1fdc37669e8e1d04/config/v1/types_infrastructure.go#L26-L40
[4]: openshift#1658
[5]: openshift/cluster-config-operator#140
[6]: openshift/installer#3831
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sttts sttts sttts left review comments

@abhinavdahiya abhinavdahiya abhinavdahiya left review comments

Assignees

@sttts sttts

@abhinavdahiya abhinavdahiya

@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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@jhixson74 @abhinavdahiya @wking @sttts @openshift-ci-robot @openshift-bot @deads2k @openshift-merge-robot