-
Notifications
You must be signed in to change notification settings - Fork 88
Disabled volume discovery and added env var to enable it if needed #1047
Conversation
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.
LGTM. Please check the CI failure.
] | ||
}, | ||
{"name": "VDVS_DISCOVER_VOLUMES", | ||
"description": "non-empty if pre-existing volumes need to be discovered ", |
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.
It seems tabs and spaces are being mix-used which caused inconsistent alignment.
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, and on the top of this github does not set proper tabstops.
Unfortunately I don't know how to set tab->spaces replacement for json editor in Code.
For everything else I did find it, but for json we'd need suffer :-) unless someone can point me out how to set it.
Add ?ts=4 to the diff URL to see it well
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 also update "docker-plugin-drivers.md" to include this option.
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 but only after we integrated plugin with ../Makefiles. Still, you are right and a note there is needed. Captured in #1051
For some reason CI is now down with a sever case of failing checkouts, can't do much about it git fetch --tags origin +refs/pull/1047/merge:
fatal: Couldn't find remote ref refs/pull/1047/merge
[info] build failed (exit code 1) |
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.
LGTM!
I have restarted the failing build https://ci.vmware.run/vmware/docker-volume-vsphere/1707
I am little paranoid so tried few scenarios. There is a bug with refCnt discovery with managed plugin.
Following check in refcnt.go going to fail;
And we treat this as stale mount and issue detach/unmount. Result: Container continues to run with no volume :( |
With refCnt DISABLED, code correctly handles plugin crash while volume is in-use by container.
In summary, this PR fixes serious issue in case we crash! However, we lose ability to detach stale mounts (across VM crashes?). I am fine submitting this PR and filing a separate issue to track broken discovery. |
Also disable the refcnt sanity test or update it with running with this option? And document the behavior with volumes left attached to the VM in case case the VM restarts and the option is set or not set. Can't this new option be part of the plugin configuration file? User may find it easier to run with the configuration file vs. update the start up scripts to set env variable. |
Yes CI is failing due to 'refcnt on restart' rest, I'll fix before merge and make sure CI is green.
I am not sure what specifically you want in documentation, if you can provide a draft I'd have an idea.
I think it's easy enough to pass option on plugin install command, so no conf file changes are needed. |
9775c9b
to
e265caa
Compare
I've opened #1050 to address @pdhamdhere point (bring the discovery back after making a repair :) ), and hid the crash recovery test. When CI passes I will merge |
Volume discovery was written to address challenges with docker/plugin startup order, and as potential auto-recovery for plugin crashes. * We do not see crashes often (as a matter of fact, only once so far , with VMCI memory issues - and it' fixed). * Docker now manages the order of plugin invocation/startup with Managed Plugins. * and, the last but not the least, during the discovery we make assumption about Docker API availability, but Docker makes API available only AFTER full startup including plugin initialization, so it could lead to bug like #1039, where docker either hangs for 15-20 sec (like with legacy plugin) or steps on it's own bugs and crash (like with managed plugin) In the spirit of taking care of the "sunny day" first, when everything works OK and our code does not crash, this commit turns off discovery on startup. Managed plugin handles all restarts and remounts fine. Legacy plugin may have issues if we crash, but first of all we never crash :-) and second legacy plugins are getting depreciated anyways
e265caa
to
93082e4
Compare
Fixes #1039
Volume discovery was written to address challenges with docker/plugin startup order,
and as potential auto-recovery for plugin crashes.
In the spirit of taking care of the "sunny day" first, when everything works OK and our code does not crash, this commit turns off discovery on startup.
Managed plugin handles all restarts and remounts fine. Legacy plugin may have issues if we crash, but first of all we never crash :-) and second legacy plugins are getting depreciated anyways
Test
//@pdhamdhere @shuklanirdesh82 //@govint