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

[DPE-5301] Add check for low storage space on pgdata volume #685

Merged
merged 17 commits into from
Sep 11, 2024

Conversation

lucasgameiroborges
Copy link
Member

@lucasgameiroborges lucasgameiroborges commented Sep 4, 2024

Issue

The charm gives no indication of low storage space on pgdata folder, which can cause unexpected failures

Solution

Added check on update_status hook + integration test.

Copy link

codecov bot commented Sep 4, 2024

Codecov Report

Attention: Patch coverage is 66.66667% with 6 lines in your changes missing coverage. Please review.

Project coverage is 70.65%. Comparing base (5db6b05) to head (0699c45).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
src/charm.py 66.66% 5 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #685      +/-   ##
==========================================
- Coverage   70.72%   70.65%   -0.07%     
==========================================
  Files          11       11              
  Lines        2951     2968      +17     
  Branches      514      517       +3     
==========================================
+ Hits         2087     2097      +10     
- Misses        754      760       +6     
- Partials      110      111       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@lucasgameiroborges lucasgameiroborges changed the title [WIP][DPE-5301] Add check for low storage space on pgdata volume [DPE-5301] Add check for low storage space on pgdata volume Sep 6, 2024
@lucasgameiroborges lucasgameiroborges marked this pull request as ready for review September 6, 2024 00:59
src/charm.py Outdated
Comment on lines 1348 to 1349
elif self.unit.status.message == INSUFFICIENT_SIZE_WARNING:
self._set_active_status()
Copy link
Contributor

Choose a reason for hiding this comment

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

We should most probably don't change the status in _set_active_status() if the message is INSUFFICIENT_SIZE_WARNING and then reset the status here before running _set_active_status().

This is because if another hook runs _set_active_status() the status will be overwritten unite the next update status. @marceloneppel what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I understand your point. Could you elaborate? My motivation for this check is to resolve the blocked status in case the storage space got released (to >10%)

Copy link
Member Author

Choose a reason for hiding this comment

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

the check for INSUFFICIENT_SIZE_WARNING is just to make sure that, if the charm was blocked due to insufficient size but the user has released enough space, then resolve. Maybe there's a better way?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's a good idea, @dragomirp.

@lucasgameiroborges, the idea is to just set something else in the status at this point, like the Active Status, and put a condition in the _set_active_status method not to change the status to Active if the current status is INSUFFICIENT_SIZE_WARNING. This way, we avoid some other places overring this status incorrectly (maybe here).

I see that this will need to be handled correctly later by improving how we manage the unit's statuses across the entire charm code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I get the reasoning for including a check inside _set_active_status so the blocked status doesn't get overwritten in other places, as for just set something else in the status at this point, like the Active Status do you mean using self.unit.status = ActiveStatus() directly? why?

Copy link
Contributor

Choose a reason for hiding this comment

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

The elif should be something like:

        elif self.unit.status.message == INSUFFICIENT_SIZE_WARNING:
            self.unit.status = ActiveStatus()
            self._set_active_status()

if _set_active_status() preserves INSUFFICIENT_SIZE_WARNING it should be unset before calling it. Otherwise it won't unset.

Copy link
Contributor

Choose a reason for hiding this comment

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

Q: are we moving the right way here?
It smells like Status/Message manipulation is going outside of our control.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh! Now I see the problem! Yes of course, thanks @dragomirp and @marceloneppel !

@taurus-forever : I agree that status management is a bit all over the place. AFAIK this is a problem in other charms too. Interesting discussion here: https://chat.canonical.com/canonical/pl/ncbsrryk378jux5r8cxjjngzna

@dragomirp dragomirp self-requested a review September 9, 2024 12:14
@lucasgameiroborges lucasgameiroborges merged commit 8bd0f2e into main Sep 11, 2024
96 checks passed
@lucasgameiroborges lucasgameiroborges deleted the lucas/warn-low-disk branch September 11, 2024 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants