1013730 - mach build: RuntimeError: couldn't find any physical disk

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[[e-mail]]

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:32.0) Gecko/20100101 Firefox/32.0 (Beta/Release)
Build ID: 20140513011231

Steps to reproduce:

trunk + mozillabuild1.9 + vs2013u2

need to delete the built files in python/psutil everytime

RuntimeError: couldn't find any physical disk

  File "c:\develop\mozilla\central\python/mozbuild/mozbuild/mach_commands.py", line 287, in build
    monitor.init(warnings_path)
  File "c:\develop\mozilla\central\python/mozbuild\mozbuild\controller\building.py", line 161, in init
    self.resources = SystemResourceMonitor(poll_interval=1.0)
  File "c:\develop\mozilla\central\testing/mozbase/mozsystemmonitor\mozsystemmonitor\resourcemonitor.py", line 179, in __init__
    io = psutil.disk_io_counters()
  File "c:\develop\mozilla\central\python/psutil\psutil\__init__.py", line 1229, in disk_io_counters
    raise RuntimeError("couldn't find any physical disk")

Comment 1

[Michael Shal [:mshal]]

I hit this on one of our Windows build machines as well while testing bug 978211. I manually ran 'diskperf -y' in a shell, and it seemed to make the problem go away. Maybe we want to catch this error and run that command automatically? I found the diskperf thing from here: https://code.google.com/p/psutil/issues/detail?id=351

Comment 2

[Michael Shal [:mshal]]

Attachment: disk_io.patch

I'd like to get this fixed, since we hit this when running mach in automation. One workaround is to get 'diskperf -y' in the build machines, but since it is possible for developers to hit it locally as well, we should fix it in tree. The attached patch makes the behavior go away, but is obviously wrong. Some questions:

1) I would think I'd need to wrap disk_io_counters() in _collect() as well, but _collect() doesn't get called. Any idea why it isn't? (And do we care on Windows?)

2) Should we automatically run 'diskperf -y' for the user if disk_io_counters() fails? Or just print a warning and not collect io stats?

Note I can't reproduce this locally on Windows 7, since 'diskperf -n' seems to not actually disable the feature. However, on a build machine (Windows Server 2008), I can run 'diskperf -n' to see the error, or 'diskperf -y' to make it go away.

Comment 3

[Gregory Szorc [:gps]]

If we must run diskperf -y to make the exception go away, my vote is to wrap calls to disk_io_counters() and call diskperf -y (if available) to make it work. Alternatively, we could wrap the entire collector thread in a try..finally that saves and restores the diskperf setting.

That being said, according to http://technet.microsoft.com/en-us/library/hh875645.aspx, the performance counters are only available after a restart. Is that not behavior in the real world? This sounds like something we may want to punt to a higher-level (bootstrap/MozillaBuild or machine provisioning).

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8473662 [details] [diff] [review]
disk_io.patch

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

I'm fine with a hack. But please test this first.

I'd prefer the robust solution as documented in my earlier comment.

Comment 5

[Jordan Lund (:jlund)]

mshal: flipping on more, branches, can we land this fix even if it's not perfect?

Comment 6

[Michael Shal [:mshal]]

Attachment: 0001-Bug-1013730-Automatically-run-diskperf-y-for-IO-stat.patch

gps isn't back yet, but I think this is what he's suggesting. I've tested it on a Windows builder - if diskperf is disabled, it will now automatically run and avoid the RuntimeError.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8496046 [details] [diff] [review]
0001-Bug-1013730-Automatically-run-diskperf-y-for-IO-stat.patch

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

I think it's safer to silently swallow the exception and not capture I/O than to enable disk perf as a possibly-unintended side-effect.

Keep in mind this code runs on users' machines as part of regular Firefox builds. I'm not comfortable silently making changes to system settings like this.

Comment 8

[Michael Shal [:mshal]]

(In reply to Gregory Szorc [:gps] from comment #7)
> I think it's safer to silently swallow the exception and not capture I/O
> than to enable disk perf as a possibly-unintended side-effect.
> 
> Keep in mind this code runs on users' machines as part of regular Firefox
> builds. I'm not comfortable silently making changes to system settings like
> this.

Oh, I thought you were suggesting to do it this way. Swallowing the exception sounds fine to me - I'll post a new patch.

Comment 9

[Michael Shal [:mshal]]

Attachment: 0001-Bug-1013730-Have-mach-ignore-broken-disk-io-stats.patch

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8498292 [details] [diff] [review]
0001-Bug-1013730-Have-mach-ignore-broken-disk-io-stats.patch

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

This /might/ cause issues with downstream systems expecting there to be values here. But if we don't have test coverage, then it's a follow-up bug.

Comment 11

[Michael Shal [:mshal]]

try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0cd7bb95ef4d

Comment 12

[Michael Shal [:mshal]]

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/ead2392a01f6

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/ead2392a01f6

Comment 15

[Michael Shal [:mshal]]

beta uplift for bug 1093897: https://hg.mozilla.org/releases/mozilla-beta/rev/daabf4d8995f