feat: Config api 2 #9546
feat: Config api 2 #9546
Conversation
| _, _ = w.Write(bytes) | ||
| } | ||
|
|
||
| func (s *ConfigAPIService) runningPlugins(w http.ResponseWriter, req *http.Request) { |
There was a problem hiding this comment.
When there are no plugins this returns "null". I expected it to return an empty array. We should have a unit test for this case.
There was a problem hiding this comment.
Did you link the wrong commit? That one is about the linter. Looks like you meant efa4eec.
I manually retested to confirm that we get an empty array now. I see in controller_test.go near the end of TestInputPluginLifecycle there's a check for length 0 calling ListRunningPlugins, but len returns 0 on empty slice or nil.
We need unit tests that make http calls. The tests in controller_test.go are great but it's not enough just to test the methods of the api type.
There was a problem hiding this comment.
There's unit tests with http calls in listener_test.go
|
It looks like this branch introduces a problem handling os signals. If I start telegraf with a config.api but no inputs or outputs, then use the api to add an inputs.cpu and an outputs.file writing to stdout, the plugins are created fine but when I hit ^C, the program doesn't terminate. On master, if I have a config file that only has an inputs.cpu and an outputs.file, I can hit ^C and it closes telegraf immediately. |
| _, _ = w.Write(bytes) | ||
| } | ||
|
|
||
| func (s *ConfigAPIService) runningPlugins(w http.ResponseWriter, req *http.Request) { |
There was a problem hiding this comment.
Did you link the wrong commit? That one is about the linter. Looks like you meant efa4eec.
I manually retested to confirm that we get an empty array now. I see in controller_test.go near the end of TestInputPluginLifecycle there's a check for length 0 calling ListRunningPlugins, but len returns 0 on empty slice or nil.
We need unit tests that make http calls. The tests in controller_test.go are great but it's not enough just to test the methods of the api type.
| } | ||
|
|
||
| func (a *api) ListRunningPlugins() (runningPlugins []Plugin) { | ||
| if a == nil { |
There was a problem hiding this comment.
This func doesn't return whether a plugin is an input or an output. Since we have some plugins with the same name that are inputs and outputs (like inputs.file and outputs.file) we need to disambiguate them here.
There was a problem hiding this comment.
plugin names should be inputs.file or outputs.file. There's a few places where this was missed and i'm currently cleaning them up.
| require.EqualValues(t, 200, resp.StatusCode) | ||
| require.NoError(t, err) | ||
|
|
||
| resp, err = http.Get(srv.URL + "/plugins/running") |
There was a problem hiding this comment.
When deleting a plugin, does a response code of 200 guarantee that the request has been processed and the plugin deleted? In the test TestStartPlugin there is a for loop checking to make sure if the plugin has started running, wondering if that is necessary here or if the behavior is different.
There was a problem hiding this comment.
It does not guarantee the plugin has been removed as they're asynchronous. Will add something to wait appropriately.
| Reason string | ||
| }{} | ||
|
|
||
| for statusResp.Status != "running" { |
There was a problem hiding this comment.
Is this for loop necessary because it could take sometime for a plugin to start running? We probably then want to return 202 to let the user know the request has been accepted and is still processing (https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/202)
There was a problem hiding this comment.
202 Accepted doesn't seem quite right; the docs say it's "uncommital and may not be acted on", but that's not true as I've already initialized the plugin at this point and it has started, it just may not have gotten very far.
| func (s *ConfigAPIService) pluginStatus(w http.ResponseWriter, req *http.Request) { | ||
| defer req.Body.Close() | ||
| id := mux.Vars(req)["id"] | ||
| if len(id) == 0 { |
There was a problem hiding this comment.
Should we also check to see if the ID matches the expected format? In the test there is a regex check like this: require.Regexp(t, `^[\da-f]{8}\d{8}$`, createResp.ID) not sure if there is a common function for this somewhere already.
| } | ||
|
|
||
| type PluginConfig struct { | ||
| ID string `json:"id"` // unique identifer |
There was a problem hiding this comment.
The ID field doesn't seem to be used. Was it intended to hold a well known string to identify the plugin config in the database? It looks like storage.Save() now uses bolt namespace "config-api" and key "plugins" that does the same thing. Can we get rid of this field and merge PluginConfig and PluginConfigCreate?
There was a problem hiding this comment.
this is used by ConfigAPIPlugin, and as far as I can tell it does and should serialize this ID value.
There was a problem hiding this comment.
I can comment it out and telegraf builds without error. My IDE says there are no references to it. The string "id" is not present in plugins/configs/api/plugin.go (where ConfigAPIPlugin is defined).
|
|
||
| // store state | ||
| if a.Storage != nil { | ||
| if err := a.Storage.Save("config-api", "plugins", &a.plugins); err != nil { |
There was a problem hiding this comment.
Storage.Load() is called once on startup and Storage.Save() is called once on shutdown. Boltdb seems like overkill in this use case. Saving to JSON would work just as well and have the advantage of being human readable.
I assume you guys have more in mind for boltdb and storage plugins, maybe more robust approach to saving (like saving periodically or after every config change)? Can you share what you have in mind related to this?
There was a problem hiding this comment.
The storage is a plugin so we aren't ruling out a json storage plugin in the future. Storage either does or will get written to any time a plugin is added or removed. it's more like a db in that you can write individual records instead of rewriting the whole file every time a single record changes.
There was a problem hiding this comment.
When you say "Storage either does or will get written to any time a plugin is added or removed" I'm not sure we're looking at the same code. Like I said before, Storage.Save() is called once at shutdown. It's not called anywhere else outside of a unit test.
If you have plans for this to work differently in the future, we need you to communicate them somehow. It would be best to have in a design doc, but it at least needs to be commented in the code. Otherwise there's no way to do a meaningful review.
|
|
||
|
|
||
| +------------+ Processors and aggregators can be ordered +--------+ | ||
| | Input +---+ and chained arbitrarily +--->+ Output | | ||
| +------------+ | | +--------+ | ||
| | | | ||
| +------------+ | +-----------+ +------------+ +-----------+ +------------+ | +--------+ | ||
| | Input +------>+ Processor +--->+ Aggregator +----->+ Processor +--->+ Aggregator +-->---->+ Output | | ||
| +------------+ | +-----------+ +------------+ +-----------+ +------------+ | +--------+ | ||
| | | | ||
| +------------+ | | +--------+ | ||
| | Input +---+ +--->+ Output | | ||
| +------------+ | | +--------+ | ||
| | | | ||
| +------------+ | | +--------+ | ||
| | Input +---+ +--->+ Output | | ||
| +------------+ +--------+ |
There was a problem hiding this comment.
can you code format this? when it gets converted to markdown the diagram gets all funky.
Looks like new artifacts were built from this PR. Get them here!Artifact URLs |
replaces #8489
resolves #7519
resolves #7993
resolves #8016