New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 806613 link

Starred by 2 users

Issue metadata

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

Blocked on:
issue 808495



Sign in to add a comment

canaries failing due to lack backport-lzma build

Project Member Reported by ahass...@chromium.org, Jan 28 2018

Issue description

canaries are failing with this error:

Traceback (most recent call last):
  File "/b/c/cbuild/repository/chromite/lib/parallel.py", line 603, in TaskRunner
    task(*x, **task_kwargs)
  File "/b/c/cbuild/repository/chromite/lib/parallel.py", line 801, in <lambda>
    fn = lambda idx, task_args: out_queue.put((idx, task(*task_args)))
  File "/b/c/cbuild/repository/chromite/lib/paygen/paygen_build_lib.py", line 244, in _GenerateSinglePayload
    dry_run=dry_run)
  File "/b/c/cbuild/repository/chromite/lib/paygen/paygen_payload_lib.py", line 830, in CreateAndUploadPayload
    dry_run=dry_run).Run()
  File "/b/c/cbuild/repository/chromite/lib/paygen/paygen_payload_lib.py", line 710, in Run
    self._drm(self._VerifyPayload)
  File "/b/c/cbuild/repository/chromite/lib/paygen/dryrun_lib.py", line 46, in __call__
    return self.Run(func, *args, **kwargs)
  File "/b/c/cbuild/repository/chromite/lib/paygen/dryrun_lib.py", line 83, in Run
    return self._Call(func, *args, **kwargs)
  File "/b/c/cbuild/repository/chromite/lib/paygen/dryrun_lib.py", line 87, in _Call
    return func(*args, **kwargs)
  File "/b/c/cbuild/repository/chromite/lib/paygen/paygen_payload_lib.py", line 684, in _VerifyPayload
    self._ApplyPayload(payload, is_delta)
  File "/b/c/cbuild/repository/chromite/lib/paygen/paygen_payload_lib.py", line 644, in _ApplyPayload
    **part_files)
  File "/b/c/cbuild/repository/src/aosp/system/update_engine/scripts/update_payload/payload.py", line 320, in Apply
    old_rootfs_part=old_rootfs_part)
  File "/b/c/cbuild/repository/src/aosp/system/update_engine/scripts/update_payload/applier.py", line 647, in Run
    self.payload.manifest.old_rootfs_info)
  File "/b/c/cbuild/repository/src/aosp/system/update_engine/scripts/update_payload/applier.py", line 595, in _ApplyToPartition
    new_part_file, new_part_info.size)
  File "/b/c/cbuild/repository/src/aosp/system/update_engine/scripts/update_payload/applier.py", line 531, in _ApplyOperations
    self._ApplyReplaceOperation(op, op_name, data, new_part_file, part_size)
  File "/b/c/cbuild/repository/src/aosp/system/update_engine/scripts/update_payload/applier.py", line 254, in _ApplyReplaceOperation
    out_data = lzma.decompress(out_data)
NameError: global name 'lzma' is not defined


The problem is that backports-lzma is not imported properly. This is is cased because chromium-os-sdk for some reason is not adding this dependency even if it is added in:
https://chromium.git.corp.google.com/chromiumos/overlays/chromiumos-overlay/+/1697f7bf464a33b649154ce315f1874739116c63/virtual/target-chromium-os-sdk/target-chromium-os-sdk-1.ebuild#483

 
Cc: vapier@chromium.org
@vapier: Any idea why the canaries sdk do not pick up backports-lzma?

Comment 2 by vapier@chromium.org, Jan 28 2018

that code is run outside of the sdk.  note the /b/c/cbuild/ prefix on paths.
So, do you know off hand how to add that dependency to outside of sdk?!!! Does that even makes sense? 

Comment 4 by vapier@chromium.org, Jan 28 2018

Cc: pprabhu@chromium.org nxia@chromium.org dgarr...@chromium.org
it's possible, but typically the infra guys want to avoid that
Well, lzma is a needed feature and one way or another we have to get it to work. I can temporary get the canaries green by reverting crrev.com/c/872125 if adding that dependency requires a push (which may take a few days to do).
but I'll let the infra guys decide if they can add this dependency or not. This only needed for python 2.7.
uploaded crrev.com/c/890384 to temporarily fix the issue.
Project Member

Comment 7 by bugdroid1@chromium.org, Jan 29 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/aosp/platform/system/update_engine/+/cb2da0df4382f546efd4269851ce814842a4b7fd

commit cb2da0df4382f546efd4269851ce814842a4b7fd
Author: Amin Hassani <ahassani@google.com>
Date: Mon Jan 29 03:09:19 2018

update_engine: Temporarily disable XZ generation

Disable generating XZ operations until  crbug.com/806613  is fixed.

BUG= chromium:806613 
TEST=unit tests pass

Change-Id: I64b9852b447f7ff1f1a45f10205e3b23a2591cca
Reviewed-on: https://chromium-review.googlesource.com/890384
Commit-Ready: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>

[modify] https://crrev.com/cb2da0df4382f546efd4269851ce814842a4b7fd/payload_generator/delta_diff_utils.cc

Comment 8 by vapier@chromium.org, Jan 29 2018

you can have cbuildbot execute commands inside of the chroot instead on a per-command basis
Cc: athilenius@chromium.org jclinton@chromium.org la...@chromium.org
Might be related to:
https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/875260

Adding the corresponding peo;e

This isn't related to https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/875260 . All that CL does is explicitly avoid building backports-lzma for Python versions >3 because lzma is already included in Python >3. We still build it for 2.7. Since 2.7 is being used here, that CL had no effective delta.
that CL also only affects code inside the SDK while this code/crash is happening outside the SDK
I think there is a reason we are running the code outside of chroot. dgarrentt@ might have more background on why. But my initial guess is that we gather all the files related to payload generation into a zip file and running them where ever necessary without being dependent on where the code is running or if the chroot is available. For example I think, and I'm probably wrong, the tryjob-payload builders do not necessarily build the sdk and chroot. Other reason could be that somehow omaha uses the zip file contents to create signed delta payloads? In any case if we run this inside chroot, then the dependencies of delta_generator can be mismatched with the ones in chroot. right?

Ugh: https://cs.corp.google.com/chromeos_public/chromite/lib/paygen/paygen_payload_lib.py?q=CreateAndUploadPayload&l=33

paygen is importing and using python modules from update_engine. The right fix would be to figure out why we want to do this outside of chroot (there's actually an --outside-chroot argument in one of the calls below)
@pprabhu: look at #12

Comment 15 by de...@google.com, Jan 30 2018

The au-generator.zip has all the binaries and libs to *generate* a payload, which doesn't use python at all. The python part is used to verify the payload against the spec in python and by the builder Paygen stage.

The reason why the generator binraies run outside the chroot is that the chroot is at a potentially different version than the au-generator you are using, so having lib or executable dependencies into the chroot is a problem more than a solution.

It used to be the case that we would run very old au-generator.zip files, nowadays we only run a somewhat old, probably the same version you just built like with canaries, but in other channels you sometimes generate deltas from older versions after the fact.

Regarding the python verifier (paycheck.py) and its python dependencies, I don't think it is packaged with the au-generator, they come from the builder manifest (I think update_engine was added at some point to have the scripts there). So if you have a python dependency from paycheck you could either add the dependency to the chroot and run just paycheck inside the chroot as a subprocess (which is a totally different flow and might bring more problems), or somehow add that dependency to the builder, together with all the other python modules the builder is required to have. Yeah, both options look problematic =/
What is the main problem with adding that dependency to the builders? lzma-backports is only a python wrapper for lzma. I don't think we need dependency to lzma itself (probably builders have it as part of XZ libraries), but just the wrapper.
Re #12, the tryjob-payload builders don't build anything, they only download build artifacts from a previous build an use them.

Downloading au-generator.zip was originally because Paygen ran on dedicated servers, not as part of the build. When paygen was merged into release builders, it was convenient to keep downloading for the sake of the tryjobs.

Re #15 We used to use the oldest version of au-generator.zip for generating deltas to ensure compatibility with the build that had to apply the delta, but that's no longer true.

We now use major/minor versions to handle compatibility issues, and the current generator binaries are supposed to "do the right thing" for old version numbers. This lets us fix bugs in the generators without all of the shenanigans we used to have to pull.

You can see most of the logic around selecting/using au_generator.zip in chromite/lib/paygen/paygen_payload_lib.py in _PrepareGenerator, and _RunGeneratorCmd.
Running paycheck inside the chroot is probably oka, but might mean some tweaking of paygen tryjobs is needed.

Paycheck was originally intended for stand alone use, but that's unimportant now. We haven't seen a corrupted payload file in years, and checksums are being emitted into the json files describing the payloads.

You could get rid of au-generator.zip altogether, and run everything inside the chroot, if you can come up with a reasonable solution for the tryjobs.

So are you suggesting we move away from au-generator.zip and generate/paycheck everything in the chroot? 
What is the best path forward here? We need this feature to get going, since we need to for UE major version upgrade too.
Sorry I didn't see your last comment. Where can I look at the tryjob configs to figure things out?
Here is the custom builder class used for paygen tryjobs:

chromite/cbuildbot/builders/release_builders.py: GeneratePayloadsBuilder

Looks like it doesn't create a chroot, or do much of anything other than run paygen in parallel for every requested channel.

Here is an example of a custom builder that goes through  the normal build stages:
  test_builder.py: UnittestStressBuilder

Blockedon: 808495
re-comment #16: it isn't a problem with this specific package, it's a problem with adding any package.  the c-i-t manages the OS images and they've been pushing back in general on doing any customization because it's painful for them to make sure everything stays in sync.  they'd rather only rely on the base image and let clients (like us) deal with uncommon deps ourselves.

if we can run everything inside the sdk, then that'd be their preferred answer.
Yeah, sure. I'm in the process of it. for an initial plan look  crbug.com/808495 
Project Member

Comment 26 by bugdroid1@chromium.org, Mar 8 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/aosp/platform/system/update_engine/+/f7006be5079ed91c93223273f3ddb69d655ba858

commit f7006be5079ed91c93223273f3ddb69d655ba858
Author: Amin Hassani <ahassani@chromium.org>
Date: Thu Mar 08 03:47:06 2018

Revert "update_engine: Temporarily disable XZ generation"

This reverts commit cb2da0df4382f546efd4269851ce814842a4b7fd.

Reason for revert: <paygens are running in chroot now sow lzma dependency should be fixed by now>

BUG= chromium:806613 
TEST=unittest

Original change's description:
> update_engine: Temporarily disable XZ generation
>
> Disable generating XZ operations until  crbug.com/806613  is fixed.
>
> BUG= chromium:806613 
> TEST=unit tests pass
>
> Change-Id: I64b9852b447f7ff1f1a45f10205e3b23a2591cca
> Reviewed-on: https://chromium-review.googlesource.com/890384
> Commit-Ready: Amin Hassani <ahassani@chromium.org>
> Tested-by: Amin Hassani <ahassani@chromium.org>
> Reviewed-by: Amin Hassani <ahassani@chromium.org>

TBR=benchan@chromium.org,senj@chromium.org,ahassani@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug:  chromium:806613 
Change-Id: Ic918b7e4febf284f38b2ca3119c1fcbf57538a89
Reviewed-on: https://chromium-review.googlesource.com/952378
Commit-Ready: Amin Hassani <ahassani@chromium.org>
Tested-by: Amin Hassani <ahassani@chromium.org>
Reviewed-by: Amin Hassani <ahassani@chromium.org>

[modify] https://crrev.com/f7006be5079ed91c93223273f3ddb69d655ba858/payload_generator/delta_diff_utils.cc

Status: Fixed (was: Untriaged)

Sign in to add a comment