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.

Sign up for GitHub

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

Jump to bottom

Add "early-abort" in asset/results cleanup jobs based on df-output #3750

Merged
merged 3 commits into from
Feb 25, 2021
Merged

Add "early-abort" in asset/results cleanup jobs based on df-output #3750

merged 3 commits into from
Feb 25, 2021

Conversation

Martchus
Copy link
Contributor

okurz
okurz requested changes Feb 23, 2021
View reviewed changes
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

I see that you add asset/results cleanup in the same step. Can we try to focus on one at a time, e.g. results only first?

etc/openqa/openqa.ini Outdated Show resolved Hide resolved
@Martchus
Copy link
Contributor Author

I see that you add asset/results cleanup in the same step. Can we try to focus on one at a time, e.g. results only first?

If it really has to be I can split the commit retrospectively (which likely means rebasing and solving conflicts of the 2nd commit later). However, I don't think it is necessary because the behavior can still be configured individually.

@okurz
Copy link
Member

okurz commented Feb 23, 2021

I see that you add asset/results cleanup in the same step. Can we try to focus on one at a time, e.g. results only first?

If it really has to be I can split the commit retrospectively (which likely means rebasing and solving conflicts of the 2nd commit later). However, I don't think it is necessary because the behavior can still be configured individually.

Sure. Whatever works best for you. Just wanted to suggest a way to get independent parts completed quicker, including deployment, risk in production, potential reverts, etc.

@Martchus Martchus force-pushed the cleanup-early-abort branch 2 times, most recently from 2917ff2 to 35edeb5 Compare February 24, 2021 11:55
@codecov
Copy link

codecov bot commented Feb 24, 2021 •
edited
Loading

Codecov Report

Merging #3750 (aefbbd2) into master (e1adf71) will increase coverage by 0.04%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3750      +/-   ##
==========================================
+ Coverage   96.42%   96.47%   +0.04%     
==========================================
  Files         367      368       +1     
  Lines       32321    32374      +53     
==========================================
+ Hits        31167    31233      +66     
+ Misses       1154     1141      -13
Impacted Files Coverage Δ
lib/OpenQA/Setup.pm 97.67% <ø> (ø)
t/config.t 100.00% <ø> (ø)
lib/OpenQA/Task/Asset/Limit.pm 100.00% <100.00%> (ø)
lib/OpenQA/Task/Job/Limit.pm 99.13% <100.00%> (-0.01%) ⬇️
lib/OpenQA/Task/Utils.pm 100.00% <100.00%> (ø)
t/37-limit_assets.t 100.00% <100.00%> (ø)
t/42-df-based-cleanup.t 99.30% <100.00%> (+0.13%) ⬆️
t/lib/OpenQA/Test/Utils.pm 81.50% <0.00%> (+3.75%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e1adf71...aefbbd2. Read the comment docs.

@Martchus Martchus changed the title WIP: Add "early-abort" in asset/results cleanup jobs based on df-output Add "early-abort" in asset/results cleanup jobs based on df-output Feb 24, 2021
@Martchus Martchus marked this pull request as ready for review February 24, 2021 14:56
Amrysliu
Amrysliu approved these changes Feb 25, 2021
View reviewed changes
Copy link
Contributor

@Amrysliu Amrysliu left a comment

Choose a reason for hiding this comment

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

LGTM

okurz
okurz approved these changes Feb 25, 2021
View reviewed changes
Copy link
Member

@okurz okurz left a comment

Choose a reason for hiding this comment

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

Awesome test, easy to read :)

@mergify mergify bot merged commit 751e321 into os-autoinst:master Feb 25, 2021
@Martchus Martchus deleted the cleanup-early-abort branch February 25, 2021 09:38
openqa-git-sync pushed a commit to os-autoinst/salt-states-openqa that referenced this pull request Apr 19, 2021
@Martchus
Skip asset/result cleanup if more than 20 % free disk space is available
1fb535c 
* That would give us the space between 80 % and 90 % usage as "buffer"
  between cleanup and the disk space alert
* Not effective until os-autoinst/openQA#3750 is
  deployed
* See https://progress.opensuse.org/issues/88121
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@okurz okurz okurz approved these changes

@Amrysliu Amrysliu Amrysliu approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Martchus @okurz @Amrysliu