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

Build dockerized #647

Merged
merged 11 commits into from Jan 15, 2018
Merged

Build dockerized #647

merged 11 commits into from Jan 15, 2018

Conversation

@rmohr
Copy link
Member

rmohr commented Jan 12, 2018

Make building within docker the one and only option. All Makefile invocations will build the code inside docker.

With this change docker and rsync are the only requirements to build KubeVirt.

@rmohr rmohr requested review from davidvossel and mpolednik Jan 12, 2018
@rmohr rmohr force-pushed the rmohr:build_dockerized branch 2 times, most recently from 9112663 to 35f4d28 Jan 12, 2018
Copy link

mpolednik left a comment

In general, I approve the approach. There are several minor questions inlined, mostly related to how dist actually looks like.

Extra comment could be made about hack directory - at this point, it's evolving into something I wouldn't call a hack but a build.

Makefile Outdated
apidocs: generate
./hack/gen-swagger-doc/gen-swagger-docs.sh v1 html
apidocs:
hack/dockerized hack/glide-checksync.sh && ./hack/generate.sh && ./hack/gen-swagger-doc/gen-swagger-docs.sh v1 html

This comment has been minimized.

Copy link
@mpolednik

mpolednik Jan 12, 2018

Guess you could just depend on generate target, the extra spin-up/down shouldn't take too much extra time.

This comment has been minimized.

Copy link
@rmohr

rmohr Jan 12, 2018

Author Member

Would look a little bit nicer, but at least after you have everything in the cache it makes a noticeable difference.

@@ -46,6 +40,6 @@ _rsync --delete --exclude '.glide*' --exclude "cluster/vagrant/.kubeconfig" --ex
docker run --rm -v "${BUILDER}:/root:rw,z" -w "/root/go/src/kubevirt.io/kubevirt" -it ${BUILDER} "$@"

# Copy the whole kubevirt data out to get generated sources and formatting changes
_rsync --exclude '.glide*' --exclude "vendor" --exclude "_out" --exclude ".vagrant" --exclude ".git" "rsync://root@127.0.0.1:${RSYNCD_PORT}/build" ${KUBEVIRT_DIR}/
_rsync --exclude '.glide*' --exclude "_out" --exclude ".vagrant" --exclude ".git" "rsync://root@127.0.0.1:${RSYNCD_PORT}/build" ${KUBEVIRT_DIR}/

This comment has been minimized.

Copy link
@mpolednik

mpolednik Jan 12, 2018

Genuinely curious, why do we no longer exclude vendor when copying build out?

This comment has been minimized.

Copy link
@rmohr

rmohr Jan 12, 2018

Author Member

It is important if you are also looking into the vendor dependencies. Another option is to commit all vendor dependencies into the source tree.

This comment has been minimized.

Copy link
@mpolednik

mpolednik Jan 12, 2018

When that comes up, consider this forward +1. Github even hides vendor/ diffs.

Makefile Outdated
rm -rf vendor/
rm -f .glide.*.hash
glide cc
hack/dockerized "rm -rf vendor/ && rm -f .glide.*.hash && glide cc"

This comment has been minimized.

Copy link
@mpolednik

mpolednik Jan 12, 2018

I'm not sure doing distclean within the docker volume has the desired effect? Similar issue with clean above; could you outline the dist process given the docker building?

This comment has been minimized.

Copy link
@rmohr

rmohr Jan 12, 2018

Author Member

right. 👍

This comment has been minimized.

Copy link
@rmohr

rmohr Jan 12, 2018

Author Member

Done.

Makefile Outdated
elif [ "`${HASH} glide.lock`" != "`cat .glide.lock.hash`" ]; then \
make sync; \
fi
hack/dockerized

This comment has been minimized.

Copy link
@mpolednik

mpolednik Jan 12, 2018

hack/dockerized hack/glide-checksync.sh ?

@rmohr rmohr force-pushed the rmohr:build_dockerized branch from 35f4d28 to 3a612cd Jan 12, 2018
@mpolednik

This comment has been minimized.

Copy link

mpolednik commented Jan 12, 2018

BTW the getting-started.md should be updated to reflect the changes (e.g. docker must be installed).

@rmohr rmohr force-pushed the rmohr:build_dockerized branch from 3a612cd to fe1f9e1 Jan 12, 2018
@rmohr

This comment has been minimized.

Copy link
Member Author

rmohr commented Jan 12, 2018

BTW the getting-started.md should be updated to reflect the changes (e.g. docker must be installed).

There now.

@rmohr rmohr force-pushed the rmohr:build_dockerized branch 2 times, most recently from 62024db to 9779bfc Jan 12, 2018

test: build
./hack/build-go.sh test ${WHAT}
test:

This comment has been minimized.

Copy link
@mpolednik

mpolednik Jan 12, 2018

Just tried running make test and had a failure due to missing vendor. Looks like this target needs to depend on sync.

This comment has been minimized.

Copy link
@mpolednik

mpolednik Jan 12, 2018

And they definitely depend on build too (pkg/virt-handler/monitor_test.go at least).

This comment has been minimized.

Copy link
@rmohr

rmohr Jan 12, 2018

Author Member

Done.

@rmohr rmohr force-pushed the rmohr:build_dockerized branch from 9779bfc to 2db2894 Jan 12, 2018
@rmohr rmohr mentioned this pull request Jan 12, 2018
@mpolednik

This comment has been minimized.

Copy link

mpolednik commented Jan 12, 2018

To avoid commenting every target: with this PR, most of the targets have "hidden" prerequisites: to run cluster-deploy one must run

  • make sync
  • make cluster-build
  • make cluster-up
  • make cluster-deploy

The target dependencies are probably out of scope of this PR, but it would be nice if some of it could be handled automatically - so for example running cluster-deploy would run all the required steps.

As for the scope of this PR, I welcome the change. Not only it aligns nicely with Kubernetes, but as a side effect now allows builds/tests to be run on different platforms easily.

$ uname -a
Darwin Alexandra 17.0.0 Darwin Kernel Version 17.0.0: Thu Aug 24 21:48:19 PDT 2017; root:xnu-4570.1.46~2/RELEASE_X86_64 x86_64
$ make test
hack/dockerized "hack/glide-checksync.sh && ./hack/check.sh && ./hack/build-go.sh install  && ./hack/build-go.sh test "
sha256:c9e019630dd2f678fece992502d7a8761b750755fba8b6b44eac5404249807f5
go version go1.9.2 linux/amd64
go version go1.9.2 linux/amd64
Compiling tests...
    compiled tests.test
?       kubevirt.io/kubevirt/pkg/api    [no test files]
=== RUN   TestSelectors
Running Suite: PodSelectors
===========================
Random Seed: 1515757574
Will run 12 of 12 specs
...

(the example actually uses Docker's linux-based VM on Darwin, making it possible to rsync the builds to dev machines)

@rmohr rmohr force-pushed the rmohr:build_dockerized branch from 2db2894 to d9f70ca Jan 12, 2018
@rmohr

This comment has been minimized.

Copy link
Member Author

rmohr commented Jan 12, 2018

@mpolednik

make sync
make cluster-build
make cluster-up
make cluster-deploy

Running that series of commands is not necessary. If the provider is not up, one has to call make cluster-up. After that make cluster-sync does all the rest.

The target dependencies are probably out of scope of this PR

They should now all have no hidden dependencies. Could you try the latest state? I don't want to confuse people too much with hidden dependencies.

@rmohr rmohr force-pushed the rmohr:build_dockerized branch 3 times, most recently from ce0a828 to 799a3c5 Jan 12, 2018
@rmohr rmohr changed the title Build dockerized [skip ci] Build dockerized Jan 13, 2018
@rmohr

This comment has been minimized.

Copy link
Member Author

rmohr commented Jan 13, 2018

ok to test

@rmohr rmohr force-pushed the rmohr:build_dockerized branch 2 times, most recently from 786e11c to e58a9f1 Jan 13, 2018
@rmohr

This comment has been minimized.

Copy link
Member Author

rmohr commented Jan 14, 2018

retest this please

Copy link
Member

nertpinx left a comment

Skimmed through the changes, apart from some documentation changes needed even before this patches it looks fine, deployment works for me out of the box.

@rmohr rmohr requested a review from stu-gott Jan 15, 2018
@rmohr

This comment has been minimized.

Copy link
Member Author

rmohr commented Jan 15, 2018

@ifireball what do we have to change to have docker support in the chroot on standard-ci?

@ifireball

This comment has been minimized.

Copy link
Member

ifireball commented Jan 15, 2018

@rmohr you need to mount the docker socket into the environment and install docker itself.

Basically just add the following to automation/check-patch.mounts:

/var/run/docker.sock:/var/run/docker.sock

And additionally add the following to automation/check-patch.packages.el7:

docker
@rmohr rmohr force-pushed the rmohr:build_dockerized branch 2 times, most recently from 702f59e to 4637f50 Jan 15, 2018
rmohr and others added 10 commits Jan 12, 2018
By also moving the manifests to _out, we have all produced artifacts in
the _out folder.

Signed-off-by: Roman Mohr <rmohr@redhat.com>
Only build containers needed for tests, in order to speed up vagrant
provisioning. The optional docker containers are just for convenience
and not required by tests.

Signed-off-by: Roman Mohr <rmohr@redhat.com>
Signed-off-by: Roman Mohr <rmohr@redhat.com>
Signed-off-by: Roman Mohr <rmohr@redhat.com>
Only if the dockerized builed is called with an allocated tty, let
docker allocate a tty.
Signed-off-by: Travis CI <travis@travis-ci.org>
Signed-off-by: Travis CI <travis@travis-ci.org>
Signed-off-by: Travis CI <travis@travis-ci.org>
@ifireball

This comment has been minimized.

Copy link
Member

ifireball commented Jan 15, 2018

@rmohr yeah, not sure why is it failing to shut down the cluster there, maybe we need to wait for something or other to finish?

@rmohr rmohr force-pushed the rmohr:build_dockerized branch from 4637f50 to 2d35886 Jan 15, 2018
Travis CI
Signed-off-by: Travis CI <travis@travis-ci.org>
@rmohr

This comment has been minimized.

Copy link
Member Author

rmohr commented Jan 15, 2018

retest this please

@davidvossel

This comment has been minimized.

Copy link
Member

davidvossel commented Jan 15, 2018

@rmohr how do you feel about following up with committing the vendor folder after this change?

@rmohr

This comment has been minimized.

Copy link
Member Author

rmohr commented Jan 15, 2018

@davidvossel +1 from me regarding to committing the vendor folder as a follow up.


sync:
glide install --strip-vendor
${HASH} glide.lock > .glide.lock.hash
hack/dockerized "glide install --strip-vendor && md5sum glide.lock > .glide.lock.hash"

This comment has been minimized.

Copy link
@davidvossel

davidvossel Jan 15, 2018

Member

I'm fine with this as long as we commit the vendor folder either in this PR or immediately after submitting this pr.

@rmohr rmohr merged commit 3cb1abd into kubevirt:master Jan 15, 2018
6 checks passed
6 checks passed
check-patch.el7.x86_64 Test is successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.02%) to 66.435%
Details
kubevirt-functional-tests/jenkins/pr/vagrant-dev All is well still in progress...
Details
kubevirt-functional-tests/jenkins/pr/vagrant-release All is well still in progress...
Details
standard-ci All tests passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.