Executor Supported Enhancements #399
Conversation
akutz
force-pushed
the
feature/executor-supported
branch
2 times, most recently
from
79cae1f to
94a7b4f
Compare
| @@ -150,6 +150,24 @@ type StorageExecutorWithSupported interface { | |||
| opts Store) (bool, error) | |||
| } | |||
|
|
|||
| // StorageExecutorWithMount is an interface that executor implementations may | |||
| // may use to become part of the mount/unmount workflow. | |||
| } else { | ||
| result = opResult | ||
| result = false |
There was a problem hiding this comment.
I'd prefer to see this as LSXSOpNone, even though they are same value.
| } | ||
|
|
||
| out, err := c.runExecutor(ctx, driverName, types.LSXCmdSupported) | ||
| if err != nil { | ||
| if err == types.ErrNotImplemented { | ||
| ctx.WithField("serviceDriver", driverName).Warn( | ||
| "supported cmd not implemented") | ||
| c.supportedCache.Set(driverName, true) | ||
| c.supportedCache.Set(driverName, types.LSXOpAllNoMount) |
There was a problem hiding this comment.
I realize that this code here isn't new, just modified, but I'm surprised it's hear. I thought the containing function here was only called if we had already done a type assertion that the executor implemented the interface that includes the Supported() method. As such, I would consider it an actual error if it's defined in the executor, but returns a Not Implemented error. I think this is probably here for backwards compatibility, but is that an issue at this point?
I guess I'm really wondering if it is safe to assume that if Supported is not implemented, we say this is LSXOpAllNomount, and I believe the answer to that is yes, since that's what we require from a minimal Executor. Which probably means this whole comment is moot. :)
There was a problem hiding this comment.
Hi @codenrhoden,
Pretty much :) It's back-compat. The only reason I'm still including it is the outstanding driver PRs there are. I don't know if they've all implemented Supported or not. I had originally used this PR to move the Supported function to the core type, but as I said, outstanding drivers made me think twice.
akutz
force-pushed
the
feature/executor-supported
branch
5 times, most recently
from
e775648 to
5dbde3e
Compare
| @@ -159,7 +182,8 @@ type ProvidesStorageExecutorCLI interface { | |||
| // StorageExecutorCLI provides a way to interact with the CLI tool built with | |||
| // the driver implementations of the StorageExecutor interface. | |||
| type StorageExecutorCLI interface { | |||
| StorageExecutorWithSupported | |||
| StorageExecutorFunctions | |||
| StorageExecutorWithMount | |||
There was a problem hiding this comment.
Looks like the StorageExecutorWithSupported interface is still defined, but not referenced. And it now has the wrong return type of bool instead of LSXSupportedOp for the Supported function. Does it need to stick around, deleted, or updated?
There was a problem hiding this comment.
As discussed on the phone, there are two interfaces:
StorageExecutorWithSupportedStorageExecutorCLI
The former is for the platform implementations of the executor and the latter is for the executor CLI itself. They're both required and correct.
Current coverage is 30.53% (diff: 0.00%)
|
This patch enhances the executor CLI to change the behavior of the "supported" function. Internally, storage platforms will still implement the same "Supported" function as previously, but now the executor CLI returns more than simply a flag. The executor CLI will now return a bitmask that corresponds to the functions supported by a storage platform on the executing host. This provides a means to indicate whether an executor supports mounting/unmounting for a given storage platform. The update also centralizes the query for future additions to the executor logic. This patch also introduces the interface StorageExecutorWithMount with two new functions, Mount and Unmount, but these functions are not yet a part of the storage workflow.
akutz
force-pushed
the
feature/executor-supported
branch
from
5dbde3e to
0886969
Compare
akutz
changed the title
This patch enhances the executor CLI to change the behavior of the "supported" function. Internally, storage platforms will still implement the same "Supported" function as previously, but now the executor CLI returns more than simply a flag. The executor CLI will now return a bitmask that corresponds to the functions supported by a storage platform on the executing host. This provides a means to indicate whether an executor supports mounting/unmounting for a given storage platform. The update also centralizes the query for future additions to the executor logic.
This patch also introduces the interface
StorageExecutorWithMountwith two new functions,MountandUnmount, but these functions are not yet a part of the storage workflow.This is a WiP. I will likely squash a future commit onto this PR that allows the new mount functionality in the executor to participate in the standard workflow. I just pushed the PR in order to allow others to review the work so far.