-
Notifications
You must be signed in to change notification settings - Fork 80
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
Plugin installer migration #173
Changes from 4 commits
2f0f38f
c000e33
c4b8094
61acc8b
6c0b5c4
e639f1b
dd56c2d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,50 @@ | ||
import logging | ||
import os | ||
|
||
from osbenchmark.builder.installers.installer import Installer | ||
from osbenchmark.builder.provision_config import BootstrapHookHandler | ||
from osbenchmark.builder.utils.config_applier import ConfigApplier | ||
from osbenchmark.builder.utils.path_manager import PathManager | ||
from osbenchmark.builder.utils.template_renderer import TemplateRenderer | ||
|
||
|
||
class PluginInstaller(Installer): | ||
def __init__(self, plugin, executor, hook_handler_class=BootstrapHookHandler): | ||
super().__init__(executor) | ||
self.logger = logging.getLogger(__name__) | ||
self.plugin = plugin | ||
self.hook_handler = hook_handler_class(self.plugin) | ||
if self.hook_handler.can_load(): | ||
self.hook_handler.load() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It could go in the If this logic is moved to the Perhaps wrapping the exception with some documentation on the failure would be a solution to the vague description. Given that this |
||
self.template_renderer = TemplateRenderer() | ||
self.path_manager = PathManager(executor) | ||
self.config_applier = ConfigApplier(executor, self.template_renderer, self.path_manager) | ||
|
||
# pylint: disable=arguments-differ | ||
def install(self, host, binaries, all_node_ips, config_vars=None): | ||
install_cmd = self._get_install_command(host, binaries) | ||
self.executor.execute(host, install_cmd) | ||
|
||
if not config_vars: | ||
config_vars = self.get_config_vars() | ||
self.config_applier.apply_configs(host, host.node, self.plugin.config_paths, config_vars) | ||
|
||
def _get_install_command(self, host, binaries): | ||
installer_binary_path = os.path.join(host.node.binary_path, "bin", "opensearch-plugin") | ||
plugin_binary_path = binaries.get(self.plugin.name) | ||
|
||
if plugin_binary_path: | ||
self.logger.info("Installing [%s] into [%s] from [%s]", self.plugin.name, host.node.binary_path, plugin_binary_path) | ||
return '%s install --batch "%s"' % (installer_binary_path, plugin_binary_path) | ||
else: | ||
self.logger.info("Installing [%s] into [%s]", self.plugin.name, host.node.binary_path) | ||
return '%s install --batch "%s"' % (installer_binary_path, self.plugin.name) | ||
|
||
def get_config_vars(self): | ||
return self.plugin.variables | ||
|
||
def invoke_install_hook(self, phase, variables, env): | ||
self.hook_handler.invoke(phase.name, variables=variables, env=env) | ||
|
||
def cleanup(self, host): | ||
pass |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
from unittest import TestCase, mock | ||
from unittest.mock import Mock | ||
|
||
from osbenchmark.builder.installers.plugin_installer import PluginInstaller | ||
from osbenchmark.builder.models.host import Host | ||
from osbenchmark.builder.models.node import Node | ||
from osbenchmark.builder.provision_config import PluginDescriptor | ||
|
||
|
||
class PluginInstallerTest(TestCase): | ||
def setUp(self): | ||
self.node = Node(binary_path="/fake_binary_path", data_paths=["/fake1", "/fake2"], name=None, | ||
pid=None, telemetry=None, port=None, root_dir=None, log_path=None, heap_dump_path=None) | ||
self.host = Host(name="fake", address="10.17.22.23", metadata={}, node=self.node) | ||
self.binaries = {"unit-test-plugin": "/data/builds/distributions"} | ||
self.all_node_ips = [] | ||
|
||
self.executor = Mock() | ||
self.plugin = PluginDescriptor(name="unit-test-plugin", config_paths=["default"], variables={"active": True}) | ||
|
||
self.plugin_installer = PluginInstaller(self.plugin, self.executor) | ||
self.plugin_installer.config_applier = Mock() | ||
|
||
def test_plugin_install_with_binary_path(self): | ||
self.plugin_installer.install(self.host, self.binaries, self.all_node_ips) | ||
|
||
self.executor.execute.assert_has_calls([ | ||
mock.call(self.host, "/fake_binary_path/bin/opensearch-plugin install --batch \"/data/builds/distributions\"") | ||
]) | ||
self.plugin_installer.config_applier.apply_configs.assert_has_calls([ | ||
mock.call(self.host, self.node, ["default"], {"active": True}) | ||
]) | ||
|
||
def test_plugin_install_without_binary_path(self): | ||
self.plugin_installer.install(self.host, {}, self.all_node_ips) | ||
|
||
self.executor.execute.assert_has_calls([ | ||
mock.call(self.host, "/fake_binary_path/bin/opensearch-plugin install --batch \"unit-test-plugin\"") | ||
]) | ||
self.plugin_installer.config_applier.apply_configs.assert_has_calls([ | ||
mock.call(self.host, self.node, ["default"], {"active": True}) | ||
]) | ||
|
||
def test_config_vars_override(self): | ||
self.plugin_installer.install(self.host, {}, self.all_node_ips, {"my": "override"}) | ||
|
||
self.executor.execute.assert_has_calls([ | ||
mock.call(self.host, "/fake_binary_path/bin/opensearch-plugin install --batch \"unit-test-plugin\"") | ||
]) | ||
self.plugin_installer.config_applier.apply_configs.assert_has_calls([ | ||
mock.call(self.host, self.node, ["default"], {"my": "override"}) | ||
]) |
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.
Should
config_vars
just be added to theInstaller.install
interface?Is there a reason that disabling the linter is preferred?
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.
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.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 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 theInstaller
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 forOpenSearchInstaller
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 theconfig_vars
are ready.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.
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'sinstall
method.With the current implementation there's no way to generate the
config_vars
without first instantiating an instance of OpenSearchInstaller and invoking theinstall
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 withprepare
andget_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 finalconfig_vars
-> apply configsI plan to make heavy use of the factory pattern when selecting the correct builder components for the
ClusterBuilder
:)