search

ARM-software / devlib

Library for interaction with and instrumentation of remote devices.
Apache License 2.0
47 stars 78 forks source link

Fix adb root #603

Closed douglas-raillard-arm closed 2 years ago

douglas-raillard-arm commented 2 years ago

This PR solves the freeze observed on some Pixel 6 root images when connecting to the target. The freeze appears to be related to some race condition in adb server when rooting/unrooting it.

Note that this PR needs a final round of validation on the actual device before being merged.

douglas-raillard-arm commented 2 years ago

@marcbonnici Updated with:

EDIT: there still seems to be some issues with the device, I'll report back once I have a go at debugging it. EDIT2: tested on the device, everything seems to work well

marcbonnici commented 2 years ago

I think this is unrelated to these changes but something that showed up during testing. If I create an AndroidTarget and then try to enable adb_root it will fail due to other connections being active (max_async + 1).

t = AndroidTarget()
t.conn.adb_root(True)
> AdbRootError: Can only restart adb server if no other connection is active
t.conn.active_connections
> (<unlocked _thread.lock object at 0x7f3bbde1e1e0>,
> defaultdict(int, {'emulator-5554': 51}))

It looks like once the connections are created during the benchmark stage they are not closed correctly. Do you see the same behavior on your setup?

douglas-raillard-arm commented 2 years ago

It looks like once the connections are created during the benchmark stage they are not closed correctly

It's on purpose: https://github.com/ARM-software/devlib/blob/492284f46dcb3b9a4633f6c284c76d36a20e8ba2/devlib/target.py#L446 Establishing a connection can be somewhat costly for e.g. ssh so that avoids re-doing that extra work, which would be needed anyway.

Wrt to adb_root(), I think it's an historical accident to have exposed that. It's really a property of the target and not any individual connection as it affects the whole server, as the "active connections" class attribute dict hack shows. On top of that, it's something that can only be safely done when the Target is created as it will interrupt brutally any sort of background command or other thread using the Target. Ideally, we should probably:

marcbonnici commented 2 years ago

Establishing a connection can be somewhat costly for e.g. ssh so that avoids re-doing that extra work, which would be needed anyway.

Right thanks for the reminder, that makes sense. I guess for ADB connections this is less important but best to keep the consistency if we can.

Wrt to adb_root(), I think it's an historical accident to have exposed that. It's really a property of the target and not any individual connection as it affects the whole server, as the "active connections" class attribute dict hack shows.

I think there's an argument for both sides here, as this is an ADB connection specific functionality rather than a target itself. The discrepancy is as you say this affects all connections rather than individual ones. Having said that I don't generally expect direct modifications to the connection itself, tbh I'd almost treat this as an internal method, as this bypassed the abstraction layer we're trying to present for runtime and therefore relies on users knowing what they're doing so I don't think this should be a problem (just confirming internally). I just wanted to ensure that this behaviour was expected.

Ideally, we should probably: deprecate it and force people to state what they want in Target ctor Only allow adb_root(enable=True/False) when it's a no-op wrt to what was setup when the Target was created, raise otherwise. Alternatively, turn adb_root() into a simple flag internally and use su or sudo to emulate it on a per-command basis (possibly a static build ?)

I'm not sure this would benefit us much in terms of usability, I think it might be better / provide a more consistent experience to just raise an error message, as it currently does. If we allow it as a no-op this could lead to inconsistent behaviour depending on the original state of the device and also we may not always have su etc. installed on the device so I think it would be best to inform the user of the issue explicitly and let them decide on the best way to solve the problem, i.e. ensure that this is set when the connection is first established.

douglas-raillard-arm commented 2 years ago

So I guess the rule is more or less that if someone instantiate a connection instance by hand, they are free to use adb_root() and it is somewhat a public API, but if the connection is managed of a Target, it becomes private. Ideally we would need a 3rd object on top of Target + Connection that would be managed by the Connection but attached to the Target. But since it's the only case where that is needed, the mutable class attr hack is probably good enough for now.

Using su to emulate adb root would probably open a whole can of worms anyway (is it even possible to install a binary without rooting the device in the first place ?).

marcbonnici commented 2 years ago

So I guess the rule is more or less that if someone instantiate a connection instance by hand, they are free to use adb_root() and it is somewhat a public API, but if the connection is managed of a Target, it becomes private. Ideally we would need a 3rd object on top of Target + Connection that would be managed by the Connection but attached to the Target. But since it's the only case where that is needed, the mutable class attr hack is probably good enough for now.

Agreed, that seems like a reasonable summary to me.

Using su to emulate adb root would probably open a whole can of worms anyway (is it even possible to install a binary without rooting the device in the first place ?).

Most definitely agree, for the majority of cases I think we'd be out of luck, however in any case I think that would be something that would need to be solved on a per device basis rather than something we could abstract reliably.