Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove anaconda #556

Merged
merged 5 commits into from
Jun 20, 2019
Merged

Remove anaconda #556

merged 5 commits into from
Jun 20, 2019

Conversation

ajeddeloh
Copy link
Contributor

  • Only for qemu right now. Trying to make sense of the buildextend-metal, it looks like it's running anaconda again?
  • Only tested with fcos
  • We ought to not use yaml for image.yaml, it's a pain to parse
  • Doesn't actually remove anaconda, just switches to not using it

@ajeddeloh ajeddeloh mentioned this pull request Jun 14, 2019
Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty easy; shouldn't be too hard to extend to metal-uefi I'd think. Will play with this too.

src/create_disk.sh Show resolved Hide resolved
# init the ostree
ostree admin init-fs rootfs
ostree pull-local "$ostree" --repo rootfs/ostree/repo
ostree admin os-init fedora-coreos --sysroot rootfs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's continue templating this from the manifest.

@cgwalters
Copy link
Member

We ought to not use yaml for image.yaml, it's a pain to parse

From shell script yes but Python is good for that right?

@ajeddeloh
Copy link
Contributor Author

From shell script yes but Python is good for that right?

Misc idea: re-render to a file in tmp it as json using python. Then bash utils can use jq and python can use the native python implementation.

@ajeddeloh
Copy link
Contributor Author

Updated so we build bios+uefi images now.

ostree admin init-fs rootfs
ostree pull-local "$ostree" --repo rootfs/ostree/repo
ostree admin os-init fedora-coreos --sysroot rootfs
kargs='root=/dev/disk/by-label/root console=ttyS0,115200n8 console=ttyS0 rootflags=defaults,prjquota rw $ignition_firstboot'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move the middle part of this to two variables ($consoleflags and $rootflags) that can be overridden via environment?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also we need to use VM_TERMINAL instead of ttyS0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lucab what environment precisely? This is run in supermin, so the environment is pretty sparse.

The kargs currently live in a kickstart. Seems like they ought to live in image.yaml?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summarizing some discussion on #fedora-coreos: We're going to keep the kargs that relate to this script in this script but move the other kargs to cmd-build since we can do thing like templating (for things like $VM_TERMINAL there since it's not in supermin. We may move them later, but that's for another PR.

@cgwalters
Copy link
Member

Misc idea: re-render to a file in tmp it as json using python. Then bash utils can use jq and python can use the native python implementation.

Sure, fine by me.

@ajeddeloh
Copy link
Contributor Author

I'll deal with doing the yaml->json re-rendering later in another PR. Doesn't need to happen in this one.

src/cmd-build Outdated
build_image() {
local size
size="$(python3 -c 'import sys, yaml; print(yaml.load(sys.stdin)["size"])' < "$configdir/image.yaml")G"
qemu-img create -f qcow2 "$workdir/diskimage.qcow2" "$size"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of $workdir this should be in the build tmpdir $(pwd) (which would require passing it as an argument to runvm for cleanliness)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case might as well pass the base image path directly to runvm, right?

@ajeddeloh
Copy link
Contributor Author

Addressed things besides the mutli-arch bit, waiting to learn a bit more about ppc64le on that.

Do we need to support arm64 on non-uefi platforms?

@r4f4
Copy link
Collaborator

r4f4 commented Jun 19, 2019

No, aarch64 is EFI-only.

src/cmd-build Outdated Show resolved Hide resolved
src/create_disk.sh Show resolved Hide resolved
@ashcrow
Copy link
Member

ashcrow commented Jun 19, 2019

cc @darkmuggle @miabbott @imcleod

@ajeddeloh
Copy link
Contributor Author

ajeddeloh commented Jun 19, 2019

Added a check so we fall back to anaconda on non-x86_64 arches. Addressed @jlebon's comments.

Also worth calling out we need coreos/fedora-coreos-config#105 to not get failed units.

Do we want to handle buildextend-metal here or in another PR?
How important is it to use the size estimation for that?
Does it make sense to use the size estimation for qemu as well?

If we do want to continue creating the small images for metal but do not for qemu then the buildextend-metal will need to recreate the image entirely since we can't shrink xfs filesystems.

@cgwalters
Copy link
Member

Added a check so we fall back to anaconda on non-x86_64 arches.

As a short term that's probably OK, will make it easier to test without disrupting the multi-arch porting in progress.

How important is it to use the size estimation for that?

If we don't we're back in the guessing game of how big the disk should be...

Does it make sense to use the size estimation for qemu as well?

No, that's where size from image.yaml comes in.

@jlebon
Copy link
Member

jlebon commented Jun 19, 2019

Do we want to handle buildextend-metal here or in another PR?

I'm assuming buildextend-metal still works fine after this, right? If so, then yeah, I don't see the harm in doing it as a follow-up!

@ajeddeloh
Copy link
Contributor Author

Right now cmd-buildextend-metal recreates entirely new images (it has to, the partition and FS are smaller). so right now the BM images are still created using anaconda.

So it sounds like this should not wait for the buildextend-metal (be and qemu only for the moment) and I'll follow up with the corresponding cmd-buildextend-metal changes.

Is cmd-buildextend-metal expected to work with non-x86 images? It hardcodes BIOS and UEFI options.

@cgwalters
Copy link
Member

So it sounds like this should not wait for the buildextend-metal (be and qemu only for the moment) and I'll follow up with the corresponding cmd-buildextend-metal changes.

I'm also OK with that.

@ajeddeloh
Copy link
Contributor Author

Ok, this should be ready for more review then. Also gated the changes to gf-platformid to only take effect on x86_64 as well.

src/gf-platformid Outdated Show resolved Hide resolved
@jlebon
Copy link
Member

jlebon commented Jun 20, 2019

This looks good to me! (Though we can probably squash 7ef9087 into ef0450c, right? Edit: and 4e490eb into 7640c90)
Let's get coreos/fedora-coreos-config#105 in first.

@ajeddeloh
Copy link
Contributor Author

I think I want to leave the latter set of commits separate. Makes it clear what changes for UEFI vs removing anaconda in general.

@jlebon
Copy link
Member

jlebon commented Jun 20, 2019

Testing this locally (privileged), I get:

+ mount /dev/sdb1 /srv/fcos/cache
mount: /srv/fcos/cache: special device /dev/sdb1 does not exist.
+ chown -h -R jlebon:jlebon /srv/fcos
chown: changing ownership of '/srv/fcos/cache/pkgcache-repo/config': Operation not permitted
chown: changing ownership of '/srv/fcos/cache/pkgcache-repo/.lock': Operation not permitted
chown: changing ownership of '/srv/fcos/cache/pkgcache-repo/tmp/cache': Operation not permitted
chown: changing ownership of '/srv/fcos/cache/pkgcache-repo/tmp': Operation not permitted
chown: changing ownership of '/srv/fcos/cache/pkgcache-repo/extensions': Operation not permitted
chown: changing ownership of '/srv/fcos/cache/pkgcache-repo/state': Operation not permitted
chown: changing ownership of '/srv/fcos/cache/pkgcache-repo/refs/heads/rpmostree/pkg/openssl-libs/1_3A1.1.1-3.fc29.x86__64': Operation not permitted
...

So there are two issues here. The first is #560 (comment). The second is that:

# ensure the user of files created do not have root ownership
trap 'chown -h -R ${USER}:${USER} ${workdir}' EXIT

which is now running in the privileged path for the first time. This will fail though, because the cache has a bunch of stuff owned by root and we're trying to chown it to something else as real UID != 0. I'm actually not entirely sure why we need this, since we're running supermin as the current user, so everything created through the 9p mount should already be owned by that user.

Dropped that, and got farther (it created the partitions at least), but now hitting:

error: Importing 90864833fa30b1aeb2ac6a5b9426bc2fdbbda227c090e70251d128d2f0835254.dirtree: fstatat(90/864833fa30b1aeb2ac6a5b9426bc2fdbbd...

Haven't debugged that yet. But probably comes down to the expanding ways in which one can now run cosa.


Tried unprivileged mode quickly. I got further but hit:

+ mkdir -p tmp/anaconda
+ img_base=tmp/fedora-coreos-30.1-base.qcow2
++ find /usr/lib/coreos-assembler-anaconda/ -name '*CHECKSUM'
++ head -1
+ checksum_location=/usr/lib/coreos-assembler-anaconda/Fedora-Everything-29-1.2-x86_64-CHECKSUM
+ '[' -n 1 ']'
+ img_qemu=fedora-coreos-30.1-qemu.qcow2
+ '[' x86_64 == x86_64 ']'
+ build_image
+ local size kargs
++ python3 -c 'import sys, yaml; print(yaml.load(sys.stdin)["size"])'
-c:1: YAMLLoadWarning: calling yaml.load() without Loader=... is deprecated, as the default Loader is unsafe. Please read https://msg.py.
+ size=8G
++ python3 -c 'import sys, yaml; args = yaml.load(sys.stdin)["extra-kargs"]; print(" ".join(args))'
-c:1: YAMLLoadWarning: calling yaml.load() without Loader=... is deprecated, as the default Loader is unsafe. Please read https://msg.py.
Traceback (most recent call last):
  File "<string>", line 1, in <module>
KeyError: 'extra-kargs'
+ kargs=

Just needs a .get("extra-args", []) to make this work I think. Also, could use safe_load to squash those warnings.

@jlebon
Copy link
Member

jlebon commented Jun 20, 2019

OK, right also needs:

diff --git a/src/create_disk.sh b/src/create_disk.sh
index a0281a2..e1029e7 100755
--- a/src/create_disk.sh
+++ b/src/create_disk.sh
@@ -42,7 +42,7 @@ mount "${disk}2" rootfs/boot/efi

 # init the ostree
 ostree admin init-fs rootfs
-ostree pull-local "$ostree" --repo rootfs/ostree/repo
+ostree pull-local "$ostree" "$ref" --repo rootfs/ostree/repo
 ostree admin os-init "$os_name" --sysroot rootfs
 allkargs='root=/dev/disk/by-label/root rootflags=defaults,prjquota rw $ignition_firstboot'
 allkargs="$allkargs $extrakargs"

@jlebon
Copy link
Member

jlebon commented Jun 20, 2019

With the workarounds for the above, I got this working in both the privileged and unprivileged paths! 🎉

Andrew Jeddeloh added 3 commits June 20, 2019 14:04
Also switch to BLS. Continue to use anaconda on other arches.
Install both uefi and bios grub, but have them read the same config.
@ajeddeloh
Copy link
Contributor Author

Ok, just pushed the $ref and python loader warning/.get() fixes. Lets fix the other unrelated issues in their own PRs

@jlebon
Copy link
Member

jlebon commented Jun 20, 2019

Awesome, let's do this!

@jlebon jlebon merged commit eb0e8cb into coreos:master Jun 20, 2019
@jlebon
Copy link
Member

jlebon commented Jun 21, 2019

So there are two issues here

Both fixed in #564.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants