LOOPP plugin config validation service #12430
Conversation
george-dorin
commented
Mar 14, 2024
•
george-dorin
commented
- Instantiate and call validation when a generic plugin config is validated
|
I see that you haven't updated any README files. Would it make sense to do so? |
…-validation # Conflicts: # core/scripts/go.mod # core/scripts/go.sum # go.mod # go.sum # integration-tests/go.mod # integration-tests/go.sum # integration-tests/load/go.mod # integration-tests/load/go.sum
|
I see you updated files related to |
| } | ||
|
|
||
| loopID := fmt.Sprintf("%s-%s-%s", p.PluginName, spec.ContractID, spec.GetID()) | ||
| cmdFn, grpcOpts, err := rc.RegisterLOOP(plugins.CmdConfig{ |
There was a problem hiding this comment.
It seems a bit odd that we're registering the LOOPP only to immediately unregister it. Do we even need to register it, and if so, why?
There was a problem hiding this comment.
When the plugin is started it requires a prometheusPort, in order to get a valid port we must register the LOOPP
There was a problem hiding this comment.
hmm, this seems inverted.
what is the goal of this code?
another red flag is starting and closing the plugin. why is that happening?
if we register and unregister for the purpose of getting a port and validating a port, then we are still open to failure later when re-register it because there is dynamic code to allocate the port. moreover, the port will be different, so what use is the first registration?
There was a problem hiding this comment.
@krehermann To validate the config we must start the binary so we can call the validateConfig() that is inside the LOOPP, this has to happen inside the LOOPP because we do not know what configuration it expects. When the LOOPP is started, as part of the registration, a Prometheus port is expected, that is why we are registering the LOOPP.
There was a problem hiding this comment.
@krehermann We thought about this quite a bit, and @george-dorin is correct -- I agree that it's counter-intuitive to register and deregister immediately but it's not clear what alternative there is given that the point of registering is to "lock" host resources such as the prometheus port.
There was a problem hiding this comment.
The point of this code is just to call out the LOOPP to validate the config; we aren't trying to do any validation with respect to allocation of system resources and we aren't guaranteeing that a successful validation implies that we will definitely start a LOOPP either.
…-validation # Conflicts: # core/scripts/go.mod # core/scripts/go.sum # go.mod # go.sum # integration-tests/go.mod # integration-tests/go.sum # integration-tests/load/go.mod # integration-tests/load/go.sum
…-validation # Conflicts: # core/scripts/go.mod # core/scripts/go.sum # go.mod # go.sum # integration-tests/go.mod # integration-tests/go.sum # integration-tests/load/go.mod # integration-tests/load/go.sum
…-validation # Conflicts: # core/scripts/go.mod # core/scripts/go.sum # go.mod # go.sum # integration-tests/go.mod # integration-tests/go.sum # integration-tests/load/go.mod # integration-tests/load/go.sum
…-validation # Conflicts: # integration-tests/go.mod # integration-tests/load/go.mod
|
