Allow preprepared CoreDNS image to be used #3906
Allow preprepared CoreDNS image to be used #3906k8s-ci-robot merged 2 commits intokubernetes-sigs:mainfrom
Conversation
|
@aaronjwood: The label(s) DetailsIn 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. DetailsInstructions 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 |
| @@ -705,6 +705,7 @@ spec: | |||
| - -conf | |||
| - /root/Corefile | |||
| image: coredns/coredns | |||
There was a problem hiding this comment.
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.
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.
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.
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.
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.
+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 DetailsNeeds 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
* 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?: