New issue
Advanced search Search tips

Issue 819324 link

Starred by 8 users

Issue metadata

Status: Verified
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug

Blocking:
issue 829807
issue 830209



Sign in to add a comment

refactor vsh to fix framing, add unit tests, and run inside container

Project Member Reported by smbar...@chromium.org, Mar 6 2018

Issue description

When showing large amounts of output, like rebuilding your toolchain in Gentoo, vsh can hang.

We're speculating that there's a blocking read/write somewhere.
 
Labels: Proj-Containers
Cc: tbuck...@chromium.org
Labels: -Pri-2 Pri-1
Status: Assigned (was: Untriaged)
Summary: refactor vsh to fix framing, add unit tests, and run inside container (was: vsh occasionally hangs with large amounts of output)
The first iteration of vsh is still a bit lacking:

1) Unit tests are needed, but it's not testable in its current state.
2) Framing is broken and the use of blocking I/O can cause vsh hangs
3) vshd only runs outside of the container, but lxc exec doesn't allocate ptys in the container. This causes weird issues with certain electron-based apps, and older versions of tmux. See https://github.com/lxc/lxd/issues/936

Do the necessary refactoring to OO-ify vsh, which will help remedy these problems.
Blocking: 830209
Blocking: 829807
Labels: Hotlist-Crostini-Platform
Project Member

Comment 6 by bugdroid1@chromium.org, May 22 2018

Project Member

Comment 7 by bugdroid1@chromium.org, May 23 2018

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

commit f75550c6e36c6af25ea5c31553ce93110a16a491
Author: Stephen Barber <smbarber@chromium.org>
Date: Wed May 23 08:25:44 2018

vm_concierge: add LaunchVshd method

BUG= chromium:819324 
TEST=none

Change-Id: I30e5daac501c9178f8cf02f72cae5e96600d9cbb
Reviewed-on: https://chromium-review.googlesource.com/1060629
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/f75550c6e36c6af25ea5c31553ce93110a16a491/dbus/vm_concierge/service.proto
[modify] https://crrev.com/f75550c6e36c6af25ea5c31553ce93110a16a491/dbus/vm_concierge/dbus-constants.h

Project Member

Comment 8 by bugdroid1@chromium.org, May 23 2018

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

commit 6e311994da7b1e0c8aca2265efa07690fca244df
Author: Stephen Barber <smbarber@chromium.org>
Date: Wed May 23 19:50:39 2018

vm_tools: vsh: refactor vsh into vsh_client

This does not significantly change vsh logic, but instead moves it into
a class to ease future refactoring.

BUG= chromium:819324 
TEST=vsh works

Change-Id: I84d011081ccbb17a45e139058a8898fc199ed610
Reviewed-on: https://chromium-review.googlesource.com/1060651
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/6e311994da7b1e0c8aca2265efa07690fca244df/vm_tools/host.gypi
[modify] https://crrev.com/6e311994da7b1e0c8aca2265efa07690fca244df/vm_tools/vsh/vsh.cc
[add] https://crrev.com/6e311994da7b1e0c8aca2265efa07690fca244df/vm_tools/vsh/vsh_client.cc
[add] https://crrev.com/6e311994da7b1e0c8aca2265efa07690fca244df/vm_tools/vsh/vsh_client.h

Project Member

Comment 10 by bugdroid1@chromium.org, May 23 2018

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

commit 2caf57c78c88447d813862cb44623a750d26e328
Author: Stephen Barber <smbarber@chromium.org>
Date: Wed May 23 19:50:40 2018

vm_tools: garcon: add LaunchVshd RPC for garcon

Add an RPC to launch vshd in a mode that will connect back to
a port on the host.

BUG= chromium:819324 
TEST=none
CQ-DEPEND=CL:1060650

Change-Id: Id113c902d81042657a0e686e8c319212031ff72f
Reviewed-on: https://chromium-review.googlesource.com/1060733
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/2caf57c78c88447d813862cb44623a750d26e328/vm_tools/garcon/service_impl.cc
[modify] https://crrev.com/2caf57c78c88447d813862cb44623a750d26e328/vm_tools/garcon/service_impl.h
[modify] https://crrev.com/2caf57c78c88447d813862cb44623a750d26e328/vm_tools/proto/container_guest.proto

Project Member

Comment 11 by bugdroid1@chromium.org, May 23 2018

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

commit bbd87f34331339a13cada68a9e2442e177aabdcd
Author: Stephen Barber <smbarber@chromium.org>
Date: Wed May 23 19:50:41 2018

vm_tools: concierge: add LaunchVshd dbus implementation

Allow invoking vshd in the guest container.

BUG= chromium:819324 
TEST=vsh to container with last patch in this series
CQ-DEPEND=CL:1060629

Change-Id: I50b35bff31ddf406688eb1d91595c8b650e6c94f
Reviewed-on: https://chromium-review.googlesource.com/1060734
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/bbd87f34331339a13cada68a9e2442e177aabdcd/vm_tools/concierge/service.h
[modify] https://crrev.com/bbd87f34331339a13cada68a9e2442e177aabdcd/vm_tools/concierge/virtual_machine.cc
[modify] https://crrev.com/bbd87f34331339a13cada68a9e2442e177aabdcd/vm_tools/concierge/virtual_machine.h
[modify] https://crrev.com/bbd87f34331339a13cada68a9e2442e177aabdcd/vm_tools/concierge/service.cc

Project Member

Comment 12 by bugdroid1@chromium.org, May 23 2018

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

commit 98a47150297ef22d612b2e2b1e3d11980e18dd35
Author: Stephen Barber <smbarber@chromium.org>
Date: Wed May 23 19:50:41 2018

vm_tools: vsh: add support for directly connecting to container

Add support for the LaunchVshd D-Bus method in concierge.

BUG= chromium:819324 
TEST=vsh --target_container=penguin

Change-Id: I7ea196240a126d113e2deae03aae3207eca2f005
Reviewed-on: https://chromium-review.googlesource.com/1060735
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/98a47150297ef22d612b2e2b1e3d11980e18dd35/vm_tools/vsh/vsh.cc

Project Member

Comment 13 by bugdroid1@chromium.org, May 31 2018

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

commit 5aaa80e6b5cfbfe1da3a3f4b79929c1a18841bf7
Author: Stephen Barber <smbarber@chromium.org>
Date: Thu May 31 19:26:10 2018

vm_tools: vsh: default to empty user for container vsh

On the client side, set the default value of the user flag to be empty,
which indicates that vshd should run as the current user in a container.
Set user to "chronos" when connecting to a VM.

Also add a check for this in vshd, since this will let us remove the hack
in the client code later.

BUG= chromium:819324 
TEST=manual

Change-Id: I77ba3bd9a607d8440f59b4ecedb401d0a8055cd3
Reviewed-on: https://chromium-review.googlesource.com/1074088
Commit-Ready: Stephen Barber <smbarber@chromium.org>
Tested-by: Stephen Barber <smbarber@chromium.org>
Reviewed-by: Stephen Barber <smbarber@chromium.org>

[modify] https://crrev.com/5aaa80e6b5cfbfe1da3a3f4b79929c1a18841bf7/vm_tools/vsh/vsh_forwarder.cc
[modify] https://crrev.com/5aaa80e6b5cfbfe1da3a3f4b79929c1a18841bf7/vm_tools/vsh/vsh.cc

Project Member

Comment 14 by bugdroid1@chromium.org, Jun 7 2018

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

commit a98cb102e16995c8ad3a630e66fd238575b4af06
Author: Stephen Barber <smbarber@chromium.org>
Date: Thu Jun 07 20:11:43 2018

vm_tools: vsh containers connect through cicerone

vsh should connect to cicerone directly to execute
the LaunchVshd RPC. Fix the dbus permissions to permit
chronos to run LaunchVshd, and allow root to run all RPCs.

Also fix an unused variable in concierge's version of LaunchVshd.

BUG= chromium:819324 , chromium:849028 
TEST=vsh to a container works

Change-Id: I2f60aa67fa293f25cfd0285b24494f1c4a0450d1
Reviewed-on: https://chromium-review.googlesource.com/1087493
Commit-Ready: ChromeOS CL Exonerator Bot <chromiumos-cl-exonerator@appspot.gserviceaccount.com>
Tested-by: Stephen Barber <smbarber@chromium.org>
Reviewed-by: Chirantan Ekbote <chirantan@chromium.org>

[modify] https://crrev.com/a98cb102e16995c8ad3a630e66fd238575b4af06/vm_tools/dbus/org.chromium.VmCicerone.conf
[modify] https://crrev.com/a98cb102e16995c8ad3a630e66fd238575b4af06/vm_tools/vsh/vsh.cc
[modify] https://crrev.com/a98cb102e16995c8ad3a630e66fd238575b4af06/vm_tools/concierge/service.cc

Status: Verified (was: Assigned)
Closing this for now as we do run in the container. Some refactoring was done but no unittests... yet.

Verified on 10798.0.0

Sign in to add a comment