New issue
Advanced search Search tips

Issue 870183 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 6
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

maitred fails to signal if run_container.sh fails.

Project Member Reported by nverne@chromium.org, Aug 2

Issue description

https://cs.corp.google.com/chromeos_public/src/platform2/vm_tools/maitred/init.cc?type=cs&q=ContainerStartupFailed+file:%5Esrc/platform2/vm_tools/+package:%5Echromeos_public$&g=0&l=1263

the signal is not reached because maitred expects argv[0] to be run_container.sh. Since we wrapped this in /sbin/minijail0 it will never be true.

Because this fails to send a signal, our Install flow dialog hangs forever if run_container.sh fails. We need to fix this if possible for M69
 
Cc: tbuck...@chromium.org nverne@chromium.org
Labels: ReleaseBlock-Beta
Status: Assigned (was: Untriaged)
Has the fix been tested on ToT/M70, was it successful? If validation was successful, please add merge request label for M69. Thanks.
This has been tested on a local build only, and hasn't landed yet on ToT.
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 3

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

commit 79457cde2042d8fc7026f4aa8b9da8b1bd1cff03
Author: Stephen Barber <smbarber@chromium.org>
Date: Fri Aug 03 00:58:37 2018

vm_tools: maitred: listen for minijail0 failure

minijail0 is used to launch the run_container shell script, so monitor
for that failure instead of run_container.sh.

BUG= chromium:870183 
TEST=crostini setup page fails if container doesn't start

Change-Id: Ia2b9e32008a17f2a5a4f992d1cd72a064221bce5
Reviewed-on: https://chromium-review.googlesource.com/1160936
Commit-Ready: Stephen Barber <smbarber@chromium.org>
Tested-by: Stephen Barber <smbarber@chromium.org>
Reviewed-by: Chirantan Ekbote <chirantan@chromium.org>

[modify] https://crrev.com/79457cde2042d8fc7026f4aa8b9da8b1bd1cff03/vm_tools/maitred/init.cc

Labels: -ReleaseBlock-Beta ReleaseBlock-Stable
Labels: Merge-Request-69
Status: Fixed (was: Assigned)
Verified on 10944.0.0 by connecting to VM and stopping the container download manually. Requesting merge to M69.
Project Member

Comment 8 by sheriffbot@chromium.org, Aug 6

Labels: -Merge-Request-69 Merge-Review-69 Hotlist-Merge-Review
This bug requires manual review: M69 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), kariahda@(iOS), cindyb@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-69 Merge-Approved-69
Merge approved, M69.
Project Member

Comment 10 by bugdroid1@chromium.org, Aug 9

Labels: merge-merged-release-R69-10895.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform2/+/44d8529ea7c1c98c6da5df2eb28bf6f96bd35136

commit 44d8529ea7c1c98c6da5df2eb28bf6f96bd35136
Author: Stephen Barber <smbarber@chromium.org>
Date: Thu Aug 09 23:10:50 2018

vm_tools: maitred: listen for minijail0 failure

minijail0 is used to launch the run_container shell script, so monitor
for that failure instead of run_container.sh.

BUG= chromium:870183 
TEST=crostini setup page fails if container doesn't start

Change-Id: Ia2b9e32008a17f2a5a4f992d1cd72a064221bce5
Reviewed-on: https://chromium-review.googlesource.com/1160936
Commit-Ready: Stephen Barber <smbarber@chromium.org>
Tested-by: Stephen Barber <smbarber@chromium.org>
Reviewed-by: Chirantan Ekbote <chirantan@chromium.org>
(cherry picked from commit 79457cde2042d8fc7026f4aa8b9da8b1bd1cff03)
Reviewed-on: https://chromium-review.googlesource.com/1170115
Reviewed-by: Stephen Barber <smbarber@chromium.org>
Commit-Queue: Stephen Barber <smbarber@chromium.org>

[modify] https://crrev.com/44d8529ea7c1c98c6da5df2eb28bf6f96bd35136/vm_tools/maitred/init.cc

Project Member

Comment 11 by sheriffbot@chromium.org, Aug 13

Cc: cindyb@chromium.org
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Approved-69

Sign in to add a comment