Sync cloudprovider configmap #206
Conversation
openshift-ci-robot
added
the
size/M
|
cc @staebler @openshift/sig-storage |
| const ( | ||
| targetNamespaceName = "openshift-kube-controller-manager" | ||
| cloudProviderConfFilePath = "/etc/kubernetes/static-pod-resources/configmaps/cloud-config/config" | ||
| configNamespace = "openshift-config" |
There was a problem hiding this comment.
@derekwaynecarr I think we want this to be openshift-config-managed to be consistent for all clouds and allow future filtering, right?
There was a problem hiding this comment.
Okay, if this has to change https://github.com/openshift/installer/pull/1516/files#diff-8db489d59cedb0a4f04c7546779f58a2R68 should be updated to reflect new location right?
There was a problem hiding this comment.
The documentation of Infrastructure should change, too, then.
https://github.com/openshift/api/blob/d2f01e7b77a6fc78b328db20285423838419fef7/config/v1/types_infrastructure.go#L28
|
|
||
| sourceCloudConfigMap := infrastructure.Spec.CloudConfig.Name | ||
| sourceCloudConfigNamespace := configNamespace | ||
| if len(sourceCloudConfigMap) == 0 { |
There was a problem hiding this comment.
In this case, I think we need to sync an empty location to clear the destination. Can you have a look at the godoc on syncconfigmap?
| recorder.Warningf("ObserverCloudProviderNames", "Failed setting cloud-config : %v", err) | ||
| errs = append(errs, err) | ||
| } | ||
| return observedConfig, errs |
There was a problem hiding this comment.
just before returning, can you look for a diff between the prevconfig and the newconfig and emit an event. @mfojtik probably has an example.
There was a problem hiding this comment.
^ does this work or did you mean deep comparsion of actual cloudprovider configuration?
openshift-ci-robot
added
size/L
| previouslyObservedConfig := map[string]interface{}{} | ||
|
|
||
| existinCloudConfig, _, err := unstructured.NestedStringSlice(existingConfig, cloudProviderConfPath...) |
There was a problem hiding this comment.
s/existinCloudConfig/existingCloudConfig/
| case "": | ||
| recorder.Warningf("ObserveCloudProvidersFailed", "Required status.platform field is not set in infrastructures.%s/cluster", configv1.GroupName) | ||
| return previouslyObservedConfig, errs | ||
| case configv1.AWSPlatform: | ||
| cloudProvider = "aws" | ||
| case configv1.VSpherePlatform: |
There was a problem hiding this comment.
Can you please add support for Azure platform as well
There was a problem hiding this comment.
I can't add support for Azure in this PR. Because it was decided that applying a opaque configmap as a cloudprovider config will only be supported for vsphere for the time being. For other cloudproviders, plan is to validate and support only options we know work.
gnufied
force-pushed
the
sync-cloudprovider-config
branch
from
429720e to
0d4cd23
Compare
openshift-ci-robot
added
the
needs-rebase
Also delete configmap if it is removed in source
gnufied
force-pushed
the
sync-cloudprovider-config
branch
from
0d4cd23 to
0ec8bd6
Compare
openshift-ci-robot
removed
the
needs-rebase
|
I have verified and validated that vsphere cloudprovider configuration for controller-manager in 4.1 works after this PR. I was able to provision and attach/detach PVCs successfully in a working cluster. @derekwaynecarr @deads2k PTAL. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: deads2k, gnufied 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 |
openshift-ci-robot
added
the
approved
This PR still depends on openshift/installer#1516 for actual
Infrastructureobject to be populated.cc @deads2k