qemuvariants: fix regression in GCP tarball format #1088

darkmuggle · 2020-01-30T22:59:12Z

GCP is picky about the tarball contents. They must:

be in a tarball with gzip compression
use the Old GNU format
be sized correctly
members must be sized right

With the new method of generating qemu variant images, CLI tar commands
were replaced with pythonic methods; this produces invalid tarballs from
GCP's perspective. To support GCP, the method of tarball creation is
reverted to using the CLI.

darkmuggle · 2020-01-30T23:08:54Z

/cc @miabbott Fix for https://bugzilla.redhat.com/show_bug.cgi?id=1796632

cgwalters · 2020-01-31T14:26:38Z

First let's try to get to a common style here. The general style here before was to have the "topic prefix" not necessarily be the full file path. In this case it actually touches two files.

Maybe just qemuvariants?

Style is also to avoid having a period . at the end.

This style is pretty much the same as the Linux kernel.

IOW, something like:

qemuvariants: Ensure GCP tarball is sparse

GCP is picky about the tarball contents. They must:

- be in a tarball with gzip compression
- use the Old GNU format
- be sized correctly
- members must be sized right

With the new method of generating qemu variant images, CLI tar commands
were replaced with pythonic methods; this produces invalid tarballs from
GCP's perspective. To support GCP, the method of tarball creation is
reverted to using the CLI.

?

cgwalters

If you've tested this I'm OK with it but some optional bits.

cgwalters · 2020-01-31T14:40:07Z

src/cosalib/qemuvariants.py

 BASEARCH = get_basearch()

+# Default flags for the creation of tarfiles
+DEFAULT_TAR_FLAGS = '-Schgh'


Expanding the comment with why we are picking these flags would be useful.

We always want -c, so it seems better to provide that automatically in the

Minor: h is there twice.

cgwalters · 2020-01-31T14:45:36Z

src/cosalib/qemuvariants.py

-                    tar.add(te, arcname=te_name)
+            # Python does not create sparse Tarfiles, so we have do it via
+            # the CLI here.
+            log.info(f"Preparing tarball {final_img} with flags '{self.tar_flags}'")


Log seems unnecessary since we print the tar command?

Given the level of logging that we already have, the extra logging seen innocuous.

Removed.

cgwalters · 2020-01-31T14:49:12Z

src/cosalib/qemuvariants.py

+            base_name = self.tar_members.pop(0)
+            tarlist = [base_name]
+            if base_name != os.path.basename(work_img):
+                copy_sparse(work_img, os.path.join(self._tmpdir, base_name))


I don't understand the circumstances in which this condition fires. Is it something like enforcing a naming scheme for the file inside the tarball? Wouldn't it be a lot cheaper to just make a symlink in that case, since we're passing h to the tar invocation?

Actually at a higher level, it seems like it'd be a lot simpler to me if the builder implementations that used tar_files just each directly called tar as they need to.

Today only gcp and vmware use this AFAICS.

This PR fixes the issue today. I can look at refactoring this out later. The original reason why I put the Tarball functionality here was because of the distaste for OOO.

Also, the latest push includes a comment about when the condition would happen.

Hmm, I don't think we need cp --sparse=always anymore with #1097. The output of qemu-img convert is already sparse. Seems like here we just want os.link() if it's to have the filenames match up? That makes for less IO too. :)

miabbott · 2020-02-03T15:19:05Z

Could you amend the commit message to include the BZ, too?

miabbott · 2020-02-03T16:10:01Z

src/cosalib/qemuvariants.py

+# Default flags for the creation of tarfiles
+# The default flags were selected because:
+#   -S: always create a sparse file
+#   -c: create a new tarball
+#   -h: derefence symlinks
+#   -g: handle incremental backups.
+# These flags were selected from prior commits for
+# tarball creation.


cgwalters

Commit message still has a trailing . (also why is "Tarball" capitalized?)

jlebon · 2020-02-03T21:29:30Z

Hmm, from looking at the Jenkins logs, it seems like tar is outputting to stdout? https://jenkins-projectatomic-ci.apps.ci.centos.org/job/coreos-assembler/job/PR-1088/6/console

darkmuggle · 2020-02-04T03:20:24Z

@jlebon it looks like we're running out of space? Any chance you can merge #1097?

``` Colin Walters (3): network: Honor MANTLE_SSH_DIR kola: Note qemu-unpriv now has networking kola: Drop torcx tests Jakub Čajka (1): kola: initial ppc64le support Stephen Lowrie (15): vendor: add aliyun related dependencies network/*ssh: Add HostKeyCallback to ClientConfig auth: add aliyun auth provider platform/api/aliyun: add the aliyun platform cmd/ore/aliyun: add image creation related aliyun calls docs: add Aliyun platform documentation Merge pull request coreos#1066 from arithx/aliyun Merge pull request coreos#1081 from cgwalters/ssh-agent-tmpdir Merge pull request coreos#1086 from cgwalters/platform-qemu-unpriv-net README: update qemu-unpriv notes Merge pull request coreos#1087 from cgwalters/kola-prune-cl kola/tests: disable additional clustered tests on qemu-unpriv Merge pull request coreos#1090 from arithx/disable_tests_on_unpriv Merge pull request coreos#1088 from arithx/unpriv_readme Merge pull request coreos#1067 from jcajka/ppc64le Stephen Milner (1): Merge pull request coreos#1070 from ashcrow/kola-crio-restart-test Steve Milner (1): kola/tests/crio/crio: Add pods and service restart test ```

darkmuggle · 2020-02-04T12:43:03Z

Okay, it passed when I can changed the location of self._tmpdir to be in the workdir. This can be merged, or we can wait for @jlebon's #1097

src/cosalib/build.py

Fixes BZ #1796632. GCP is picky about the tarball contents. They must: - be in a tarball with gzip compression - use the Old GNU format - be sized correctly - members must be sized right With the new method of generating qemu variant images, CLI tar commands were replaced with pythonic methods; this produces invalid tarballs from GCP's perspective. To support GCP, the method of tarball creation is reverted to using the CLI.

openshift-ci-robot · 2020-02-04T18:51:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, darkmuggle

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [cgwalters,darkmuggle]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

This bump brings in the change to the default kernel cmdline to use `ip=dhcp,dhcp6`, which is required for proper bootstrapping of RHCOS in an IPv6 environment. (See BZ#1793591) This also brings in the RHEL 8.1.1 content and a fix for properly generating GCP images (coreos/coreos-assembler#1088). [0] https://bugzilla.redhat.com/show_bug.cgi?id=1793591 Signed-off-by: Micah Abbott <[email protected]>

README: update qemu-unpriv notes

cgwalters reviewed Jan 31, 2020

View reviewed changes

miabbott reviewed Feb 3, 2020

View reviewed changes

cgwalters approved these changes Feb 3, 2020

View reviewed changes

cgwalters changed the title ~~cosalib/qemuvariants.py: fix regression in GCP Tarball format.~~ qemuvariants: fix regression in GCP tarball format Feb 4, 2020

cgwalters reviewed Feb 4, 2020

View reviewed changes

src/cosalib/build.py Outdated Show resolved Hide resolved

cgwalters approved these changes Feb 4, 2020

View reviewed changes

cgwalters merged commit 30fbac4 into coreos:master Feb 4, 2020

miabbott mentioned this pull request Feb 5, 2020

Bug 1793591: use ip=dhcp,dhcp6 by default on kernel command line openshift/installer#3062

Merged

darkmuggle deleted the pr/fix_gcp branch February 18, 2020 21:48

jcajka pushed a commit to jcajka/coreos-assembler that referenced this pull request Mar 24, 2020

Merge pull request coreos#1088 from arithx/unpriv_readme

71d5d00

README: update qemu-unpriv notes

qemuvariants: fix regression in GCP tarball format #1088

qemuvariants: fix regression in GCP tarball format #1088

Uh oh!

Conversation

darkmuggle commented Jan 30, 2020

Uh oh!

darkmuggle commented Jan 30, 2020

Uh oh!

cgwalters commented Jan 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

cgwalters Jan 31, 2020

Choose a reason for hiding this comment

Uh oh!

cgwalters Jan 31, 2020

Choose a reason for hiding this comment

Uh oh!

darkmuggle Feb 3, 2020

Choose a reason for hiding this comment

Uh oh!

cgwalters Jan 31, 2020

Choose a reason for hiding this comment

Uh oh!

darkmuggle Feb 3, 2020

Choose a reason for hiding this comment

Uh oh!

darkmuggle Feb 3, 2020

Choose a reason for hiding this comment

Uh oh!

jlebon Feb 4, 2020

Choose a reason for hiding this comment

Uh oh!

miabbott commented Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miabbott Feb 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cgwalters left a comment

Choose a reason for hiding this comment

Uh oh!

jlebon commented Feb 3, 2020

Uh oh!

darkmuggle commented Feb 4, 2020

Uh oh!

darkmuggle commented Feb 4, 2020

Uh oh!

Uh oh!

openshift-ci-robot commented Feb 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cgwalters commented Jan 31, 2020 •

edited

Loading

miabbott commented Feb 3, 2020 •

edited

Loading

miabbott Feb 3, 2020 •

edited

Loading