Skip to content
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 #282

Closed

Conversation

phvalguima
Copy link
Contributor

@phvalguima phvalguima commented May 2, 2024

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.

Dev experience

The idea is to make it easier to add management for separated plugins without impacting the rest of the code. A dev willing to add a new plugin must decide: do we need to manage a relation or just config options on the charm?
If not, then we can add a config-only plugin
If yes, then we will need new a new object to encapsulate the plugin handling: the "DataProvider"

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.

The relation-based plugins

These plugins are more elaborate, as they have to process events specific to the given plugin. We also must consider the case of large deployments, where data may come via dedicated relation or the peer-cluster relation.

These plugins should be managed by a separate entity, named the relation manager. Defining a common structure for the relation manager is outside of the scope of this PR.

For example, repository-s3 and OpenSearch backup.

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:

  1. 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
  2. opensearch_plugin_manager.py: add the new plugin to the plugin dict; the manager must be able to instantiate this new plugin
  3. 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
  4. models.py: add any relation data model to this lib

Using the new plugin data provider

While specific classes takes care of the plugin's API calls (e.g. /_snapshot API for the backup plugin is done by OpenSearchBackup class), the data provider facilitates the exchange of relation data between the specific class and the plugin_manager itself. This way, the plugin manager can apply any cluster-wide configurations that are needed for that plugin.

We need a class to do deal with relation specifics as some plugins may expect different relations depending on their deployment description, e.g. OpenSearchBackupPlugin. The OpenSearchPluginDataProvider encapsulates that logic away from the main plugin classes.

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.

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:

  1. Addresses low ranging fruits where we reduce the number of restarts and add caching support
  2. 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
  3. 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:

  1. Renaming method check_plugin_manager_ready to check_plugin_manager_ready_for_api
  2. Any plugin that needs to manage things via API call should check the health of the cluster using check_plugin_manager_ready_for_api
  3. Moving opensearch_distro.version to load the workload_version file we have present instead of an API call: this is two fold, 1. removes the dependency to a cluster to be ready and 2. makes this method in-sync with recent changes for upgrades logic
  4. Waive the need of loading the default settings if this particular unit is powered down: which makes sense, in this moment we can do any config changes as we will eventually powered it back up later
  5. If /_cluster/settings is available: apply the configs via API and do not add a restart request
  6. On config-changed handler, the upgrade_in_progress check gets precedence and will continuously defer config-changed event until upgrade is finished before calling the plugin manager
  7. Create a OpenSearchKeyStoreNotReadyYetError: responsible to identify the keystore has not been initialized yet across the cluster and hence, we cannot manage any plugins that use it; however, we always apply the opensearch.yml changes from that plugin
  8. Add cached_property whenever it makes sense, also adds logic to clean the cache if there was any relevant changes to its content.

That still frees the config_changed to just call plugin_manager.run() before everything is set, as the run() method changes hard configuration only.

Closes #252, #280

@phvalguima
Copy link
Contributor Author

@carlcsaposs-canonical I have reshuffled the self.upgrade_in_progress here. Now, we check and defer before even doing the plugin_manager.run() (hence, no options will change until the upgrade is finished). Is this the right way? In another sense, it holds the user from doing any changes while the upgrade is in progress.

@carlcsaposs-canonical
Copy link
Contributor

@phvalguima from the current diff, it seems like if there's an IP change and no config change, a warning message will be logged

ip change should be supported during in-progress upgrade (so if only ip change, there shouldn't be a warning)

@carlcsaposs-canonical carlcsaposs-canonical removed their request for review May 2, 2024 09:10
@phvalguima phvalguima marked this pull request as draft June 3, 2024 20:44
@phvalguima phvalguima removed the request for review from Mehdi-Bendriss June 3, 2024 20:45
@phvalguima phvalguima force-pushed the DPE-4251-plugin-manager-change-before-started-set branch from 39ddb84 to 3edb4d6 Compare June 9, 2024 22:13
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
@phvalguima phvalguima changed the title [DPE-4251] Plugin Manager handles the config changed before started [DPE-4251][DPE-4196][DPE-3603] Plugin Management Re-work (1/3): Plugin Manager handles the config changed before started Jun 11, 2024
@phvalguima phvalguima marked this pull request as ready for review June 12, 2024 04:51
@phvalguima phvalguima requested review from Mehdi-Bendriss and reneradoi and removed request for reneradoi June 12, 2024 04:51
@phvalguima phvalguima force-pushed the DPE-4251-plugin-manager-change-before-started-set branch from 4e8839f to ec15f2e Compare June 14, 2024 19:46
@phvalguima phvalguima marked this pull request as draft June 15, 2024 09:24
@phvalguima phvalguima removed the request for review from Mehdi-Bendriss June 15, 2024 09:25
@phvalguima phvalguima marked this pull request as ready for review June 17, 2024 05:16
.github/workflows/ci.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@Mehdi-Bendriss Mehdi-Bendriss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Pedro, I left some comments and questions

lib/charms/opensearch/v0/helper_charm.py Outdated Show resolved Hide resolved
lib/charms/opensearch/v0/opensearch_backups.py Outdated Show resolved Hide resolved
lib/charms/opensearch/v0/opensearch_backups.py Outdated Show resolved Hide resolved
lib/charms/opensearch/v0/opensearch_distro.py Outdated Show resolved Hide resolved
lib/charms/opensearch/v0/opensearch_keystore.py Outdated Show resolved Hide resolved
lib/charms/opensearch/v0/opensearch_backups.py Outdated Show resolved Hide resolved
lib/charms/opensearch/v0/opensearch_backups.py Outdated Show resolved Hide resolved
lib/charms/opensearch/v0/opensearch_plugins.py Outdated Show resolved Hide resolved
lib/charms/opensearch/v0/opensearch_plugins.py Outdated Show resolved Hide resolved
lib/charms/opensearch/v0/opensearch_plugins.py Outdated Show resolved Hide resolved
@phvalguima phvalguima requested a review from Mehdi-Bendriss July 8, 2024 17:44
@phvalguima phvalguima changed the title [DPE-4251][DPE-4196][DPE-3603] Plugin Management Re-work (1/3): Plugin Manager handles the config changed before started [DPE-4251][DPE-4196][DPE-3603] Plugin Management Refactor Jul 11, 2024
@phvalguima phvalguima changed the title [DPE-4251][DPE-4196][DPE-3603] Plugin Management Refactor [DPE-4251][DPE-4196][DPE-3603][DPE-4604] Plugin Management Refactor Jul 17, 2024
@phvalguima phvalguima marked this pull request as draft July 17, 2024 15:37
@phvalguima phvalguima changed the title [DPE-4251][DPE-4196][DPE-3603][DPE-4604] Plugin Management Refactor [DPE-4196] Plugin Management Refactor Sep 3, 2024
@phvalguima phvalguima closed this Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[RFE] Extend plugin manager
3 participants