[bug#39734] scripts: Emit GC hint if free space is lower than absolute and relative threshold.
diff mbox series

Message ID 20200222104653.1324-1-mail@ambrevar.xyz
State Accepted
Headers show
Series
  • [bug#39734] scripts: Emit GC hint if free space is lower than absolute and relative threshold.
Related show

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch success View Laminar job

Commit Message

Pierre Neidhardt Feb. 22, 2020, 10:46 a.m. UTC
* guix/scripts.scm (%disk-space-warning-absolute): New variable.
(warn-about-disk-space): Test against %disk-space-warning-absolute.
Fix error in display-hint due to extraneous 'profile' argument.
---
 guix/scripts.scm | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

Comments

Ludovic Courtès Feb. 22, 2020, 11:12 a.m. UTC | #1
Hi!

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

> * guix/scripts.scm (%disk-space-warning-absolute): New variable.
> (warn-about-disk-space): Test against %disk-space-warning-absolute.
> Fix error in display-hint due to extraneous 'profile' argument.

[...]

> +(define %disk-space-warning-absolute
> +  ;; The decimal number of GiB of free disk space below which a warning is
> +  ;; emitted.
> +  (make-parameter (match (and=> (getenv "GUIX_DISK_SPACE_WARNING_ABSOLUTE")
> +                                string->number)
> +                    (#f        17.0)
> +                    (threshold threshold))))

Perhaps we should arrange for ‘GUIX_DISK_SPACE_WARNING’ to handle both
cases?

That is, we’d first try to convert it with ‘size->number’; if that
works, it’s an absolute measure, and if it returns #f, then pass the
string to ‘string->number’ and assume it’s a fraction.

Does that make sense?

> +         (absolute-threshold-in-bytes (* 1024 1024 1024 absolute-threshold)))

Always use bytes internally; that is, convert to bytes at the UI border.

Thanks,
Ludo’.
Pierre Neidhardt Feb. 22, 2020, 11:39 a.m. UTC | #2
Ludovic Courtès <ludo@gnu.org> writes:

> Hi!
>
> Pierre Neidhardt <mail@ambrevar.xyz> skribis:
>
>> * guix/scripts.scm (%disk-space-warning-absolute): New variable.
>> (warn-about-disk-space): Test against %disk-space-warning-absolute.
>> Fix error in display-hint due to extraneous 'profile' argument.
>
> [...]
>
>> +(define %disk-space-warning-absolute
>> +  ;; The decimal number of GiB of free disk space below which a warning is
>> +  ;; emitted.
>> +  (make-parameter (match (and=> (getenv "GUIX_DISK_SPACE_WARNING_ABSOLUTE")
>> +                                string->number)
>> +                    (#f        17.0)
>> +                    (threshold threshold))))
>
> Perhaps we should arrange for ‘GUIX_DISK_SPACE_WARNING’ to handle both
> cases?
>
> That is, we’d first try to convert it with ‘size->number’; if that
> works, it’s an absolute measure, and if it returns #f, then pass the
> string to ‘string->number’ and assume it’s a fraction.

I thought of something similar too, but this needs discussion:

- If we change the meaning of a value like "1", we are breaking backward compatibility.

- GUIX_DISK_SPACE_WARNING is currently undocumented, so I guess it's OK
  to break backward compatibility.  However, it'd be nice to document it :)

- Currently (size->number "1.MiB") "leaves" the Guile instance on error.  Which
  I find quite weird, I'd expect it to return #f when it cannot parse
  the input.

- Currently (size->number "0.8") returns 1.  If we want to use your
  suggestion, we would need to change the behaviour so that it returns
  #f.

- Alternatively, since we are breaking backward compatibility anyways,
  we could parse a trailing percent sign "%" to decide whether the value
  is relative or absolute.

Anyways, the issue was originally about dealing with both small and big
partitions, and for this we need both a default absolute threshold and a
default relative threshold.  Does that make sense?

>> +         (absolute-threshold-in-bytes (* 1024 1024 1024 absolute-threshold)))
>
> Always use bytes internally; that is, convert to bytes at the UI border.

What do you mean?
Ludovic Courtès Feb. 23, 2020, 11:46 a.m. UTC | #3
Hello!

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> Hi!
>>
>> Pierre Neidhardt <mail@ambrevar.xyz> skribis:
>>
>>> * guix/scripts.scm (%disk-space-warning-absolute): New variable.
>>> (warn-about-disk-space): Test against %disk-space-warning-absolute.
>>> Fix error in display-hint due to extraneous 'profile' argument.
>>
>> [...]
>>
>>> +(define %disk-space-warning-absolute
>>> +  ;; The decimal number of GiB of free disk space below which a warning is
>>> +  ;; emitted.
>>> +  (make-parameter (match (and=> (getenv "GUIX_DISK_SPACE_WARNING_ABSOLUTE")
>>> +                                string->number)
>>> +                    (#f        17.0)
>>> +                    (threshold threshold))))
>>
>> Perhaps we should arrange for ‘GUIX_DISK_SPACE_WARNING’ to handle both
>> cases?
>>
>> That is, we’d first try to convert it with ‘size->number’; if that
>> works, it’s an absolute measure, and if it returns #f, then pass the
>> string to ‘string->number’ and assume it’s a fraction.
>
> I thought of something similar too, but this needs discussion:
>
> - If we change the meaning of a value like "1", we are breaking backward compatibility.

We can assume values <= 100 are a percentage.

> - GUIX_DISK_SPACE_WARNING is currently undocumented, so I guess it's OK
>   to break backward compatibility.  However, it'd be nice to document it :)

Yup.

> - Currently (size->number "1.MiB") "leaves" the Guile instance on error.  Which
>   I find quite weird, I'd expect it to return #f when it cannot parse
>   the input.

Fixed.

> - Currently (size->number "0.8") returns 1.  If we want to use your
>   suggestion, we would need to change the behaviour so that it returns
>   #f.

Hmm.

> - Alternatively, since we are breaking backward compatibility anyways,
>   we could parse a trailing percent sign "%" to decide whether the value
>   is relative or absolute.

Right.

> Anyways, the issue was originally about dealing with both small and big
> partitions, and for this we need both a default absolute threshold and a
> default relative threshold.  Does that make sense?

It does!

So how about this:

  We check for a trailing “%”, and if there’s one, assume it’s a percentage.

  Else, call ‘size->number’.  If we get an integer < 100, assume it’s a
  percentage, otherwise assume it’s an absolute size in bytes.

How does that sound?

Thanks!

Ludo’.
Pierre Neidhardt Feb. 23, 2020, 11:49 a.m. UTC | #4
Ludovic Courtès <ludo@gnu.org> writes:

> So how about this:
>
>   We check for a trailing “%”, and if there’s one, assume it’s a percentage.
>
>   Else, call ‘size->number’.  If we get an integer < 100, assume it’s a
>   percentage, otherwise assume it’s an absolute size in bytes.
>
> How does that sound?

I think there is a misunderstanding.  If we have only _one_ threshold,
there is no way to set sane defaults for both big and small partitions.

Or were you thinking of something else?
Ludovic Courtès Feb. 23, 2020, 2:34 p.m. UTC | #5
Pierre Neidhardt <mail@ambrevar.xyz> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> So how about this:
>>
>>   We check for a trailing “%”, and if there’s one, assume it’s a percentage.
>>
>>   Else, call ‘size->number’.  If we get an integer < 100, assume it’s a
>>   percentage, otherwise assume it’s an absolute size in bytes.
>>
>> How does that sound?
>
> I think there is a misunderstanding.  If we have only _one_ threshold,
> there is no way to set sane defaults for both big and small partitions.

Oh, I must have misunderstood.

You mean you’d want to be able to set both an absolute threshold and a
relative threshold?  (Why “big and small” actually?)

Why not, but it seems to me we might be going overboard for something
that’s just meant as a hint to help people learn about ‘guix gc’.  :-)

Ludo’.
Pierre Neidhardt Feb. 23, 2020, 2:41 p.m. UTC | #6
See the original discussion:

https://lists.gnu.org/archive/html/guix-devel/2020-02/msg00303.html

Our users fall into 2 categories:

- Those with a "small" store partition, say 25 GiB.
- Those with a "big" store partition, say 1 TiB.

If we use a relative threshold, users with a big partition will get
a hint all the time.  This may mislead newcomers into thinking that they
should run "guix gc" while they still have 50 GiB of free space.

If we use an absolute threshold, users with a small partition will get a
hint all the time.

In both cases, the hints becomes irrelevant if it's triggered for the
wrong reason.

The fix is to use both thresholds and to check against the lowest one.

Does that make more sense?

Cheers!
Ludovic Courtès Feb. 23, 2020, 4:09 p.m. UTC | #7
Hi!

Pierre Neidhardt <mail@ambrevar.xyz> skribis:

> See the original discussion:
>
> https://lists.gnu.org/archive/html/guix-devel/2020-02/msg00303.html

Oops, I’m lagging behind and had not seen it yet, sorry about that!

> Our users fall into 2 categories:
>
> - Those with a "small" store partition, say 25 GiB.
> - Those with a "big" store partition, say 1 TiB.
>
> If we use a relative threshold, users with a big partition will get
> a hint all the time.  This may mislead newcomers into thinking that they
> should run "guix gc" while they still have 50 GiB of free space.
>
> If we use an absolute threshold, users with a small partition will get a
> hint all the time.
>
> In both cases, the hints becomes irrelevant if it's triggered for the
> wrong reason.
>
> The fix is to use both thresholds and to check against the lowest one.
>
> Does that make more sense?

It does, yes.

So back to the original patch… I think we should use ‘size->number’ for
the absolute threshold.

I’m also still mildly reluctant to the addition of
‘GUIX_DISK_SPACE_WARNING_ABSOLUTE’ (one more variable and a weird
name—“absolute warning”?).  An option could be to allow
‘GUIX_DISK_SPACE_WARNING’ to optionally contain both thresholds.  These
would be valid values:

  1GiB;10%
  15            ;15% relative, and default absolute
  15%           ;same
  1G            ;1G absolute, and default relative

Thus ‘%disk-space-warning’ would hold a pair.

Does that make sense?

Apologies for the misunderstanding!

Ludo’.
Pierre Neidhardt Feb. 23, 2020, 4:36 p.m. UTC | #8
Sounds good, I'll send a new patch as soon as possible! :)
Pierre Neidhardt Feb. 23, 2020, 9:05 p.m. UTC | #9
Ludovic Courtès <ludo@gnu.org> writes:

> So back to the original patch… I think we should use ‘size->number’ for
> the absolute threshold.
>
> I’m also still mildly reluctant to the addition of
> ‘GUIX_DISK_SPACE_WARNING_ABSOLUTE’ (one more variable and a weird
> name—“absolute warning”?).  An option could be to allow
> ‘GUIX_DISK_SPACE_WARNING’ to optionally contain both thresholds.  These
> would be valid values:
>
>   1GiB;10%
>   15            ;15% relative, and default absolute
>   15%           ;same
>   1G            ;1G absolute, and default relative

The above logic implies that we test against size->number, and if it
fails then we set the relative threshold.

But a size->number quits Guile.  Should we error out instead?
Pierre Neidhardt Feb. 23, 2020, 9:11 p.m. UTC | #10
Pierre Neidhardt <mail@ambrevar.xyz> writes:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> So back to the original patch… I think we should use ‘size->number’ for
>> the absolute threshold.
>>
>> I’m also still mildly reluctant to the addition of
>> ‘GUIX_DISK_SPACE_WARNING_ABSOLUTE’ (one more variable and a weird
>> name—“absolute warning”?).  An option could be to allow
>> ‘GUIX_DISK_SPACE_WARNING’ to optionally contain both thresholds.  These
>> would be valid values:
>>
>>   1GiB;10%
>>   15            ;15% relative, and default absolute
>>   15%           ;same
>>   1G            ;1G absolute, and default relative

Something else: in your example it's not logically possible to make out
if a raw number is relative or absolute.
So I suggest to break backward compatibility and do this:

   1GiB;10%
   15            ;15 absolute (15 bytes), and default relative
   15%           ;15% relative, and default absolute
   1G            ;1G absolute, and default relative

Thoughts?
Ludovic Courtès Feb. 23, 2020, 9:40 p.m. UTC | #11
Pierre Neidhardt <mail@ambrevar.xyz> skribis:

> Ludovic Courtès <ludo@gnu.org> writes:
>
>> So back to the original patch… I think we should use ‘size->number’ for
>> the absolute threshold.
>>
>> I’m also still mildly reluctant to the addition of
>> ‘GUIX_DISK_SPACE_WARNING_ABSOLUTE’ (one more variable and a weird
>> name—“absolute warning”?).  An option could be to allow
>> ‘GUIX_DISK_SPACE_WARNING’ to optionally contain both thresholds.  These
>> would be valid values:
>>
>>   1GiB;10%
>>   15            ;15% relative, and default absolute
>>   15%           ;same
>>   1G            ;1G absolute, and default relative
>
> The above logic implies that we test against size->number, and if it
> fails then we set the relative threshold.
>
> But a size->number quits Guile.

More precisely, it throws a 'quit exception.  :-)

So you could always do: (false-if-exception (size->number x)).

HTH,
Ludo’.
Pierre Neidhardt Feb. 24, 2020, 7:52 a.m. UTC | #12
Update sent.
Ludovic Courtès Feb. 24, 2020, 10:21 a.m. UTC | #13
Pierre Neidhardt <mail@ambrevar.xyz> skribis:

> Pierre Neidhardt <mail@ambrevar.xyz> writes:
>
>> Ludovic Courtès <ludo@gnu.org> writes:

[...]

>>>   1GiB;10%
>>>   15            ;15% relative, and default absolute
>>>   15%           ;same
>>>   1G            ;1G absolute, and default relative
>
> Something else: in your example it's not logically possible to make out
> if a raw number is relative or absolute.
> So I suggest to break backward compatibility and do this:
>
>    1GiB;10%
>    15            ;15 absolute (15 bytes), and default relative
>    15%           ;15% relative, and default absolute
>    1G            ;1G absolute, and default relative
>
> Thoughts?

What about having the rule that a number below 100 is a percentage, as
discussed before?  (It doesn’t make much sense to have an absolute
threshold of 42 bytes.)

Ludo’.
Pierre Neidhardt Feb. 24, 2020, 10:26 a.m. UTC | #14
Ludovic Courtès <ludo@gnu.org> writes:

> What about having the rule that a number below 100 is a percentage, as
> discussed before?  (It doesn’t make much sense to have an absolute
> threshold of 42 bytes.)

Could do that.  Seems a bit ad-hoc though.
Pierre Neidhardt Feb. 24, 2020, 1:20 p.m. UTC | #15
Patch updated.

Patch
diff mbox series

diff --git a/guix/scripts.scm b/guix/scripts.scm
index 77cbf12350..f8cce3a542 100644
--- a/guix/scripts.scm
+++ b/guix/scripts.scm
@@ -188,25 +188,35 @@  Show what and how will/would be built."
                     (#f        .05)               ;5%
                     (threshold (/ threshold 100.)))))
 
+(define %disk-space-warning-absolute
+  ;; The decimal number of GiB of free disk space below which a warning is
+  ;; emitted.
+  (make-parameter (match (and=> (getenv "GUIX_DISK_SPACE_WARNING_ABSOLUTE")
+                                string->number)
+                    (#f        17.0)
+                    (threshold threshold))))
+
 (define* (warn-about-disk-space #:optional profile
                                 #:key
-                                (threshold (%disk-space-warning)))
+                                (relative-threshold (%disk-space-warning))
+                                (absolute-threshold (%disk-space-warning-absolute)))
   "Display a hint about 'guix gc' if less than THRESHOLD of /gnu/store is
 available."
   (let* ((stats      (statfs (%store-prefix)))
          (block-size (file-system-block-size stats))
          (available  (* block-size (file-system-blocks-available stats)))
          (total      (* block-size (file-system-block-count stats)))
-         (ratio      (/ available total 1.)))
-    (when (< ratio threshold)
-      (warning (G_ "only ~,1f% of free space available on ~a~%")
-               (* ratio 100) (%store-prefix))
+         (relative-threshold-in-bytes (* total relative-threshold))
+         (absolute-threshold-in-bytes (* 1024 1024 1024 absolute-threshold)))
+    (when (< available (min relative-threshold-in-bytes
+                            absolute-threshold-in-bytes))
+      (warning (G_ "only ~,1f GiB of free space available on ~a~%")
+               available (%store-prefix))
       (display-hint (format #f (G_ "Consider deleting old profile
 generations and collecting garbage, along these lines:
 
 @example
 guix gc --delete-generations=1m
-@end example\n")
-                            profile)))))
+@end example\n"))))))
 
 ;;; scripts.scm ends here