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 To Go 1.15.0 #371

Merged
merged 2 commits into from
Aug 31, 2020
Merged

Conversation

seanmalloy
Copy link
Member

As part of the k8s 1.19 release cycle the Go version is being bumped to
1.15.0. Updating the descheduler to use Go 1.15 prior to the descheduler
v0.19.0 release.

See below issues for reference:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 12, 2020
@seanmalloy seanmalloy mentioned this pull request Aug 12, 2020
@seanmalloy
Copy link
Member Author

/hold

We should wait until the k/k repo is successfully updated to use Go 1.15.0. Also, the descheduler test-infra jobs for the master branch should be updated to use Go 1.15.0.

@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 Aug 12, 2020
@seanmalloy
Copy link
Member Author

The k/k repo has been updated with Go 1.15.0: kubernetes/kubernetes#93939. I believe this plan for the k8s 1.19 release is still to use Go 1.15.0.

I believe all that needs to be done now for the descheduler is to update test-infra jobs to use Go 1.15. Then this PR can be retested and merged.

@seanmalloy
Copy link
Member Author

Created PR kubernetes/test-infra#18836 to update the descheduler test-intra jobs to use Go 1.15.

@ingvagabund
Copy link
Contributor

@seanmalloy is the code currently vendored in the descheduler buildable with go1.15?

@seanmalloy
Copy link
Member Author

@seanmalloy is the code currently vendored in the descheduler buildable with go1.15?

Yes, I was able to compile with go 1.15 and run "go mod vendor" and "go mod tidy". No changes to the vendor directory were made when updating from go 1.14 to go 1.15.

@seanmalloy
Copy link
Member Author

seanmalloy commented Aug 17, 2020

I did notice that make verify fails with Go 1.15. I will have to figure out how to fix this before we can merge this.

Error details:

make verify
./hack/verify-gofmt.sh
./hack/verify-vendor.sh
Vendor Verified.
Removing ./hack/../_tmp/kube-vendor.3Zn7Ec
./_output/bin/golangci-lint run
WARN [runner/gosimple] Can't run megacheck because of compilation errors in packages [sigs.k8s.io/descheduler/pkg/apis/componentconfig sigs.k8s.io/descheduler/pkg/apis/componentconfig/v1alpha1 sigs.k8s.io/descheduler/pkg/api sigs.k8s.io/descheduler/pkg/api/v1alpha1 sigs.k8s.io/descheduler/pkg/descheduler/scheme sigs.k8s.io/descheduler/cmd/descheduler/app/options sigs.k8s.io/descheduler/pkg/descheduler/client sigs.k8s.io/descheduler/pkg/descheduler/evictions/utils sigs.k8s.io/descheduler/pkg/utils sigs.k8s.io/descheduler/cmd/descheduler/app sigs.k8s.io/descheduler/test sigs.k8s.io/descheduler/pkg/descheduler [sigs.k8s.io/descheduler/pkg/descheduler.test] sigs.k8s.io/descheduler/pkg/descheduler/evictions [sigs.k8s.io/descheduler/pkg/descheduler/evictions.test] sigs.k8s.io/descheduler/pkg/descheduler/node [sigs.k8s.io/descheduler/pkg/descheduler/node.test] sigs.k8s.io/descheduler/pkg/descheduler/pod [sigs.k8s.io/descheduler/pkg/descheduler/pod.test] sigs.k8s.io/descheduler/pkg/descheduler/strategies [sigs.k8s.io/descheduler/pkg/descheduler/strategies.test] sigs.k8s.io/descheduler/test/e2e [sigs.k8s.io/descheduler/test/e2e.test]]: pkg/apis/componentconfig/doc.go:1: /Users/foo/tech/go/src/fmt/scan.go:334:15: RuneLen not declared by package utf8 and 1988 more errors: run `golangci-lint run --no-config --disable-all -E typecheck` to see all errors
helm lint ./charts/descheduler
==> Linting ./charts/descheduler

1 chart(s) linted, 0 chart(s) failed

@seanmalloy
Copy link
Member Author

I got a little bit closer. I had to update golangci-lint to 1.30.0 to make it work with Go 1.15. But now the new version of golangci-lint is failing with the below errors:

./_output/bin/golangci-lint run
pkg/descheduler/strategies/lownodeutilization_test.go:817:27: copylocks: literal copies lock value from *fakePtr: k8s.io/client-go/testing.Fake (govet)
				&fake.Clientset{Fake: *fakePtr},
				                      ^
pkg/descheduler/strategies/lownodeutilization_test.go:825:50: copylocks: literal copies lock value from *fakePtr: k8s.io/client-go/testing.Fake (govet)
			LowNodeUtilization(ctx, &fake.Clientset{Fake: *fakePtr}, strategy, item.nodes, podEvictor)
			                                              ^
make: *** [Makefile:89: lint] Error 1

@ingvagabund @lixiang233 @damemi any ideas what I need to change to make the new version of golangci-lint happy?

@lixiang233
Copy link
Contributor

pkg/descheduler/strategies/lownodeutilization_test.go:817:27: copylocks: literal copies lock value from *fakePtr: k8s.io/client-go/testing.Fake (govet)

@seanmalloy This error haapens because Fake contains sync.RWMutex and we are passing a copy of Fake which may cause dead lock(it should be referred to through a pointer).

I think we can refactor newFake to create a fake.Clientset just like NewSimpleClientset. E.g.

func newFakeClient(objects ...runtime.Object) *fake.Clientset {
	...

	fakeClient := &fake.Clientset{}
	fakeClient.AddReactor("list", "pods", func(action core.Action) (bool, runtime.Object, error) {
		...
	})
	fakeClient.AddReactor("*", "*", core.ObjectReaction(o))
	fakeClient.AddWatchReactor("*", core.DefaultWatchReactor(watch.NewFake(), nil))

	return fakeClient
}

@ingvagabund
Copy link
Contributor

@lixiang233 @seanmalloy let's drop the fake completely: #385

@seanmalloy
Copy link
Member Author

I will rebase this PR after #385 is merged. Then I think it might work.

@ingvagabund
Copy link
Contributor

/retest

1 similar comment
@ingvagabund
Copy link
Contributor

/retest

As part of the k8s 1.19 release cycle the Go version is being bumped to
1.15.0. Updating the descheduler to use Go 1.15 prior to the descheduler
v0.19.0 release.

See below issues for reference:
* kubernetes/release#1421
* kubernetes/kubernetes#93484
@seanmalloy
Copy link
Member Author

/hold cancel
/assign @damemi @ingvagabund

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 31, 2020
@ingvagabund
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 Aug 31, 2020
Copy link
Contributor

@damemi damemi left a comment

Choose a reason for hiding this comment

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

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, seanmalloy

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 Aug 31, 2020
@k8s-ci-robot k8s-ci-robot merged commit 74d6be3 into kubernetes-sigs:master Aug 31, 2020
@seanmalloy seanmalloy deleted the go-1.15 branch August 31, 2020 19:23
briend pushed a commit to briend/descheduler that referenced this pull request Feb 11, 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. lgtm "Looks good to me", indicates that a PR is ready to be merged. 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