-
Notifications
You must be signed in to change notification settings - Fork 23
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
[WIP]Common: Added generic utils to common #79
Conversation
@r0h4n @nthomas-redhat @shtripat @brainfunked Its initial WIP patch for adding general utilities. @shtripat Can you please help me by providing an issue for this, when you create one for the re factoring specification. |
250f00a
to
c86f74e
Compare
@r0h4n @nthomas-redhat @shtripat Please review. |
@shtripat If you find it easy to use these utils as part of your re-factoring, please go ahead. If you find it would make your re-factoring patch huge , I will send a separate PR afterwards to do that. In any case, please provide your thoughts |
@nnDarshan I feel doing this as part of different PR is good as refactoring PR is already huge and I am targetting more of cleanup, movement to common module and proper class hierarchies etc as part of my PR. @r0h4n @brainfunked @nthomas-redhat thoughts?? @nnDarshan also add spec and issues link in commit message |
tendrl-spec: Tendrl/specifications#31 |
def restart(self): | ||
attr = self.attributes | ||
attr.update({"state": "restarted"}) | ||
return self.__run_module(attr) |
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.
Is it possible to add a status() function as well. Few modules need that I feel.
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 ansible module which we are using to manage the service has only 4 types of operations namely start, stop, restart and reload(http://docs.ansible.com/ansible/service_module.html refer state under options). So its not possible with this approach.
I feel when we monitor systemd dbus signals we will be able to obtain the service states
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.
ok
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.
start and stop are idempotent actions that will not run commands unless necessary, so you can rely on these itself and you get the status in return. Can we take that approach?
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.
But using this approach we can't just read the status without changing it right ? If we already know that we want the state to be either started or stopped we can directly use the start/stopped method. I am not getting the advantage of using a status that way. Could you please elaborate ?
@@ -7,13 +7,15 @@ Source0: %{name}-%{version}.tar.gz | |||
License: LGPLv2+ | |||
URL: https://github.com/Tendrl/common | |||
|
|||
BuildRequires: ansible |
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 remember on rhel by default ansible 2.2 was not getting downloaded. We should explicitly mention > 2.2 here if you use any ansible 2.2 specific feature.
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.
Done
import AnsibleRunner | ||
|
||
|
||
class Test_ansible_runner_constructor(object): |
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.
Better use CamelCase for all the class names
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.
Done
@@ -0,0 +1,38 @@ | |||
from ansible_module_runner import AnsibleExecutableGenerationFailed |
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 feel source could be named as package_installer.py
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.
Done
def restart(self): | ||
attr = self.attributes | ||
attr.update({"state": "restarted"}) | ||
return self.__run_module(attr) |
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.
ok
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.
LGTM
|
||
class Command(object): | ||
def __init__(self, command): | ||
self.attributes = {"_raw_params": command} |
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 needs a whitelist of good commands we use or some sorta security check
This patch adds generic utils like command-executor, package-installer, service-manager to common as utility. tendrl-spec: Tendrl/specifications#31 tendrl-bug-id: Tendrl#80 Signed-off-by: nnDarshan <darshan.n.2024@gmail.com>
This commit adds the unit tests for the general purpose utils namely installer, command executor and service manager. tendrl-spec: Tendrl/specifications#31 tendrl-bug-id: Tendrl#80 Signed-off-by: nnDarshan <darshan.n.2024@gmail.com>
Addressed some review comments tendrl-spec:refactor_common_code_and_move tendrl-bug-id: Tendrl#80 Signed-off-by: nnDarshan <darshan.n.2024@gmail.com>
Added a list of safe commands, only these safe commands can be executed by the util. tendrl-spec:add_utils_to_common tendrl-bug-id: Tendrl#80 Signed-off-by: nnDarshan <darshan.n.2024@gmail.com>
d04dcb7
to
01c156f
Compare
@r0h4n @nthomas-redhat @brainfunked Please Review. Have addressed the commets |
@@ -5,9 +5,25 @@ | |||
ANSIBLE_MODULE_PATH = "core/commands/command.py" | |||
LOG = logging.getLogger(__name__) | |||
|
|||
SAFE_COMMAND_LIST = [ |
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 we externalize these command in tendrl owned yaml file? Just a thought, but if we are very strict on list of commands supported, then it ok to keep inside code. My only point is just for a new command we would need a code change later.
Comment??
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.
My opinion is having it in code is better, keeping the security in mind.
@nthomas-redhat @r0h4n @brainfunked Please provide your thoughts
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.
ok
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.
Looks good to me
from ansible_module_runner import AnsibleRunner | ||
import logging | ||
|
||
ANSIBLE_MODULE_PATH = "core/system/systemd.py" |
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 supports only systemd services only right?
If we want to support old versions of LINUX where systemd is not available(ex;RHEL6), this would not be enough. I think the service module(https://github.com/ansible/ansible-modules-core/blob/devel/system/service.py) has support for different init modules including systemd and BSD init. Have considered that?
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.
Done
def restart(self): | ||
attr = self.attributes | ||
attr.update({"state": "restarted"}) | ||
return self.__run_module(attr) |
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.
start and stop are idempotent actions that will not run commands unless necessary, so you can rely on these itself and you get the status in return. Can we take that approach?
Using service module for service management as it is more generic. Also provided configurable util execution path. tendrl-spec:add_utils_to_common tendrl-bug-id: Tendrl#80 Signed-off-by: nnDarshan <darshan.n.2024@gmail.com>
This patch adds generic utils like command-executor,
package-installer, service-manager to common as utility.
Signed-off-by: nnDarshan darshan.n.2024@gmail.com