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

change kubeadm coredns addon images name to coredns/coredns #7570

Merged
merged 1 commit into from
Apr 30, 2021

Conversation

MRoci
Copy link
Contributor

@MRoci MRoci commented Apr 28, 2021

What type of PR is this?

/kind bug

What this PR does / why we need it:
at the moment kubeadm has hardcoded that the name of the coreDNS image
is coredns/coredns.
When using k8s.gcr.io as image repository this is false for versions
<=1.7.0 which are located at k8s.gcr.io/coredns:${VERSION}.
This causes the deployment to be unable to pull images until the
kubernetes-apps role ovverrides it with the correct image location

docker.io fullfills the assumptions made by kubeadm that the image is
found under the name coredns/coredns

Also on gcr.io newest tags such as +1.8.0 seems not to be available at
the moment, making impossible to upgrade the coredns component

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Apr 28, 2021
@k8s-ci-robot
Copy link
Contributor

Welcome @MRoci!

It looks like this is your first PR to kubernetes-sigs/kubespray 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/kubespray has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 28, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @MRoci. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot requested review from EppO and holmsten April 28, 2021 15:52
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 28, 2021
@MRoci
Copy link
Contributor Author

MRoci commented Apr 28, 2021

/assign @mirwan

Copy link
Member

@floryut floryut left a comment

Choose a reason for hiding this comment

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

/ok-to-test

I was the one that switch this one to k8s.gcr.io when we had everything in dockerhub and they added a limit.. but you're right as it's not even up to date with 1.21 default (1.8.0) 😢

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 28, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floryut, MRoci

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2021
@tvanderka
Copy link

This PR sets kubeadm 1.20 config pointing to docker.io/coredns:1.7.0 which is wrong. Kubeadm switched to coredns/coredns in 1.21, for coredns v1.8.0 (note the "v", you can also pull "k8s.gcr.io/coredns/coredns:v1.7.0"). No need to switch to docker.io

See kubernetes/kubernetes#101300

@muzi502
Copy link
Contributor

muzi502 commented Apr 29, 2021

This PR sets kubeadm 1.20 config pointing to docker.io/coredns:1.7.0 which is wrong. Kubeadm switched to coredns/coredns in 1.21, for coredns v1.8.0 (note the "v", you can also pull "k8s.gcr.io/coredns/coredns:v1.7.0"). No need to switch to docker.io

See kubernetes/kubernetes#101300

You are right, should use k8s.gcr.io/coredns/coredns begin v1.21

These are coredns's tag in different image repo :

  • k8s.gcr.io/coredns/coredns
root@debian:/root # skopeo list-tags docker://k8s.gcr.io/coredns/coredns
{
    "Repository": "k8s.gcr.io/coredns/coredns",
    "Tags": [
        "v1.6.6",
        "v1.6.7",
        "v1.6.9",
        "v1.7.0",
        "v1.7.1",
        "v1.8.0",
        "v1.8.3"
    ]
}
  • k8s.gcr.io/coredns
root@debian:/root # skopeo list-tags docker://k8s.gcr.io/coredns
{
    "Repository": "k8s.gcr.io/coredns",
    "Tags": [
        "1.0.1",
        "1.0.1__amd64_linux",
        "1.0.1__arm64_linux",
        "1.0.1__arm_linux",
        "1.0.1__ppc64le_linux",
        "1.0.1__s390x_linux",
        "1.0.6",
        "1.0.6__amd64_linux",
        "1.0.6__arm64_linux",
        "1.0.6__arm_linux",
        "1.0.6__ppc64le_linux",
        "1.0.6__s390x_linux",
        "1.1.3",
        "1.1.3__amd64_linux",
        "1.1.3__arm64_linux",
        "1.1.3__arm_linux",
        "1.1.3__ppc64le_linux",
        "1.1.3__s390x_linux",
        "1.2.2",
        "1.2.3",
        "1.2.4",
        "1.2.6",
        "1.3.0",
        "1.3.1",
        "1.5.0",
        "1.6.2",
        "1.6.5",
        "1.6.6",
        "1.6.7",
        "1.7.0"
    ]
}
  • docker.io/coredns/coredns
root@debian:/root # skopeo list-tags docker://docker.io/coredns/coredns
{
    "Repository": "docker.io/coredns/coredns",
    "Tags": [
        "0.9.10",
        "0.9.9",
        "006",
        "007",
        "008",
        "009",
        "010",
        "011",
        "1.0.0",
        "1.0.1",
        "1.0.2",
        "1.0.3",
        "1.0.4",
        "1.0.5",
        "1.0.6",
        "1.1.0",
        "1.1.1",
        "1.1.2",
        "1.1.3",
        "1.1.4",
        "1.2.0",
        "1.2.1",
        "1.2.2",
        "1.2.3",
        "1.2.4",
        "1.2.5",
        "1.2.6",
        "1.3.0",
        "1.3.1",
        "1.4.0",
        "1.5.0",
        "1.5.1",
        "1.5.2",
        "1.6.0",
        "1.6.1",
        "1.6.2",
        "1.6.3",
        "1.6.4",
        "1.6.5",
        "1.6.6",
        "1.6.7",
        "1.6.9",
        "1.7.0",
        "1.7.1",
        "1.8.0",
        "1.8.3",
        "coredns-amd64",
        "coredns-arm",
        "coredns-arm64",
        "coredns-ppc64le",
        "coredns-s390x",
        "latest"
    ]
}

@MRoci MRoci force-pushed the kubeadm-coredns-img branch from de83100 to f628dee Compare April 29, 2021 07:21
@floryut
Copy link
Member

floryut commented Apr 29, 2021

You are right, should use k8s.gcr.io/coredns/coredns begin v1.21

These are coredns's tag in different image repo :

  • k8s.gcr.io/coredns/coredns
root@debian:/root # skopeo list-tags docker://k8s.gcr.io/coredns/coredns
{
    "Repository": "k8s.gcr.io/coredns/coredns",
    "Tags": [
        "v1.6.6",
        "v1.6.7",
        "v1.6.9",
        "v1.7.0",
        "v1.7.1",
        "v1.8.0",
        "v1.8.3"
    ]
}

Then let's switch to this {{ kube_image_repo }}/coredns/coredns and v1.x.x 🤷

@MRoci MRoci changed the title switch to docker.io for kubeadm coredns addon change kubeadm coredns addon images name to coredns/coredns Apr 29, 2021
@MRoci MRoci force-pushed the kubeadm-coredns-img branch from f628dee to feec8ce Compare April 29, 2021 07:27
@MRoci
Copy link
Contributor Author

MRoci commented Apr 29, 2021

@floryut updated the PR in order to use new gcr naming conventions, but maybe doing so using kubeadm 1.20 this will lead in trying to pull k8s.gcr.io/coredns:v1.7.0 which is also non existing ?
Maybe we can add a check on kubeadm version to prevent this ?

@floryut
Copy link
Member

floryut commented Apr 29, 2021

@floryut updated the PR in order to use new gcr naming conventions, but maybe doing so using kubeadm 1.20 this will lead in trying to pull k8s.gcr.io/coredns:v1.7.0 which is also non existing ?
Maybe we can add a check on kubeadm version to prevent this ?

Hum I'll let reviewers provide their input but I would say that a warning in the release note will be enough to warn users.

@champtar
Copy link
Contributor

I really like to be able to use latest kubespray release but the previous k8s version, can we just add a if depending on kubeadm / k8s version ?

@floryut
Copy link
Member

floryut commented Apr 29, 2021

I really like to be able to use latest kubespray release but the previous k8s version, can we just add a if depending on kubeadm / k8s version ?

Well as 1.19 also use coredns 1.7.0 it's ok
Ok I understand, sorry took me some time, the hardcoding "coredns/coredns" is only for 1.21, version 1.19/1.20 are still on "coredns" only, what a mess ~~
We do need to have a if on the kubernetes version.. keeping everyting on k8s.gcr.io, but depending on the version pointing to coredns/coredns:v1.xx or coredns:1.xx

@MRoci MRoci force-pushed the kubeadm-coredns-img branch from feec8ce to 94cb583 Compare April 29, 2021 13:31
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 29, 2021
@MRoci
Copy link
Contributor Author

MRoci commented Apr 29, 2021

@floryut ok, now it should be able to handle both cases: with kubeadm >=1.21 it uses coredns/coredns:v1.xx, coredns:1.xx is used otherwise

follow new naming conventions for gcr's coredns image.
starting from 1.21 kubeadm assumes it to be `coredns/coredns`:
this causes the kubeadm deployment being unable to pull image, beacuse `v`
was also added in image tag, until the role `kubernetes-apps` ovverides
it with the old name, which is only compatible with <=1.7.

Backward comptability with kubeadm <=1.20 is mantained checking
kubernetes version and falling back to old names (`coredns:1.xx`) when
the version is less than 1.21
@MRoci MRoci force-pushed the kubeadm-coredns-img branch from 94cb583 to f4ae844 Compare April 29, 2021 13:47
@champtar
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 30, 2021
@k8s-ci-robot k8s-ci-robot merged commit a0ee569 into kubernetes-sigs:master Apr 30, 2021
k8s-ci-robot pushed a commit that referenced this pull request May 11, 2021
@floryut floryut mentioned this pull request May 11, 2021
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Jun 12, 2021
…ubernetes-sigs#7570)

follow new naming conventions for gcr's coredns image.
starting from 1.21 kubeadm assumes it to be `coredns/coredns`:
this causes the kubeadm deployment being unable to pull image, beacuse `v`
was also added in image tag, until the role `kubernetes-apps` ovverides
it with the old name, which is only compatible with <=1.7.

Backward comptability with kubeadm <=1.20 is mantained checking
kubernetes version and falling back to old names (`coredns:1.xx`) when
the version is less than 1.21
LuckySB pushed a commit to southbridgeio/kubespray that referenced this pull request Jun 17, 2021
sakuraiyuta pushed a commit to sakuraiyuta/kubespray that referenced this pull request Apr 16, 2022
…ubernetes-sigs#7570)

follow new naming conventions for gcr's coredns image.
starting from 1.21 kubeadm assumes it to be `coredns/coredns`:
this causes the kubeadm deployment being unable to pull image, beacuse `v`
was also added in image tag, until the role `kubernetes-apps` ovverides
it with the old name, which is only compatible with <=1.7.

Backward comptability with kubeadm <=1.20 is mantained checking
kubernetes version and falling back to old names (`coredns:1.xx`) when
the version is less than 1.21
sakuraiyuta pushed a commit to sakuraiyuta/kubespray that referenced this pull request Apr 16, 2022
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants