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.

Sign up for GitHub

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

Jump to bottom

Build versioned v2plugin from versioned binaries #1040

Merged
merged 8 commits into from
Nov 9, 2017
Merged

Build versioned v2plugin from versioned binaries #1040

merged 8 commits into from
Nov 9, 2017

Conversation

chrisplo
Copy link
Contributor

@chrisplo chrisplo commented Oct 31, 2017 •
edited
Loading

This enables build and caching locally or in a build machine the
v2plugin before creating test environment VMs and failing much faster
on any PR issues, which is good for CI turnaround.

v2plugin_rootfs (called by host-pluginfs-create) builds a docker
container runtime with a versioned archive of netplugin binaries, then
extracts the entire filesystem from the container, which is now left as
a versioned tarball.

The versioned archive of netplugin binaries used for v2plugin can now be
created in two ways:

  • run-build (dependency for demo-v2plugin) will compile then version an
    archive with built assets (runs only on the VM)
  • tar target (locally or in VM)

host-pluginfs-create can be run in the VM or locally

Leaving the plugin rootfs as a tarball has a few advantages reasons:

  • docker in a VM had internal issues when creating the plugin when the
    rootfs was unarchived on the host and exposed though a virtualbox
    mount, at least on OS X
  • the rootfs is versioned

This adjusted host-plugin-release and demo-v2plugin to both use a new
target 'host-pluginfs-unpack'

Drive-by:

  • Fix Dockerfile by adding objdb which is needed by vendored packages
  • Fix config.template for the CONTIV_V2PLUGIN_NAME replacement

@chrisplo
Copy link
Contributor Author

chrisplo commented Oct 31, 2017 •
edited
Loading

checking on a version naming bug during testing

@chrisplo chrisplo changed the title Build versioned v2plugin from versioned binaries WIP: Build versioned v2plugin from versioned binaries Oct 31, 2017
@chrisplo
Copy link
Contributor Author

chrisplo commented Nov 1, 2017 •
edited
Loading

#1042 helps to sort out version naming issues, will rebase after that lands but early comments welcome done

unclejack
unclejack reviewed Nov 1, 2017
View reviewed changes
Makefile Outdated
@@ -129,7 +134,7 @@ update:
# setting CONTIV_NODES=<number> while calling 'make demo' can be used to bring
# up a cluster of <number> nodes. By default <number> = 1
start:
CONTIV_DOCKER_VERSION="$${CONTIV_DOCKER_VERSION:-$(DEFAULT_DOCKER_VERSION)}" CONTIV_NODE_OS=${CONTIV_NODE_OS} vagrant up
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are you making this change here? It seems unrelated to the purpose of the PR.

Copy link
Contributor Author

@chrisplo chrisplo Nov 3, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

was reducing duplication (the build.sh defaults this to same value), but unrelated to PR, backing this out

unclejack
unclejack reviewed Nov 1, 2017
View reviewed changes
Makefile Outdated
@@ -6,7 +6,6 @@
.PHONY: all all-CI build clean default unit-test release tar checks go-version gofmt-src \
golint-src govet-src run-build compile-with-docker

DEFAULT_DOCKER_VERSION := 1.12.6
Copy link
Contributor

Choose a reason for hiding this comment

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

Please leave this alone.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will back this change out

unclejack
unclejack requested changes Nov 1, 2017
View reviewed changes
Copy link
Contributor

@unclejack unclejack left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The Docker version related changes need to be removed. We need to be able to support specifying custom Docker version. We'll use this again soon.

unclejack
unclejack reviewed Nov 1, 2017
View reviewed changes
Makefile Outdated
@@ -213,7 +218,7 @@ ssh:
@vagrant ssh netplugin-node1 -c 'bash -lc "cd /opt/gopath/src/github.com/contiv/netplugin/ && bash"' || echo 'Please run "make demo"'

ssh-build: start
vagrant ssh netplugin-node1 -c 'bash -lc "source /etc/profile.d/envvar.sh && cd /opt/gopath/src/github.com/contiv/netplugin && make run-build install-shell-completion"'
Copy link
Contributor

Choose a reason for hiding this comment

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

This change doesn't seem related to the goal of this PR. Can you split this in a separate cleanup commit instead, please?

@unclejack
Copy link
Contributor

The permissions of some of the files have been changed. Are those changes intentional or accidental?

@chrisplo
Copy link
Contributor Author

chrisplo commented Nov 1, 2017 •
edited
Loading

the jenkins file was unintentional, the bash script was intentional

I'm holding working on this till #1042 is resolved 1042 merged

@chrisplo chrisplo force-pushed the v2plugin_local_versioned branch 3 times, most recently from a89707c to b947648 Compare November 3, 2017 21:53
@chrisplo
Build versioned v2plugin from versioned binaries
3215ee1 
This enables build and docker image caching on a build machine for the
v2plugin.

The new target demo-v2plugin-from-local does everything it can locally
before spawning the VM, failing much faster on any PR issues,
which is good for CI turnaround.  It runs a little faster than
demo-v2plugin after base image layers are cached.

v2plugin_rootfs (called by host-pluginfs-create) builds a docker
container runtime with a versioned archive of netplugin binaries, then
extracts the entire filesystem from the container, which is now left as
a versioned tarball.

The versioned archive of netplugin binaries used for v2plugin can now be
created in two ways:
* run-build (dependency for demo-v2plugin) will compile then version an
  archive with built assets (runs only on the VM)
* tar target (locally or in VM)

host-pluginfs-create can be run in the VM or locally

The plugin rootfs is left as a tarball because:
* docker in a VM had internal issues when creating the plugin when the
  rootfs was unarchived on the host and exposed though a virtualbox
  mount, at least on OS X
* the rootfs is versioned

This adjusted host-plugin-release and demo-v2plugin both use a new
target 'host-pluginfs-unpack'

Drive-by:
* Fix config.template for the CONTIV_V2PLUGIN_NAME replacement
* gtar support for OS X
* use cp -a for maintaining owner

Signed-off-by: Chris Plock <chrisplo@cisco.com>
@chrisplo
review feedback
0ce1f51
@chrisplo
comments, revert a var rename for smaller diff
f750f16
@chrisplo
more dockerignore
13fcf40
@chrisplo chrisplo changed the title WIP: Build versioned v2plugin from versioned binaries Build versioned v2plugin from versioned binaries Nov 3, 2017
@chrisplo
Copy link
Contributor Author

chrisplo commented Nov 6, 2017

build PR

@unclejack
Copy link
Contributor

build PR

@unclejack
Copy link
Contributor

build PR

2 similar comments
@chrisplo
Copy link
Contributor Author

chrisplo commented Nov 8, 2017

build PR

@chrisplo
Copy link
Contributor Author

chrisplo commented Nov 8, 2017

build PR

dseevr
dseevr reviewed Nov 8, 2017
View reviewed changes
Makefile Outdated
host-plugin-restart host-swarm-restart)

# Just like demo-v2plugin except builds are done locally and cached
demo-v2plugin-from-local: export CONTIV_DOCKER_VERSION ?= 1.13.1
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this var and the one for CONTIV_DOCKER_VERSION under demo-v2plugin below be consolidated into a variable at the top of the file so it only needs to be changed in one place?

install/v2plugin/Dockerfile
COPY netplugin netmaster netctl startcontiv.sh /
# copy in binaries and scripts
ARG TAR_FILE
ADD ${TAR_FILE} /
Copy link
Contributor

Choose a reason for hiding this comment

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

If you're copying local files, use COPY instead of ADD ... generally you will not want to use ADD for anything, and all of Docker's official Dockerfiles have moved from ADD to COPY now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the ADD exploded the tar file, I'd have to create another layer for a RUN to expand, there is nothing suggesting deprecation on the docs:
https://docs.docker.com/engine/reference/builder/#add

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, right. "TAR_FILE" lol (used to looking for .tar.gz and so on) 😂

@dseevr
Copy link
Contributor

dseevr commented Nov 8, 2017

Do we need to worry about spaces in paths with any of this stuff?

@chrisplo
Copy link
Contributor Author

chrisplo commented Nov 8, 2017

build PR

@chrisplo
Copy link
Contributor Author

chrisplo commented Nov 8, 2017 •
edited
Loading

only place I could see that isn't a subdirectory is here, which was a pre-existing issue in many placements if GOPATH has spaces:
https://github.com/contiv/netplugin/pull/1040/files#diff-b67911656ef5d18c4ae36cb6741b7965R320

@chrisplo
Copy link
Contributor Author

chrisplo commented Nov 8, 2017

build PR

dseevr
dseevr approved these changes Nov 9, 2017
View reviewed changes
.dockerignore
@@ -1,11 +1,15 @@
# we want to get the git commit SHA but not all the binary repo data
.git/objects/pack
bin/
**/*.pyc
Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot to mention this earlier... could you do a git rm **/*.pyc and see if anything is currently checked in which shouldn't be? Could also be in a separate PR

.dockerignore
*.tar
**/*.tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

Same on this guy

unclejack
unclejack approved these changes Nov 9, 2017
View reviewed changes
Copy link
Contributor

@unclejack unclejack left a comment

Choose a reason for hiding this comment

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

LGTM

@unclejack unclejack merged commit 06fa667 into contiv:master Nov 9, 2017
@chrisplo chrisplo deleted the v2plugin_local_versioned branch November 9, 2017 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dseevr dseevr dseevr approved these changes

@unclejack unclejack unclejack approved these changes

Assignees
No one assigned
Labels
squash when merging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@chrisplo @unclejack @dseevr