-
Notifications
You must be signed in to change notification settings - Fork 614
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
Upgrade Kubernetes libraries to 1.18.4 #844
Conversation
This is a hard change as it requires a v1.18 client since older clients no longer implement the interface. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I would like us to have a plan for how we're going to test against a 1.18 cluster, but otherwise, unopposed. |
Please, change this https://github.com/virtual-kubelet/virtual-kubelet/blob/master/.circleci/config.yml#L67 to test w/ 1.18. Eventually, we can add sequential runs that test the supported versions of Kubernetes, or 1.1{6,7,8} as we speak. |
I just noticed this TODO item in package lockdeps
import (
// TODO(Sargun): Remove in Go1.13
// This is a dep that `go mod tidy` keeps removing, because it's a transitive dep that's pulled in via a test
// See: https://github.com/golang/go/issues/29702
_ "github.com/prometheus/client_golang/prometheus"
_ "golang.org/x/sys/unix"
) I'm not familiar with this issue. @sargun, should I remove as indicated? |
@adrienjt I can deal with cleanup after this is merged. |
Sorry for asking this after approval, but can the commit history be redone, please? It's quite misleading to read commit messages that no longer hold, like the installation of conntrack and e2e running on Kubernetes 1.18. |
7d1cf91
to
225c1bd
Compare
required by k8s libs at 1.18
225c1bd
to
9c6b48c
Compare
You're the best! Thank you very much. |
@@ -64,14 +67,16 @@ func (ts *EndToEndTestSuite) TestGetPods(t *testing.T) { | |||
// TestGetStatsSummary creates a pod having two containers and queries the /stats/summary endpoint of the virtual-kubelet. | |||
// It expects this endpoint to return stats for the current node, as well as for the aforementioned pod and each of its two containers. | |||
func (ts *EndToEndTestSuite) TestGetStatsSummary(t *testing.T) { | |||
ctx := context.Background() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this context be cancelled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote this PR as a no-op upgrade of a dependency, so when no context was available, I simply created a background context (no-op context). Maybe this PR creates opportunities for improvements, but I believe they'd be out-of-scope.
In this particular case, what's the value of cancelling the context? This is a test function where the context is used consecutively in a series of client-go calls.
Thanks to new v0.x.y tags in Kubernetes 1.17+ for libraries (client-go, etc.), it is no longer necessary to replace their requirements by paths in the staging area of the Kubernetes repo at the said version's Git commit hash. Upgrade Kubernetes libraries to 1.17 #834 (comment)
client-go introduced a major breaking change in Kubernetes 1.18, which is to pass context.Context to most functions.
This PR simplifies virtual-kubelet's go.mod file and updates calls to client-go.
EDIT
apimachinery (and maybe others) from v1.18 require go1.13, so I upgraded the Go version as well.
I also tried to run e2e tests in CI on 1.18 but gave up for now. This is not blocking IMO. Tests still pass on 1.17. We should actually run tests on multiple Kubernetes versions following a policy like https://kubernetes.io/docs/setup/release/version-skew-policy/