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.
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
Squashed commit of adding support for cli plugin validation reporting. #5665
base: main
Are you sure you want to change the base?
Squashed commit of adding support for cli plugin validation reporting. #5665
Changes from 8 commits
6c8a88c
7c42e8f
4fccfd2
40c57bf
800f890
a005a10
25ebf31
5ddf246
afe6263
7064969
8a6aa40
4995563
c0615b9
d05c2d1
4f2cafa
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This runs a new server instance. I think that this approach is problematic in multiple ways. It requires to maintain the
ValidateOnly
setting in the config, and have different flows of the run function depending of its value. Since we don't want to run a server instance, executing this function doesn't seem to be appropriate when running the validate command.This requires that when the run function is updated, we need to remember that it's also called as part of the validate command, and consider if the new code can be executed as part of the validation or not.
I believe that as is in this PR, running the validate command will connect to the backend database, run migrations, create the data directory if needed and start the runtime collector for metrics. It also has the side effect of output the logs as if the server is running, which is a surprising behavior as part of a validate command, and makes the parsing of the output more difficult.
Although it could be changed to not do those things, I think that is an example of how problematic this can be if we miss to update the run function with the appropriate
ValidateOnly
condition. Running a non intended database migration can be particularly problematic.I think that a better approach would be to not involve the run function as part of the execution of a validate command.
The validate command would just 1) Load the server/agent config as is it currently does (which validates the server/agent config) and 2) Go through each configured plugin and call the plugin's Validate function.
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.
I think there is a disconnect here. The entire plan since the beginning was to run a limited server instance that could load the plugins, and then run the plugin Validate() funcs, as detailed over the last six months of development. This was mentioned in multiple contributor's meetings, and never met a hint of "we don't want to do this". The exact words were "we will launch a new server instance in such a way it doesn't interfere with the running server instance, it will load the plugins, validate the configuration, and then shut down."
The approach you suggested was the initial approach, and it didn't work. In a call with @azdagron on October 29th, the reason it didn't work was explained to me:
Andrew directed the solution to include a "ValidateOnly" flag that previously didn't exist, which is why the submission took a couple of extra weeks to refactor and debug. While the solution seems less elegant than other approaches, it actually resulted in less code (meaning less maintenance footprint).
The ValidateOnly returns from Run() to shutdown the server as soon as the plugins validate, before any of the processing of data is performed.
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.
Thanks for the detailed response. I've analyzed this and I still think that we can and should avoid executing Server.run as part of the validate command.
I believe that we can achieve this by having a separate function for validation (one for the server, one for the agent) that is similar to the catalog.Load function but it calls the Validate function instead of Configure. We should also be able to use non-functional loggers and metrics, avoiding to output logs. Validation of the datastore plugin configuration should also be possible, I think.
I've done a quick POC of how this would work, it can be found here: main...amartinezfayo:validate-server-cli
It can validate all configured regular plugins and the sql datastore plugin, without the need of opening database connections, creating the data directory or going through migrations. It should be updated to leverage all the notes from the Validate response and not only the error.