Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Node/Driver labeling mechanism #2
Node/Driver labeling mechanism #2
Changes from 1 commit
950746d
68b1d5e
b582653
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename to isVolumeMigratable? Or isVolumePluginMigratable? It's confusing to see isMigratable and isNodeMigratable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am doing large scale fix for all naming to be consistent with #2 (comment).
thanks for pointing this out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should you regenerate the clients?
I'd use
DriverMigrated
(orVolumePluginMigrated
to emphasize what's migrated)OnNode
it's implied by CSINodeInfo. The other field is noTopologyKeysOnNode
, butTopologyKeys
.Is
prefix for booleans, likeAttachRequired
orReadOnly
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) - isMigratable
.Please flag any incorrect usage in code reviews
@ddebroy @leakingtapan