New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 762328 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Feature



Sign in to add a comment

Log more information about why the system is rebooting or shutting down

Project Member Reported by derat@chromium.org, Sep 5 2017

Issue description

powerd logs the various internal reasons that it has for shutting the system down or rebooting it, but when it gets a RequestRestart or RequestShutdown D-Bus method call from Chrome, update_engine, or some other process, it's extremely difficult to figure out why it was sent. Chrome doesn't log anything here, as far as I can tell; we just call chromeos::PowerManagerClient::RequestRestart() from random places across the code.

The RequestRestart D-Bus method at least has an (optional) int32 argument containing a RequestRestartReason enum value, but there are only two possible values for it (REQUEST_RESTART_FOR_USER and REQUEST_RESTART_FOR_UPDATE), and most/all callers don't pass anything, resulting in powerd just using the default of "user request".

RequestShutdown doesn't even take a reason arg.

When powerd calls powerd_setuid_helper to actually shut down the system, it passes a reason arg that gets logged in a "Shutting down for <job>: <reason>" message by the pre-shutdown Upstart job. When it reboots, we don't pass a reason, resulting in unknown-reason getting logged.

Life would probably be better with the following changes:

a) powerd's RequestRestart and RequestShutdown D-Bus methods should take optional human-readable strings describing why the system is being shut down. powerd should log these strings (and maybe also pass them to init so they can be logged to /var/log/messages).

b) In powerd, ShutdownReason::USER_REQUEST should probably be renamed to something like ShutdownReason::EXTERNAL. These requests are external to powerd and didn't necessarily come from the user.

c) I should do an audit of Chrome and other callers to make them always pass shutdown/restart reasons to powerd.
 

Comment 1 by derat@chromium.org, Sep 7 2017

Status: Started (was: Assigned)
Project Member

Comment 2 by bugdroid1@chromium.org, Sep 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/system_api/+/5b991d25aa2919564444db96c35db650b6e1409b

commit 5b991d25aa2919564444db96c35db650b6e1409b
Author: Daniel Erat <derat@chromium.org>
Date: Fri Sep 08 04:21:29 2017

system_api: Add power_manager::RequestShutdownReason.

Add an enum containing reasons that can be passed in
RequestShutdown D-Bus method calls to powerd. Also add a
REQUEST_RESTART_OTHER value to RequestRestartReason so that
everything doesn't get lumped into REQUEST_RESTART_FOR_USER.

BUG= chromium:762328 
TEST=none

Change-Id: I3ba78c606f98b9964abd370a05fd4ad3c17d3126
Reviewed-on: https://chromium-review.googlesource.com/655812
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>

[modify] https://crrev.com/5b991d25aa2919564444db96c35db650b6e1409b/dbus/power_manager/dbus-constants.h

Project Member

Comment 3 by bugdroid1@chromium.org, Sep 8 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c3b60f663869fe3a2d101b57c75edcb2530702de

commit c3b60f663869fe3a2d101b57c75edcb2530702de
Author: Daniel Erat <derat@chromium.org>
Date: Fri Sep 08 22:28:12 2017

Roll src/third_party/cros_system_api/ a8a31cea1..5b991d25a (2 commits)

https://chromium.googlesource.com/chromiumos/platform/system_api.git/+log/a8a31cea143f..5b991d25aa29

$ git log a8a31cea1..5b991d25a --date=short --no-merges --format='%ad %ae %s'
2017-09-07 derat system_api: Add power_manager::RequestShutdownReason.
2017-09-06 jkardatzke Added D-Bus cryptohome constants that were missing.

Created with:
  roll-dep src/third_party/cros_system_api

Bug:  762328 
Change-Id: I1236fbdc5f3aa212adab89fe86c9cdc0b76f9b76
Reviewed-on: https://chromium-review.googlesource.com/656957
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Commit-Queue: Dan Erat <derat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500709}
[modify] https://crrev.com/c3b60f663869fe3a2d101b57c75edcb2530702de/DEPS

Project Member

Comment 4 by bugdroid1@chromium.org, Sep 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/03de51e251b223c6ae396eb94f034a20d8796f64

commit 03de51e251b223c6ae396eb94f034a20d8796f64
Author: Daniel Erat <derat@chromium.org>
Date: Sat Sep 09 00:51:51 2017

chromeos: Pass more info to powerd about shutdown/reboot.

Make Chrome include a reason enum and human-readable string
in RequestRestart and RequestShutdown D-Bus method calls
made to powerd. powerd will log these, making it much easier
to decipher user feedback reports of unexpected shutdowns
and reboots.

Bug:  762328 
Change-Id: Ib0e9fbdf6f5076d1a23065044e25ceba83323a71
Reviewed-on: https://chromium-review.googlesource.com/656300
Commit-Queue: Dan Erat <derat@chromium.org>
Reviewed-by: Michael Giuffrida <michaelpg@chromium.org>
Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#500767}
[modify] https://crrev.com/03de51e251b223c6ae396eb94f034a20d8796f64/ash/shutdown_controller.cc
[modify] https://crrev.com/03de51e251b223c6ae396eb94f034a20d8796f64/chrome/browser/chromeos/app_mode/app_session.cc
[modify] https://crrev.com/03de51e251b223c6ae396eb94f034a20d8796f64/chrome/browser/chromeos/login/screens/error_screen.cc
[modify] https://crrev.com/03de51e251b223c6ae396eb94f034a20d8796f64/chrome/browser/chromeos/login/screens/reset_screen.cc
[modify] https://crrev.com/03de51e251b223c6ae396eb94f034a20d8796f64/chrome/browser/chromeos/login/wizard_controller.cc
[modify] https://crrev.com/03de51e251b223c6ae396eb94f034a20d8796f64/chrome/browser/chromeos/policy/remote_commands/device_command_reboot_job.cc
[modify] https://crrev.com/03de51e251b223c6ae396eb94f034a20d8796f64/chrome/browser/chromeos/system/automatic_reboot_manager.cc
[modify] https://crrev.com/03de51e251b223c6ae396eb94f034a20d8796f64/chrome/browser/extensions/api/runtime/chrome_runtime_api_delegate.cc
[modify] https://crrev.com/03de51e251b223c6ae396eb94f034a20d8796f64/chrome/browser/lifetime/application_lifetime.cc
[modify] https://crrev.com/03de51e251b223c6ae396eb94f034a20d8796f64/chrome/browser/ui/webui/chromeos/login/enable_debugging_screen_handler.cc
[modify] https://crrev.com/03de51e251b223c6ae396eb94f034a20d8796f64/chrome/browser/ui/webui/chromeos/login/encryption_migration_screen_handler.cc
[modify] https://crrev.com/03de51e251b223c6ae396eb94f034a20d8796f64/chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc
[modify] https://crrev.com/03de51e251b223c6ae396eb94f034a20d8796f64/chromeos/dbus/fake_power_manager_client.cc
[modify] https://crrev.com/03de51e251b223c6ae396eb94f034a20d8796f64/chromeos/dbus/fake_power_manager_client.h
[modify] https://crrev.com/03de51e251b223c6ae396eb94f034a20d8796f64/chromeos/dbus/power_manager_client.cc
[modify] https://crrev.com/03de51e251b223c6ae396eb94f034a20d8796f64/chromeos/dbus/power_manager_client.h
[modify] https://crrev.com/03de51e251b223c6ae396eb94f034a20d8796f64/extensions/shell/browser/shell_desktop_controller_aura.cc
[modify] https://crrev.com/03de51e251b223c6ae396eb94f034a20d8796f64/extensions/shell/browser/shell_runtime_api_delegate.cc

Project Member

Comment 5 by bugdroid1@chromium.org, Sep 9 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/8a98e2aa8e9662f1c9da2371c92608b52e207824

commit 8a98e2aa8e9662f1c9da2371c92608b52e207824
Author: Daniel Erat <derat@chromium.org>
Date: Sat Sep 09 05:08:55 2017

power: Log more when rebooting or shutting down.

Update powerd to read an optional caller-supplied reason
enum from RequestShutdown D-Bus method calls, and to read
and log a new optional human-readable description from both
RequestRestart and RequestShutdown calls.

BUG= chromium:762328 
TEST=updated chrome calls and checked that reasons and
     descriptions are logged to
     /var/log/power_manager/powerd.LATEST
CQ-DEPEND=I3ba78c606f98b9964abd370a05fd4ad3c17d3126

Change-Id: Iebec22242327d9b6d34f70540cbe2a2a9fb88e88
Reviewed-on: https://chromium-review.googlesource.com/656060
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>

[modify] https://crrev.com/8a98e2aa8e9662f1c9da2371c92608b52e207824/power_manager/powerd/daemon.cc
[modify] https://crrev.com/8a98e2aa8e9662f1c9da2371c92608b52e207824/power_manager/powerd/daemon_unittest.cc
[modify] https://crrev.com/8a98e2aa8e9662f1c9da2371c92608b52e207824/power_manager/common/power_constants.cc
[modify] https://crrev.com/8a98e2aa8e9662f1c9da2371c92608b52e207824/power_manager/common/power_constants.h

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 12 2017

The following revision refers to this bug:
  https://chrome-internal.googlesource.com/chromeos/ap-daemons/+/445932084ac0725d4e9ae11625b52f1367eecdcf

commit 445932084ac0725d4e9ae11625b52f1367eecdcf
Author: Daniel Erat <derat@chromium.org>
Date: Tue Sep 12 17:30:22 2017

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/8babe70529419b6287bb302de09202ce30b24079

commit 8babe70529419b6287bb302de09202ce30b24079
Author: Daniel Erat <derat@chromium.org>
Date: Tue Sep 12 17:30:22 2017

power: Update Request{Restart,Shutdown} D-Bus signatures.

Update org.chromium.PowerManager.xml's signatures for
powerd's RequestRestart and RequestShutdown D-Bus methods to
include "reason" and "description" arguments. These are
optional, but there's apparently no way to express that in a
D-Bus XML file. I think. It's not like there are docs.

BUG= chromium:762328 
TEST=built power_manager-client and packages that use it
CQ-DEPEND=Ia5c6eb1705902dd8f87de701f4ee15fc3b389a3d
CQ-DEPEND=CL:*451212

Change-Id: Ifcfcdde0df20e8e341e8aa9bc0002b10c56cd5e7
Reviewed-on: https://chromium-review.googlesource.com/658608
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>

[modify] https://crrev.com/8babe70529419b6287bb302de09202ce30b24079/power_manager/dbus_bindings/org.chromium.PowerManager.xml

Project Member

Comment 8 by bugdroid1@chromium.org, Sep 12 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/aosp/platform/system/update_engine/+/b7f45dfcc72b1cfed2432cf30b36d815ff1af0d4

commit b7f45dfcc72b1cfed2432cf30b36d815ff1af0d4
Author: Daniel Erat <derat@chromium.org>
Date: Tue Sep 12 17:30:22 2017

update_engine: Pass reboot description to powerd.

Make update_engine pass a description of why it's requesting
a reboot to the Chrome OS power manager.

BUG= chromium:762328 
TEST=built with FEATURES=test
CQ-DEPEND=Ifcfcdde0df20e8e341e8aa9bc0002b10c56cd5e7

Change-Id: Ia5c6eb1705902dd8f87de701f4ee15fc3b389a3d
Reviewed-on: https://chromium-review.googlesource.com/658609
Commit-Ready: Dan Erat <derat@chromium.org>
Tested-by: Dan Erat <derat@chromium.org>
Reviewed-by: Ben Chan <benchan@chromium.org>

[modify] https://crrev.com/b7f45dfcc72b1cfed2432cf30b36d815ff1af0d4/power_manager_chromeos.cc

Comment 9 by derat@chromium.org, Sep 12 2017

Status: Fixed (was: Started)

Comment 11 by dchan@chromium.org, Jan 22 2018

Status: Archived (was: Fixed)

Comment 12 by dchan@chromium.org, Jan 23 2018

Status: Fixed (was: Archived)

Sign in to add a comment