Skip to content

[aap_containerized] collect podman inspect logs - #4064

Merged
TurboTurtle merged 1 commit into
sosreport:mainfrom
snagoor:podman_inspect
Jun 24, 2025
Merged

[aap_containerized] collect podman inspect logs#4064
TurboTurtle merged 1 commit into
sosreport:mainfrom
snagoor:podman_inspect

Conversation

@snagoor

@snagoor snagoor commented Jun 24, 2025

Copy link
Copy Markdown
Contributor

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@example.com?
  • 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?

@packit-as-a-service

Copy link
Copy Markdown

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-4064
  • And now you can install the packages.

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

@arif-ali arif-ali left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You are removing podman logs and adding podman inspect, should we not collect both?

I don't know enough about podman, this is where I need to be educated :)

Comment thread sos/report/plugins/aap_containerized.py Outdated

# Copy podman container log files in plugin sub directory
# under aap_containers_log
# Copy podman container log and inspect files into their respective subdirectories

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please ensure that the characters on this line are no more than 79 characters

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ack

suggest_filename=f"{log_file}",
subdir="aap_containers_log"
suggest_filename=f"{container}.log",
subdir="aap_container_logs"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

any reason why you're changing this? I'm no RH expert, but if the RH folks have any automation against the old filename or people are looking for the old name, then this shouldn't change

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@arif-ali this is just a place-holder directory. It never mattered and i incorrectly named initially as aap_containers_log instead of aap_container_logs which sounds more logical.

@snagoor

snagoor commented Jun 24, 2025

Copy link
Copy Markdown
Contributor Author

You are removing podman logs and adding podman inspect, should we not collect both?

I don't know enough about podman, this is where I need to be educated :)

If you observe the change we do collect both podman logs and podman inspect within the same for loop.

Signed-off-by: Nagoor Shaik <nshaik@redhat.com>
@arif-ali

Copy link
Copy Markdown
Member

You are removing podman logs and adding podman inspect, should we not collect both?
I don't know enough about podman, this is where I need to be educated :)

If you observe the change we do collect both podman logs and podman inspect within the same for loop.

you're right, I totally saw through the change, my bad

@arif-ali arif-ali added Reviewed/Needs 2nd Ack Require a 2nd ack from a maintainer Status/Needs Review This issue still needs a review from project members labels Jun 24, 2025

@arif-ali arif-ali left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@arif-ali

Copy link
Copy Markdown
Member

btw, have you tried the runas parameter, that could help without having to use su - {username} on all these commands

@snagoor

snagoor commented Jun 24, 2025

Copy link
Copy Markdown
Contributor Author

btw, have you tried the runas parameter, that could help without having to use su - {username} on all these commands

I tried it initially, it never worked for me. Same was discussed here #4030 (comment) @TurboTurtle said he will look into it later.

@TurboTurtle

Copy link
Copy Markdown
Member

I tried it initially, it never worked for me. Same was discussed here #4030 (comment) @TurboTurtle said he will look into it later.

The issue there was using runas in conjunction with container - we don't currently introspect non-root container states, which is what throws that off. I think this should work with just runas, but I won't block on that here since we'd ultimately want both of those working together, and doing a partial "fix" now would still need to be replaced.

I'm hoping to have some time in the next month or so to get non-root container introspection hooked up.

@TurboTurtle TurboTurtle added Reviewed/Ready for Merge Has been reviewed, ready for merge Kind/Collection New or updated command or file collection and removed Reviewed/Needs 2nd Ack Require a 2nd ack from a maintainer Status/Needs Review This issue still needs a review from project members labels Jun 24, 2025
@TurboTurtle
TurboTurtle merged commit f612b3b into sosreport:main Jun 24, 2025
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Kind/Collection New or updated command or file collection Reviewed/Ready for Merge Has been reviewed, ready for merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants