-
Notifications
You must be signed in to change notification settings - Fork 10
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
[DPE-4196] Plugin Management Refactor #435
Conversation
…run_cmd on Keystore class
…er-change-before-started-set
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.
Nice! After some discussions around this PR, my opinion is that it successfully enables integrating plugins into the codebase keeping the following items in a centralized development environment:
- Plugins that are configured via relations
- Secrets that need to be created for storing credentials (related to plugins specifically)
- Large deployment data abstraction (including passing secrets around)
Follow up work
There are some leftover tasks that there should be looked at once this is merged. I'm writing them down here, but I'll create issues for these once merged:
- Look at default settings on
<Plugin|Relation>RelData(Model)
classes. These defaults are not adding value, since the validator should be the main point where checks happen. But just removing defaults interacts with Plugindata
andconfig
methods, so I would rather have specific work done on this to make sure validation is well done. - Some extra
abstraction | constant
is needed around s3 (and azure) secrets, since there is too many literal strings being used around, which makes following the flow of data hard within the code.
Implements Azure storage integration. Changes: - All general objects (`OpenSearchBackups<nameOfClass>`) that worked under S3 integration have been renamed to be specific about S3. - `OpenSearchBackupBase` is now generic for both Azure and S3 integrators. - On `OpenSearchBaseCharm`, `self.backup()` is reworked to take into account both relations existing, or not. It will then generate the correct sub class for the appropriate relation. - Constants and error messages are a bit more generic to allow use on both relation error scenarios - New model `AzureRelData` for the integrator databag. --------- Co-authored-by: Pedro Guimaraes <pedro.guimaraes@canonical.com>
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.
We will merge this one - as we wait for #541 to follow and address the remaining feedback points.
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.
Re-approving.
The main goal is to: (1) separate API call (/_cluster) between plugin-specific calls and cluster settings; (2) curb the requirements for restart for any plugin changes; (3) get a faster response time using cached entries wherever possible; (4) add new use-case with secret handling within the plugin logic itself; and (5) define models so we can standardize the plugin data exchanged between the objects and via relation.
The config-only plugin
These are plugin configured via config options. In this case, it is only needed to add a new OpenSearchPlugin -child class that manages the config options to be added or removed from the cluster.
For example, opensearch-knn receives the config from the charm and returns the options to be set in the opensearch.yml.
New Plugin Manager Infra
Now, the plugin manager is able to manage plugins that depend on config options, API calls and secrets. Whenever adding a new plugin, we should consider:
opensearch_plugins.py: this plugin should have a representation that is consumable by plugin_manager; it should be composed of all the configurations and keys to be added or removed to the cluster's main configuration
opensearch_plugin_manager.py: add the new plugin to the plugin dict; the manager must be able to instantiate this new plugin
opensearch_{plugin-name}.py: if the plugin is managed by a given relation, this lib will implement the relation manager and interface with OpenSearch's plugin-specific APIs
models.py: add any relation data model to this lib
Secret Management
Each plugin that handles the specific secrets must implement the secret management logic in its operation. The goal is to avoid filling the opensearch_secrets.py methods with ifs for each plugin case and separating / isolating each plugin code.
Adds Azure Backup Support
This PR builds on top of #531 and provides all the azure backup support and integration with the Blob store.
Remove unneeded restarts and add caching
We ensure that any configuration changes that come from plugin management are applied via API before being persisted on config files. If the API responds with a 200 status, then we should only write the new value to the configuration and finish without a need for restart.
In case the service is down and API is not available, we can assume we will eventually start the service back up. In this case, it suffices to write the config entries to the files and leave to the next start to pick them up.
This task is going to be divided into 3x parts:
Addresses low ranging fruits where we reduce the number of restarts and add caching support
Three main actions: (i) Merge {add,delete}_plugin together and its equivalents in OpenSearchPluginConfig class; (ii) we receive one big dictionary where a key: None means we want to simply delete that entry; and (iii) the main OpenSearchKeystore must observe secret changes and update its values accordingly
Returns unit tests: this is going to be commented out whilst Parts 1 and 2 happen, given this part of the code was covered with extensive testing
The current implementation of plugin_manager.run waits for the cluster to be started before processing its config changed. We relax this demand and open to the option where the cluster is not yet ready, so we can modify the configuration without issuing a restart request.
#252 is closed with OpenSearchPluginRelationsHandler interface. It allows plugins to define how they will handle its relation(s). opensearch_backup module extends this interface and defines a checker to process either small or large deployments details.
Other relevant changes:
Closes #252, #280, #244