Plugin installer migration #173

engechas · 2022-03-31T19:59:34Z

Description

Migrates the PluginInstaller. Also slight refactors to OpenSearchInstaller to rename provisioner_defaults -> installer_defaults and make config_vars an optional param for the install method. The CompositeInstaller coming in the next PR will use this new parameter to pass in config_vars overrides.

Issues Resolved

#134

Check List

New functionality includes testing
- All unit and integration tests pass
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Chase Engelbrecht <[email protected]>

…dd unit tests for PluginInstaller Signed-off-by: Chase Engelbrecht <[email protected]>

Signed-off-by: Chase Engelbrecht <[email protected]>

travisbenedict · 2022-03-31T22:42:39Z

osbenchmark/builder/installers/opensearch_installer.py

@@ -26,9 +26,10 @@ def __init__(self, provision_config_instance, executor, hook_handler_class=Boots
        self.config_applier = ConfigApplier(executor, self.template_renderer, self.path_manager)
        self.host_cleaner = HostCleaner(self.path_manager)

-    def install(self, host, binaries, all_node_ips):
+    # pylint: disable=arguments-differ
+    def install(self, host, binaries, all_node_ips, config_vars=None):


Should config_vars just be added to the Installer.install interface?

Is there a reason that disabling the linter is preferred?

config_vars shouldn't be part of this method's signature at all IMO. An installer should be responsible for owning and defining its own config_vars rather than having some passed into it.

The trouble comes when both OS and plugin(s) are set to be installed. In this case, the config_vars from all sources must be considered. Adding config_vars to the signature allows the CompositeInstaller to pass in the finalized config_vars from all sources without changing the general flow of the sub-installers. OpenSearchInstaller can be consumed as a standalone Installer implementation or utilized by the CompositeInstaller via the same install method.

I feel that I am missing a design pattern that would eliminate the need to add config_vars to this method. I will think on it some more and try to come up with a better solution.

I agree with your assessment that config_vars shouldn't be a part of this method. Its essentially an implementation detail that the user of this method shouldn't care about. Also, that's why the Installer interface shouldn't & doesn't have it.

Please correct me if I'm wrong, but it seems like each installer already just makes a single install call in its lifetime. Constructor is instance specific and can be different for classes implementing the same interface, have you considered having the config_vars as an optional constructor param for OpenSearchInstaller and making it an class level variable?

I believe this way the CompositeInstaller can have multiple sub-installers, and not break their general flow. However, if you mean the CompositeInstaller needs results from sub-installers to come up with the final config_vars, I'd still recommend the same approach, but using a factory class the generates the OS installer instance after the config_vars are ready.

The CompositeInstaller does need results from the sub-installers to come up with the final config_vars. Specifically, it needs the Node object generated by the OpenSearchInstaller's install method.

With the current implementation there's no way to generate the config_vars without first instantiating an instance of OpenSearchInstaller and invoking the install method to get a Node. I think a further layer of abstraction to separate preparing the installation and applying the config data is the solution here: create new OpenSearchPreparer and PluginPreparer classes with prepare and get_config_vars methods.

Then the OpenSearch and PluginInstallers would take in a respective instance of these Preparer classes and perform the whole installation flow (including applying the config) within the install method. The CompositeInstaller would have instances of both OpenSearchPreparer and PluginPreparer, with the flow being: call prepare methods -> call get_config_vars methods -> generate final config_vars -> apply configs

I plan to make heavy use of the factory pattern when selecting the correct builder components for the ClusterBuilder :)

achitojha · 2022-04-01T06:21:27Z

osbenchmark/builder/installers/opensearch_installer.py

@@ -26,9 +26,10 @@ def __init__(self, provision_config_instance, executor, hook_handler_class=Boots
        self.config_applier = ConfigApplier(executor, self.template_renderer, self.path_manager)
        self.host_cleaner = HostCleaner(self.path_manager)

-    def install(self, host, binaries, all_node_ips):
+    # pylint: disable=arguments-differ
+    def install(self, host, binaries, all_node_ips, config_vars=None):


I agree with your assessment that config_vars shouldn't be a part of this method. Its essentially an implementation detail that the user of this method shouldn't care about. Also, that's why the Installer interface shouldn't & doesn't have it.

Please correct me if I'm wrong, but it seems like each installer already just makes a single install call in its lifetime. Constructor is instance specific and can be different for classes implementing the same interface, have you considered having the config_vars as an optional constructor param for OpenSearchInstaller and making it an class level variable?

I believe this way the CompositeInstaller can have multiple sub-installers, and not break their general flow. However, if you mean the CompositeInstaller needs results from sub-installers to come up with the final config_vars, I'd still recommend the same approach, but using a factory class the generates the OS installer instance after the config_vars are ready.

achitojha · 2022-04-01T06:26:21Z

osbenchmark/builder/installers/plugin_installer.py

+        if self.hook_handler.can_load():
+            self.hook_handler.load()


Does this need to happen during the object construction itself? Would it make sense to move this to the install logic?

I'm not fundamentally against it but prefer lightweight constructors. Think of scenarios where these fail and what stack trace they leave - I feel it makes more sense if we say the plugin failed to install /load. Instead if the constructor throws an exception, I suspect there's things like class not found exception or something which isn't a good description of what happened.

It could go in the install logic. The rationale for having it here is to fail fast if an exception is thrown when loading the hook_handler.

If this logic is moved to the install method, then the exception it creates on failure would only be thrown after the infrastructure has been provisioned and the necessary data downloaded and installed on the host. By failing in the constructor, no resources are created or work done for a scenario that couldn't possibly succeed.

Perhaps wrapping the exception with some documentation on the failure would be a solution to the vague description. Given that this hook_handler logic is used in multiple Installers, adding that handling into the factory generating Installer instances would be a good approach.

Signed-off-by: Chase Engelbrecht <[email protected]>

engechas · 2022-04-01T22:20:29Z

After thinking more on it, there's no need to distinguish between a SingularBareInstaller and a CompositeBareInstaller as the flow can be the exact same via looping. Instead, I've opted to create a BareInstaller that handles both scenarios

weifenh · 2022-04-05T17:57:31Z

osbenchmark/builder/installers/bare_installer.py

+    def _get_node(self, preparer_to_node):
+        nodes_list = list(filter(lambda node: node is not None, preparer_to_node.values()))
+
+        assert len(nodes_list) == 1, "Exactly one node must be provisioned per host, but found nodes: {}".format(nodes_list)


You have preparers_to_nodes as a list, so I guess that one node per host may be changed in the future?

preparers_to_nodes is a map of a Preparer to the return value of its prepare method, which can be either a Node object or None. Only 1 Preparer should be generating a Node, the others are used to install additional components (i.e. plugins) to that same Node

Allowing more than one node per host could be implemented in the future, but there doesn't seem to be much justification for supporting it at the moment.

weifenh · 2022-04-05T17:59:32Z

osbenchmark/builder/installers/bare_installer.py

+from osbenchmark.builder.utils.template_renderer import TemplateRenderer
+
+
+class BareInstaller(Installer):


The naming is fine, but hope to understand what installations are attributed to "bare"..?

Bare essentially means "not in a container". The original implementation used this name and I carried forward since I can't think of a better one. Open to suggestions if anyone has ideas

engechas added 4 commits March 31, 2022 14:41

Initial PluginInstaller migration

2f0f38f

Signed-off-by: Chase Engelbrecht <[email protected]>

More PluginInstaller migration

c000e33

Signed-off-by: Chase Engelbrecht <[email protected]>

Make config_vars optional param in OpenSearch and PluginInstallers, a…

c4b8094

…dd unit tests for PluginInstaller Signed-off-by: Chase Engelbrecht <[email protected]>

Add disable=arguments-differ for linter

61acc8b

Signed-off-by: Chase Engelbrecht <[email protected]>

travisbenedict reviewed Mar 31, 2022

View reviewed changes

achitojha reviewed Apr 1, 2022

View reviewed changes

Refactor to use Preparer interface in SingularBareInstaller

6c0b5c4

Signed-off-by: Chase Engelbrecht <[email protected]>

engechas changed the title ~~Plugin installer mig~~ Plugin installer migration Apr 1, 2022

engechas added 2 commits April 1, 2022 17:19

Create BareInstaller

e639f1b

Signed-off-by: Chase Engelbrecht <[email protected]>

Add space at end of file for linter

dd56c2d

Signed-off-by: Chase Engelbrecht <[email protected]>

weifenh approved these changes Apr 5, 2022

View reviewed changes

travisbenedict approved these changes Apr 5, 2022

View reviewed changes

engechas merged commit c67c982 into opensearch-project:main Apr 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin installer migration #173

Plugin installer migration #173

engechas commented Mar 31, 2022 •

edited

Loading

travisbenedict Mar 31, 2022

engechas Mar 31, 2022

achitojha Apr 1, 2022

engechas Apr 1, 2022

achitojha Apr 1, 2022

achitojha Apr 1, 2022

engechas Apr 1, 2022

engechas commented Apr 1, 2022

weifenh Apr 5, 2022

engechas Apr 5, 2022

weifenh Apr 5, 2022

engechas Apr 5, 2022

		from osbenchmark.builder.utils.template_renderer import TemplateRenderer


		class BareInstaller(Installer):

Plugin installer migration #173

Plugin installer migration #173

Conversation

engechas commented Mar 31, 2022 • edited Loading

Description

Issues Resolved

Check List

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engechas commented Apr 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

engechas commented Mar 31, 2022 •

edited

Loading