Note: the main reason we need a separate ProbeAttachablePluginsWithCSIMigrationDisabled is because OpenShift Cinder does not implement the Attachable interface.

@ddebroy which repo is OpenShift Cinder?

do existing probe functions special case for Cinder? Want to avoid this if possible

This should probably have its own section instead of being in the background and motivations. I'm thinking something like "Roadmap" or "Phased Rollout"

clarify "probed"

clarify "largely skipped" -> migrated to CSI. :)

I'm not sure about the introduction of this new terminology "supersceding" the plugin. Lets continue to call it "translated/migrated" so we don't call the same thing by multiple names

can this be done on a plugin by plugin basis or will all in-tree plugins not be probed at the same time? please clarify in text. (I think it should have the option to be done piecemeal)

This will be doable in a plugin-by-plugin basis. Will clarify.

there is a phase 4. Code deleleted from k8s/k8s entirely

won't they also return an error? This should just be an error path saying "plugin not found" or whatever it is currently

Correct. I will change the above sentence accordingly.

technically it should have been translated/migrated already and we should be trying to get the CSI Plugin instead of the nil in-tree plugin. If we ever try to get the nil in-tree plugin that seems like a real error, no?

Exactly. If [1] is done completely and correctly, [2] should not be necessary for majority of the cases. I will clarify this.

nit: spelling

I'm not clear on what this paragraph means. Could you clarify?

Overall, in this section, I call out the main changes necessary in the different controllers to work correctly with CSIMigrationInTreeOff feature flag enabled.

For the Expansion Controller, I mean to say that it's syncHandler at https://github.com/kubernetes/kubernetes/blob/28e800245e910b65b56548f36172ce525a554dc8/pkg/controller/volume/expand/expand_controller.go#L238 will be refactored to determine csiResizerName based on migration feature flags. The current structure involving invocation of FindExpandablePluginBySpec followed by volumePlugin.IsMigratedToCSI() will no longer work with CSIMigrationInTreeOff feature flag enabled.

I will expand this out and try to explain this better.

is this behavior different from when CSIMigrationInTreeOff is off? We should always do translation at the same place.

IIUC the only behavior this new flag should actually "toggle" is "probing the in-tree plugin". All the other changes you are planning to make were to support that working; but the flag shouldn't be toggling any other behavior.

We will have at least one additional set of areas where the behavior will be different when CSIMigrationInTreeOff is disabled but CSIMigration is enabled: all the functions that invoke nodeUsingCSIPlugin need to pass legacy/untranslated PVs in the generated functions.

So I was thinking of not performing the translations "early" as described for CSIMigrationInTreeOff when CSIMigrationInTreeOff is disabled. If we perform the translations early, we will not have enough context to translate back to in-tree PVs around the current invocations of nodeUsingCSIPlugin as we will not know if we are processing a "native" CSI PV (which can be left as is) or a "translated" in-tree => CSI PV (that should be translated back to in-tree).

We can construct a heuristic to translate CSI => in-tree around invocations of nodeUsingCSIPlugin: if [i] nodeUsingCSIPlugin returns false [ii] CSIMigration is enabled [iii] CSIMigrationInTreeOff is disabled and [iv] csi driver is a migrated driver, we can always translate CSI PV to in-tree. However if someone was expecting to see an error in this scenario when starting with a "native" CSI PV, they won't see the error which may be confusing and also a bit incompatible with current behavior.

Overall, both approach feels a bit convoluted. Thoughts?

nodeUsingCSIPlugin should trivially return true when CSIMigrationInTreeOff=true. Since this is basically a commit, the node should not have a different "migration status" than the controller

A third/better alternative: we can mark the PVs that are translated from in-tree to CSI "early" with an annotation and use that to translate back to in-tree when nodeUsingCSIPlugin returns true during Phase 1 (CSIMigrationInTreeOff=false and CSIMigration=true).

The annotation will help differentiate PVs that are translated to CSI from PVs that are "native" CSI. This will allow us to have largely common code paths for both Phase 1 and Phase 2.

And the overall CSI Migration flag right?

I was initially thinking that CSIMigration can be implicit if CSIMigrationInTreeOff is enabled. My assumption was it did not make sense to have CSIMigrationInTreeOff enabled but CSIMigration disabled. However thinking about it a little more we can require that for CSIMigrationInTreeOff to take effect, CSIMigration needs to be enabled. I think that is what you are suggesting would be ideal?

yeah I think we should actually try to throw a validation error if CSIMigrationInTreeOff is set but CSIMigration is not as a sanity check

Might be worth adding the benefit here is that if you use this feature flag the in-tree plugin code can never be executed i.e. we get the benefit of plugins not being able to take down the master components at this time

ditto here - not installation - we care about "migration feature flag state on the node"

what happens if this expectation is violated? Might be worth mentioning that the post-migration code will still run and we'll just get errors - no fallback

I'm still not convinced this should ever happen. But might be an implementation detail - we can leave the design as is for now

It is indeed an implementation detail and is only relevant for FindProvisionablePluginByName (which can be refactored to avoid it). Will mention this explicitly.

unless CSIMigrationInTreeOff is set - then we will never skip migration