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

When insights client is killed the directories in /var/tmp are not removed rhbz#2009773 #3396

Merged
merged 2 commits into from
Jun 14, 2022

Conversation

ahitacat
Copy link
Contributor

All Pull Requests:

Check all that apply:

  • Have you followed the guidelines in our Contributing document, including the instructions about commit messages?
  • Is this PR to correct an issue?
  • Is this PR an enhancement?

Complete Description of Additions/Changes:

rhbz#200973
When insights-client is killed with a SIGKILL signal, as does the oom-killer, the directories for the archive created in /var/tmp aren't removed. That leaves a lot of directories in this path, increasing the size of the /var/tmp folder.

This fix adds the following:

  • Handle SIGTERM signal
  • Archive is created in a directory with the same prefix. Each time a new archive is created, it removes the previous one if it exists.
  • --keep-archive will be storing the archive outside /var/tmp, and is up to each user to manage those stored archives
  • Tests for this new performance

@ahitacat ahitacat added the client These issues represent work to be done by the "client" team. label Apr 22, 2022
@ahitacat ahitacat requested review from subpop, strider and Glutexo April 22, 2022 13:32
@ahitacat ahitacat marked this pull request as ready for review April 26, 2022 14:55
try:
os.makedirs(keep_archive_dir)
except OSError as e:
logger.error(e)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We often get bug reports complaining about stack traces in console output. Maybe we can change this line to logger.debug? Does printing the exception here print the entire stack trace or just the exception's class & message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It prints the stack trace but it was a leftover of debugging it, also I have noticed the test wasn't good for this method

@ahitacat ahitacat requested a review from subpop April 27, 2022 16:56
@xiangce
Copy link
Contributor

xiangce commented May 18, 2022

@subpop, @ahitacat - May I know the latest status of this PR? Can we merge and release it this week? Thanks.

@ahitacat
Copy link
Contributor Author

@xiangce This PR still needs review.

@ahitacat
Copy link
Contributor Author

@xiangce this PR is reviewed and can be merged

@xiangce xiangce merged commit d11d4e7 into RedHatInsights:master Jun 14, 2022
xiangce pushed a commit that referenced this pull request Jun 14, 2022
…moved rhbz#2009773 (#3396)

* New prefix for temp archive directory and tests

Signed-off-by: ahitacat <[email protected]>

* keep-archive is stored in /var/cache

Signed-off-by: ahitacat <[email protected]>
(cherry picked from commit d11d4e7)
xiangce pushed a commit that referenced this pull request Sep 6, 2024
…moved rhbz#2009773 (#3396)

* New prefix for temp archive directory and tests

Signed-off-by: ahitacat <[email protected]>

* keep-archive is stored in /var/cache

Signed-off-by: ahitacat <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client These issues represent work to be done by the "client" team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants