-
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
RHINENG-13973 execute pcp commands #576
base: main
Are you sure you want to change the base?
Conversation
ros/processor/suggestions_engine.py
Outdated
|
||
response = requests.get(archive_URL, timeout=10) | ||
def create_output_dir(request_id, host): | ||
output_dir = f"/tmp/pmlogextract-output-{request_id}/" |
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.
Though we need the directory and files for temporary purpose however these files are getting passed to next command pmlogsummary
, so in this situation I don't think using /tmp
is wise idea. The initial impression says the /tmp
is managed by systemd-tmpfiles-clean.service
so its possible that these could be deleted before pmlogsummary
requires it.
Should we look into adding small persistent volume? maybe 250Mb is enough? and there we do these file operations?
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.
In that case, we can use the /var/tmp
directory. Implementing a persistent volume might be unnecessary for now, especially since I haven't observed insights-core utilizing that.
We can switch to using the /var/tmp
directory to store temporary files/directories that may need to survive for longer durations.
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 need to check! If I understand correctly the insights-core does not have duties about collecting data from containers? those are instances of VM?
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 /var/tmp
is better choice, I do see security concerns as well when using these directories,
https://www.redhat.com/en/blog/polyinstantiating-tmp-and-vartmp-directories
I have captured both the usage and the security concern in new Jira ticket RHINENG-15674 and we can evaluate it later. As of now lets go with /var/tmp
.
5d35381
to
55e62a0
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #576 +/- ##
==========================================
- Coverage 74.51% 73.66% -0.86%
==========================================
Files 34 34
Lines 1797 1906 +109
==========================================
+ Hits 1339 1404 +65
- Misses 458 502 +44
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
55e62a0
to
deb3367
Compare
deb3367
to
a1bf938
Compare
Why do we need this change? 💭
Run
pmlogextract
on the extracted.index
file, followed bypmlogsummary
on the resulting output file.Documentation update? 📝
Security Checklist 🔒
Upon raising this PR please go through RedHatInsights/secure-coding-checklist
💂♂️ Checklist 🎯
Additional 📣
Feel free to add any other relevant details such as links, notes, screenshots, here.