dynamic host volumes: client-side tests, comments, tidying #24747
Conversation
| // HostVolumePluginExternal calls an executable on disk. All operations should | ||
| // be idempotent, and safe to be called concurrently per volume ID. | ||
| // For each call, the executable's stdout and stderr may be logged, so plugin | ||
| // authors should not include any sensitive information in their plugin outputs. |
There was a problem hiding this comment.
These docstrings are likely to make their way into a spec for plugin authors. We should probably use the term "must" rather than "should" here in that RFC 2119 sense of the terms.
There was a problem hiding this comment.
MUST This word, or the terms "REQUIRED" or "SHALL", mean that the definition is an absolute requirement of the specification.
I don't think this matches, because including secret strings doesn't break anything, and we have no way of enforcing it.
There was a problem hiding this comment.
Just because it's not enforceable doesn't mean it's compliant with the spec. But "should be idempotent" is probably a better candidate for the stronger "must" wording, because that definitely will break things.
There was a problem hiding this comment.
oh! I totally missed the "should" on this "be idempotent" -- I totally agree on that one!
| HostVolumePluginMkdirID: &HostVolumePluginMkdir{ | ||
| ID: HostVolumePluginMkdirID, | ||
| TargetPath: config.SharedMountDir, | ||
| log: logger.With("plugin_id", HostVolumePluginMkdirID), |
There was a problem hiding this comment.
Nitpick: this logger won't be named host_volume_manager but the loggers we instantiate for the external plugins will, because they get cloned from hvm.log.With. Maybe we should hoist the logger.Named to the top of this method and then have the mkdir plugin call .With on that?
Sprucing things up after the initial push to get it working.
I added test coverage before moving funcs around to ensure no functional changes.
Lots of lines, but hopefully an easy read!