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

Bump vendored kubernetes to v1.12.3 #140

Merged
merged 2 commits into from
Dec 6, 2018

Conversation

dkoshkin
Copy link
Contributor

@dkoshkin dkoshkin commented Dec 5, 2018

Is this a bug fix or adding new feature?
Bug fix

What is this PR about? / Why do we need it?
When trying to test and build locally I get:

➜  aws-ebs-csi-driver git:(master) make test
go test -v -race ./pkg/...
go: verifying k8s.io/[email protected]: checksum mismatch
	downloaded: h1:JEj2cxR+5T31U4klP5hI5dxjr6udRIHm+4dzCEPk498=
	go.sum:     h1:J94sY8nziFyzMNhnbD5CiSZTo7yWytir0ia+vJuwYfc=
make: *** [test] Error 1
➜  aws-ebs-csi-driver git:(master) go version
go version go1.11.2 darwin/amd64

I'm not really sure why this works in CI and when running make image, maybe its a darwin thing 🤷‍♂️.

If I delete mod.sum and recreate it, the hash for k8s.io/kubernetes is different

-k8s.io/kubernetes v1.12.2 h1:J94sY8nziFyzMNhnbD5CiSZTo7yWytir0ia+vJuwYfc=
+k8s.io/kubernetes v1.12.2 h1:JEj2cxR+5T31U4klP5hI5dxjr6udRIHm+4dzCEPk498=

Either way, Kubernetes 1.12.3 is out with a fix for a major vulnerability and bumping up the version fixes this mod issue also.

What testing is done?
After updating to the new version:

➜  aws-ebs-csi-driver git:(vendored-kubernetes) make
mkdir -p bin
CGO_ENABLED=0 GOOS=linux go build -ldflags "-X github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver.driverVersion=0.1.0-alpha -X github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver.gitCommit=643014b968afff5cff6909ace80f7de56861291e -X github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver.buildDate=2018-12-05T19:24:56Z" -o bin/aws-ebs-csi-driver ./cmd/
➜  aws-ebs-csi-driver git:(vendored-kubernetes) echo $?
0
➜  aws-ebs-csi-driver git:(vendored-kubernetes) make test
go test -v -race ./pkg/...
...
➜  aws-ebs-csi-driver git:(vendored-kubernetes) echo $?
0

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Dec 5, 2018
@k8s-ci-robot
Copy link
Contributor

Hi @dkoshkin. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes 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 added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 5, 2018
@leakingtapan
Copy link
Contributor

/ok-to-test

@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 Dec 5, 2018
@leakingtapan
Copy link
Contributor

@dkoshkin which go version are you using? Now travis job failed with similar error message

@leakingtapan
Copy link
Contributor

Could be related to this old issue: golang/go#27925

@dkoshkin
Copy link
Contributor Author

dkoshkin commented Dec 5, 2018

@leakingtapan Im using:

➜  aws-ebs-csi-driver git:(master) go version
go version go1.11.2 darwin/amd64

I see you have already commented on that issue, what version are you using?

Looks like the hash is different for Linux and Darwin from this comment.

@leakingtapan
Copy link
Contributor

leakingtapan commented Dec 5, 2018

I see you have already commented on that issue, what version are you using?

Im using go version go1.11 darwin/amd64

Could you try bump the go version in our traivs build to see if it helps?

@bertinatto
Copy link
Member

bertinatto commented Dec 6, 2018

Strange, I'm using go version go1.11.2 linux/amd64 and I don't get any errors.

If I delete mod.sum and recreate it, the hash for k8s.io/kubernetes is different

Yeah, this happened to me many times in the past. I thought it was fixed in 1.11.2 because it ceased to happen for me.

-k8s.io/kubernetes v1.12.2 h1:J94sY8nziFyzMNhnbD5CiSZTo7yWytir0ia+vJuwYfc=
+k8s.io/kubernetes v1.12.2 h1:JEj2cxR+5T31U4klP5hI5dxjr6udRIHm+4dzCEPk498=

I'm surprised that you didn't get v1.13 instead (since you deleted go.[mod,sum]).

@dkoshkin dkoshkin force-pushed the vendored-kubernetes branch from 643014b to 3cc15ea Compare December 6, 2018 13:43
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. 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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 6, 2018
@dkoshkin dkoshkin force-pushed the vendored-kubernetes branch from c09ca60 to d6486de Compare December 6, 2018 14:22
@dkoshkin
Copy link
Contributor Author

dkoshkin commented Dec 6, 2018

@leakingtapan, @bertinatto so it does look like its a Go 1.11.1 issue, but requires to clean cache - golang/go#27925 (comment).

The issue was being caused due to the version differences between our dev machines (go 1.11.2), and the fact that the builder image and Travis were both still on Go 1.11.1, I've updated both of those versions and CI passed.

I was also able to reproduce this issue locally:
Running make in below container will fail, switching to 1.11.2 builds correctly.

docker run -v `pwd`:`pwd` -w `pwd` -i -t golang:1.11.1

Also let me know if you want me to modify this PR to update to Kubernetes 1.13, but that seems a little risky as it was just released.

@dkoshkin dkoshkin force-pushed the vendored-kubernetes branch from d6486de to 291b5d2 Compare December 6, 2018 14:50
@leakingtapan
Copy link
Contributor

Also let me know if you want me to modify this PR to update to Kubernetes 1.13, but that seems a little risky as it was just released.

We could do it later to not embrace the risk all at once

@leakingtapan
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 Dec 6, 2018
@leakingtapan
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dkoshkin, leakingtapan

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 Dec 6, 2018
@k8s-ci-robot k8s-ci-robot merged commit f3cec38 into kubernetes-sigs:master Dec 6, 2018
@coveralls
Copy link

coveralls commented Dec 8, 2018

Pull Request Test Coverage Report for Build 225

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 51.596%

Totals Coverage Status
Change from base Build 220: 0.0%
Covered Lines: 582
Relevant Lines: 1128

💛 - Coveralls

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. 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.

5 participants