Go 1.13 + Moving to go modules #461

avalluri · 2019-11-09T04:16:16Z

Set of changes to move to go-1.13 release and introducing go modules for dependency tracking. With this change we no more use 'dep' tool going forward.

pohly

Something is odd. Despite -mod=vendor, make test ends up downloading some stuff:

GOOS=linux GO111MODULE=on GOFLAGS=-mod=vendor go test -run none ./pkg/... ./test/e2e
?   	github.com/intel/pmem-csi/pkg/coverage	[no test files]
?   	github.com/intel/pmem-csi/pkg/ndctl	[no test files]
?   	github.com/intel/pmem-csi/pkg/pmem-common	[no test files]
?   	github.com/intel/pmem-csi/pkg/pmem-csi-driver	[no test files]
?   	github.com/intel/pmem-csi/pkg/pmem-device-manager	[no test files]
?   	github.com/intel/pmem-csi/pkg/pmem-exec	[no test files]
?   	github.com/intel/pmem-csi/pkg/pmem-grpc	[no test files]
ok  	github.com/intel/pmem-csi/pkg/pmem-ns-init	0.011s [no tests to run]
?   	github.com/intel/pmem-csi/pkg/pmem-registry	[no test files]
ok  	github.com/intel/pmem-csi/pkg/pmem-state	0.006s [no tests to run]
?   	github.com/intel/pmem-csi/pkg/pmem-vgm	[no test files]
ok  	github.com/intel/pmem-csi/pkg/registryserver	0.012s [no tests to run]
ok  	github.com/intel/pmem-csi/test/e2e	0.035s [no tests to run]
go: downloading github.com/docker/go-units v0.4.0
go: downloading golang.org/x/net v0.0.0-20191027233614-53de4c7853b5
go: downloading k8s.io/utils v0.0.0-20190801114015-581e00157fb1
go: downloading k8s.io/klog v1.0.0
go: downloading google.golang.org/grpc v1.24.0
...

You can see that after rm -rf $GOPATH/pkg/mod.

Some of your later commits are fixes for the previous ones. Please squash. It might have to be in a single commit.

Dockerfile

go.mod

Jenkinsfile

avalluri · 2019-11-13T08:45:26Z

Something is odd. Despite -mod=vendor, make test ends up downloading some stuff:

GOOS=linux GO111MODULE=on GOFLAGS=-mod=vendor go test -run none ./pkg/... ./test/e2e
?   	github.com/intel/pmem-csi/pkg/coverage	[no test files]
?   	github.com/intel/pmem-csi/pkg/ndctl	[no test files]
?   	github.com/intel/pmem-csi/pkg/pmem-common	[no test files]
?   	github.com/intel/pmem-csi/pkg/pmem-csi-driver	[no test files]
?   	github.com/intel/pmem-csi/pkg/pmem-device-manager	[no test files]
?   	github.com/intel/pmem-csi/pkg/pmem-exec	[no test files]
?   	github.com/intel/pmem-csi/pkg/pmem-grpc	[no test files]
ok  	github.com/intel/pmem-csi/pkg/pmem-ns-init	0.011s [no tests to run]
?   	github.com/intel/pmem-csi/pkg/pmem-registry	[no test files]
ok  	github.com/intel/pmem-csi/pkg/pmem-state	0.006s [no tests to run]
?   	github.com/intel/pmem-csi/pkg/pmem-vgm	[no test files]
ok  	github.com/intel/pmem-csi/pkg/registryserver	0.012s [no tests to run]
ok  	github.com/intel/pmem-csi/test/e2e	0.035s [no tests to run]
go: downloading github.com/docker/go-units v0.4.0
go: downloading golang.org/x/net v0.0.0-20191027233614-53de4c7853b5
go: downloading k8s.io/utils v0.0.0-20190801114015-581e00157fb1
go: downloading k8s.io/klog v1.0.0
go: downloading google.golang.org/grpc v1.24.0
...

These ad hoc package downloads are coming from make test-kustomize while building kustomize binary.
We have 2 options if we want to avoid these package downloads:

Download kustomize release binary instead of building from source
Maintain kustomize dependencies part of vendor tree.

The same applies for govm binary while make start.

pohly · 2019-11-13T09:20:11Z

These ad hoc package downloads are coming from make test-kustomize while building kustomize binary.

That's okay. I just wanted to be sure that we are vendoring correctly (i.e. vendor is up-to-date) and really use it. Speaking of "up-to-date": we probably need a test for that. Otherwise we might end up making source code changes or dependency changes (go get) that aren't followed by "go mod tidy && go mod vendor" and/or committed without the updated vendor dir. Can you copy the test that I wrote for kubernetes-csi and integrate it into "make test"? We don't need the dep part and the if check for skipping the test must be modified to work in Jenkins.

https://github.com/kubernetes-csi/csi-release-tools/blob/master/verify-vendor.sh

We have 2 options if we want to avoid these package downloads:

Download kustomize release binary instead of building from source

Maintain kustomize dependencies part of vendor tree.

I found that downloading from github.com can be pretty slow, sometimes even slower than building from source. I'm just pointing that out as something that we need to check before deciding. But in this case the actual binaries seems to be hosted on AWS and getting them was fast for me, so this might be the best solution.

I'd prefer to not add tool sources to our vendor dir if we can avoid it because it makes our BOM larger and harder to maintain.

Makefile

pohly · 2019-11-14T10:40:02Z

There's a new failure in the CI (https://cloudnative-k8sci.southcentralus.cloudapp.azure.com/blue/organizations/jenkins/pmem-csi/detail/PR-461/8/pipeline):

[2019-11-14T10:09:09.184Z] PATH="/mnt/workspace/pmem-csi_PR-461/_work/bin:$PATH" test/start-kubernetes.sh
[2019-11-14T10:09:09.184Z] get current user: user: Current requires cgo or $USER set in environment

avalluri · 2019-11-14T11:20:53Z

There's a new failure in the CI (https://cloudnative-k8sci.southcentralus.cloudapp.azure.com/blue/organizations/jenkins/pmem-csi/detail/PR-461/8/pipeline):
[2019-11-14T10:09:09.184Z] PATH="/mnt/workspace/pmem-csi_PR-461/_work/bin:$PATH" test/start-kubernetes.sh
[2019-11-14T10:09:09.184Z] get current user: user: Current requires cgo or $USER set in environment

I suspect due to change in govm version, I will check.

pohly · 2019-11-14T11:24:20Z

Can you copy the test that I wrote for kubernetes-csi and integrate it into "make test"? We don't need the dep part and the if check for skipping the test must be modified to work in Jenkins.

@avalluri: are you working on that or shall I add that later?

avalluri · 2019-11-14T11:29:53Z

Can you copy the test that I wrote for kubernetes-csi and integrate it into "make test"? We don't need the dep part and the if check for skipping the test must be modified to work in Jenkins.

@avalluri: are you working on that or shall I add that later?

I will do that, part of this PR.

pohly · 2019-11-14T15:29:36Z

test/test.make

 .PHONY: test_vendor_bom
 test: test_vendor_bom
 test_vendor_bom:
 	@ if ! diff -c \
 		<(tail -n +2 vendor-bom.csv | sed -e 's/;.*//') \
-		<((grep '^  name =' Gopkg.lock  | sed -e 's/.*"\(.*\)"/\1/') | LC_ALL=C LANG=C sort); then \
+		<(for path in $$(go list -f '{{.Path}}' -m all); do if [ -d "vendor/$$path" ]; then echo "$$path"; fi; done | LC_ALL=C LANG=C sort); then \


I started wondering whether we need $(GO) (and thus GOFLAGS=-mod=vendor) here instead of just go. Sure enough, using go results in downloading packages (https://cloudnative-k8sci.southcentralus.cloudapp.azure.com/blue/organizations/jenkins/pmem-csi/detail/PR-461/14/pipeline/51).

There's just one snag: with GOFLAGS=-mod=vendor, some modules aren't listed anymore (for example, k8s.io/kubectl) even though we have that module in vendor. This needs further investigation, don't merge.

Raised the question upstream: golang/go#35589

@avalluri: please remove this commit and instead cherry-pick this one: pohly@43dfe7f

I switched to a different approach. Does that make sense?

Yes, sound more promising than depending on go list. I will cherry-pick.

@avalluri: please remove this commit and instead cherry-pick this one: pohly@43dfe7f

Done

pohly

We no longer need to check out in GOPATH (https://github.com/intel/pmem-csi/blob/devel/README.md#get-source-code).

Please change that section. I suggest:

Get source code

PMEM-CSI uses Go modules and thus can be checked out and (if that should be desired) built anywhere in the filesystem. Pre-built container images are available and thus users don't need to build from source, but they will still need some additional files. To get the source code, use:

git clone https://github.com/intel/pmem-csi

Result of below set of commands: go mod init go get k8s.io/kubernetes@release-1.16 go get k8s.io/<k8s-family-projects>@kubernetes-1.16.2 go mod tidy go mod vendor The way package paths returned by v1.13 'go list' is different from previous go versions. Made changes to rest_runtime target according to this. TODO: We can drop Gopkg.lock file, but 'test_vendor_bom' make target uses this file to retrieve the package list to validate vendor tree. One should replace this check with go module equivalent.

We used to base the analysis on the modules imported by dep. With the switch to go mod vendoring we can no longer do that and it also wasn't quite complete: some of the imported modules embed code from elsewhere which sometimes has other licenses. Therefore we now look for license files and list anything that has a separate license.

When go modules enabled user code is not intended be in GOROOT(/go) hence placing source code in /src. Otherwise we hit with below error: go: inconsistent vendoring in /go/src/github.com/intel/pmem-csi: go.mod requires github.com/intel/pmem-csi but vendor/modules.txt does not include it. run 'go mod tidy; go mod vendor' to sync make: *** [Makefile:70: pmem-csi-driver] Error 1 Ref: golang/go#34657 Jenkinsfile: Place the pmem-csi source out of GOPATH

…ource Building from source needs downloading of all dependent source packages, instead we download the release binaries. Change in Kustomize version resulted in changes to few generated deployment files.

This to fix build failure appeared in CI after recent govm update: ----- make start tar zxf _work/govm_0.9-alpha_Linux_amd64.tar.gz -C _work/bin/ PATH="/mnt/workspace/pmem-csi_PR-461/_work/bin:$PATH" test/start-kubernetes.sh get current user: user: Current requires cgo or $USER set in environment make: *** [test/start-stop.make:13: start] Error 1 -----

After recent govm update, observed that occasionally cluster creation fails due to ssh connection timeout, Hence increasing the timeout to 2min.

avalluri · 2019-11-15T10:35:15Z

PMEM-CSI uses Go modules and thus can be checked out and (if that should be desired) built anywhere in the filesystem. Pre-built container images are available and thus users don't need to build from source, but they will still need some additional files. To get the source code, use:

git clone https://github.com/intel/pmem-csi

Good point. Done

avalluri · 2019-11-15T10:37:02Z

Added new commit that increases the ssh timeout while creating test cluster.

pohly · 2019-11-15T10:41:43Z

test/start-kubernetes.sh

@@ -78,7 +78,7 @@ case ${TEST_DISTRO} in
        ;;
 esac

-SSH_TIMEOUT=60


So the cluster now comes up slower than before? That's worrisome, but not much that we can do about it. 🤷‍♂️

To my surprise, It was not observed in CI but on my dev machine, the failure was quite frequent.

avalluri · 2019-11-15T12:53:47Z

A test failure in e2e tests, I suspect unrelated this change:

19-11-15T12:26:56.220Z]   late binding
[2019-11-15T12:26:56.220Z]   �[90m/src/pmem-csi/test/e2e/storage/csi_volumes.go:122�[0m
[2019-11-15T12:26:56.220Z]     �[91m�[1mworks [AfterEach]�[0m
[2019-11-15T12:26:56.220Z]     �[90m/src/pmem-csi/test/e2e/storage/csi_volumes.go:163�[0m
[2019-11-15T12:26:56.220Z] 
[2019-11-15T12:26:56.220Z]     �[91msame volumes before and after the test
[2019-11-15T12:26:56.220Z]     Expected
[2019-11-15T12:26:56.220Z]         <map[string][]string | len:3>: {
[2019-11-15T12:26:56.220Z]             "LVM Volumes on Node 1": [""],
[2019-11-15T12:26:56.220Z]             "LVM Volumes on Node 2": [""],
[2019-11-15T12:26:56.220Z]             "LVM Volumes on Node 3": [""],
[2019-11-15T12:26:56.220Z]         }
[2019-11-15T12:26:56.220Z]     to equal
[2019-11-15T12:26:56.220Z]         <map[string][]string | len:3>: {
[2019-11-15T12:26:56.220Z]             "LVM Volumes on Node 1": [""],
[2019-11-15T12:26:56.220Z]             "LVM Volumes on Node 2": [""],
[2019-11-15T12:26:56.220Z]             "LVM Volumes on Node 3": [
[2019-11-15T12:26:56.221Z]                 "WARNING: Failed to connect to lvmetad. Falling back to device scanning.",
[2019-11-15T12:26:56.221Z]             ],
[2019-11-15T12:26:56.221Z]         }�[0m
[2019-11-15T12:26:56.221Z] 
[2019-11-15T12:26:56.221Z]     /src/pmem-csi/test/e2e/storage/sanity.go:1039

okartau · 2019-11-15T13:02:48Z

A test failure in e2e tests, I suspect unrelated this change:

That is from recently merged volume leak test.
Seems to be a LVM subsystem problem getting LV volums list
Such error did not happen during "baking time" of those tests, so we need to add code to deal with such condition. (probably retries ?)

okartau · 2019-11-15T13:04:35Z

actually, as this is just added Warning text and real operations did succeed, it is just question of filtering out warning text, no retry needed.

avalluri · 2019-11-15T13:08:47Z

actually, as this is just added Warning text and real operations did succeed, it is just question of filtering out warning text, no retry needed.

We need to disable lvmetad checking on VM hosts as we did in our docker images:

RUN echo "global { use_lvmetad = 0 }" >> /etc/lvm/lvm.conf && \
    echo "activation { udev_sync = 0 udev_rules = 0 }" >> /etc/lvm/lvm.conf

okartau · 2019-11-15T13:19:54Z

We need to disable lvmetad checking

Good idea!
lets move that thread into freshly created #468

avalluri · 2019-11-15T13:21:42Z

I am 😕. How come CI results are green. Clearly, there is one test failure.

okartau · 2019-11-15T13:32:24Z

you mean one test in "production 1.15 Clear" failed?
But there is retry(2) option that makes it try again, and 2nd attempt worked.

avalluri · 2019-11-15T13:32:32Z

@pohly Is it good enough to merge.

avalluri · 2019-11-15T13:33:07Z

you mean one test in "production 1.15 Clear" failed?
But there is retry(2) option that makes it try again, and 2nd attempt worked.

Ok, got it. Thanks.

avalluri force-pushed the go-1.13 branch 2 times, most recently from 2b07ea3 to 6695f7d Compare November 9, 2019 19:46

avalluri mentioned this pull request Nov 10, 2019

Devicemanager: New error codes + Device Manager unit tests and few other fixes. #420

Merged

avalluri force-pushed the go-1.13 branch from 6695f7d to d309477 Compare November 10, 2019 07:18

pohly suggested changes Nov 11, 2019

View reviewed changes

Dockerfile Outdated Show resolved Hide resolved

go.mod Show resolved Hide resolved

Jenkinsfile Show resolved Hide resolved

avalluri force-pushed the go-1.13 branch 2 times, most recently from dbad5c4 to 5e78c5d Compare November 14, 2019 09:34

pohly suggested changes Nov 14, 2019

View reviewed changes

Makefile Show resolved Hide resolved

pohly mentioned this pull request Nov 14, 2019

Go vendor test #466

Merged

avalluri force-pushed the go-1.13 branch 5 times, most recently from 100c547 to 66d91ba Compare November 14, 2019 13:48

pohly reviewed Nov 14, 2019

View reviewed changes

pohly suggested changes Nov 15, 2019

View reviewed changes

avalluri and others added 6 commits November 15, 2019 12:34

Makefile: Download Kustomize,govm binaries instead of building from s…

21ee507

…ource Building from source needs downloading of all dependent source packages, instead we download the release binaries. Change in Kustomize version resulted in changes to few generated deployment files.

test/start-kubernetes.sh: Increase ssh timout

585afab

After recent govm update, observed that occasionally cluster creation fails due to ssh connection timeout, Hence increasing the timeout to 2min.

avalluri force-pushed the go-1.13 branch from 66d91ba to 585afab Compare November 15, 2019 10:34

pohly reviewed Nov 15, 2019

View reviewed changes

avalluri requested a review from pohly November 15, 2019 13:32

okartau mentioned this pull request Nov 15, 2019

Volume leak test affected by side-effect lvs WARNING text #468

Closed

pohly approved these changes Nov 15, 2019

View reviewed changes

pohly merged commit 14bbca4 into intel:devel Nov 15, 2019

avalluri deleted the go-1.13 branch October 20, 2020 20:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Go 1.13 + Moving to go modules #461

Go 1.13 + Moving to go modules #461

avalluri commented Nov 9, 2019

pohly left a comment

avalluri commented Nov 13, 2019

pohly commented Nov 13, 2019

pohly commented Nov 14, 2019

avalluri commented Nov 14, 2019

pohly commented Nov 14, 2019

avalluri commented Nov 14, 2019

pohly Nov 14, 2019

pohly Nov 14, 2019

pohly Nov 15, 2019

avalluri Nov 15, 2019

avalluri Nov 15, 2019

pohly left a comment

avalluri commented Nov 15, 2019

avalluri commented Nov 15, 2019

pohly Nov 15, 2019

avalluri Nov 15, 2019

avalluri commented Nov 15, 2019

okartau commented Nov 15, 2019

okartau commented Nov 15, 2019

avalluri commented Nov 15, 2019

okartau commented Nov 15, 2019

avalluri commented Nov 15, 2019

okartau commented Nov 15, 2019

avalluri commented Nov 15, 2019

avalluri commented Nov 15, 2019

Go 1.13 + Moving to go modules #461

Go 1.13 + Moving to go modules #461

Conversation

avalluri commented Nov 9, 2019

pohly left a comment

Choose a reason for hiding this comment

avalluri commented Nov 13, 2019

pohly commented Nov 13, 2019

pohly commented Nov 14, 2019

avalluri commented Nov 14, 2019

pohly commented Nov 14, 2019

avalluri commented Nov 14, 2019

pohly Nov 14, 2019

Choose a reason for hiding this comment

pohly Nov 14, 2019

Choose a reason for hiding this comment

pohly Nov 15, 2019

Choose a reason for hiding this comment

avalluri Nov 15, 2019

Choose a reason for hiding this comment

avalluri Nov 15, 2019

Choose a reason for hiding this comment

pohly left a comment

Choose a reason for hiding this comment

avalluri commented Nov 15, 2019

avalluri commented Nov 15, 2019

pohly Nov 15, 2019

Choose a reason for hiding this comment

avalluri Nov 15, 2019

Choose a reason for hiding this comment

avalluri commented Nov 15, 2019

okartau commented Nov 15, 2019

okartau commented Nov 15, 2019

avalluri commented Nov 15, 2019

okartau commented Nov 15, 2019

avalluri commented Nov 15, 2019

okartau commented Nov 15, 2019

avalluri commented Nov 15, 2019

avalluri commented Nov 15, 2019