[OCPCLOUD-1109] External cloud volume plugin observer for KCM #994

Danil-Grigorev · 2021-02-16T13:25:25Z

This implementation allows to preserve in-tree storage related cloud interaction code for external cloud providers. Will be used in openshift/cluster-kube-controller-manager-operator#450

More details in https://github.com/openshift/enhancements/pull/463/files#diff-3e0e2c48e70215076dfe36c13768a823ab7080d929d80292f37db2ef5a2121e8R191-R197

openshift-ci-robot · 2021-02-16T13:25:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Danil-Grigorev
To complete the pull request process, please assign sttts after the PR has been reviewed.
You can assign the PR to them by writing /assign @sttts in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…server This allows to have only storage in-tree functionality enabled in KCM.

JoelSpeed · 2021-02-17T15:09:53Z

pkg/operator/configobserver/cloudprovider/observe_cloud_volume_plugin.go

+		ret = configobserver.Pruned(ret, c.cloudProviderConfigPath, c.cloudProviderNamePath)
+	}()
+
+	listers := genericListers.(InfrastructureLister)


What happens if this assertion fails?

It should not with proper use. The consumer of this method will be a part of the list here, this argument is populated internally. It is done similarly in other places.

JoelSpeed · 2021-02-17T15:14:04Z

pkg/operator/configobserver/cloudprovider/observe_cloud_volume_plugin.go

+// NewCloudVolumePluginObserver returns a new cloudprovider observer for syncing cloud provider specific
+// information to controller-manager, leaving only storage controller loops enabled.
+func NewCloudVolumePluginObserver(targetNamespaceName string, cloudVolumePluginPath, cloudProviderConfigPath []string) configobserver.ObserveConfigFunc {
+	cloudObserver := &cloudProviderObserver{


Why are we reusing the cloudProviderObserver, would it not be better to have a dedicated observer for this CloudVolumePluginObserver?

I'm reusing the tested part of that code. The behavior and argument types are very similar in both NewCloudVolumePluginObserver and NewCloudProviderObserver, and which method will be used is decided on this line https://github.com/openshift/library-go/pull/994/files#diff-e571c7ed0d7124cda88d781ed909db675ef38ce8c54b4cab07b1f3e0283a519bR22 This is more error-prone for something which will stay in a cluster for a couple of releases.

How many repos will use this function? Also, a separate package seems proper here.

so, i'm a little confused. is it better to make a new copy here or am i misinterpreting the code?

JoelSpeed · 2021-02-17T15:20:55Z

pkg/operator/configobserver/cloudprovider/observe_cloudprovider.go

+	// Set cloudProviderConfig value
+	existingCloudConfig, _, err := unstructured.NestedStringSlice(existingConfig, c.cloudProviderConfigPath...)
+	if err != nil {
+		errs = append(errs, err)
+		// keep going on read error from existing config
+	}
+
+	cloudProviderConfig, configErrs := c.getCloudProviderConfig(genericListers, recorder, infrastructure)
+	if configErrs != nil {
+		errs = append(errs, configErrs...)
+	}


How is the cloud provider config handled currently? Surely this is handled in MCO somehow already? How does this tie in when the cloud provider is external?

Here is how it is set in KCMO. This method will co-exist on the same list of observers, and will be placed on the next line. Those two methods logic is exclusive, either NewCloudVolumePluginObserver sets the cloud-provider flag (well --external-cloud-volume-plugin) or NewCloudProviderObserver does it. MCO handles it differently, we don't use it there.

deads2k · 2021-03-22T12:04:20Z

pkg/operator/configobserver/cloudprovider/observe_cloud_volume_plugin.go

+	cloudProvider := getPlatformName(infrastructure.Status.Platform, recorder)
+	if len(cloudProvider) > 0 {
+		if err := unstructured.SetNestedStringSlice(observedConfig, []string{cloudProvider}, volumePluginPath...); err != nil {
+			errs = append(errs, err)


if we have an error here, why don't we return the previous config, since the previous was correct at some point in time and there is a high likelihood here that we'l be returning something partial or worse?

deads2k · 2021-03-22T12:05:09Z

pkg/operator/configobserver/cloudprovider/observe_cloud_volume_plugin.go

+		recorder.Eventf("ObserveCloudVolumePlugin", "Invalid featuregate.%s/cluster format: %v", err)
+	} else if !external {
+		// Running in-tree volumes - no configuration needed. Skip
+		return previouslyObservedConfig, errs


returning the previous config isn't a skip, it's the previous. If you want to return no args, then return an empty config

deads2k · 2021-03-22T12:05:31Z

pkg/operator/configobserver/cloudprovider/observe_cloud_volume_plugin.go

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


why wouldn't we return the previous config when hitting this condition?

deads2k · 2021-03-22T12:05:36Z

pkg/operator/configobserver/cloudprovider/observe_cloud_volume_plugin.go

+
+	featureGate, err := listers.FeatureGateLister().Get("cluster")
+	if errors.IsNotFound(err) {
+		recorder.Eventf("ObserveCloudVolumePlugin", "Optional featuregate.%s/cluster not found", configv1.GroupName)


why wouldn't we return the previous config when hitting this condition?

JoelSpeed · 2021-03-22T12:06:59Z

@Danil-Grigorev Didn't we decide we can't go ahead without CSI migration? Therefore we won't be need this?

deads2k · 2021-03-22T12:09:20Z

pkg/operator/configobserver/cloudprovider/observe_cloud_volume_plugin.go

+	// Set cloudProviderConfig value
+	existingCloudConfig, _, err := unstructured.NestedStringSlice(existingConfig, c.cloudProviderConfigPath...)
+	if err != nil {
+		errs = append(errs, err)


looks like you should return the previous config.

deads2k · 2021-03-22T12:10:24Z

pkg/operator/configobserver/cloudprovider/observe_cloud_volume_plugin.go

+
+// NewCloudVolumePluginObserver returns a new cloudprovider observer for syncing cloud provider specific
+// information to controller-manager, leaving only storage controller loops enabled.
+func NewCloudVolumePluginObserver(targetNamespaceName string, cloudVolumePluginPath, cloudProviderConfigPath []string) configobserver.ObserveConfigFunc {


There may be a misunderstanding of what happens when you return an error. When you return an error, your returned configuration is still used. This is because you're one of a dozen config loops and we cannot halt all configuration on a single observation failure. This means that you must return the best possible config you can. Usually, this is the previous configuration value you have.

if returning a configuration that is basically the same as the previous, is it best practice to make a copy and return the copy?

elmiko

this is mostly making sense to me, i am curious about some of the open questions though.

elmiko · 2021-04-12T20:39:13Z

pkg/operator/configobserver/cloudprovider/observe_cloud_volume_plugin.go

+
+// NewCloudVolumePluginObserver returns a new cloudprovider observer for syncing cloud provider specific
+// information to controller-manager, leaving only storage controller loops enabled.
+func NewCloudVolumePluginObserver(targetNamespaceName string, cloudVolumePluginPath, cloudProviderConfigPath []string) configobserver.ObserveConfigFunc {


if returning a configuration that is basically the same as the previous, is it best practice to make a copy and return the copy?

elmiko · 2021-04-12T20:40:44Z

pkg/operator/configobserver/cloudprovider/observe_cloud_volume_plugin.go

+// NewCloudVolumePluginObserver returns a new cloudprovider observer for syncing cloud provider specific
+// information to controller-manager, leaving only storage controller loops enabled.
+func NewCloudVolumePluginObserver(targetNamespaceName string, cloudVolumePluginPath, cloudProviderConfigPath []string) configobserver.ObserveConfigFunc {
+	cloudObserver := &cloudProviderObserver{


so, i'm a little confused. is it better to make a new copy here or am i misinterpreting the code?

Danil-Grigorev · 2021-04-30T09:48:50Z

I'm closing this PR as recent CI runs confirmed that PV handling will be broken on kubelet side (volume will becomes unmountable). Kubelet is missing an equivalent of --external-cloud-volume-plugin flag, so this breakage is finite. The proposal will be updated with the procedure of simultaneous migration to CSI and out-of-tree cloud providers.

cc @jsafrane @JoelSpeed @elmiko @lobziik

Thanks for review @deads2k

/close

Danil-Grigorev added 2 commits February 16, 2021 14:00

Add fg dependency in cloudprovider observer

e4f38b1

Add external-cloud-volume-plugin observer for KCM

066f0b4

openshift-ci-robot requested review from deads2k and smarterclayton February 16, 2021 13:25

Danil-Grigorev added 2 commits February 17, 2021 13:07

Decouple cloudProviderConfig observer method and add tests

7b00898

Integrate cloudProviderConfig observer method in CloudVolumePlugin ob…

a751237

…server This allows to have only storage in-tree functionality enabled in KCM.

JoelSpeed reviewed Feb 17, 2021

View reviewed changes

deads2k reviewed Mar 22, 2021

View reviewed changes

Danil-Grigorev mentioned this pull request Mar 23, 2021

[OCPCLOUD-1070] Add observe external switch to cloudpovider observer #895

Merged

elmiko reviewed Apr 12, 2021

View reviewed changes

Danil-Grigorev closed this Apr 30, 2021

[OCPCLOUD-1109] External cloud volume plugin observer for KCM #994

[OCPCLOUD-1109] External cloud volume plugin observer for KCM #994

Uh oh!

Conversation

Danil-Grigorev commented Feb 16, 2021

Uh oh!

openshift-ci-robot commented Feb 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Mar 22, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elmiko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Danil-Grigorev commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Danil-Grigorev commented Apr 30, 2021 •

edited

Loading