Skip to content

cmdlib: Use kola qemuexec, delete qemu-kvm code #1289

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 2 commits into from
Apr 22, 2020
Merged

cmdlib: Use kola qemuexec, delete qemu-kvm code #1289

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
541908a
build: Identify target disk by wwn
cgwalters Apr 13, 2020
6a78bde
cmdlib: Use kola qemuexec
cgwalters Mar 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 4 additions & 8 deletions src/cmd-buildextend-metal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,20 +234,16 @@ if [ -n "${ref_is_temp}" ]; then
ref_arg=${commit}
fi

target_drive=("-drive" "if=virtio,id=target,format=${image_format},file=${path}.tmp,cache=unsafe")
target_device_opt="scsi-disk,drive=target,wwn=0x2a"
# we need 4096 block size for ECKD DASD and (obviously) metal4k
if [[ $image_type == dasd || $image_type == metal4k ]]; then
device_type=virtio-blk
if [[ $image_type == dasd ]]; then
device_type=virtio-blk-ccw
fi
target_drive=("-drive" "if=none,id=target,format=${image_format},file=${path}.tmp,cache=unsafe" \
"-device" "${device_type},drive=target,physical_block_size=4096,logical_block_size=4096,scsi=off")
target_device_opt="${target_device_opt},physical_block_size=4096,logical_block_size=4096"
fi
target_drive=("-drive" "if=none,id=target,format=${image_format},file=${path}.tmp,cache=unsafe" \
"-device" "virtio-scsi-${devtype},id=scsitarget" "-device" "${target_device_opt}")

runvm "${target_drive[@]}" -- \
/usr/lib/coreos-assembler/create_disk.sh \
--disk /dev/vda \
--buildid "${build}" \
--imgid "${img}" \
--grub-script /usr/lib/coreos-assembler/grub.cfg \
Expand Down
58 changes: 6 additions & 52 deletions src/cmdlib.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,34 +45,7 @@ case $arch in
"s390x") DEFAULT_TERMINAL="ttysclp0"; devtype=ccw ;;
*) fatal "Architecture $(arch) not supported"
esac
export DEFAULT_TERMINAL

if [ -x /usr/libexec/qemu-kvm ]; then
QEMU_KVM="/usr/libexec/qemu-kvm"
else
# Enable arch-specific options for qemu
case "$(arch)" in
"x86_64") QEMU_KVM="qemu-system-$(arch) -accel kvm" ;;
"aarch64") QEMU_KVM="qemu-system-$(arch) -accel kvm -M virt,gic-version=host" ;;
"ppc64le") QEMU_KVM="qemu-system-ppc64 -accel kvm" ;;
"s390x") QEMU_KVM="qemu-system-$(arch) -accel kvm -M s390-ccw-virtio" ;;
*) fatal "Architecture $(arch) not supported"
esac
fi

if grep -q 'kubepods' /proc/1/cgroup; then
# only use 1 core on kubernetes since we can't determine how much we can actually use
QEMU_PROCS=1
elif [ "$(nproc)" -gt 16 ]; then
# cap qemu smp at some reasonable level to not exceed limitation of some platforms
QEMU_PROCS=16
else
QEMU_PROCS="$(nproc)"
fi
# cap smp and dissable VGA as it "eats" output from console on some platforms, ppc64le most notably
QEMU_KVM+=" -smp ${QEMU_PROCS} -vga none"
export QEMU_KVM
export QEMU_PROCS
export DEFAULT_TERMINAL devtype

_privileged=
has_privileges() {
Expand Down Expand Up @@ -489,8 +462,7 @@ EOF

cachedisk=()
if [ -f "${workdir}/cache/cache2.qcow2" ]; then
cachedisk=("-drive" "if=none,id=drive-scsi0-0-0-1,discard=unmap,file=${workdir}/cache/cache2.qcow2" \
"-device" "scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=1,drive=drive-scsi0-0-0-1,id=scsi0-0-0-1")
cachedisk=("-drive" "if=virtio,discard=unmap,file=${workdir}/cache/cache2.qcow2,index=2")
fi

# support local dev cases where src/config is a symlink
Expand All @@ -500,33 +472,15 @@ EOF
srcvirtfs=("-virtfs" "local,id=source,path=${workdir}/src/config,security_model=none,mount_tag=source")
fi

scsibus="bus=pci.0,addr=0x3"
arch_args=
case $arch in
"aarch64")
# 'pci' bus doesn't work on aarch64
scsibus="bus=pcie.0,addr=0x3"
arch_args='-bios /usr/share/AAVMF/AAVMF_CODE.fd'
;;
"s390x") scsibus="devno=fe.0.0003" ;;
esac

rm -f "${workdir}/tmp/rc"

#shellcheck disable=SC2086
${QEMU_KVM} ${arch_args:-} \
-nodefaults -nographic -m 2048 -no-reboot -cpu host \
-kernel "${vmbuilddir}/kernel" \
-initrd "${vmbuilddir}/initrd" \
-netdev user,id=eth0,hostname=supermin \
-device virtio-net-"${devtype}",netdev=eth0 \
-device virtio-scsi-"${devtype}",id=scsi0,"${scsibus}" \
-object rng-random,filename=/dev/urandom,id=rng0 -device virtio-rng-"${devtype}",rng=rng0 \
-drive if=none,id=drive-scsi0-0-0-0,snapshot=on,file="${vmbuilddir}/root" \
-device scsi-hd,bus=scsi0.0,channel=0,scsi-id=0,lun=0,drive=drive-scsi0-0-0-0,id=scsi0-0-0-0,bootindex=1 \
kola qemuexec -m 2048 --auto-cpus -U -- -no-reboot -nodefaults -serial stdio \
-kernel "${vmbuilddir}/kernel" -initrd "${vmbuilddir}/initrd" \
-drive "if=virtio,format=raw,snapshot=on,file=${vmbuilddir}/root,index=1" \
"${cachedisk[@]}" \
-virtfs local,id=workdir,path="${workdir}",security_model=none,mount_tag=workdir \
"${srcvirtfs[@]}" -serial stdio -append "root=/dev/sda console=${DEFAULT_TERMINAL} selinux=1 enforcing=0 autorelabel=1" \
"${srcvirtfs[@]}" -append "root=/dev/vda console=${DEFAULT_TERMINAL} selinux=1 enforcing=0 autorelabel=1" \
"${qemu_args[@]}"

if [ ! -f "${workdir}"/tmp/rc ]; then
Expand Down
21 changes: 20 additions & 1 deletion src/create_disk.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ run as part of 'coreos-assembler build'.
EOC
}

disk=
rootfs_size="0"
boot_verity=0
rootfs_type="xfs"
Expand Down Expand Up @@ -78,7 +79,25 @@ udevtrig() {

export PATH=$PATH:/sbin:/usr/sbin
arch="$(uname -m)"
disk="${disk:?--disk must be defined}"

if [ -z "${disk:-}" ]; then
# hex 0x2a = 42 in decimal; this is set in cmd-buildextend-metal.
# We use the WWN as an unambiguous way to identify our target disk,
# independent of other devices attached to the VM (caches, etc.)
Copy link
Member

@jlebon jlebon Apr 22, 2020

Choose a reason for hiding this comment

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

Note virtio-blk does support something similar as well with serial=... (that's what coreos/ignition#905 uses). This is fine too though! (I would think virtio-scsi is slower than virtio-blk for our use case, but since it's not a long-running VM or anything, it's probably negligible overall. Not to mention our cache qcow2 has been using SCSI for a while too now.)

Copy link
Member Author

@cgwalters cgwalters Apr 22, 2020

Choose a reason for hiding this comment

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

Ahh right...hmm, yes, I think if I just steal these bits from your osmet PR we could switch to serial and /dev/disk/by-id. Will try. What I ran into is that blkid didn't know how to parse the serials or something, and I thought running udev outside of systemd would be harder than that.

# we want /dev/disk symlinks for coreos-installer
/usr/lib/systemd/systemd-udevd --daemon
/usr/sbin/udevadm trigger --settle

Copy link
Member Author

@cgwalters cgwalters Apr 22, 2020

Choose a reason for hiding this comment

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

OK I fought this in cgwalters@75aeaba and couldn't get it to work; I think somehow the target drive is ending up as /dev/vda. See also coreos/ignition#905 (comment)

Copy link
Member

@jlebon jlebon Apr 22, 2020

Choose a reason for hiding this comment

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

Replied in coreos/ignition#905 (comment). But yeah, let's just always enable udev to make all this cleaner.

Copy link
Member Author

@cgwalters cgwalters Apr 22, 2020

Choose a reason for hiding this comment

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

OK so...I tried to use this for the root device but that's in the supermin code which has its own custom init that only handles /dev/vda or UUIDs (and for some reason it uses random UUIDs still for the rootfs...)

The flip side of this is...using WWNs is also natural for people doing installs to real devices (e.g. with coreos-installer), so it's somewhat useful to also do that for our builds.

Copy link
Member Author

@cgwalters cgwalters Apr 22, 2020

Choose a reason for hiding this comment

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

I think my vote hence is to land this, since it's tested and an improvement, we we can do something else as followup.

Copy link
Member

@jlebon jlebon Apr 22, 2020

Choose a reason for hiding this comment

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

Yup, sure. WFM!

Copy link
Member

@jlebon jlebon Apr 24, 2020

Choose a reason for hiding this comment

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

Follow-up to this in #1398!

wwn=000000000000002a
for dev in /sys/block/*; do
if grep -F -e "${wwn}" "${dev}/device/wwid" 2>/dev/null; then
disk="/dev/$(basename ${dev})"
break
fi
done
if [ -z "${disk:-}" ]; then
echo "failed to find disk with wwn ${wwn}" 1>&2
bash
exit 1
fi
fi

buildid="${buildid:?--buildid must be defined}"
imgid="${imgid:?--imgid must be defined}"
ostree="${ostree:?--ostree-repo must be defined}"
Expand Down