1130336 - Create a function to determine: total, used and free disk space

Status: RESOLVED FIXED

(Release Engineering :: Applications: MozharnessCore, defect)

Description

[Massimo Gervasini [:massimo]]

Attachment: [mozharness] added disk_info in ScriptMixin.patch

I am creating the RunnerMixin, it needs to delete or move some directories based on the available disk space. So I thought it might be useful to have a generic mozharness function that shows some disk info: total used and free space.

Comment 1

[Jordan Lund (:jlund)]

Comment on attachment 8560392 [details] [diff] [review]
[mozharness] added disk_info in ScriptMixin.patch

Review of attachment 8560392 [details] [diff] [review]:
-----------------------------------------------------------------

this is a great addition to script.py!

non blocker but since it's part of core, it be really cool to have one or two tests

::: mozharness/base/script.py
@@ +381,5 @@
> +               * GB
> +               * TB
> +            returns: size converted from source_format to destination_format.
> +            It raises KeyError if source or destination format are not valid.
> +            It raises TypeError if size is not a number

should we be try/catching these?

Comment 2

[Massimo Gervasini [:massimo]]

Attachment: [mozharness] Bug 1130336 - Create a function to determine: total, used and free disk space.patch

Hi Jordan,
Thanks for the review.

changes in this patch:
* moved the code into a separated module: mozharness.base.diskutils and since it's pretty basic, there are no mixins there.
* added tests
* it does not need _is_windows(), _is_darwin(), _is_linux()
* every error in this module raises a DiskutilsError exception.

I am open to any changes/suggestions.

Comment 3

[Massimo Gervasini [:massimo]]

Comment on attachment 8561996 [details] [diff] [review]
[mozharness] Bug 1130336 - Create a function to determine: total, used and free disk space.patch

Hey Jordan,

I have an updated version of this patch, I'll upload it shortly.

Comment 4

[Massimo Gervasini [:massimo]]

Attachment: [mozharness] Bug 1130336 - Create a function to determine: total, used and free disk space.patch

Hi Jordan, 

here's the updated version of this patch.

* This module does not require any Mixin: the get_size() method logs the operation using the standard python logging module (it uses the logger named 'Multi'). The log level can be changed using mozharness levels (DEBUG, WARNING, ...) - this is the reason there's a new function, in log.py: numeric_log_level(), it converts mozharness log levels (strings) to standard module levels (numbers)

* more tests

Comment 5

[Jordan Lund (:jlund)]

Comment on attachment 8563451 [details] [diff] [review]
[mozharness] Bug 1130336 - Create a function to determine: total, used and free disk space.patch

Review of attachment 8563451 [details] [diff] [review]:
-----------------------------------------------------------------

once again, you've leveled up this code :)

::: mozharness/base/diskutils.py
@@ +30,5 @@
> +import logging
> +from mozharness.base.log import INFO, numeric_log_level
> +
> +# use mozharness log
> +mhlog = logging.getLogger('Multi')

ah interesting. So I just dove into BaseLogger in detail and I didn't realize it relies heavily on built in logging. So of course mh can use getLogger(name) to grab the 'Multi' logger we created in a script and, since we have that created already, it will be configured perfectly? Have you tried this with an existing script? I would be thrilled if you could show me it works fine as that would solve a lot of things for https://bugzilla.mozilla.org/show_bug.cgi?id=1101183#c7 :D

I know we usually (always?) use 'Multi' exclusively, but maybe this should be configurable in some way?

@@ +147,5 @@
> +                disk_info = DiskSize()._windows_size(path)
> +            except AttributeError:
> +                # No luck! This is not a posix nor window platform
> +                # raise an exception
> +                raise DiskutilsError('Unsupported platform')

will this always be an error due to being an unsupported platform? Could we raise AttributeError another way?

Comment 6

[Simarpreet Singh]

(In reply to Jordan Lund (:jlund) from comment #5)
> Comment on attachment 8563451 [details] [diff] [review]
> [mozharness] Bug 1130336 - Create a function to determine: total, used and
> free disk space.patch
> 
> Review of attachment 8563451 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> once again, you've leveled up this code :)
> 
> ::: mozharness/base/diskutils.py
> @@ +30,5 @@
> > +import logging
> > +from mozharness.base.log import INFO, numeric_log_level
> > +
> > +# use mozharness log
> > +mhlog = logging.getLogger('Multi')
> 
> ah interesting. So I just dove into BaseLogger in detail and I didn't
> realize it relies heavily on built in logging. So of course mh can use
> getLogger(name) to grab the 'Multi' logger we created in a script and, since
> we have that created already, it will be configured perfectly? Have you
> tried this with an existing script? I would be thrilled if you could show me
> it works fine as that would solve a lot of things for
> https://bugzilla.mozilla.org/show_bug.cgi?id=1101183#c7 :D
> 
> I know we usually (always?) use 'Multi' exclusively, but maybe this should
> be configurable in some way?
> 

Piggyback question: Is 'Multi' here referring to the MultiFileLogger? [1]

[1]: http://mxr.mozilla.org/build/source/mozharness/mozharness/base/log.py#363


> @@ +147,5 @@
> > +                disk_info = DiskSize()._windows_size(path)
> > +            except AttributeError:
> > +                # No luck! This is not a posix nor window platform
> > +                # raise an exception
> > +                raise DiskutilsError('Unsupported platform')
> 
> will this always be an error due to being an unsupported platform? Could we
> raise AttributeError another way?

Comment 7

[Jordan Lund (:jlund)]

> > I know we usually (always?) use 'Multi' exclusively, but maybe this should
> > be configurable in some way?
> > 
> 
> Piggyback question: Is 'Multi' here referring to the MultiFileLogger? [1]

that's the one. you can see we give it a logger name here: http://mxr.mozilla.org/build/source/mozharness/mozharness/base/log.py#368

AFAIK, we use the multi logger class everywhere. Aki, wanted to have the option to support a conglomerate of log levels into a single file vs splitting levels into individual files.

Comment 8

[Massimo Gervasini [:massimo]]

Thanks for the review, Jordan.

(In reply to Jordan Lund (:jlund) from comment #5)
> Comment on attachment 8563451 [details] [diff] [review]
> [mozharness] Bug 1130336 - Create a function to determine: total, used and
> free disk space.patch
> 
> Review of attachment 8563451 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> once again, you've leveled up this code :)
> 
> ::: mozharness/base/diskutils.py
> @@ +30,5 @@
> > +import logging
> > +from mozharness.base.log import INFO, numeric_log_level
> > +
> > +# use mozharness log
> > +mhlog = logging.getLogger('Multi')
> 
> ah interesting. So I just dove into BaseLogger in detail and I didn't
> realize it relies heavily on built in logging. So of course mh can use
> getLogger(name) to grab the 'Multi' logger we created in a script and, since
> we have that created already, it will be configured perfectly? Have you
> tried this with an existing script?
I have tested it in our staging environment and it works fine. I was also planning to do more testing, but I got some delays. I am still waiting on the results on ash before landing this.


> I know we usually (always?) use 'Multi' exclusively, but maybe this should
> be configurable in some way?
I like the idea but this is tricky, I see some options here:
* passing a configuration - but this probably will require some extra code for getting the logger values from the configuration object and we are back to the mixin world.
* writing a function in log.py that returns the logger name mozharness is using 
* enforce we will always use 'Multi' for mozharness.
Any other options/ideas?

> 
> @@ +147,5 @@
> > +                disk_info = DiskSize()._windows_size(path)
> > +            except AttributeError:
> > +                # No luck! This is not a posix nor window platform
> > +                # raise an exception
> > +                raise DiskutilsError('Unsupported platform')
> 
> will this always be an error due to being an unsupported platform?
 yes
> Could we raise AttributeError another way?
At the moment there are no "unsupported platforms". We should never hit that line (except in tests), and if we ever do I'd like it to raise a clear error. 
I have discarded the idea of returning a special crafted DiskInfo for this case (e.g. setting all the properties to None/-1/''/...) because this could lead to unexpected results. In my opinion, if we don't know ho to calculate the disk space on a platform, we should just raise an Exception an let the caller script decide what to do. 
About the exception class, I have decided to use DiskutilsError; AttributeError, is too generic. With a DiskutilsError error is clear something bad just happened in the diskutils module.

Comment 9

[Jordan Lund (:jlund)]

> I have tested it in our staging environment and it works fine. I was also
> planning to do more testing, but I got some delays. I am still waiting on
> the results on ash before landing this.

coola boola

> 
> > I know we usually (always?) use 'Multi' exclusively, but maybe this should
> > be configurable in some way?
> I like the idea but this is tricky, I see some options here:
> * passing a configuration - but this probably will require some extra code
> for getting the logger values from the configuration object and we are back
> to the mixin world.
> * writing a function in log.py that returns the logger name mozharness is
> using
> * enforce we will always use 'Multi' for mozharness.
> Any other options/ideas?

hmm, I wonder if we should just drop support for single logger. not sure how useful it is. maybe track down aki in #releng and see what he thinks :)


> I have discarded the idea of returning a special crafted DiskInfo for this
> case (e.g. setting all the properties to None/-1/''/...) because this could
> lead to unexpected results. In my opinion, if we don't know ho to calculate
> the disk space on a platform, we should just raise an Exception an let the
> caller script decide what to do. 
> About the exception class, I have decided to use DiskutilsError;
> AttributeError, is too generic. With a DiskutilsError error is clear
> something bad just happened in the diskutils module.

cool, makes sense to me :)

Comment 10

[Massimo Gervasini [:massimo]]

It works! Tested using a crafted version of mozharness that forces each call to query_abs_dir(), to run Diskutils.get_size(...) with 3 different log level: INFO, DEBUG and ERROR 

Here's the full log of yesterday's nightly desktop build on ash: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015/02/2015-02-26-08-31-45-ash/ash-linux-nightly-bm71-build1-build2.txt.gz

Jordan, if you're happy with this test, I'm going to land this patch.

Comment 11

[Jordan Lund (:jlund)]

(In reply to Massimo Gervasini [:mgerva] from comment #10)
> It works! Tested using a crafted version of mozharness that forces each call
> to query_abs_dir(), to run Diskutils.get_size(...) with 3 different log
> level: INFO, DEBUG and ERROR 
> 
> Here's the full log of yesterday's nightly desktop build on ash:
> http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015/02/2015-02-26-08-
> 31-45-ash/ash-linux-nightly-bm71-build1-build2.txt.gz
> 
> Jordan, if you're happy with this test, I'm going to land this patch.

I don't want to block but I think we should try catlee's suggestion and pastebins from irc: having a ROOT_LOGGER that 'listens' for any logging and so we can do things like getLogger(__name__) instead of 'Multi'.

Simar is likely going to be picking this up for all parts of mh so I personally am okay if you don't want to change script.py / log.py to do support this and leave your patch as is.

Comment 12

[Massimo Gervasini [:massimo]]

Attachment: [mozharness] Bug 1130336 - Create a function to determine: total, used and free disk space II.patch

Thanks Jordan.

I agree, using logging.getlogger(__name__) it's better than hardcoding 'Multi'.

Changes in this version:
* added ROOT_LOGGER
* when we use ROOT_LOGGER in new_logger(), we don't need the "logger_name" parameter
* removed logger_name property from BaseLogger


and this is what is logged by a custom version of mozharness [1]:
...
02:45:49     INFO - Disk space info (in GB) total: 108 used: 46 free: 56
02:45:49    ERROR - Disk space info (in GB) total: 108 used: 46 free: 56
...
each time query_abs_dir() is called (as expected) 

full log here: http://buildbot-master94.bb.releng.use1.mozilla.com:8001/builders/Linux%20ash%20nightly/builds/2/steps/run_script/logs/stdio (requires VPN - job stopped manually, to avoid triggering of downstream jobs)

[1] https://hg.mozilla.org/users/mgervasini_mozilla.com/mozharness-new-logger/

Comment 13

[Jordan Lund (:jlund)]

Comment on attachment 8571288 [details] [diff] [review]
[mozharness] Bug 1130336 - Create a function to determine: total, used and free disk space II.patch

Review of attachment 8571288 [details] [diff] [review]:
-----------------------------------------------------------------

looks really good! thanks for going all the way with this.

the log.py changes make me nervous. is there a safe way we can roll this out? I know mh is pinned in-tree for builds and tests but everything releng used outside of that will pick this change up immediately. We should deploy this to default and pin the default rev on a branch or two. Then verify that builds and tests are logging correctly and the bits are being stored/uploaded like before.

iiuc - we don't need this anymore: http://mxr.mozilla.org/build/source/mozharness/mozharness/base/script.py?force=1#1366

::: mozharness/base/log.py
@@ +25,5 @@
>  DEBUG, INFO, WARNING, ERROR, CRITICAL, FATAL, IGNORE = (
>      'debug', 'info', 'warning', 'error', 'critical', 'fatal', 'ignore')
>  
>  
> +LOG_LEVELS = {

so if someone uses self.log('info', msg) instead of self.log(INFO, msg), it will break.

I'm not sure if we do that anywhere today but just pointing it out

Comment 14

[Massimo Gervasini [:massimo]]

Attachment: [mozharness] Bug 1130336 - Create a function to determine: total, used and free disk space III.patch

Changes in this patch:
* removed log_config['logger_name'] = 'Multi' from mozharness/base/script.py

I have landed this patch in: https://hg.mozilla.org/users/mgervasini_mozilla.com/mozharness-1130336, and forced ash to use this mozharness repo.

We should have some results in few hours

Comment 15

[Massimo Gervasini [:massimo]]

> ::: mozharness/base/log.py
> @@ +25,5 @@
> >  DEBUG, INFO, WARNING, ERROR, CRITICAL, FATAL, IGNORE = (
> >      'debug', 'info', 'warning', 'error', 'critical', 'fatal', 'ignore')
> >  
> >  
> > +LOG_LEVELS = {
> 
> so if someone uses self.log('info', msg) instead of self.log(INFO, msg), it
> will break.
> 
> I'm not sure if we do that anywhere today but just pointing it out
As it is now, we have two different sets of log levels: numbers from python logging module and strings from mozharness. We should probably use numbers in mozharness too, but this could be a tricky project. 

Do you know the reason mozharness log levels are strings?

Comment 16

[Jordan Lund (:jlund)]

(In reply to Massimo Gervasini [:mgerva] from comment #15)
> > ::: mozharness/base/log.py
> > @@ +25,5 @@
> > >  DEBUG, INFO, WARNING, ERROR, CRITICAL, FATAL, IGNORE = (
> > >      'debug', 'info', 'warning', 'error', 'critical', 'fatal', 'ignore')
> > >  
> > >  
> > > +LOG_LEVELS = {
> > 
> > so if someone uses self.log('info', msg) instead of self.log(INFO, msg), it
> > will break.
> > 
> > I'm not sure if we do that anywhere today but just pointing it out
> As it is now, we have two different sets of log levels: numbers from python
> logging module and strings from mozharness. We should probably use numbers
> in mozharness too, but this could be a tricky project. 
> 
> Do you know the reason mozharness log levels are strings?

not sure anymore. smells like artifacts of some style aki was trying to use initially.

Comment 17

[Jordan Lund (:jlund)]

Comment on attachment 8571877 [details] [diff] [review]
[mozharness] Bug 1130336 - Create a function to determine: total, used and free disk space III.patch

r+ assuming all is well in ash

Comment 18

[Massimo Gervasini [:massimo]]

Comment on attachment 8571877 [details] [diff] [review]
[mozharness] Bug 1130336 - Create a function to determine: total, used and free disk space III.patch

Thanks Jordan,

I had no logging issues on ash, I have landed it.

https://hg.mozilla.org/build/mozharness/rev/1638197c5171

Comment 19

[Chris Cooper [:coop] (he/him)]

mozharness production tag moved to: https://hg.mozilla.org/build/mozharness/rev/1638197c5171