-
Notifications
You must be signed in to change notification settings - Fork 632
Allow preprepared CoreDNS image to be used #3906
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
Allow preprepared CoreDNS image to be used #3906
Conversation
|
@aaronjwood: The label(s) In response to this:
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-sigs/prow repository. |
|
Welcome @aaronjwood! |
|
Hi @aaronjwood. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
|
/area conformance-machinery |
|
/ok-to-test |
|
The failing test looks like some kind of flake...? Is it known to anybody? |
|
/retest |
|
I missed which test was failing, looks like everything is in good shape now. Tagging conformance leads for review @candita @sunjayBhatia @arkodg @LiorLieberman |
|
Wanted to follow up on this and see if we can get it merged. Thanks! |
|
and what if using |
conformance/base/manifests.yaml
Outdated
| - args: | ||
| - -conf | ||
| - /root/Corefile | ||
| image: coredns/coredns |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can probably also pull directly from registry.k8s.io/coredns/coredns:v1.12.0 and while the imagePullPolicy is great, we avoid any sort of rate limiting at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer this too, I didn't realize the image existed here. When I poked around in the past I didn't see anything that referenced CoreDNS having an image here.
I'll update the URI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there's no latest in this registry are folks ok moving to pinning a specific version here? Someone/something will need to keep it moving going forward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am ok with keeping it pinned (tho it is good to have some second opinion), and tbh I prefer it :) IMHO latest can bring some undesired behavior and as this is used for test, we may want consistency.
Also, usually the CoreDNS version is bumped together with Kubernetes version (eg.: kubernetes/kubernetes#131151)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, I've seen the arguments for pinning vs not pinning go both ways. I'll update this and see if we can reach a consensus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to keep it pinned so we have a point in time test manifest that can be reproducible - would hate for a new version of coredns to affect an old version of our conformance tests.
1d9fd9d to
ab44ea9
Compare
ab44ea9 to
d868a0d
Compare
|
Any interest in merging this? |
|
Thanks @aaronjwood! /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aaronjwood, robscott 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 |
* Switch to registry.k8s.io/coredns/coredns, only pull if not present * Properly support multiple embedded manifests
* Switch to registry.k8s.io/coredns/coredns, only pull if not present * Properly support multiple embedded manifests
What type of PR is this?
/kind bug
/area conformance-machinery
What this PR does / why we need it:
For folks that are pulling images from Docker's public registry in an unauthenticated way rate limits are an immediate problem. This PR fixes three things:
IfNotPresent, allowing for a cluster with an already-prepared CoreDNS image to be usedregistry.k8s.io/coredns/corednsgetContentsFromPathOrURLto correctly handle multiplefs.FS. If this was already supported client code could've easily done something like this to work around the issue:Does this PR introduce a user-facing change?: