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

mantle/kola/tests: Use qemu platform only #3285

Merged
merged 3 commits into from
Jun 13, 2023
Merged

Conversation

travier
Copy link
Member

@travier travier commented Dec 14, 2022

mantle/kola/tests: Replace 'qemu-unpriv' with 'qemu'

There is no longer a privileged 'qemu' platform and the 'qemu' and
'qemu-unpriv' platforms are the same.

Let's use qemu everywhere in order to be able to remove all
'qemu-unpriv' usage.


docs: Replace 'qemu-unpriv' with 'qemu'

There is no longer a privileged 'qemu' platform and the 'qemu' and
'qemu-unpriv' platforms are the same.

Let's use qemu everywhere in order to be able to remove all
'qemu-unpriv' usage.


kola: Remove 'qemu-unpriv' and make 'qemu' the default

There is no longer a privileged 'qemu' platform and the 'qemu' and
'qemu-unpriv' platforms are the same.

cgwalters
cgwalters previously approved these changes Dec 14, 2022
@travier
Copy link
Member Author

travier commented Dec 15, 2022

Note: I have not tested that yet

@@ -372,12 +372,9 @@ The json file should have the following fields at the minimum with the api key b
```

### qemu
`qemu` is run locally and needs no credentials, but does need to be run as root.
Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we should remove this entire file in favor of the doc in docs

mantle/kola/harness.go Outdated Show resolved Hide resolved
jlebon
jlebon previously approved these changes Jan 27, 2023
mantle/cmd/kola/options.go Outdated Show resolved Hide resolved
@travier
Copy link
Member Author

travier commented Feb 2, 2023

Cleanup for RHCOS in openshift/os#1142.

We could also consider dropping JSON support for kola tests configs as we no longer have that now.

@travier
Copy link
Member Author

travier commented Feb 2, 2023

I am not certain about #3285 (comment) but apart from that this should be ready.

Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

mostly LGTM a few comments

Extra Credit: The commit messages could use some context (i.e. there used to be qemu-unpriv and qemu but now there's only one, etc etc...).

mantle/kola/tests/fips/fips.go Outdated Show resolved Hide resolved
docs/mantle/credentials.md Outdated Show resolved Hide resolved
mantle/cmd/kola/options.go Outdated Show resolved Hide resolved
@travier
Copy link
Member Author

travier commented Feb 3, 2023

Will have to figure out the commits where those changes were previously made

dustymabe
dustymabe previously approved these changes Feb 3, 2023
Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

jlebon
jlebon previously approved these changes Feb 3, 2023
@travier
Copy link
Member Author

travier commented Feb 21, 2023

I'm testing this change and the diff of the list of tests between COSA before and after this PR is not empty so I have to double check things. It adds things but it also removes some.

@dustymabe
Copy link
Member

I'm testing this change and the diff of the list of tests between COSA before and after this PR is not empty so I have to double check things. It adds things but it also removes some.

Thank you for being thorough here!

@travier
Copy link
Member Author

travier commented Feb 22, 2023

Updated diff:

--- main.list.sorted    2023-02-22 15:56:22.260544662 +0100
+++ new.list.sorted     2023-02-22 15:56:32.531605489 +0100
@@ -6,10 +6,11 @@
 coreos.ignition.groups
 coreos.ignition.instantiated.enable-unit[all]
 coreos.ignition.journald-log
+coreos.ignition.mount.disks
+coreos.ignition.mount.partitions
 coreos.ignition.once
-coreos.ignition.security.tls
 coreos.ignition.sethostname
-coreos.ignition.ssh.key
+coreos.ignition.symlink
 coreos.ignition.systemd.enable-service
 coreos.ignition.v2.users
 coreos.network.initramfs.second-boot
@@ -18,8 +19,6 @@
 coreos.tls.fetch-urls
 crio.base
 fcos.filesystem
-fcos.ignition.misc.empty
-fcos.ignition.v3.noop
 fcos.internet
 fcos.network.listeners
 fcos.users.shells
@@ -44,6 +43,8 @@
 rhcos.selinux.boolean.persist
 rhcos.selinux.manage
 rhcos.services-disabled
+rhcos.sssd
+rhcos.upgrade.from-ocp-rhcos
 rootfs.uuid
 rpmostree.install-uninstall
 rpmostree.status

travier added 2 commits June 12, 2023 17:28
There is no longer a privileged 'qemu' platform and the 'qemu' and
'qemu-unpriv' platforms are the same.

Let's use qemu everywhere in order to be able to remove all
'qemu-unpriv' usage.
There is no longer a privileged 'qemu' platform and the 'qemu' and
'qemu-unpriv' platforms are the same.

Let's use qemu everywhere in order to be able to remove all
'qemu-unpriv' usage.
@travier
Copy link
Member Author

travier commented Jun 12, 2023

Summary of the change in tests -/+ here, based on my understanding of the tests, may be incomplete and will need review, based on kola list output:

Tests that will be skipped:

  • coreos.ignition.security.tls: This requires two systems to test Ignition fetching over HTTPS with a custom CA and thus can not run on QEMU "unpriv" AFAIU
  • coreos.ignition.ssh.key: This is redundant on QEMU has we always provision SSH keys via Ignition
  • fcos.ignition.misc.empty: Requires the Ignition config to be fetched from cloud platform metadata
  • fcos.ignition.v3.noop: Same as above

Additional tests that will be run:

  • coreos.ignition.mount.disks
  • coreos.ignition.mount.partitions
  • coreos.ignition.symlink
  • ext.config.ignition.delete-config
  • ext.config.ignition.kargs
  • ext.config.networking.bridge-static-via-kargs
  • ext.config.networking.force-persist-ip
  • ext.config.networking.ifname-karg.everyboot-systemd-link-file
  • ext.config.networking.ifname-karg.udev-rule-firstboot-propagation
  • ext.config.networking.kargs-rd-net
  • ext.config.networking.mtu-on-bond-ignition
  • ext.config.networking.mtu-on-bond-kargs
  • ext.config.networking.nameserver
  • ext.config.networking.no-default-initramfs-net-propagation.bootif
  • ext.config.networking.no-persist-ip
  • ext.config.networking.prefer-ignition-networking
  • ext.config.networking.team-dhcp-via-ignition
  • ext.config.ntp.chrony.coreos-platform-chrony-config
  • ext.config.ntp.chrony.dhcp-propagation
  • ext.config.ntp.timesyncd.dhcp-propagation
  • ext.config.reboot
  • ext.config.root-reprovision.linear
  • ext.config.root-reprovision.raid1
  • ext.config.root-reprovision.swap-before-root
  • ext.config.secex.ensure
  • ext.config.secex.reboot
  • ext.config.var-mount.luks
  • ext.config.var-mount.scsi-id
  • ext.config.var-mount.simple
  • rhcos.sssd
  • rhcos.upgrade.from-ocp-rhcos

So overall this should now run more tests that before but a double check would be nice.

Summary of the steps taken to get this output:

cosa kola list > fcos.main.list
# build cosa from this branch
podman build -t localhost/coreos-assembler . --from quay.io/coreos-assembler/coreos-assembler:latest
export COREOS_ASSEMBLER_CONTAINER=localhost/coreos-assembler
cosa kola list > fcos.dev.list
sed -i "s/  \+/ /g" *.list
sed -i -e 's/\r*$/\r/' *.list
sort -o fcos.main.list fcos.main.list
sort -o fcos.dev.list fcos.dev.list
vimdiff fcos.main.list fcos.dev.list

@travier travier requested a review from bgilbert June 12, 2023 16:55
@travier
Copy link
Member Author

travier commented Jun 12, 2023

@dustymabe
Copy link
Member

maybe we should also mv mantle/platform/machine/unprivqemu mantle/platform/machine/qemu?

@dustymabe
Copy link
Member

dustymabe commented Jun 12, 2023

Summary of the change in tests -/+ here, based on my understanding of the tests, may be incomplete and will need review, based on kola list output:

Honestly I don't think there is any real change in behavior here. Yes, the kola list output is different but I think that is mostly because previously we mixed using qemu and qemu-unpriv in ExcludePlatforms or Platforms and I don't think kola list did enough evaluation to resolve that. If you actually run tests I think you'll notice that it behaves the same.

Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. A few comments..

mantle/cmd/kola/options.go Outdated Show resolved Hide resolved
There is no longer a privileged 'qemu' platform and the 'qemu' and
'qemu-unpriv' platforms are the same.
@travier
Copy link
Member Author

travier commented Jun 13, 2023

Thanks, updated.

Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

@travier travier merged commit 941cad2 into coreos:main Jun 13, 2023
@travier travier deleted the rm-qemu-unpriv branch June 13, 2023 14:16
cgwalters added a commit to cgwalters/rpm-ostree that referenced this pull request Jun 19, 2023
@cgwalters
Copy link
Member

Just for reference this broke (one of) rpm-ostree's test suites, xref coreos/rpm-ostree#4477
Probably worth remembering to do a code search for removals in the future.

@travier
Copy link
Member Author

travier commented Jun 21, 2023

Arg, thanks! Sorry for the trouble.

lukewarmtemp pushed a commit to lukewarmtemp/rpm-ostree that referenced this pull request Aug 8, 2023
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.

5 participants