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

test images: Mirrors dockerhub images to staging #95567

Merged

Conversation

claudiubelu
Copy link
Contributor

@claudiubelu claudiubelu commented Oct 14, 2020

What type of PR is this?

What this PR does / why we need it:

Dockerhub will introduce rate limiting in November, and a lot of E2E tests are relying on the busybox image. It could potentially become an issue causing jobs to fail because of this.

Ideally, we'd have the busybox image mirrored on gcr.io, but that could take some time. Until then, we can just have the Image Builder mirror the image for us in the staging registry and use that for tests until this issue is solved. The busybox image should NOT be promoted out of staging.

During the sig-testing meeting, it was decided that we should do the same for the other images are hosted on dockerhub.

Two different versions of httpd and nginx have to build, and thus, the have different folders. An ALIAS file was added to httpd-new and nginx-new in order to keep the same image name.

Note that the image docker.io/gluster/glusterdynamic-provisioner:v1.0 only has a linux/amd64 image.

Which issue(s) this PR fixes:

Fixes #

Related PR: Adds Image Builder jobs for these images: kubernetes/test-infra#20265

Special notes for your reviewer:

Mitigate dockerhub changes rolling out Nov 1st: kubernetes/test-infra#19477

Image building logs: https://paste.ubuntu.com/p/9q8HtpwpnW/

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Oct 14, 2020
@claudiubelu
Copy link
Contributor Author

/cc @spiffxp @BenTheElder

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 14, 2020
@k8s-ci-robot k8s-ci-robot added area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 14, 2020
@justaugustus
Copy link
Member

/retest
The rationale SGTM!

@oomichi
Copy link
Member

oomichi commented Oct 14, 2020

/retest

1 similar comment
@claudiubelu
Copy link
Contributor Author

/retest

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 20, 2020
@claudiubelu claudiubelu force-pushed the test-images/linux-busybox branch 3 times, most recently from 527d4fe to 3b0f4cc Compare October 20, 2020 19:23
@claudiubelu
Copy link
Contributor Author

Image building logs: https://paste.ubuntu.com/p/jDvZHshhdV/ (+ commit from: #95032)

Note: I was already intending to add the nginx and httpd images (+ Windows support: #77269), so I've reused some bits from there.

@claudiubelu claudiubelu changed the title test images: Adds linux images to busybox test images: Mirrors dockerhub images to staging Oct 20, 2020
@spiffxp
Copy link
Member

spiffxp commented Oct 20, 2020

/priority important-soon
/kind cleanup
/triage accepted
/hold
/cc @thockin @BenTheElder
I'd like your input

@claudiubelu
Copy link
Contributor Author

/open

@claudiubelu
Copy link
Contributor Author

/reopen

@k8s-ci-robot k8s-ci-robot reopened this Dec 15, 2020
@k8s-ci-robot
Copy link
Contributor

@claudiubelu: Reopened this PR.

In response to this:

/reopen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 16, 2020
@justaugustus
Copy link
Member

/hold

I feel like this may be the wrong approach here
i.e.,:

  • creating new Dockerfiles to maintain
  • building and repushing

What I think we want is:

  • a remote-to-remote repo copy
  • digest preservation
  • no fiddling with multi-arch
  • stored in the mirror registry so others are aware and can take advantage of these mirrors

@claudiubelu -- can you get me the exact list of images/tags that you're interested in, and give me a few days to work a prototype, so you can see what I mean?

cc: @wilsonehusin

@claudiubelu
Copy link
Contributor Author

claudiubelu commented Dec 16, 2020

/hold

I feel like this may be the wrong approach here
i.e.,:

* creating new Dockerfiles to maintain

* building and repushing

What I think we want is:

* a remote-to-remote repo copy

* digest preservation

* no fiddling with multi-arch

* stored in the mirror registry so others are aware and can take advantage of these mirrors

@claudiubelu -- can you get me the exact list of images/tags that you're interested in, and give me a few days to work a prototype, so you can see what I mean?

cc: @wilsonehusin

From #97027 , these are the images that are currently on dockerhub but used in E2E tests (as it can be seen in kubernetes/test/utils/image/manifest.go:

docker.io/gluster/glusterdynamic-provisioner:v1.0
docker.io/library/busybox:1.29
docker.io/library/httpd:2.4.38-alpine
docker.io/library/httpd:2.4.39-alpine
docker.io/library/nginx:1.14-alpine
docker.io/library/nginx:1.15-alpine
docker.io/library/perl:5.26
docker.io/library/redis:5.0.5-alpine

IMO, we can still go ahead with the images: busybox, httpd, nginx, since ultimately, what we need is not just a simple mirror for them. On dockerhub, they do not contain any Windows images in their manifest lists, but we need them for our test jobs (the Kubernetes busybox image already has a Windows manifest list). I have a PR opened for the nginx and httpd images here: #77269

@BenTheElder
Copy link
Member

I think that's a good point #95567 (comment),
also, nothing precludes migrating from a repo to repo copy implementation later (except that it sounds like that won't be sufficient b/c windows), but in the meantime people reported hitting issues with pull limits running tests right now.

I asked for this explicitly when the topic came up in SIG testing, I'm inclined to recommend we take this straightforward approach now to bring things under our control.

Additionally: performing a build of a custom image discourages users from trying to use kubernetes as a mirror of these images for arbitrary purposes (which we shouldn't take the cost or maintenance load for), instead of seeing these as all our e2e custom images, even if customization is very light.

@BenTheElder
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 17, 2020
Dockerhub will introduce rate limiting in November, and a lot of E2E tests
are relying on the busybox image. It could potentially become an issue
causing jobs to fail because of this.

Ideally, we'd have the busybox image mirrored on gcr.io, but that could take
some time. Until then, we can just have the Image Builder mirror the image
for us in the staging registry and use that for tests until this issue is
solved. The busybox image should NOT be promoted out of staging.

During the sig-testing meeting, it was decided that we should do the same
for the other images are hosted on dockerhub.

Two different versions of httpd and nginx have to be built, and thus, the have
different folders. An ALIAS file was added to httpd-new and nginx-new in order
to keep the same image name.
We are planing to test and support 20H2 release of Windows, thus,
we need to build test images for it as well. The busybox image already
has a BASEIMAGE entry for it, but we also need to add it to the image-util.sh's
windows_os_versions, so the OS Version can be properly included in the
manifest list.
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 17, 2020
@claudiubelu
Copy link
Contributor Author

FWIW, the image build logs for the images added in this PR using this PR: https://paste.ubuntu.com/p/zYHHTF4QkW/

@claudiubelu
Copy link
Contributor Author

/hold

I feel like this may be the wrong approach here
i.e.,:

* creating new Dockerfiles to maintain

* building and repushing

What I think we want is:

* a remote-to-remote repo copy

* digest preservation

* no fiddling with multi-arch

* stored in the mirror registry so others are aware and can take advantage of these mirrors

@claudiubelu -- can you get me the exact list of images/tags that you're interested in, and give me a few days to work a prototype, so you can see what I mean?

cc: @wilsonehusin

Any news?

@jayunit100
Copy link
Member

Hi folks hitting rate limits on e2es. What's the simplest way to unblock this?

We can run our own builds and publish if needed . What's the easiest thing to do here that is iterable ?

@dims
Copy link
Member

dims commented Feb 4, 2021

going with @BenTheElder 's comment here : #95567 (comment)

we can always iterate

/approve
/lgtm
/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 4, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 4, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, claudiubelu, dims, spiffxp

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note-none Denotes a PR that doesn't merit a release note. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants