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

new testimage and systemd-image #21356

Merged
merged 1 commit into from
Feb 9, 2024

Conversation

edsantiago
Copy link
Member

@edsantiago edsantiago commented Jan 24, 2024

Simply because it's been a while since the last testimage
build, and I want to confirm that our image build process
still works.

Added /home/podman/healthcheck. This saves us having to
podman-build on each healthcheck test. Removed now-
unneeded _build_health_check_image helper.

testimage: bump alpine 3.16.2 to 3.19.0

systemd-image: f38 to f39

  • tzdata now requires dnf install, not reinstall
    (this is exactly the sort of thing I was looking for)

PROBLEMS DISCOVERED:

  • in e2e, fedoraMinimal is now == SYSTEMD_IMAGE. This
    screws up some of the image-count tests (CACHE_IMAGES).

  • "alter tarball" system test now barfs with tar < 1.35.

TODO: completely replace fedoraMinimal with SYSTEMD_IMAGE in all tests.

Signed-off-by: Ed Santiago [email protected]

None

@openshift-ci openshift-ci bot added release-note-none approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 24, 2024
@edsantiago edsantiago force-pushed the new_testimages branch 2 times, most recently from ae7fb30 to e9e7bfb Compare January 25, 2024 00:51
Copy link

Cockpit tests failed for commit e9e7bfb15fd5c6c101f589ceb5c26386c3164b3e. @martinpitt, @jelly, @mvollmer please check.

@edsantiago edsantiago force-pushed the new_testimages branch 2 times, most recently from 05a8d2d to 03d3d83 Compare January 25, 2024 12:14
Copy link
Member Author

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

Comments to help reviewers. (And one to befuddle them)

Comment on lines +12 to +13
CITEST_IMAGE = "quay.io/libpod/testimage:20240123" //nolint:revive,stylecheck
SYSTEMD_IMAGE = "quay.io/libpod/systemd-image:20240124" //nolint:revive,stylecheck
Copy link
Member Author

Choose a reason for hiding this comment

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

For future: is there a good reason for duplicatingly defining CITEST_IMAGE and others in config_TAGGED.go instead of just config.go? Maybe historically these were different? I am absolutely not going to change that in this already-too-big PR, but I'd like to consider doing so in a followup.

Comment on lines 50 to 51
# FIXME: tar < 1.35 is broken. The second tar command below
# barfs with "Skipping to next header" then "Exiting with failure status"
Copy link
Member Author

Choose a reason for hiding this comment

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

DANGER WILL ROBINSON. This really creeps me out; much like #19407 this is almost certainly a bug in tar, but the other way around: last time, old-tar worked and new-rawhide-tar was broken; now new-tar works and old-tar is broken, and all I did was change a source image. I can't tell yet if this bug was intentionally fixed in 1.35, or if it's just pure happenstance that 1.35 passes. I will be looking into this today.

@@ -33,15 +33,12 @@ function _check_health {
}

@test "podman healthcheck" {
_build_health_check_image healthcheck_i
Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, hivemind, here's a stumper:

before after
root 48 28
rootless 30 21

Time, in seconds, for hack/bats 220 (healthcheck.bats).

  • root is consistently slower than rootless
  • the new code shaves 20s from root, but only 9s from rootless
    • times are now closer together, but root & rootless still differ
  • the word "root" does not appear in 220-healthcheck.bats. No conditional skips or extra tests AFAICT.
  • there is a full moon

Results are consistent across O(10) runs.

Very low priority, this is a just-curious question: any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

Rootless vs root would be different systemd daemons (the user instance vs actual PID1 systemd) which could account for some of this? I assume user instance is less busy.

LABEL created_by="$create_script @ $create_script_rev"
LABEL created_at=$create_time_z
# Note the reinstall of tzdata is required (https://bugzilla.redhat.com/show_bug.cgi?id=1870814)
Copy link
Member Author

Choose a reason for hiding this comment

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

See the BZ for details. TL;DR tzdata is no longer in fedora-minimal at all.

@edsantiago edsantiago force-pushed the new_testimages branch 7 times, most recently from fbffd4d to a613463 Compare February 1, 2024 19:02
@edsantiago
Copy link
Member Author

ping, I'm pretty happy with this, and it's been passing (except for the new rawhide-VM flake #21504) in my hammer-CI PR.

@rhatdan
Copy link
Member

rhatdan commented Feb 5, 2024

LGTM

Simply because it's been a while since the last testimage
build, and I want to confirm that our image build process
still works.

Added /home/podman/healthcheck. This saves us having to
podman-build on each healthcheck test. Removed now-
unneeded _build_health_check_image helper.

testimage: bump alpine 3.16.2 to 3.19.0

systemd-image: f38 to f39
  - tzdata now requires dnf **install**, not reinstall
    (this is exactly the sort of thing I was looking for)

PROBLEMS DISCOVERED:
  - in e2e, fedoraMinimal is now == SYSTEMD_IMAGE. This
    screws up some of the image-count tests (CACHE_IMAGES).

  - "alter tarball" system test now barfs with tar < 1.35.

TODO: completely replace fedoraMinimal with SYSTEMD_IMAGE
in all tests.

Signed-off-by: Ed Santiago <[email protected]>
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 9, 2024
Copy link
Contributor

openshift-ci bot commented Feb 9, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, Luap99

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@openshift-merge-bot openshift-merge-bot bot merged commit 4b1f0b0 into containers:main Feb 9, 2024
85 checks passed
@edsantiago edsantiago deleted the new_testimages branch February 27, 2024 14:54
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label May 28, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators May 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note-none
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants