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

Update Docker to v20.10.5 #11195

Merged
merged 1 commit into from Apr 11, 2021

Conversation

bmelbourne
Copy link
Contributor

What this PR does / why we need it:
Update default Docker version to v20.10.5 for all Kubernetes versions v1.21+.

https://github.com/kubernetes/kubernetes/blob/v1.21.0/build/dependencies.yaml#L72-L77

Which issue(s) this PR fixes:
N/A

Special notes for your reviewer:

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 10, 2021
@bmelbourne
Copy link
Contributor Author

@hakman @olemarkus @rifelpet
I've added the following code snippet to func TestPopulateCluster_DockerVersion(t *testing.T) but it's not setting DockerVersion as it doesn't appear to recognise k8s v1.21.0 when it populates cluster.spec.KubernetesVersion.

https://github.com/kubernetes/kops/blob/master/upup/pkg/fi/cloudup/populatecluster_test.go#L374
https://github.com/kubernetes/kops/blob/master/upup/pkg/fi/cloudup/populate_cluster_spec.go#L220-L225

		{
			KubernetesVersion: "1.21.0",
			DockerVersion:     "20.10.5",
		},

I've also added the following code snippet to func (b *DockerOptionsBuilder) BuildOptions(o interface{}) in docker.go.

		if b.IsKubernetesGTE("1.21") {
			docker.Version = fi.String("20.10.5")

When I run hack/update-expected.sh, I get the following error.

--- FAIL: TestPopulateCluster_DockerVersion (0.00s)
    populatecluster_test.go:411: Unexpected DockerVersion:

If I change the k8s version to 1.19.0 in both code snippets, the updated-expected.sh script reports no errors.

How is the KubernetesVersion validated for the integration tests?

Copy link
Member

@hakman hakman left a comment

Choose a reason for hiding this comment

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

@bmelbourne I took a quick look and I think you are changing too much. Unit tests should not be updated on each new Docker release. Same goes for docs examples.

To fix the error, you will need to update TestPopulateCluster_DockerVersion to always set container runtime to docker. As you know, starting kOps 1.20 the default container runtime is containerd and so, there is no Docker version by default.

--- a/upup/pkg/fi/cloudup/populatecluster_test.go
+++ b/upup/pkg/fi/cloudup/populatecluster_test.go
@@ -392,11 +392,16 @@ func TestPopulateCluster_DockerVersion(t *testing.T) {
                        KubernetesVersion: "1.17.0",
                        DockerVersion:     "19.03.15",
                },
+               {
+                       KubernetesVersion: "1.21.0",
+                       DockerVersion:     "20.10.5",
+               },
        }
 
        for _, test := range grid {
                _, c := buildMinimalCluster()
                c.Spec.KubernetesVersion = test.KubernetesVersion
+               c.Spec.ContainerRuntime = "docker"
 
                full, err := build(c)
                if err != nil {

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 10, 2021
@bmelbourne
Copy link
Contributor Author

@hakman
Thanks for responding. Good to discover only a simple fix is required. I suspected it had something to do with the default container runtime being set as containerd but as this set of integration tests was specific to Docker, I just wanted to be sure that I hadn't overlooked something obvious elsewhere in the codebase.

I've reduced the number of changes to keep the PR focused on upgrading Docker to v20.10.5.

upup/pkg/fi/cloudup/containerd.go Outdated Show resolved Hide resolved
upup/pkg/fi/cloudup/docker_test.go Outdated Show resolved Hide resolved
docs/cluster_spec.md Outdated Show resolved Hide resolved
docs/cluster_spec.md Outdated Show resolved Hide resolved
@hakman
Copy link
Member

hakman commented Apr 10, 2021

Thanks @bmelbourne. I added a few more suggestions that should reduce the scope even more.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 10, 2021
Copy link
Member

@hakman hakman left a comment

Choose a reason for hiding this comment

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

Last set of changes, you should remove the WIP after it. :)

@bmelbourne bmelbourne changed the title [WIP] Update Docker to v20.10.5 Update Docker to v20.10.5 Apr 11, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 11, 2021
@bmelbourne
Copy link
Contributor Author

/retest

@bmelbourne bmelbourne requested a review from hakman April 11, 2021 11:13
@hakman
Copy link
Member

hakman commented Apr 11, 2021

/retest

1 similar comment
@hakman
Copy link
Member

hakman commented Apr 11, 2021

/retest

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman

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 11, 2021
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2021
@bmelbourne
Copy link
Contributor Author

/retest

@hakman
Copy link
Member

hakman commented Apr 11, 2021

/lgtm
/retest

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 11, 2021
@k8s-ci-robot k8s-ci-robot merged commit 034bd35 into kubernetes:master Apr 11, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Apr 11, 2021
@bmelbourne bmelbourne deleted the update-docker-20.10.5 branch April 13, 2021 17:18
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/channels area/documentation cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants