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.

Sign up for GitHub

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

Jump to bottom

go1.14 fixup #92438

Merged
merged 9 commits into from Jun 24, 2020
Merged

go1.14 fixup #92438

merged 9 commits into from Jun 24, 2020

Conversation

liggitt
Copy link
Member

@liggitt liggitt commented Jun 23, 2020 •
edited

What type of PR is this?
/kind cleanup

follow-up to #88638

  • Updates required go version to 1.14.4
  • Fixes gofmt/govet errors
  • Fixes a test/cmd error message check to match what go 1.14 net/http produces
  • Updates staticcheck to a level that works with go1.14 (and tolerates newly found staticcheck errors)
  • Updates go.mod files to specify go 1.14
  • Updates hack/{update-vendor.sh,update-vendor-licenses.sh,lint-dependencies.sh} to work with go 1.14 vendor behavior
go1.14.4 is now the minimum version required for building Kubernetes

/sig release

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. sig/release Categorizes an issue or PR as relevant to SIG Release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/code-generation labels Jun 23, 2020
@k8s-ci-robot k8s-ci-robot added area/kubectl area/test approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jun 23, 2020
@liggitt
Copy link
Member Author

liggitt commented Jun 23, 2020

/cc @BenTheElder @justaugustus

@BenTheElder
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2020
justaugustus
justaugustus requested changes Jun 23, 2020
View reviewed changes
Copy link
Member

@justaugustus justaugustus left a comment

Choose a reason for hiding this comment

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

@liggitt -- One blocking question about dependencies.yaml

And then, non-blocking: what's the signal on when/if we should make these changes in staging? Should this be part of the go bump docs?

/hold

hack/lib/golang.sh
@@ -468,7 +468,7 @@ EOF
local go_version
IFS=" " read -ra go_version <<< "$(go version)"
local minimum_go_version
minimum_go_version=go1.13.4
minimum_go_version=go1.14.4
Copy link
Member

Choose a reason for hiding this comment

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

Should this always be the latest go version we've bumped to?
If so, can you add an entry in build/dependencies.yaml for this path/regex?

Copy link
Member

Choose a reason for hiding this comment

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

it should, BUT that would require the image bump to go in before the PR, FWIW

Copy link
Member

Choose a reason for hiding this comment

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

Ooof. Chicken and egg! Thanks for clarifying, @BenTheElder!
/hold cancel

Copy link
Member

Choose a reason for hiding this comment

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

we still have this chicken and egg either way, the question is just which repo we want to merge first 🤷‍♂️

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jun 23, 2020
@justaugustus
Copy link
Member

/test pull-kubernetes-integration

@liggitt
Copy link
Member Author

liggitt commented Jun 23, 2020

/hold

still working through staticcheck, golang.org/x/... deps, and hack/update-vendor failures

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 23, 2020
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2020
@liggitt
Copy link
Member Author

liggitt commented Jun 23, 2020

awesome, platform-specific staticcheck differences... fixed up in https://github.com/kubernetes/kubernetes/compare/b55f384056dfd975b760fb1ae7264e8320dca287..d9bb0b8ee18fe68654ef1a68611f905859b3b7bc

@BenTheElder
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2020
@liggitt
Copy link
Member Author

liggitt commented Jun 24, 2020

/retest

@liggitt
Copy link
Member Author

liggitt commented Jun 24, 2020

grr
#89719

/retest

@BenTheElder
Copy link
Member

/retest

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jun 24, 2020 •
edited

@liggitt: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-kubernetes-conformance-image-test be372789b49218c274b146fda90695489bad0643 link /test pull-kubernetes-conformance-image-test
pull-kubernetes-e2e-gce-storage-snapshot be372789b49218c274b146fda90695489bad0643 link /test pull-kubernetes-e2e-gce-storage-snapshot
pull-kubernetes-e2e-gci-gce-ipvs be372789b49218c274b146fda90695489bad0643 link /test pull-kubernetes-e2e-gci-gce-ipvs
pull-kubernetes-e2e-gce-csi-serial be372789b49218c274b146fda90695489bad0643 link /test pull-kubernetes-e2e-gce-csi-serial
pull-kubernetes-e2e-gce-storage-slow be372789b49218c274b146fda90695489bad0643 link /test pull-kubernetes-e2e-gce-storage-slow
pull-kubernetes-e2e-azure-file-windows d9bb0b8 link /test pull-kubernetes-e2e-azure-file-windows
pull-kubernetes-e2e-azure-disk-windows d9bb0b8 link /test pull-kubernetes-e2e-azure-disk-windows

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot merged commit 4de511a into kubernetes:master Jun 24, 2020
@k8s-ci-robot k8s-ci-robot added this to the v1.19 milestone Jun 24, 2020
@liggitt liggitt deleted the 1.14 branch June 24, 2020 03:56
This was referenced Jun 24, 2020
Merged
Merged
Merged
@AkihiroSuda
Copy link
Member

Why was support for 1.13 dropped?

@BenTheElder
Copy link
Member

BenTheElder commented Jun 29, 2020 via email

@AkihiroSuda
Copy link
Member

I suggest continuing support for Go 1.13 exceptionally.

Go 1.14 introduced a breaking change that requires retrying syscall executions on receiving EINTR: https://golang.org/doc/go1.14#runtime

Because of this change, a lot of runtime components including containerd, runc (<= rc90), Moby, and CNI haven't yet migrated to Go 1.14.
Requiring Go 1.14 as a hard dependency is likely to break downstream build pipelines.

Also, I even doubt that Kubernetes has really completed support for Go 1.14.
I can't find EINTR handlers in the repo (as of aadaa5d) except in a single function:

$ git grep EINTR | grep -v ^vendor
pkg/kubelet/eviction/threshold_notifier_linux.go:               if err == unix.EINTR {

( Fortunately migration to Go 1.15 would be much smoother, as Go 1.15 can handle most of the EINTR stuffs automatically: https://tip.golang.org/doc/go1.15#os )

@liggitt liggitt mentioned this pull request Jun 29, 2020
Merged
@liggitt
Copy link
Member Author

liggitt commented Jun 29, 2020

Given the support window of go1.13 (it will go out of support before Kubernetes 1.19 is released), I think k8s 1.19 will stay on go1.14+

I opened #92611 as an experiment for what it could look like to isolate the bits that actually required go1.14 to let someone who wanted to build with k8s libraries build with go1.13. Since that change appears to be very small, I'd be amenable to keeping that in place until we move to go1.15.

@BenTheElder
Copy link
Member

BenTheElder commented Jun 29, 2020 via email

@liggitt
Copy link
Member Author

liggitt commented Jun 29, 2020

Downstream build pipelines should not be depending on a single host go binary, we make no guarantees about what timeframe we will upgrade go in.

I'm less concerned about build pipelines building kubernetes components, and am more sympathetic to something using the published k8s libraries.

@github-actions github-actions bot mentioned this pull request Jun 30, 2020
Open
This was referenced Jul 7, 2020
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justaugustus justaugustus justaugustus requested changes

@BenTheElder BenTheElder BenTheElder left review comments

@cblecker cblecker cblecker approved these changes

@fejta fejta Awaiting requested review from fejta

@hongchaodeng hongchaodeng Awaiting requested review from hongchaodeng

Assignees

@BenTheElder BenTheElder

@cblecker cblecker

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/apiserver area/cloudprovider area/code-generation area/conformance Issues or PRs related to kubernetes conformance tests area/dependency Issues or PRs related to dependency changes area/ipvs area/kubectl area/kubelet area/release-eng Issues or PRs related to the Release Engineering subproject area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/cli Categorizes an issue or PR as relevant to SIG CLI. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/release Categorizes an issue or PR as relevant to SIG Release. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.19
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@liggitt @BenTheElder @justaugustus @k8s-ci-robot @fejta-bot @AkihiroSuda @cblecker