Node/Driver labeling mechanism #2

davidz627 · 2018-10-25T22:10:34Z

What this PR does / why we need it:

This PR adds a field to the CSINodeInfo object to specify whether each specific driver/node combination has migration turned on. This in turn is used by the Attach/Detach controller to decide whether to attach a volume with migration or not.

This superscedes the mechanism described here: https://github.com/kubernetes/community/blame/master/contributors/design-proposals/storage/csi-migration.md#L236

Part of the implementation for feature: kubernetes/enhancements#625

/assign @jsafrane @ddebroy @leakingtapan

pkg/volume/util/operationexecutor/operation_generator.go

staging/src/k8s.io/csi-api/pkg/apis/csi/v1alpha1/types.go

pkg/volume/csi/nodeinfomanager/nodeinfomanager.go

davidz627 · 2018-10-26T18:27:25Z

@ddebroy comments addressed!

pkg/volume/csi/nodeinfomanager/nodeinfomanager.go

…gration

davidz627 · 2018-10-26T23:51:38Z

@jsafrane PTAL, I believe this should solve the node/controller race/sync issues

pkg/controller/volume/attachdetach/testing/testvolumespec.go

jsafrane · 2018-10-29T13:09:25Z

staging/src/k8s.io/csi-api/pkg/apis/csi/v1alpha1/types.go

+	// migration to CSI for this driver. It is used by storage controllers on
+	// the control plane to determine whether to use a migrated version of the
+	// plugin for this driver on this node.
+	IsDriverMigratableOnNode bool `json:"isDriverMigratableOnNode"`


should you regenerate the clients?

I'd use DriverMigrated (or VolumePluginMigrated to emphasize what's migrated)

OnNode it's implied by CSINodeInfo. The other field is no TopologyKeysOnNode, but TopologyKeys.

We don't use Is prefix for booleans, like AttachRequired or ReadOnly.

Again, "Migratable" vs "Migrated". Does this field say that now CSI must be used (= driver is migrated, don't attach the old way) or may be used (=migratable, attach any way you want, kubelet will handle that)?

yes, I think the naming has been a little inconsistent.

Going forwards I will refer to anything related to feature flags as "Migrated"
whereas the fact that translation library exists is "Migratable"

The difference being that feature flags is something the admin sets to choose to migrate so it is Migrated. Whereas there is another concept of - does the logic to do this exist (we have translation logic for this plugin) - is Migratable.

Please flag any incorrect usage in code reviews

@ddebroy @leakingtapan

jsafrane · 2018-10-29T13:14:20Z

pkg/volume/util/operationexecutor/operation_generator.go

+			return false, fmt.Errorf("failed to get CSI node info for node %v from informer: %v", string(nodeName), err)
+		}
+	} else {
+		glog.Warningf("CSINodeInfo informer not synced, please check that CSINodeInfo CRD is installed. If so, this warning should not appear again after ~1 minute")


There is still some crazy race there: CRD gets installed, informer is (slowly) syncing and it gets an old CSINodeInfo. User/kubelet changes CSINodeInfo, the informer is not synced yet and the volume plugin does API GET at this line. It gets new CSINodeInfo.

Now, the informer finishes sync and it still has old CSINodeInfo, with the new one enqueued (or waiting somewhere in API server). Second call to this function does informer.Get(), which returns the old value. It seems the plugin goes back in time.

In this particular case, the informer has old CSINodeInfo for very short time (depending on API server connectivity), so it perhaps does not really matter...

Can we just return error here and kubelet / A/D controller re-tries in a while? Frankly, I think that whole CSI volume plugin should return errors in all calls until the informers are synced.

I am a little confused about the race you described. Are you saying that if informer is synced it may have stale API objects? I was under the impression that synced informer means that any GET to the informer will guarantee to be the freshest object, otherwise what's the point in having an inconsistent cache?

The only two options I see here in the current code are:

Informer synced -> get from informer (freshest object)

Informer not yet synced -> get from API Server (freshest object)

Yes, informers can return old data. It does List + Watch and the watch can have delay on network. That's OK. Informer will let you know when it gets new data.

GET from API server can get old data too - at the time your client parses the data there may be already new on the server. This time, you don't get event when it changes.

In this particular case, GET from API server could be faster than initial List + Watch and return something newer than the informer. Under "normal" conditions the informer syncs very quickly, so this discussion is more or less academical.

pkg/volume/csi/nodeinfomanager/nodeinfomanager.go

jsafrane · 2018-10-29T13:41:38Z

pkg/volume/util/operationexecutor/operation_generator.go

+	return false, fmt.Errorf("plugin %v is migratable but driver %v is not installed. Please install the driver and retry", pluginName, driverName)
+}
+
+func isMigratable(og *operationGenerator, spec *volume.Spec, uniqueVolumeName v1.UniqueVolumeName, nodeName types.NodeName) (bool, error) {


Rename to isVolumeMigratable? Or isVolumePluginMigratable? It's confusing to see isMigratable and isNodeMigratable.

I am doing large scale fix for all naming to be consistent with #2 (comment).

thanks for pointing this out.

davidz627 · 2018-10-29T23:46:35Z

@jsafrane @ddebroy @saad-ali

I've noticed a problem with using CSINodeInfo (this may be the same one Jan mentioned in the comments):
By using CSINodeInfo we have lost some fidelity in understanding the cases of node migration flags and driver installation. In order to solve races in upgrade/downgrade we need to distinguish these cases:

// if yes flag and yes driver -> node is migrated
// if yes flag and no driver -> throw an error about installing driver
// if no flag and yes driver -> node is not migrated
// if no flag and no driver -> node is not migrated

However, by using the CSINodeInfo object we lost the ability to distinguish the two cases where there is no driver (since there will be no corresponding CSINodeInfo object for that driver) and we want different behavior for the two cases.

I can see this being solved by pre-creating CSIDriverInfo for each migrated driver, and adding a field DriverInstalled bool to the CSIDriverInfo object that is false on creation, and is flipped to true when the driver is installed. In this case we will be able to see whether the migration flags are set or not whether the driver is installed or not and we can distinguish the above cases.

We would have to change a lot of unrelated (to migration) logic to instead of parsing the CSIDriverInfo existence as a proxy for driver installation, to specifically checking the DriverInstalled field on the object.

jsafrane · 2018-10-30T09:04:55Z

I am slightly confused by CSIDriverInfo - do you mean global CSIDriver or node specific CSINodeInfo? I'm assuming CSIDriver, CSINodeInfo makes little sense.

I am not sure that a flag in CSIDriver would help. What if CSIDriver itself does not exist? It's still optional.

I think we should take a step back and re-think how a driver is installed and how each Kubernetes component knows about it.

davidz627 · 2018-10-30T18:01:31Z

I am slightly confused by CSIDriverInfo - do you mean global CSIDriver or node specific CSINodeInfo? I'm assuming CSIDriver, CSINodeInfo makes little sense.

I am not sure that a flag in CSIDriver would help. What if CSIDriver itself does not exist? It's still optional.

I think we should take a step back and re-think how a driver is installed and how each Kubernetes component knows about it.

I mean the CSIDriverInfo object that is inside CSINodeInfo.

davidz627 · 2018-10-31T21:26:05Z

This PR is on hold until CSINodeInfo changes go in

davidz627 · 2019-03-05T19:08:38Z

/close
replaced by kubernetes#74835

Kubernetes CSI Migration Library Update for necessary interface function

950746d

davidz627 mentioned this pull request Oct 25, 2018

[WIP] CSI Migration Node/Driver labeling mechanism [PLEASE REVIEW XREF-ED PR] kubernetes/kubernetes#70256

Closed

davidz627 force-pushed the feature/migrationNodeLabel branch from ea95006 to a2c11af Compare October 25, 2018 23:52