Unity 8

Merge lp:~mterry/unity8/unlock-script into lp:unity8

unlock-script
Merge into trunk

Proposed by Michael Terry on 2014-03-21

Status:	Merged
Approved by:	Michael Terry on 2014-04-08
Approved revision:	784
Merged at revision:	828
Proposed branch:	lp:~mterry/unity8/unlock-script
Merge into:	lp:unity8
Diff against target:	63 lines (+45/-0) 3 files modified debian/unity8-autopilot.install (+1/-0) tools/CMakeLists.txt (+2/-0) tools/unlock-device (+42/-0)
To merge this branch:	bzr merge lp:~mterry/unity8/unlock-script
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
PS Jenkins bot (community)	continuous-integration		Approve on 2014-04-07
Andy Doan (community)			Approve on 2014-04-07
Francis Ginther		2014-03-21	Needs Fixing on 2014-03-26
Review via email: mp+212170@code.launchpad.net

Commit message

Provide a all-in-one script for getting a device to an unlocked state.

Description of the change

Provide a all-in-one script for getting a device to an unlocked state.

Our current interface for letting jenkins scripts unlock the greeter is a python method designed to be called from inside the device. But once a split greeter lands, this will no longer be the right abstraction level. Unlocking will require lightdm config changes and a reboot driven from outside the device.

So what I'd like to do is start providing an all-in-one script that jenkins can start calling today. Then my split greeter branch can update the script to do the new correct things, and jenkins doesn't have to care about that change.

So I've just moved some code from mediumtests-runner-touch.sh into an 'unlock-device' script. We can land this; then I can change jenkins to call this new script.

== Checklist ==
* Are there any related MPs required for this MP to build/function as expected? Please list.
- No

* Did you perform an exploratory manual test run of your code change and any related functionality?
- Yes, ran the script manually for testing.

* If you changed the packaging (debian), did you subscribe the ubuntu-unity team to this MP?
- I'm in that team, and it was just to install the new file.

* If you changed the UI, has there been a design review?
- N/A

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-21:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2594/rebuild

review: Needs Fixing (continuous-integration)

lp:~mterry/unity8/unlock-script updated on 2014-03-24

781. By Michael Terry on 2014-03-24: Make script look prettier
782. By Michael Terry on 2014-03-24: Ship script in autopilot package

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-24:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2605/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-24:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2611/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Andy Doan (doanac) wrote on 2014-03-26:

Can you explain this a little more. I assume to use this you need to:

1) install unity8-autopilot on the host (due to how the script is written)
2) install unity8-autopilot on the target (due to the needs of the script)

or install unity8-autopilot on the target and copy this file to the host?

The script also deals with reboot/retry logic. Personally, I'd just rather the script be completely target based and let CI deal with reboot/retry ugliness. Just have your script exit 0 for success.

Revision history for this message

Francis Ginther (fginther) wrote on 2014-03-26:

For me, the really important part is the UNLOCK_SCRIPT, I think everything around it can be minimized and allow the other ci infrastructure to manage retries if necessary. So I recommend:

1) Remove the retry logic, just return 0 if the unlock is good, and non-zero otherwise.
2) There's a better way to specify the ANDROID_SERIAL that I found from lp:ubuntu-test-cases/touch

while getopts s: opt; do
        case $opt in
        s)
                export ANDROID_SERIAL=$OPTARG
                ;;
        esac
done

Then just remove the '-s $ANDROID_SERIAL' from the adb calls. If you want to provide some protection when there are multiple devices and a specific one isn't specified you can add:

if [ -z $ANDROID_SERIAL ] ; then
        # ensure we only have one device attached
        lines=$(adb devices | wc -l)
        if [ $lines -gt 3 ] ; then
                echo "ERROR: More than one device attached, please use -s option"
                echo
                usage
                exit 1
        fi
fi

usage() {
cat <<EOF
usage: $0 [-s ANDROID_SERIAL]

Unlocks unity8

OPTIONS:
-h Show this message
-s Specify the serial of the device to unlock

EOF
}

review: Needs Fixing

lp:~mterry/unity8/unlock-script updated on 2014-03-26

783. By Michael Terry on 2014-03-26: Review nits

Revision history for this message

Michael Terry (mterry) wrote on 2014-03-26:

@doanac: This script needs unity8-autopilot installed on target for sure. Whether it's installed on host or host just pulls this script from target is up to jenkins.

@fginther: OK, used your ANDROID_SERIAL trick.

@both: I've removed the retry logic, but under protest. The retry logic was presumably introduced due to the vagaries of using autopilot scripting. But that is an implementation detail of this script, not a universal constant. In fact, with the split greeter, we can get to an unlocked state without using autopilot. So no retries should be needed. But no biggie either way. Doesn't hurt to have extra retry logic sitting above this script.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-03-26:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2645/rebuild

review: Approve (continuous-integration)

Revision history for this message

Andy Doan (doanac) wrote on 2014-03-28:

I would still prefer that this be written purely for the target. ie just:

UNLOCK_SCRIPT='
import dbus, logging;
from unity8 import process_helpers as helpers;
logging.basicConfig(level=logging.INFO);
bus = dbus.SystemBus().get_object("com.canonical.powerd", "/com/canonical/powerd");
cookie = bus.requestSysState("unlock-device-hold", 1, dbus_interface="com.canonical.powerd");
helpers.restart_unity_with_testability();
bus.clearSysState(cookie, dbus_interface="com.canonical.powerd");
helpers.unlock_unity()
'

UNLOCK_OUTPUT=$(adb shell "sudo -u phablet -i python3 -c '$UNLOCK_SCRIPT'" 2>&1)
if echo "$UNLOCK_OUTPUT" | grep 'Greeter unlocked' >/dev/null; then
  echo "I: Unlock passed"
  exit 0
else
  echo "I: Unlock failed, script output: '$UNLOCK_OUTPUT'"
  exit 1
fi

You don't want to get into the business of knowing how to reboot a device. We've had to build our own script to make this reliable in CI:

http://bazaar.launchpad.net/~ubuntu-test-case-dev/ubuntu-test-cases/touch/view/head:/scripts/reboot-and-wait

adb-wait-for-device isn't nearly as friendly as you'd expect it to be. Additionally, we already reboot the device in CI to prep it, so we'd be adding an extra reboot for every test we run.

Revision history for this message

Francis Ginther (fginther) wrote on 2014-03-28:

Thanks for updating per the review comments.

I hadn't considered the reboot to be a concern in my first review, but I can understand Andy's comment. If you consider the reboot to be necessary for the common case, can you provide a means to disable it via an environment variable or option?

Revision history for this message

Michael Terry (mterry) wrote on 2014-03-31:

Regarding the reboot, adding the reboot to this script is actually the entire point of this merge. Two things:

1) This specific reboot to unlock is already in jenkins's scripts. I'm moving it to this script, and once this lands, we can remove the reboots from jenkins's scripts. So it's not a net increase of reboots. We're just shuffling them around.

2) The reason I want them in this script is because I believe this is the semantically correct place for them; the right abstraction level. I'm trying to prepare for the split greeter feature landing. When that happens, in order to unlock, we will *need* a reboot after making some LightDM config file changes (that is, this script needs to control when the reboot happens, not jenkins). But jenkins shouldn't know about such details. All that jenkins wants is a button to press that says "get my device to an unlocked state." And that's what this script provides. Everything inside this magic black box of a script is implementation details. And the reboot falls into that category.

This reasoning was also my motivation for having the retries in here instead of jenkins. But while I was flexible on that, it doesn't make sense to take the reboot out.

Revision history for this message

Michael Terry (mterry) wrote on 2014-03-31:

(And in case it isn't clear, the split greeter version of this script needs to make a config change, reboot, then delete its config change. So we need this script to be on the host, not the target.)

Revision history for this message

Andy Doan (doanac) wrote on 2014-03-31:

> Regarding the reboot, adding the reboot to this script is actually the entire
> point of this merge. Two things:
>
> 1) This specific reboot to unlock is already in jenkins's scripts. I'm moving
> it to this script, and once this lands, we can remove the reboots from
> jenkins's scripts. So it's not a net increase of reboots. We're just
> shuffling them around.

Thats fair enough. I'll still have one issue in daily image testing though. After we reboot the device we run a "system-settle" script before unlocking the screen. So with this new approach you are introducing, I need some type of hook around L50:

49 +adb "wait-for-device"
50 +sleep 20
51 +UNLOCK_OUTPUT=$(adb shell "sudo -u phablet -i python3 -c '$UNLOCK_SCRIPT'" 2>&1)

That lets us run system-settle and records if it failed or not.

Revision history for this message

Michael Terry (mterry) wrote on 2014-04-01:

As for system-settle, why do your jenkins scripts need to run it before unlocking? I'm guessing you worry about the system being too loaded to unlock with autopilot? We have the sleep and retries for that, right? Could you just system-settle after unlocking instead of before?

I'm just leery of making this unlock script anything more than a black box. That said, it wouldn't kill me to add a "--settle=command" argument. With a default of "sleep 20" I guess... I'll work on that.

lp:~mterry/unity8/unlock-script updated on 2014-04-01

784. By Michael Terry on 2014-04-01: Add -w argument to specify settle command

Revision history for this message

Andy Doan (doanac) wrote on 2014-04-01:

> As for system-settle, why do your jenkins scripts need to run it before
> unlocking? I'm guessing you worry about the system being too loaded to unlock
> with autopilot? We have the sleep and retries for that, right? Could you
> just system-settle after unlocking instead of before?

My worry is that if system-settle takes too long, the screen will wind up being locked.

> I'm just leery of making this unlock script anything more than a black box.
> That said, it wouldn't kill me to add a "--settle=command" argument. With a
> default of "sleep 20" I guess... I'll work on that.

I understand. We've built up a set of requirements over time that are a bit awkward.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-01:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2700/rebuild

review: Needs Fixing (continuous-integration)

Revision history for this message

Michael Terry (mterry) wrote on 2014-04-01:

OK, a -w argument has been added. Any other comments or does it look good?

Revision history for this message

Andy Doan (doanac) wrote on 2014-04-01:

I think its workable. I'm really not a fan of this handling the reboot logic as its going to require us to re-work our harness:

http://bazaar.launchpad.net/~ubuntu-test-case-dev/ubuntu-test-cases/touch/view/head:/scripts/run-autopilot-tests.sh

notice things like how we grab powerd locks which will now be broken due to the reboot. I understand your points, its just we've had unusual requirements forced on us. I'll need a few days before I can refactor our test harness and be sure things as they do in production.

Revision history for this message

Michael Terry (mterry) wrote on 2014-04-01:

Yeah, I know changing the side effects of unlocking is annoying, sorry.

Note that landing this doesn't mean you have to change tests in lockstep. The existing unlock support via python will continue to work.

But by landing this and then migrating jenkins to this script, everything will "just work" with the split greeter.

Revision history for this message

Andy Doan (doanac) on 2014-04-07:

review: Approve

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-04-07:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity8-ci/2744/rebuild

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Michael Terry

Mohammad Rakibul Hasan

Unity Team

 === modified file 'debian/unity8-autopilot.install'
 --- debian/unity8-autopilot.install	2014-01-17 06:50:33 +0000
 +++ debian/unity8-autopilot.install	2014-04-01 14:10:00 +0000
@@ -1,1 +1,2 @@
  usr/lib/python*/*/unity*
++usr/share/unity8/unlock-device
 === modified file 'tools/CMakeLists.txt'
 --- tools/CMakeLists.txt	2013-12-20 15:27:59 +0000
 +++ tools/CMakeLists.txt	2014-04-01 14:10:00 +0000
@@ -9,3 +9,5 @@
  install(TARGETS ${SCOPE_TOOL}
      RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
+     )
++
++install(FILES unlock-device DESTINATION ${SHELL_APP_DIR})
 === added file 'tools/unlock-device'
 --- tools/unlock-device	1970-01-01 00:00:00 +0000
 +++ tools/unlock-device	2014-04-01 14:10:00 +0000
@@ -0,0 +1,42 @@
++#!/bin/bash
++# -*- Mode: sh; indent-tabs-mode: nil; tab-width: 4 -*-
++
++# Our goal here is to get an Ubuntu Touch device into a testable unlocked state.
++# This will include a reboot.
++
++WAIT_COMMAND="sleep 20"
++
++while getopts s:w: opt; do
++        case $opt in
++        s)
++                export ANDROID_SERIAL=$OPTARG
++                ;;
++        w)
++                WAIT_COMMAND=$OPTARG
++                ;;
++        esac
++done
++
++UNLOCK_SCRIPT='
++import dbus, logging;
++from unity8 import process_helpers as helpers;
++logging.basicConfig(level=logging.INFO);
++bus = dbus.SystemBus().get_object("com.canonical.powerd", "/com/canonical/powerd");
++cookie = bus.requestSysState("unlock-device-hold", 1, dbus_interface="com.canonical.powerd");
++helpers.restart_unity_with_testability();
++bus.clearSysState(cookie, dbus_interface="com.canonical.powerd");
++helpers.unlock_unity()
++'
++
++adb reboot
++sleep 5
++adb "wait-for-device"
++eval "$WAIT_COMMAND"
++UNLOCK_OUTPUT=$(adb shell "sudo -u phablet -i python3 -c '$UNLOCK_SCRIPT'" 2>&1)
++if echo "$UNLOCK_OUTPUT" | grep 'Greeter unlocked' >/dev/null; then
++    echo "I: Unlock passed"
++    exit 0
++else
++    echo "I: Unlock failed, script output: '$UNLOCK_OUTPUT'"
++    exit 1
++fi