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

[sos] Add 'upload' component to upload existing reports and files #3746

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jcastill
Copy link
Member

@jcastill jcastill commented Aug 9, 2024

This commit marks the beginning of the addition of a new 'upload' component for sos, which can be used to upload already created sos reports, collects, or other files like logs or vmcores to a policy defined location.

The user needs to specify a file location, and can make use of any of the options that exist nowadays for the --upload option.

This first commit includes:

  • The initial framework for the 'upload' component.
  • The new man page for 'sos upload'.
  • The code in the component 'help' to show information about the component.
  • The code in sos/init.py to deal with the component.
  • And modifications to setup.py to build the man pages.

Related: RHEL-23032, SUPDEV-138, CLIOT-481


Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines

  • Is the commit message split over multiple lines and hard-wrapped at 72 characters?
  • Is the subject and message clear and concise?
  • Does the subject start with [plugin_name] if submitting a plugin patch or a [section_name] if part of the core sosreport code?
  • Does the commit contain a Signed-off-by: First Lastname [email protected]?
  • Are any related Issues or existing PRs properly referenced via a Closes (Issue) or Resolved (PR) line?
  • Are all passwords or private data gathered by this PR obfuscated?

@jcastill
Copy link
Member Author

jcastill commented Aug 9, 2024

Huge thanks to @pmoravec for all the help reviewing this, suggesting improvements, and finding bugs.
This is a first implementation of the 'upload' component, that can help users to upload already created sos or other files that support organizations can find useful.
In the future, the idea is to hook 'report' and 'collector' components to this one if the maintainers think is a good idea.

Copy link

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

  • sudo yum install -y dnf-plugins-core on RHEL 8
  • sudo dnf install -y dnf-plugins-core on Fedora
  • dnf copr enable packit/sosreport-sos-3746
  • And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

Copy link
Member

@arif-ali arif-ali left a comment

Choose a reason for hiding this comment

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

Great idea

Some initial comments

man/en/sos-upload.1 Outdated Show resolved Hide resolved
man/en/sos-upload.1 Outdated Show resolved Hide resolved
sos/upload/__init__.py Outdated Show resolved Hide resolved
sos/upload/__init__.py Outdated Show resolved Hide resolved
@jcastill
Copy link
Member Author

jcastill commented Aug 9, 2024

@arif-ali about these ones:

sos/upload/init.py:103:0: C0325: Unnecessary parens after '=' keyword (superfluous-parens)
Fixed
sos/upload/init.py:141:0: C0325: Unnecessary parens after 'if' keyword (superfluous-parens)
Fixed
sos/upload/init.py:142:0: C0325: Unnecessary parens after 'if' keyword (superfluous-parens)
Fixed

************* Module sos.upload
sos/upload/init.py:47:8: R1725: Consider using Python 3 style super() without arguments (super-with-arguments)
Do I need to fix this one? Are we doing the same in the rest of the code, or is a nice-thing to have but not necessary?

sos/upload/init.py:42:46: W0613: Unused argument 'in_place' (unused-argument)
This is there to help with hooking report and others in the future, but can remove it now.

sos/upload/init.py:43:17: W0613: Unused argument 'hook_commons' (unused-argument)
Same as above as far as I know.

sos/upload/init.py:155:24: R1722: Consider using 'sys.exit' instead (consider-using-sys-exit)
I'll make this change now.

@jcastill jcastill force-pushed the jcastillo-add-upload-subsystem branch from d5b6c64 to 0e6bc72 Compare August 9, 2024 12:42
@arif-ali
Copy link
Member

arif-ali commented Aug 9, 2024

With R1725, I made the changes a few months back, and hence enabled the check, so let's do this here too.

With the unused variable. If your 100% sure you're going to be using them then potentially you could add the following before the line

#pylint: disable=unused-argument

@jcastill jcastill force-pushed the jcastillo-add-upload-subsystem branch from 0e6bc72 to 9c60d66 Compare August 9, 2024 13:46
@jcastill
Copy link
Member Author

jcastill commented Aug 9, 2024

With R1725, I made the changes a few months back, and hence enabled the check, so let's do this here too.

Done, should be in the version I just pushed.

With the unused variable. If your 100% sure you're going to be using them then potentially you could add the following before the line

#pylint: disable=unused-argument

Nice one! But I ended up removing it. I'll re-add them in the future when I have ready the code for hooking report etc.


.PP
.SH DESCRIPTION
upload is an sos subcommand to upload sos reports, logs, vmcores, or other files to a policy defined remote location, or an user defined one.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: s/an user/a user/.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think is 'an' because 'user' starts with a vowel, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

The rule is "first sound of word", not first letter :) E.g. https://english.stackexchange.com/questions/105116/is-it-a-user-or-an-user (though I am not sure how authoritative that source is).

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice, TIL. Fixed in the next push.

man/en/sos-upload.1 Outdated Show resolved Hide resolved
sos/help/__init__.py Outdated Show resolved Hide resolved
@@ -298,7 +298,7 @@ def get_upload_url(self):
self.ui_log.info("No case id provided, uploading to SFTP")
return RH_SFTP_HOST
rh_case_api = "/support/v1/cases/%s/attachments"
return RH_API_HOST + rh_case_api % self.case_id
return RH_API_HOST + rh_case_api % self.commons['cmdlineopts'].case_id
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change? AFAIK self.case_id might be blank (and common's case_id set) only in scenario "case id not in cmdline, batch not in cmdline" - should not upload query for case_id, then? (or am I wrong here with my assumption)?

(that concern is valid for sure (while I can be wrong on its impact to this code change):

# python3 bin/sos upload /var/tmp/sosreport-pmoravec-rhel8-012345678-2024-08-13-gbiatgg.tar.xz

sos upload (version 4.7.2)
This utility is used to upload files to a policy-default location.

The archive to be uploaded may contain data considered sensitive and its content
should be reviewed by the originating organization before being passed to any
third party.

No configuration changes will be made to the system running this utility.


Press ENTER to continue, or CTRL-C to quit


Attempting to upload file /var/tmp/sosreport-pmoravec-rhel8-012345678-2024-08-13-gbiatgg.tar.xz to case 
No case id provided, uploading to SFTP
No case id provided, uploading to SFTP
Attempting upload to Red Hat Secure FTP
..

Copy link
Member Author

Choose a reason for hiding this comment

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

This is one of the things we talked about internally when I first started playing with 'upload'. If you remember, the issue was that without this change, we were getting 'None' on the case_id and it was failing to build the url, and so failed to upload. I have the feeling that I've done something wrong on the upload side and I'm not passing the case_id correctly.
My hope is that more experienced eyes, or at least fresher, can tell me where I'm failing.

@pmoravec
Copy link
Contributor

When I run:

python3 bin/sos upload /var/tmp/sosreport-pmoravec-rhel8-01234567-2024-08-13-gbiatgg.tar.xz

and "Press ENTER to continue", and then nothing, then I get a final error:

..
Attempting to upload file /var/tmp/sosreport-pmoravec-rhel8-01234567-2024-08-13-gbiatgg.tar.xz to case 
No case id provided, uploading to SFTP
No case id provided, uploading to SFTP
Attempting upload to Red Hat Secure FTP
Please visit the following URL to authenticate this device: https://sso.redhat.com/device?user_code=SOME-CODE
User anonUser used for anonymous upload. Please inform your support engineer so they may retrieve the data.
Upload attempt failed: 'RHELPolicy' object has no attribute 'upload_directory'

I think the upload did not succeed at the end..

defined location. These files can be either sos reports,
sos collections, or other kind of files like: vmcores,
application cores, logs, etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

Extra line..?

Comment on lines 92 to 97
def _fmt_msg(self, msg):
width = 80
_fmt = ''
for line in msg.splitlines():
_fmt = _fmt + fill(line, width, replace_whitespace=False) + '\n'
return _fmt
Copy link
Contributor

Choose a reason for hiding this comment

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

We declare this method on many places / in most of components. Isn't it worth declaring in SosComponent directly?

Copy link
Member Author

Choose a reason for hiding this comment

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

This should be done in another PR, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer having extra commit within this PR, but I am ok with a new issue/PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll do it here then :)

@pmoravec
Copy link
Contributor

When pressing Ctrl+C on ress ENTER to continue, or CTRL-C to quit prompt, I get backtrace:

Press ENTER to continue, or CTRL-C to quit
^CTraceback (most recent call last):
  File "/root/sos-main/sos/upload/__init__.py", line 120, in intro
    input(prompt)
KeyboardInterrupt

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "bin/sos", line 22, in <module>
    sos.execute()
  File "/root/sos-main/sos/__init__.py", line 187, in execute
    self._component.execute()
  File "/root/sos-main/sos/upload/__init__.py", line 137, in execute
    self.intro()
  File "/root/sos-main/sos/upload/__init__.py", line 123, in intro
    self.exit("Exiting on user cancel", 130)
AttributeError: 'SoSUpload' object has no attribute 'exit'

@jcastill
Copy link
Member Author

@pmoravec thank you for finding this, I thought we solved these issues:

When I run:

python3 bin/sos upload /var/tmp/sosreport-pmoravec-rhel8-01234567-2024-08-13-gbiatgg.tar.xz

and "Press ENTER to continue", and then nothing, then I get a final error:

..
Attempting to upload file /var/tmp/sosreport-pmoravec-rhel8-01234567-2024-08-13-gbiatgg.tar.xz to case 
No case id provided, uploading to SFTP
No case id provided, uploading to SFTP

I'll check the double messaging here, looks horrible.

Attempting upload to Red Hat Secure FTP
Please visit the following URL to authenticate this device: https://sso.redhat.com/device?user_code=SOME-CODE
User anonUser used for anonymous upload. Please inform your support engineer so they may retrieve the data.
Upload attempt failed: 'RHELPolicy' object has no attribute 'upload_directory'


I'll check this one as well, I remember we had a similar issue with a previous implementation.

I think the upload did not succeed at the end..

No, it should not succeed in that case.

@jcastill
Copy link
Member Author

When pressing Ctrl+C on ress ENTER to continue, or CTRL-C to quit prompt, I get backtrace:

Press ENTER to continue, or CTRL-C to quit
^CTraceback (most recent call last):
  File "/root/sos-main/sos/upload/__init__.py", line 120, in intro
    input(prompt)
KeyboardInterrupt

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "bin/sos", line 22, in <module>
    sos.execute()
  File "/root/sos-main/sos/__init__.py", line 187, in execute
    self._component.execute()
  File "/root/sos-main/sos/upload/__init__.py", line 137, in execute
    self.intro()
  File "/root/sos-main/sos/upload/__init__.py", line 123, in intro
    self.exit("Exiting on user cancel", 130)
AttributeError: 'SoSUpload' object has no attribute 'exit'

I'll check this, should be easy to fix.

@jcastill
Copy link
Member Author

When pressing Ctrl+C on ress ENTER to continue, or CTRL-C to quit prompt, I get backtrace:

Press ENTER to continue, or CTRL-C to quit
^CTraceback (most recent call last):
  File "/root/sos-main/sos/upload/__init__.py", line 120, in intro
    input(prompt)
KeyboardInterrupt

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "bin/sos", line 22, in <module>
    sos.execute()
  File "/root/sos-main/sos/__init__.py", line 187, in execute
    self._component.execute()
  File "/root/sos-main/sos/upload/__init__.py", line 137, in execute
    self.intro()
  File "/root/sos-main/sos/upload/__init__.py", line 123, in intro
    self.exit("Exiting on user cancel", 130)
AttributeError: 'SoSUpload' object has no attribute 'exit'

Fixed. I used exit() instead of _exit(), which is the one implemented in Soscomponent.

Copy link
Member

@TurboTurtle TurboTurtle left a comment

Choose a reason for hiding this comment

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

At a bare minimum, a new component should be implementing all the abstractions that it needs to operate solo, not acting as a wrapper to existing functionality.

This means the upload logic needs to be separated from its current location in Policy, and implemented as a discrete unit. Policy should then control the default setting, and users should be able to direct sos to chose an upload target/profile/whatever we want to call it as an override. E.G. if I have an sos report locally on my Fedora workstation that was taken from a RHEL box, and I am unable due to some network policy to directly upload from the RHEL box, then on my Fedora system I should be able to send that sos report to Red Hat.

Further, any current or future usage of the component's functionality should go through the actual component code. Much like we do with sos clean, when --clean is used for a report being generated. We hook into the component from within report, to ensure we use the exact code flow for cleaning the archive as we would by running a clean after-the-fact.

This commit marks the beginning of the addition of a new 'upload'
component for sos, which can be used to upload already created
sos reports, collects, or other files like logs or vmcores to
a policy defined location.

The user needs to specify a file location, and can make use of any
of the options that exist nowadays for the --upload option.

This first commit includes:
- The initial framework for the 'upload' component.
- The new man page for 'sos upload'.
- The code in the component 'help' to show information about
the component.
- The code in sos/__init__.py to deal with the component.
- And modifications to setup.py to build the man pages.

Related: RHEL-23032, SUPDEV-138, CLIOT-481

Signed-off-by: Jose Castillo <[email protected]>
…onent

The function _fmt_msg() was in three different subsystems. This
commit removes the code from collector, cleaner, and upload
into SosComponent.

Signed-off-by: Jose Castillo <[email protected]>
@jcastill jcastill force-pushed the jcastillo-add-upload-subsystem branch from 9c60d66 to 72dd27c Compare August 13, 2024 15:16
@jcastill
Copy link
Member Author

At a bare minimum, a new component should be implementing all the abstractions that it needs to operate solo, not acting as a wrapper to existing functionality.

This means the upload logic needs to be separated from its current location in Policy, and implemented as a discrete unit. Policy should then control the default setting, and users should be able to direct sos to chose an upload target/profile/whatever we want to call it as an override. E.G. if I have an sos report locally on my Fedora workstation that was taken from a RHEL box, and I am unable due to some network policy to directly upload from the RHEL box, then on my Fedora system I should be able to send that sos report to Red Hat.

Further, any current or future usage of the component's functionality should go through the actual component code. Much like we do with sos clean, when --clean is used for a report being generated. We hook into the component from within report, to ensure we use the exact code flow for cleaning the archive as we would by running a clean after-the-fact.

I agree with everything above, but the idea behind this PR is to be a first implementation to get the upload component started, and then move things carefully from policy to upload. Could this approach be acceptable?

@pmoravec
Copy link
Contributor

I support this initial implementation of the feature to let enhance sos capabilities for a low cost. The discussion about refactorization (what precisely should be moved to some new *Upload* classes) can be lengthy, while we can already offer this feature as is.

I was thinking to raise the same concern, but I realized I would see beneficial for the discussion about refactorization if we already has some implementation in hand. With the current code, it is hard for me to specify "cut this away from here and put it (there)", if we have no "(there)". With the SosUpload class, I have better description of the "(there)". So having this initial implementation merged, it will be much more easier to have tat conversation - at least for me, this can be subjective.

If somebody sees as a potential threat "we accept this initial implementation, but will never refactor the code as needed, and we dont want that technical debt here", then I can make a commitment: once there will be an agreement about the refactorisation and if nobody(*) will have time to implement it, I will work on such PR.

(*) nobody including Jose as the primary person to implement. I assume he will be the primary person to make his own feature to make it complete. On the other side, there can be various reasons he won't be able to do the refactorisation (time, other work on sos, willingness, whatever). And then anybody else (with me as the volunteer with above commitment) can contribute that way.

@jcastill
Copy link
Member Author

If somebody sees as a potential threat "we accept this initial implementation, but will never refactor the code as needed, and we dont want that technical debt here", then I can make a commitment: once there will be an agreement about the refactorisation and if nobody(*) will have time to implement it, I will work on such PR.

(*) nobody including Jose as the primary person to implement. I assume he will be the primary person to make his own feature to make it complete. On the other side, there can be various reasons he won't be able to do the refactorisation (time, other work on sos, willingness, whatever). And then anybody else (with me as the volunteer with above commitment) can contribute that way.

On this note, I already started moving things around from policies/distros just after I sent this PR - this is not something I want to leave abandoned, or done in six months time or more, but as soon as possible. But also I want to make sure I cover all the possible cases, and the upload code in policies has been there for a long time, working perfectly, so want to be extra careful while refactoring.

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.

4 participants