-
Notifications
You must be signed in to change notification settings - Fork 595
[aap_containerized] Add heuristic for detecting username #4150
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
[aap_containerized] Add heuristic for detecting username #4150
Conversation
|
Kindly asking @snagoor for a review. Esp. I am not fully happy with the heuristic what user runs a podman container (what if all pods are down? what if multiple users run some container?). But I am not aware of anything better. |
Forgetting to provide mandatory username of the AAP containerized deployment leads to skipping plugin completely and lack of collected data. Let add a heuristic "is there a sole user running some podman container?" to detect the user in such a case. Closes: sosreport#4150 Signed-off-by: Pavel Moravec <pmoravec@redhat.com>
6cbee72 to
8253ed1
Compare
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
TurboTurtle
left a 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.
ack to code, minor nit/suggestion below.
Forgetting to provide mandatory username of the AAP containerized deployment leads to skipping plugin completely and lack of collected data. Let add a heuristic "is there a sole user running some podman container?" to detect the user in such a case. Closes: sosreport#4150 Signed-off-by: Pavel Moravec <pmoravec@redhat.com>
8253ed1 to
d8b9885
Compare
@pmoravec thanks for tagging me. AAP Containerized deployment uses a non-root user (as it runs rootless podman containers). We can't reliably predict the username as its set by the end-user as per their choice. So, its mandatory for the end-user to provide the username or else we can skip collecting the data from this plugin (this was my intention when I decided to write it initially). I will try to answer your queries here
I will go through this PR and share my feedback. |
|
Thanks @pmoravec for the update. I appreciate the effort to improve usability, but I have a few concerns:
So, my final word would be stick to the old design and log a warning to pass the username as its mandatory for this plugin. |
|
Thanks for the feedback. My aim was to improve the current behaviour - detect by some heuristic (with limitations we both are aware of, but those are rather corner cases) the right user only in the case the user is not properly set. Running The use case "somebody forgets to set the username" happens. It happened to me (who forgot to ask for the plugin option), it happened to a few colleagues, it can happen to a customer who simply forgets to set it despite said. Then we get a sosreport with largely incomplete information and extra time of one more roundtrip to the customer is needed. My aim is to limit those delays, nothing else. I know the heuristic has its limitations as you describe. Still I see it beneficial in case the AAP username is not set (not such rare case..? I can check to have some stats here), since - rule of thumb percentages follow:
Comparing the pros and cons (on guesstimated percentages), I think the overall is a gain. Further, we can improve the heuristic in either way:
we should be able to detect them with pretty high probability
Does either idea sound useful? |
|
@pmoravec I'm okay with the suggested change. Logging the guessed username with a disclaimer will help avoid confusion. Let's proceed with this approach. |
|
@TurboTurtle is this good to merge or wanna you (re)review? |
Forgetting to provide mandatory username of the AAP containerized deployment leads to skipping plugin completely and lack of collected data. Let add a heuristic "is there a sole user running some podman container?" to detect the user in such a case. Closes: sosreport#4150 Signed-off-by: Pavel Moravec <pmoravec@redhat.com>
Forgetting to provide mandatory username of the AAP containerized deployment leads to skipping plugin completely and lack of collected data.
Let add a heuristic "is there a sole user running some podman container?" to detect the user in such a case.
Closes: #4150
Please place an 'X' inside each '[]' to confirm you adhere to our Contributor Guidelines