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

Add Go modules support #4241

Merged
merged 4 commits into from
May 13, 2019
Merged

Add Go modules support #4241

merged 4 commits into from
May 13, 2019

Conversation

myhro
Copy link
Contributor

@myhro myhro commented May 10, 2019

Moving away from Go dep to Go modules allowed the build infrastructure to be cleaned up a bit. The related docs were also updated.

The whole diff is somewhat long after the vendor/ directory removal, so I guess reviewing it per commit can make the process a bit easier.

This closes #4234.

myhro added 2 commits May 9, 2019 20:34
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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
Copy link
Contributor

Hi @myhro. Thanks for your PR.

I'm waiting for a 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 needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 10, 2019
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@afbjorklund
Copy link
Collaborator

"Somewhat" (2,178 files, +323 −929,569 lines)

@myhro
Copy link
Contributor Author

myhro commented May 10, 2019

I signed it

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 10, 2019
@myhro
Copy link
Contributor Author

myhro commented May 10, 2019

/assign @tstromberg

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: myhro
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: tstromberg

If they are not already assigned, you can assign the PR to them by writing /assign @tstromberg in a comment when ready.

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

@tstromberg
Copy link
Contributor

@minikube-bot OK to test

@tstromberg
Copy link
Contributor

This change looks beautiful. Once the integration tests are run, I'll merge it.

@myhro
Copy link
Contributor Author

myhro commented May 10, 2019

Thank you for the review. Jenkins build is broken, I'll investigate how to fix it.

@tstromberg
Copy link
Contributor

tstromberg commented May 10, 2019

@myhro - We do something funny and build the kvm2 driver binary from inside a Docker image running an older release of Debian for libvirt compatibility.

Let me or @sharifelgamal know if you need any help in getting with getting this sorted out.

@myhro
Copy link
Contributor Author

myhro commented May 10, 2019

@tstromberg The kvm2 driver build system I found quite clever and was able to fix it by adding git to its Docker image. jteeuwen/go-bindata is a tool I would like to replace, as it is not maintained anymore, but right now I don't have an alternative for it.

Debugging the last failure, I noticed that it was trying to build the hyperkit driver under the karalabe/xgo-1.10.x image which, as the name suggests, is based on Go 1.10, a version that doesn't support modules. I've changed it to karalabe/xgo-1.12.x and was able to build the driver with:

MINIKUBE_BUILD_IN_DOCKER=y make out/docker-machine-driver-hyperkit

Now I see that it still failed on Jenkins, so I'll keep digging.

@myhro
Copy link
Contributor Author

myhro commented May 10, 2019

@tstromberg Sorry, I only saw that after sending the last comment:

Status: Downloaded newer image for karalabe/xgo-1.12.x:latest
docker: Error response from daemon: devmapper: Thin Pool has 148835 free data
blocks which is less than minimum required 163840 free data blocks. Create more
free space in thin pool or use dm.min_free_space option to change behavior.
See 'docker run --help'.
Makefile:282: recipe for target 'out/docker-machine-driver-hyperkit' failed
make: *** [out/docker-machine-driver-hyperkit] Error 125

Did the Jenkins builder run out of disk space?

P.s.: I can join Slack if you prefer to avoid the comment ping-pong.

@tstromberg
Copy link
Contributor

tstromberg commented May 10, 2019

I found https://stackoverflow.com/questions/47457012/how-to-increase-the-space-data-space-total-used-by-docker-that-value-is-less - which mentioned running "docker system prune". Going to do so on our Jenkins master, which apparently has hundreds of old images around:

sudo docker system df

TYPE                TOTAL               ACTIVE              SIZE                RECLAIMABLE
Images              434                 0                   66.97GB             66.97GB (100%)
Containers          0                   0                   0B                  0B
Local Volumes       0                   0                   0B                  0B

@tstromberg
Copy link
Contributor

@minikube-bot OK to test

@tstromberg
Copy link
Contributor

It looks like perhaps a merge is in order, since jibber jabber is a recent addition? Jenkins got farther this time: passing the hyperkit build, but failing kvm2:

15:16:39 pkg/minikube/constants/constants.go:27:2: cannot find package "github.com/blang/semver" in any of:
15:16:39 	/usr/local/go/src/github.com/blang/semver (from $GOROOT)
15:16:39 	/var/lib/jenkins/go/src/github.com/blang/semver (from $GOPATH)
15:16:39 pkg/minikube/console/console.go:27:2: cannot find package "github.com/cloudfoundry-attic/jibber_jabber" in any of:
...
15:16:39 	/usr/local/go/src/k8s.io/kubernetes/cmd/kubeadm/app/constants (from $GOROOT)
15:16:39 	/var/lib/jenkins/go/src/k8s.io/kubernetes/cmd/kubeadm/app/constants (from $GOPATH)
15:16:39 Makefile:345: recipe for target 'out/docker-machine-driver-kvm2' failed
15:16:39 make: *** [out/docker-machine-driver-kvm2] Error 1
15:16:39 make: *** Waiting for unfinished jobs....

@tstromberg
Copy link
Contributor

Here's the PR that added jibber jabber 7 days ago: #4199

@myhro
Copy link
Contributor Author

myhro commented May 11, 2019

Thanks for fixing the disk space issue. The out/docker-machine-driver-kvm2 task is failing because Go modules aren't available. As it doesn't runs under Docker, different than the hyperkit one, and it's a straight call to go build, I suspect that the Jenkins builder has Go < 1.12 currently installed. I can't confirm that suspicion, as the Go version information isn't available in the build logs.

I'm seeing two possible solutions for this problem:

  • We can consider that every driver has to be built under Docker, changing out/docker-machine-driver-kvm2 to behave like out/docker-machine-driver-hyperkit.
  • Confirm the Go version hypothesis and, if it's true, update the Jenkins builder to use an up-to-date Go version.

Which one you prefer?

Edit: actually we need Go 1.12. I've seen some problems building the project under 1.11.

@tstromberg
Copy link
Contributor

tstromberg commented May 13, 2019

go v1.12 is a requirement, as per https://github.com/kubernetes/minikube/blob/master/docs/contributors/build_guide.md

I did notice that the Jenkins master was stuck on go v1.12rc1, so I've upgraded the master to v1.12.5. I'm having similar issues building locally, and I believe the difference between our environments is that Jenkins and myself are building from ~/go/src, which disables go modules by default.

I believe that this PR will need to do is set env GO111MODULE=on in the Makefile to guarantee
build consistency across environments. I would rather not Docker become a build dependency unless strictly necessary.

.travis.yml Outdated
@@ -3,8 +3,9 @@ os: linux
sudo: required

go:
- 1.x
go_import_path: k8s.io/minikube
- 1.12.x
Copy link
Contributor

Choose a reason for hiding this comment

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

If possible, leave this at 1.x so that we will continue to auto-update when v1.13 comes along.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@tstromberg
Copy link
Contributor

Thank you for taking this beast of a PR on, by the way. I just learned today that a tool we would like to use for localization (golang.org/x/text/cmd/gotext) is incompatible with vendor directories, so this PR is perfectly timed. You are a hero =)

@tstromberg
Copy link
Contributor

@minikube-bot OK to test

@myhro
Copy link
Contributor Author

myhro commented May 13, 2019

Thank you for clarifying the version and upgrading the server. I agree that it makes way more sense to use GO111MODULE=on and not worry about the import path anymore. I still removed it from the .travis.yml file just to be clear that it's not needed.

I just learned today that a tool we would like to use for localization (golang.org/x/text/cmd/gotext) is incompatible with vendor directories, so this PR is perfectly timed.

I didn't knew that there were projects incompatible with vendor directories. Thanks for the tip. I'm glad that we are taking care of that detail. :-)

I'm a bit confused, as there are no timestamps in the Jenkins logs, but looks like the build worked and the integration tests are running now.

@tstromberg
Copy link
Contributor

Looks like the build works great with the most recent changes.

I'll wait for the rest of the integration tests to pass, and then merge it in. Thank you again!

@sharifelgamal sharifelgamal merged commit ac3bdd1 into kubernetes:master May 13, 2019
@tstromberg
Copy link
Contributor

Huzzah! 🎉 Thank you!

@myhro
Copy link
Contributor Author

myhro commented May 13, 2019

That's great. Thank you very much for your support, Thomas. It would be way harder without it.

Hope we can work together again on other features/PRs. :-)

@myhro myhro deleted the go-mod branch May 13, 2019 20:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migrate from dep to Go Modules
6 participants