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

CI: use local registry #5584

Merged

Conversation

edsantiago
Copy link
Collaborator

CI VMs now include a local registry prepopulated with all
container images needed by tests.

Signed-off-by: Ed Santiago santiago@redhat.com

None

Copy link

Ephemeral COPR build failed. @containers/packit-build please check.

Copy link
Collaborator 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.

I was not expecting this to be so easy!

@containers/buildah-maintainers this is not yet ready for merge -- I need to figure out a way to conditionalize on-my-desktop runs vs in-CI-using-cache runs -- but it's very close. Feedback welcome.

@@ -4577,9 +4577,11 @@ EOM
}

@test "bud copy chown with newuser" {
_prefetch quay.io/fedora/fedora
_prefetch $SAFEIMAGE
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Eliminate unused image

skip_if_no_runtime
skip_if_in_container
_prefetch alpine debian
_prefetch alpine ubuntu
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

eliminate unused image (for all purposes here, the ubuntu image works the same as debian)

@@ -29,35 +29,6 @@ fromreftest() {
rm -fr ${pushdir}
}

@test "from-by-digest-s1" {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Welp. Docker registry image no longer supports v1 schema. Code has been excised. There is no way to push a v2s1 image to this registry. Is it OK to just remove all this v1 testing?

Comment on lines +113 to +116
# FIXME FIXME FIXME: I don't see the point of these. Any reason not to delete?
# run_buildah from --quiet --pull=true $WITH_POLICY_JSON docker.io/centos:7
# run_buildah rm $output
# run_buildah rmi docker.io/centos:7
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These all look like dups of the code above.

@@ -58,7 +58,7 @@ function mkcw_check_image() {
skip "cryptsetup not found"
fi
_prefetch busybox
_prefetch bash
_prefetch $SAFEIMAGE
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If there is/was anything special about bash, it was not obvious from comments, so it gets nuked

Copy link
Member

Choose a reason for hiding this comment

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

The bash image has three layers, compared to, say busybox, which has one. The test attempts to confirm that content from the base image and content added at conversion time both show up in the disk image that mkcw is generating.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thank you! $SAFEIMAGE has two layers, would that be sufficient? If not, could you suggest a three-layer image from this list? https://github.com/containers/automation_images/pull/357/files#diff-7b013edc1f5a127319d75ae1f8ae443e0bfa4d891960c27ee284bbb107ddd290R56

Copy link
Member

Choose a reason for hiding this comment

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

One of those layers is empty, but it should be fine. The error we were attempting to avoid was causing the disk image to lose content from layers that weren't the last one (and as far as this test is concerned, for images with one layer, the only layer is also the last layer), and the presence of the second layer, empty as it is, means the image still meets the test expectations.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. Repushed with a warning comment

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2024
@edsantiago edsantiago force-pushed the ci-vms-with-local-registry branch 3 times, most recently from c641c36 to 379e37b Compare July 1, 2024 21:56
edsantiago added a commit to edsantiago/container_automation_images that referenced this pull request Jul 2, 2024
...to minimize hiccups. RUN-2091 in Jira. Network registries
are too unreliable; they cause too many flakes in CI. Here
we set up a registry running on each VM, prepopulated with
all container images used in podman and buildah tests.

Related PRs:
   containers/podman#22726
   containers/buildah#5584

Once those merge, podman and buildah CI tests will fetch
images from this local registry.

Signed-off-by: Ed Santiago <santiago@redhat.com>
edsantiago added a commit to edsantiago/container_automation_images that referenced this pull request Jul 2, 2024
...to minimize hiccups. RUN-2091 in Jira. Network registries
are too unreliable; they cause too many flakes in CI. Here
we set up a registry running on each VM, prepopulated with
all container images used in podman and buildah tests.

Related PRs:
   containers/podman#22726
   containers/buildah#5584

Once those merge, podman and buildah CI tests will fetch
images from this local registry.

Signed-off-by: Ed Santiago <santiago@redhat.com>
edsantiago added a commit to edsantiago/container_automation_images that referenced this pull request Jul 2, 2024
...to minimize hiccups. RUN-2091 in Jira. Network registries
are too unreliable; they cause too many flakes in CI. Here
we set up a registry running on each VM, prepopulated with
all container images used in podman and buildah tests.

Related PRs:
   containers/podman#22726
   containers/buildah#5584

Once those merge, podman and buildah CI tests will fetch
images from this local registry.

Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago edsantiago force-pushed the ci-vms-with-local-registry branch 2 times, most recently from bd2573a to 177edd7 Compare July 8, 2024 14:24
edsantiago added a commit to edsantiago/container_automation_images that referenced this pull request Jul 8, 2024
...to minimize hiccups. RUN-2091 in Jira. Network registries
are too unreliable; they cause too many flakes in CI. Here
we set up a registry running on each VM, prepopulated with
all container images used in podman and buildah tests.

Related PRs:
   containers/podman#22726
   containers/buildah#5584

Once those merge, podman and buildah CI tests will fetch
images from this local registry.

Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago
Copy link
Collaborator Author

Two flakes, both in what looks like network pulls (dnf, image pull from k8s.io). The k8s.io one is probably a good candidate for future caching.

@edsantiago edsantiago force-pushed the ci-vms-with-local-registry branch 2 times, most recently from 3551994 to 379d816 Compare July 11, 2024 10:57
@edsantiago edsantiago marked this pull request as ready for review July 11, 2024 19:25
@edsantiago
Copy link
Collaborator Author

Podman PR has merged. I'm considering this ready. @containers/buildah-maintainers PTAL.

@rhatdan
Copy link
Member

rhatdan commented Jul 11, 2024

/approve
LGTM
@nalind @giuseppe @flouthoc @mheon PTAL

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM

As of containers/automation_images#357
our CI VMs include a local registry preloaded with all(*)
images used in tests.

 * where "all" means "most".

This commit defines a new registries-cached.conf, used in tests,
that redirects docker and quay to the new local registry. The
hope is that this will reduce CI flakes.

Since tests change over time, and new tests may require new
images, this commit also adds a mechanism for pulling in
remote images at test run time. Obviously this negates
the purpose of the cache, since it introduces a flake
pain point. The idea is: DO NOT DO THIS UNLESS ABSOLUTELY
NECESSARY, and then, if we have to do this, hurry up and
spin new CI VMs that include the new image(s).

Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago edsantiago force-pushed the ci-vms-with-local-registry branch 2 times, most recently from b6eeba6 to d9260e2 Compare July 17, 2024 18:17
Make tests pass when using a local cache registry (as triggered
by $CI_USE_REGISTRY_CACHE being non-empty).

Mostly just change images: consolidate to a smaller set, like,
replace the few instances of debian with ubuntu, use $SAFEIMAGE
where we can.

The most significant change was with some s1 (schema 1?) images.
Those exist on quay, but cannot be mirrored locally: the local
registry forbids schema 1 images. I choose to skip these tests
when running against a local registry.

Second most significant change is running containerized tests
with --net=host. We need this to access the registry running
on a localhost port. (One alternative would be to bind the
registry on all interfaces, cringe, then perform sed magic
on the registries-cache.conf file changing 127.0.0.1 to
host.containers.internal, more cringe).

Also, some cleanup.

Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago
Copy link
Collaborator Author

ping, this code has been working well in podman. I'd like to get this merged so I can get working on the required treadmill changes (for vendoring into podman).

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

openshift-ci bot commented Jul 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: edsantiago, flouthoc, giuseppe, rhatdan

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:
  • OWNERS [flouthoc,giuseppe,rhatdan]

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

@rhatdan
Copy link
Member

rhatdan commented Jul 19, 2024

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Jul 19, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 16bd9a3 into containers:main Jul 19, 2024
36 of 37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants