Closed Bug 1013730 Opened 5 years ago Closed 5 years ago

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

Categories

(Firefox Build System :: General, defect)

32 Branch
x86_64
Windows 8.1
defect
Not set

Tracking

(firefox34 fixed, firefox-esr31 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
mozilla35
Tracking Status
firefox34 --- fixed
firefox-esr31 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: c, Assigned: mshal)

References

Details

Attachments

(1 file, 2 obsolete files)

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")
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
Attached patch disk_io.patch (obsolete) — Splinter Review
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.
Assignee: nobody → mshal
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #8473662 - Flags: feedback?(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 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.
Attachment #8473662 - Flags: feedback?(gps)
mshal: flipping on more, branches, can we land this fix even if it's not perfect?
Flags: needinfo?(mshal)
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.
Attachment #8473662 - Attachment is obsolete: true
Attachment #8496046 - Flags: review?(ted)
Flags: needinfo?(mshal)
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.
(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.
Attachment #8496046 - Flags: review?(ted)
Attachment #8496046 - Attachment is obsolete: true
Attachment #8498292 - Flags: review?(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.
Attachment #8498292 - Flags: review?(gps) → review+
Duplicate of this bug: 929310
Blocks: 1055918
https://hg.mozilla.org/mozilla-central/rev/ead2392a01f6
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Flags: qe-verify-
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.