RFC: forwarding control requests to plugins #1682
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
krnowak
force-pushed
the
krnowak/plugin-controls
branch
2 times, most recently
from
b76e069 to
e93a0cb
Compare
|
Updated the PR. There are still some more problems though:
|
| p.iowaitMode = !p.iowaitMode | ||
| w.WriteHeader(http.StatusOK) | ||
| fmt.Fprint(w, "{}") | ||
| } |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
Could you update examples/plugins/README.md? |
krnowak
force-pushed
the
krnowak/plugin-controls
branch
from
e93a0cb to
8bb4ea3
Compare
|
|
||
| ```json | ||
| { | ||
| "AppID": "some ID of an app" |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
krnowak
force-pushed
the
krnowak/plugin-controls
branch
2 times, most recently
from
b0c1be0 to
4b537fa
Compare
|
Updated. There are still some failures, likely because of changing |
|
|
||
| ## <a id="protocol"></a>Protocol | ||
| Each plugin should have an unique ID. It is forbidden to change it | ||
| during plugin's lifetime. The scope probe will get the plugin's ID |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
|
This looks like it's all backwards-compatibility with the existing controls? But wanted to check the interaction between them. For example, what happens if you have both? |
This can be done by calling TESTDIRS="./report ./probe" make tests
So it displays differences behind interface that would otherwise go unnoticed (like string vs []byte).
Changing plugin's ID only complicates control handling in plugins so let's ban it.
The socket has the "http_requests.sock" filename, so ID should be "http_requests", not "http-requests".
We will want to put plugin id in a control id, which is sent to an app and then to GUI. When we get a control request from GUI, we will want to extract the plugin ID from the control name. To do it unambiguously we need some separator made of chars that are not allowed in a plugin name. This is to avoid the situation when there are two plugins: "Plugin" and "PluginFoo". "Plugin" exposes a control named "FooControl" and "PluginFoo" exposes a control named "Control". Faking the control names which will be sent to the app would result in two "PluginFooControl". One possible option for plugin ID and control name separator would be "/", but that won't work, since the request sent from GUI to the app to <probe>/<node>/<control> would actually be <probe>/<node>/<plugin>/<control> and as such wouldn't match the URL template in RegisterControlRoutes().
Plugin ID must be non-empty when the plugin is created and the followup reports cannot change it.
It is not a singleton anymore. Instead it is an object with a registry backend. The default registry backend is provided, which is equivalent to what used to be before. Custom backend can be provided for testing purposes. The registry also supports batch operations to remove and add handlers as an atomic step.
Thanks to that, plugins can react to requests from controls they exposed. To make it work, plugins registry modifies each plugin's report by prepending the plugin ID to the control name the plugin has exposed before sending it to the app. Then the registry installs the control request handler for this faked control name, which forwards the request to the correct plugin. This adds a new API endpoint to plugins next to "/report" - a "/control" entry. The body of the request is the JSON-encoded xfer.Request instance.
The solution is taken from the toplevel Makefile.
It exposes a button that allows switching between showing an iowait
statistics and an idle statistics. When the button is pressed it
should be replaced with other button. The button is shown in the host
node.
This is a rather nasty case as it shows several problems:
- Button control races
- The way the NodeControl currently works creates races between
plugins adding buttons to the same node. This is because
NodeControls are not really merged, but rather one of the two are
chosen based on a NodeControls' timestamps, so the older one is
thrown away entirely. In the end GUI can switch randomly between
showing controls from one plugin or from another.
- Showing outdated statistics
- When pressing the button to switch to show the other statistics,
the old ones are still shown for several seconds.
- Slowness of the updates in GUI
- Pressing the button yields no immediate reaction. Changes happen
after several seconds. Probably related to the previous point.
This is better than writing JSON strings by hand, which is error-prone.
This is better than writing JSON strings by hand, which is error-prone.
Plugins are queried for reports two times in a second. That's often enough to get the shortcut reports. The reports are sent together with the response.
Just to make sure that all the node controls are rewritten, even those that don't have a counterpart in topology controls.
This commit makes the LatestMap type a sort of a base class, that should not be used directly. This also adds a generator for LatestMap "concrete" types with a specific data type in the LatestEntry.
This LatestMap will hold a struct that has more information about the state of the node control.
This allows plugins to add controls to nodes that already have some controls set by other plugin. Previously only the last plugin that sets the controls in the node would have its controls visible. That was because of NodeControls' Merge function that actually weren't merging data from two inputs, but rather returning data that was newer and discarding the older one.
Give a bit more information about how to write a plugin.
The new probe will convert all node's LatestControls to Controls, so the old app can consume them. Also, the new app will convert all node's Controls to LatestControl, so it can consume the reports from old probes.
krnowak
force-pushed
the
krnowak/plugin-controls
branch
from
6000729 to
0ecb908
Compare
|
Updated to fix the merge conflict. |
|
LGTM |
Merged
This was referenced Aug 16, 2016
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is a tentative PR that implements forwarding control requests to plugins.
Problems so far:I made iowait example plugin a controller (adds a button that switches between showing iowait and idle), but:GUI shows plugin's statistics in a separate node, while it used to be in the same oneGUI shows no button from plugin