Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
246353004:
Remove files in /var/lib/whitelist before attempting login with telemetry. (Closed)
|
Created:
6 years, 8 months ago by achuithb Modified:
6 years, 7 months ago CC:
chromium-reviews, telemetry+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionRemove files in /var/lib/whitelist before attempting login with telemetry.
BUG=358728
TEST=Manual
NOTRY=True
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266051
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266619
Patch Set 1 #
Total comments: 4
Patch Set 2 : Fixes #Patch Set 3 : Add an option #Patch Set 4 : Default false #
Messages
Total messages: 17 (0 generated)
Bartosz - this should be safe right?
https://codereview.chromium.org/246353004/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/backends/chrome/cros_browser_backend.py (right): https://codereview.chromium.org/246353004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/cros_browser_backend.py:66: self._cri.RmRF('/var/lib/whitelist/*') Note that this only takes effect if session_manager is stopped when you clear the directory - is that guaranteed here?
On 2014/04/23 00:25:32, achuithb wrote: > Bartosz - this should be safe right? Hold off on reviewing this. I'm going to stop the ui before clearing these files and Local State.
I'll post a fix tomorrow https://codereview.chromium.org/246353004/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/backends/chrome/cros_browser_backend.py (right): https://codereview.chromium.org/246353004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/cros_browser_backend.py:66: self._cri.RmRF('/var/lib/whitelist/*') On 2014/04/23 07:21:22, Mattias Nissler wrote: > Note that this only takes effect if session_manager is stopped when you clear > the directory - is that guaranteed here? Nope, I realized I wasn't doing this right looking at https://chromium-review.googlesource.com/#/c/194143/
https://codereview.chromium.org/246353004/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/backends/chrome/cros_browser_backend.py (right): https://codereview.chromium.org/246353004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/cros_browser_backend.py:66: self._cri.RmRF('/var/lib/whitelist/*') On 2014/04/23 07:26:52, achuithb wrote: > On 2014/04/23 07:21:22, Mattias Nissler wrote: > > Note that this only takes effect if session_manager is stopped when you clear > > the directory - is that guaranteed here? > > Nope, I realized I wasn't doing this right looking at > https://chromium-review.googlesource.com/#/c/194143/ Yes, this is why we have methods in autotest that handle doing this correctly, in client/cros/ownership.py You should probably be calling that code.
On 2014/04/23 14:54:57, Chris Masone wrote: > https://codereview.chromium.org/246353004/diff/1/tools/telemetry/telemetry/co... > File tools/telemetry/telemetry/core/backends/chrome/cros_browser_backend.py > (right): > > https://codereview.chromium.org/246353004/diff/1/tools/telemetry/telemetry/co... > tools/telemetry/telemetry/core/backends/chrome/cros_browser_backend.py:66: > self._cri.RmRF('/var/lib/whitelist/*') > On 2014/04/23 07:26:52, achuithb wrote: > > On 2014/04/23 07:21:22, Mattias Nissler wrote: > > > Note that this only takes effect if session_manager is stopped when you > clear > > > the directory - is that guaranteed here? > > > > Nope, I realized I wasn't doing this right looking at > > https://chromium-review.googlesource.com/#/c/194143/ > > Yes, this is why we have methods in autotest that handle doing this correctly, > in client/cros/ownership.py > > You should probably be calling that code. Unfortunately the code duplication can't be avoided since this code is in the chromium repository.
This should be now fixed. Mattias, PTAL. https://codereview.chromium.org/246353004/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/backends/chrome/cros_browser_backend.py (right): https://codereview.chromium.org/246353004/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/backends/chrome/cros_browser_backend.py:66: self._cri.RmRF('/var/lib/whitelist/*') On 2014/04/23 07:21:22, Mattias Nissler wrote: > Note that this only takes effect if session_manager is stopped when you clear > the directory - is that guaranteed here? Done.
On 2014/04/23 22:14:13, achuithb wrote: > This should be now fixed. Mattias, PTAL. > > https://codereview.chromium.org/246353004/diff/1/tools/telemetry/telemetry/co... > File tools/telemetry/telemetry/core/backends/chrome/cros_browser_backend.py > (right): > > https://codereview.chromium.org/246353004/diff/1/tools/telemetry/telemetry/co... > tools/telemetry/telemetry/core/backends/chrome/cros_browser_backend.py:66: > self._cri.RmRF('/var/lib/whitelist/*') > On 2014/04/23 07:21:22, Mattias Nissler wrote: > > Note that this only takes effect if session_manager is stopped when you clear > > the directory - is that guaranteed here? > > Done. So, what is your plan for dealing with changes in this cleanup code? I made some about a week ago, so it does happen. How will you deal with the silent regressions that will result?
On 2014/04/23 22:17:44, Chris Masone wrote: > On 2014/04/23 22:14:13, achuithb wrote: > > This should be now fixed. Mattias, PTAL. > > > > > https://codereview.chromium.org/246353004/diff/1/tools/telemetry/telemetry/co... > > File tools/telemetry/telemetry/core/backends/chrome/cros_browser_backend.py > > (right): > > > > > https://codereview.chromium.org/246353004/diff/1/tools/telemetry/telemetry/co... > > tools/telemetry/telemetry/core/backends/chrome/cros_browser_backend.py:66: > > self._cri.RmRF('/var/lib/whitelist/*') > > On 2014/04/23 07:21:22, Mattias Nissler wrote: > > > Note that this only takes effect if session_manager is stopped when you > clear > > > the directory - is that guaranteed here? > > > > Done. > > So, what is your plan for dealing with changes in this cleanup code? I made some > about a week ago, so it does happen. How will you deal with the silent > regressions that will result? I don't know if we have a reasonable choice other than to fix these in both places. We have similar issues with cryptohome - we want to know when the cryptohome is mounted but can't use the cryptohome library. We could pull some of the cryptohome/ownership libraries into chrome and export them to cros (note that this telemetry code may run on a linux dev server over ssh, not just locally on the DUT). We could make some shared 3rd party repo that can be used both by telemetry and cros, or we could try to call python libraries remotely (which assumes that the DUT has a test image - telemetry currently works with a dev image). All of these solutions are going to make various stakeholders unhappy and are pretty extreme. The amount of duplicated code is still small enough that such extreme measures to force the sharing seem unwarranted. If you have a simple solution that I haven't thought of, I'll be happy to take a look!
I don't know telemetry well enough to approve this, but my concern is resolved now.
lgtm
The CQ bit was checked by achuith@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/achuith@chromium.org/246353004/40001
Message was sent while issue was closed.
Change committed as 266051
The CQ bit was checked by achuith@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/achuith@chromium.org/246353004/80001
Message was sent while issue was closed.
Change committed as 266619 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
