-
Notifications
You must be signed in to change notification settings - Fork 378
spec: consolidate xxxProbe calls into Identity.Probe #190
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
spec: consolidate xxxProbe calls into Identity.Probe #190
Conversation
jdef
commented
Feb 10, 2018
•
edited
Loading
edited
- Remove ControllerProbe RPC.
- Remove NodeController RPC.
- Introduce Identity.Probe RPC, clarify its intent.
- supersedes Capabilities and Probe RPCs for Identity service. #184
- fixes Clarify Identity Service vs Probe request #171
674bab3
to
78b5ea6
Compare
|
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
spec.md
Outdated
|
||
A Plugin MUST implement this RPC call. | ||
The primary utility of the Probe RPC is deployment verification: this can be a one time or periodic check to ensure that a plugin instance is healthy and has all dependencies available. | ||
This information can be used, for example, to monitor the health of the plugin and redeploy the plugin (or take other automated measures) when it becomes unhealthy. |
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.
Deployment verification and checking of dependencies is something I would lump in to "initialization checks" and not the responsibility of Probe
. By implying that it is, we implicitly expect a Probe
call to happen around deployment time, and it may not happen until sometime afterwards.
I would suggest rewording this to be as generic as possible: The primary utility of the Probe RPC is to verify the plugin is in a healthy state. If an unhealthy state is reported, via a non-success response, the CO MAY take action (such as restarting the plugin container) to try and bring the plugin to a healthy state.
spec.md
Outdated
@@ -1457,6 +1443,7 @@ Supervised plugins MAY be isolated and/or resource-bounded. | |||
##### Available Services | |||
|
|||
* Plugin Packages MAY support all or a subset of CSI services; service combinations MAY be configurable at runtime by the Plugin Supervisor. | |||
* A plugin must know the "mode" in which it's operating: mode of operation is NOT discoverable via interaction with the CO via the CSI specification. |
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.
How about: A plugin must know the "mode" in which it's operating (e.g. node, controller, or both). This specification does not dictate the mechanism by which mode of operation must be discovered, leaving it to the SP to decide.
Good suggestions, will incorporate
…On Feb 16, 2018 8:36 PM, "Saad Ali" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In spec.md
<#190 (comment)>
:
> @@ -513,6 +522,41 @@ message PluginCapability {
If the plugin is unable to complete the GetPluginCapabilities call successfully, it MUST return a non-ok gRPC code in the gRPC status.
+#### `Probe`
+
+A Plugin MUST implement this RPC call.
+The primary utility of the Probe RPC is deployment verification: this can be a one time or periodic check to ensure that a plugin instance is healthy and has all dependencies available.
+This information can be used, for example, to monitor the health of the plugin and redeploy the plugin (or take other automated measures) when it becomes unhealthy.
Deployment verification and checking of dependencies is something I would
lump in to "initialization checks" and not the responsibility of Probe.
By implying that it is, we implicitly expect a Probe call to happen
around deployment time, and it may not happen until sometime afterwards.
I would suggest rewording this to be as generic as possible: The primary
utility of the Probe RPC is to verify the plugin is in a healthy state. If
an unhealthy state is reported, via a non-success response, the CO MAY take
action (such as restarting the plugin container) to try and bring the
plugin to a healthy state.
------------------------------
In spec.md
<#190 (comment)>
:
> @@ -1457,6 +1443,7 @@ Supervised plugins MAY be isolated and/or resource-bounded.
##### Available Services
* Plugin Packages MAY support all or a subset of CSI services; service combinations MAY be configurable at runtime by the Plugin Supervisor.
+ * A plugin must know the "mode" in which it's operating: mode of operation is NOT discoverable via interaction with the CO via the CSI specification.
How about: A plugin must know the "mode" in which it's operating (e.g.
node, controller, or both). This specification does not dictate the
mechanism by which mode of operation must be discovered, leaving it to the
SP to decide.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#190 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACPVLFb5XgEnK5NAKuMx11ER_jXwG3TWks5tVi0agaJpZM4SAw1T>
.
|
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.
Modulo @saad-ali's suggestion. The rest looks good to me.
* Remove ControllerProbe RPC. * Remove NodeController RPC. * Introduce Identity.Probe RPC, clarify its intent.
78b5ea6
to
948e2ec
Compare
incorporated latest feedback, rebased to master |
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
LGTM. Merging. |