-
Notifications
You must be signed in to change notification settings - Fork 540
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
[collect] Handle custom node and inherited configs with collections #3852
base: main
Are you sure you want to change the base?
Changes from 3 commits
783ca2b
d6bc604
4b56b74
b2d5a71
2922e7b
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 |
---|---|---|
|
@@ -51,6 +51,8 @@ def __init__(self, address, commons, password=None, local_sudo=None, | |
self.hostlen = commons['hostlen'] | ||
self.need_sudo = commons['need_sudo'] | ||
self.sos_options = commons['sos_options'] | ||
self.node_config_file = self.opts.node_config_file | ||
self.inherit_config_file = self.opts.inherit_config_file | ||
self.local = False | ||
self.host = None | ||
self.cluster = None | ||
|
@@ -762,12 +764,35 @@ def execute_sos_command(self): | |
try: | ||
path = False | ||
checksum = False | ||
config_file_arg = '' | ||
if self.opts.node_config_file: | ||
config_file_arg = f'--config-file={self.opts.node_config_file}' | ||
elif self.opts.inherit_config_file: | ||
if not self.local: | ||
remote_config = os.path.join(self.tmpdir, | ||
'remote_sos.conf') | ||
self.run_command(f"mkdir -pv {self.tmpdir}", | ||
timeout=10) | ||
self._transport.copy_file_to_remote( | ||
self.opts.inherit_config_file, | ||
remote_config) | ||
TrevorBenson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
config_file_arg = f'--config-file={remote_config}' | ||
else: | ||
config_file_arg = ( | ||
f'--config-file={self.opts.inherit_config_file}') | ||
self.sos_cmd = ( | ||
f"{self.sos_cmd} {config_file_arg}" | ||
if config_file_arg else self.sos_cmd) | ||
Comment on lines
+780
to
+782
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. Isn't it easier just to have
? But I am ok with the current code. |
||
res = self.run_command(self.sos_cmd, | ||
timeout=self.opts.timeout, | ||
use_shell=True, | ||
need_root=True, | ||
use_container=True, | ||
env=self.sos_env_vars) | ||
if self.opts.inherit_config_file and not self.local: | ||
self.run_command(f"rm {remote_config} ; rmdir {self.tmpdir}", | ||
use_shell=True, | ||
need_root=True) | ||
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. Yeah, same here. Very much not a fan of doing 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. I'll presume you mean leaving the inherited conf in the tmp directory and letting It works for my use case, as long as we are comfortable that the file remains might (although probably uncommon) store |
||
if res['status'] == 0: | ||
for line in res['output'].splitlines(): | ||
if fnmatch.fnmatch(line, '*sosreport-*tar*'): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -352,6 +352,37 @@ def _get_hostname(self): | |
self.log_info(f"Hostname set to {self._hostname}") | ||
return self._hostname | ||
|
||
def copy_file_to_remote(self, fname, dest): | ||
"""Copy a local file, fname, to dest on the remote node | ||
|
||
:param fname: The name of the file to copy | ||
:type fname: ``str`` | ||
|
||
:param dest: Where to save the file to remotely | ||
:type dest: ``str`` | ||
|
||
:returns: True if file was successfully copied to remote, or False | ||
:rtype: ``bool`` | ||
""" | ||
attempts = 0 | ||
try: | ||
while attempts < 5: | ||
attempts += 1 | ||
ret = self._copy_file_to_remote(fname, dest) | ||
if ret: | ||
return True | ||
self.log_info(f"File copy attempt {attempts} failed") | ||
self.log_info("File copy failed after 5 attempts") | ||
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.
|
||
return False | ||
except Exception as err: | ||
self.log_error("Exception encountered during config copy attempt " | ||
f"{attempts} for {fname}: {err}") | ||
raise err | ||
|
||
def _copy_file_to_remote(self, fname, dest): | ||
TrevorBenson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
raise NotImplementedError( | ||
f"Transport {self.name} does not support file copying") | ||
|
||
def retrieve_file(self, fname, dest): | ||
"""Copy a remote file, fname, to dest on the local node | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -41,6 +41,9 @@ def _retrieve_file(self, fname, dest): | |
def _format_cmd_for_exec(self, cmd): | ||
return cmd | ||
|
||
def _copy_file_to_remote(self, fname, dest): | ||
pass | ||
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. Hmm. I was initially going to say this should return This might need some attention later on depending on how much use this functionality sees, but for now I think it's fine. |
||
|
||
def _read_file(self, fname): | ||
if os.path.exists(fname): | ||
with open(fname, 'r', encoding='utf-8') as rfile: | ||
|
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 initial thought was that this would be a toggle, whereby if
True
we implicitly take the config-file used locally, sosos.conf
by default or whatever gets set.Is there a use case you have where a user would have config_file_A locally that is used for the
collect
and localreport
collections, but would have config_file_B also local to the system that is running collect but should be used only for remote node collections?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 suspect any difference is mostly my misinterpretation of the discussion
Not intentional change to your thoughts on implementation.
Possibly, but not one of my requirements. I plan to use the same config for the collector and the local/remote nodes.
I'll attempt to layout the resulting behavior from this change.
Overview
Three arguments
Logic
--inherit-config-file
isFalse
--config-file
&--node-config-file
unset--config-file
set to/etc/sos/custom1.conf
custom1.conf
sos.conf
--node-config
set to/etc/sos/custom2.conf
/etc/sos/sos.conf
/etc/sos/custom2.conf
for the local and remote nodes .--inherit-config-file
isTrue
--config-file
&--node-config-file
unset/etc/sos/sos.conf
/etc/sos/sos.conf
is copied to all nodes in a temp path./tmp/sos.XXXXX.conf
(local could use default path)--config-file
set to/etc/sos/custom3.conf
/etc/sos/custom3.conf
/etc/sos/custom3.conf
is copied to all nodes in a temp path./tmp/sos.XXXXX.conf
(local ...)--node-config-file
&--inherit-config-file
are an incompatible argument pair. Potentially also--config-file
&--node-config-file
.Does this correctly describe what you were thinking?
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've got this logic and additional comments worked out. I will add the new commit for review, but can be dropped/reverted if this wasn't what you were thinking of.