[OCPCLOUD-1070] Add observe external switch to cloudpovider observer by Danil-Grigorev · Pull Request #895 · openshift/library-go

Danil-Grigorev · 2020-09-16T14:13:23Z

This method is used in KCMO and KAPIO to set the cloud-provider flags.

FeatureGate "ExternalCloudProvider" will short-circuit logic to "external" for supported platforms.

This PR is the main blocker for work on CCM integration in Openshift: https://issues.redhat.com/browse/OCPCLOUD-1070

openshift-ci-robot · 2020-09-17T09:58:11Z

@Danil-Grigorev: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/unit	`036f815`	link	`/test unit`

Full PR test history. Your PR dashboard.

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.

openshift-bot · 2020-12-16T10:29:45Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

openshift-bot · 2021-01-15T12:21:30Z

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

Danil-Grigorev · 2021-02-15T10:01:48Z

/remove-lifecycle rotten

JoelSpeed · 2021-02-17T10:46:38Z

+
+	external, err := cloudprovider.IsCloudProviderExternal(infrastructure.Status.Platform, featureGate)
+	if err != nil {
+		recorder.Eventf("ObserveCloudProviderNames", "Invalid featuregate.%s/cluster format: %v", err)


This error description seems oddly specific given its for a call IsCloudProviderExternal, would it not make more sense for it to say something like Could not determine external cloud provider state: %v?

JoelSpeed · 2021-02-17T13:31:30Z

/lgtm

Code changes look ok to me, how has this been tested?

Danil-Grigorev · 2021-02-17T13:36:56Z

/lgtm

Code changes look ok to me, how has this been tested?

cluster-bot allows to do that, with revendored change in openshift/cluster-kube-controller-manager-operator#450 I was using my branch until this moment.

deads2k · 2021-03-22T11:58:04Z

+		if err := unstructured.SetNestedStringSlice(observedConfig, []string{"external"}, c.cloudProviderNamePath...); err != nil {
+			errs = append(errs, err)
+		}
+		return observedConfig, errs


You need to set if err := listers.ResourceSyncer().SyncConfigMap to the correct (empty I think) value, in this case.

That is right, but #994 requires this cloud-config to be copied even for external cloud-config option, as there still will be a consumer in some cases, like for KCM.

That is right, but #994 requires this cloud-config to be copied even for external cloud-config option, as there still will be a consumer in some cases, like for KCM.

Coding by side-effect is bad practice. As I see it, if you want to rely on the same configmap, it makes sense to produce a separate configobserver function which both of these methods can rely upon.

As coded here, you aren't doing any copy when you start external anyway, so this broke the active resource sync.

You are right, sorry for confusion. We definitely don't need a side effect on configobserver which will copy cloud-config to destination for the --external-cloud-provider-plugin flag in another PR, it will do this separately. But we should not remove it from there as well, so I think that this part is currently correct. It won't remove cloud-config which some other observer might place there. #994 will address that.

mfojtik · 2021-06-14T12:29:36Z

library-go/pkg/operator/configobserver/cloudprovider/observe_cloudprovider.go

Line 96 in 7a65fdb

    
           validCloudProviders := sets.NewString("aws", "azure", "gce", "openstack", "vsphere")

how this change affects ^ ?

JoelSpeed · 2021-06-14T12:37:47Z

how this change affects ^ ?

We discussed this with Stefan on a call. For now, the observer has a side effect where for certain platforms, it will synchronise the configmap as part of the config observation. We have created this PR so that we don't change the logic here and this side effect is still in place.

In the longer term, anything that actually needs the configmap to be synced will have to do this explicitly. Eg using the resourcesynccontroller directly in their own sync loops. Because we still need the configmap for KCM, we will implement the sync there as part of 4.9. Then the plan is to remove this sync so that this method is side effect free. We will also be syncing it directly in our CCCMO. I think by the time we fully migrate, only CCCMO and storage will actually need this and KCM/KAS/MCO will no longer need them.

Danil-Grigorev · 2021-06-14T13:07:33Z

library-go/pkg/operator/configobserver/cloudprovider/observe_cloudprovider.go

Line 96 in 7a65fdb

validCloudProviders := sets.NewString("aws", "azure", "gce", "openstack", "vsphere")

how this change affects ^ ?

This list is unaffected, until IsCloudProvider says otherwise.

This method is used in KCMO and KAPIO to set the cloud-provider flags. FeatureGate will short-circuit logic to "external" for supported platforms. - Adds fg dependency in cloudprovider observer

elmiko

looks good to me @Danil-Grigorev , given our discussion with the team today will we add another patch on top of this for the upcoming changes to IsCloudProviderExternal ?
/lgtm

Danil-Grigorev · 2021-06-15T10:10:49Z

looks good to me @Danil-Grigorev , given our discussion with the team today will we add another patch on top of this for the upcoming changes to IsCloudProviderExternal ?
/lgtm

Yes, there will be a signature change coming short after for Azure and Azure Stack Hub support, but this PR depends on API change which didn't merge yet, so it could wait. This PR could merge first, and then #1077 could also update the usage in cloud provider observer.

Fedosin · 2021-06-15T10:53:57Z

/lgtm

lobziik · 2021-06-15T16:32:10Z

/lgtm

soltysh

/lgtm
/assign @deads2k
for approval

sttts · 2021-07-02T08:37:46Z

/approve

openshift-ci · 2021-07-02T08:38:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Danil-Grigorev, elmiko, Fedosin, JoelSpeed, lobziik, soltysh, sttts

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:

~~pkg/operator/OWNERS~~ [sttts]

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 added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2020

openshift-ci-robot requested review from deads2k and smarterclayton September 16, 2020 14:13

Danil-Grigorev force-pushed the observe-external branch 2 times, most recently from 909099b to 036f815 Compare September 17, 2020 09:52

Danil-Grigorev mentioned this pull request Sep 22, 2020

Describing steps to support out-of-tree providers openshift/enhancements#463

Merged

openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 16, 2020

openshift-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 15, 2021

Danil-Grigorev force-pushed the observe-external branch 3 times, most recently from 45293b7 to 8fc60e1 Compare February 14, 2021 20:09

openshift-ci-robot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Feb 15, 2021

Danil-Grigorev mentioned this pull request Feb 15, 2021

Initial AWS CCM deployment using upstream image openshift/cluster-cloud-controller-manager-operator#15

Merged

4 tasks

JoelSpeed reviewed Feb 17, 2021

View reviewed changes

Danil-Grigorev force-pushed the observe-external branch from 8fc60e1 to e2e9965 Compare February 17, 2021 12:47

Danil-Grigorev changed the title ~~[WIP] Add observe external switch to cloudpovider observer~~ Add observe external switch to cloudpovider observer Feb 17, 2021

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 17, 2021

Danil-Grigorev force-pushed the observe-external branch from e2e9965 to c0ce7f5 Compare February 17, 2021 12:54

Danil-Grigorev requested a review from JoelSpeed February 17, 2021 12:55

openshift-ci-robot assigned JoelSpeed Feb 17, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 17, 2021

deads2k reviewed Mar 22, 2021

View reviewed changes

Comment thread pkg/operator/configobserver/cloudprovider/observe_cloudprovider.go Outdated

Danil-Grigorev requested a review from sttts May 18, 2021 06:00

JoelSpeed mentioned this pull request May 19, 2021

[OCPCLOUD-1109] Add ObserveCloudVolumePlugin config observer for KCM config openshift/cluster-kube-controller-manager-operator#525

Merged

mfojtik reviewed Jun 14, 2021

View reviewed changes

Comment thread pkg/operator/configobserver/cloudprovider/observe_cloudprovider.go Outdated

mfojtik reviewed Jun 14, 2021

View reviewed changes

Comment thread pkg/operator/configobserver/cloudprovider/observe_cloudprovider.go Outdated

Implement IsCloudProviderExternal in CloudProviderObserver

372794f

This method is used in KCMO and KAPIO to set the cloud-provider flags. FeatureGate will short-circuit logic to "external" for supported platforms. - Adds fg dependency in cloudprovider observer

Danil-Grigorev force-pushed the observe-external branch from 5b0e868 to 372794f Compare June 14, 2021 16:10

Danil-Grigorev requested review from elmiko and mfojtik June 14, 2021 16:10

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2021

elmiko approved these changes Jun 14, 2021

View reviewed changes

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jun 14, 2021

openshift-ci Bot assigned Fedosin Jun 15, 2021

openshift-ci Bot assigned lobziik Jun 15, 2021

Danil-Grigorev changed the title ~~Add observe external switch to cloudpovider observer~~ [OCPCLOUD-1070] Add observe external switch to cloudpovider observer Jun 16, 2021

soltysh approved these changes Jun 25, 2021

View reviewed changes

openshift-ci Bot assigned deads2k and soltysh Jun 25, 2021

This was referenced Jun 29, 2021

[OCPCLOUD-1070] Set --cloud-provider=external for supported platforms openshift/cluster-kube-controller-manager-operator#450

Merged

[OCPCLOUD-1070] Set --cloud-provider=external for supported platforms openshift/cluster-kube-apiserver-operator#953

Merged

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 2, 2021

openshift-merge-robot merged commit 886aa35 into openshift:master Jul 2, 2021

Danil-Grigorev mentioned this pull request Jul 2, 2021

Revendor library-go and openshift/api for Azure Stack Hub and Azure support openshift/cluster-cloud-controller-manager-operator#77

Merged

Conversation

Danil-Grigorev commented Sep 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Sep 17, 2020

Uh oh!

openshift-bot commented Dec 16, 2020

Uh oh!

openshift-bot commented Jan 15, 2021

Uh oh!

Danil-Grigorev commented Feb 15, 2021

Uh oh!

JoelSpeed Feb 17, 2021

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Feb 17, 2021

Uh oh!

Danil-Grigorev commented Feb 17, 2021

Uh oh!

deads2k Mar 22, 2021

Choose a reason for hiding this comment

Uh oh!

Danil-Grigorev Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

deads2k Mar 23, 2021

Choose a reason for hiding this comment

Uh oh!

Danil-Grigorev Mar 24, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mfojtik commented Jun 14, 2021

Uh oh!

JoelSpeed commented Jun 14, 2021

Uh oh!

Danil-Grigorev commented Jun 14, 2021

Uh oh!

elmiko left a comment

Choose a reason for hiding this comment

Uh oh!

Danil-Grigorev commented Jun 15, 2021

Uh oh!

Fedosin commented Jun 15, 2021

Uh oh!

lobziik commented Jun 15, 2021

Uh oh!

soltysh left a comment

Choose a reason for hiding this comment

Uh oh!

sttts commented Jul 2, 2021

Uh oh!

openshift-ci Bot commented Jul 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Danil-Grigorev commented Sep 16, 2020 •

edited

Loading