Remove docker binary from weaveexec container #3110

dtshepherd · 2017-09-04T06:15:38Z

Creating weaveutil commands to remove docker binary dependency in weaveexec.

This is still a WIP... Need to port the docker run, docker create, docker ps, parts of docker inspect (although probably want high-level weaveutil commands for check_running, etc).

@bboreham

bboreham · 2017-09-04T13:54:34Z

Thanks for this, @dtshepherd. It does seem a little more complicated than we need - e.g. we never call kill with a signal so we don't need a table to decode them. Or the timeout parameter.

dtshepherd · 2017-09-04T14:47:42Z

Ok I'll remove the extra options. I'm working on some of the docker inspect replacements now, which aren't using docker inspect directly.

Any idea how to get the CircleCI tests to pass? It complains about a missing secret key.

bboreham · 2017-09-04T15:08:00Z

The secret key is only accessible to PRs in the repo, to avoid people submitting PRs which steal the key and/or steal the resources it controls.

I can push your branch to the repo and the tests will run.

dtshepherd · 2017-09-05T04:45:52Z

Finished replacing docker inspect in weave script (check_runnning, check_not_running, ask_version, rewrite_etc_hosts, with_container_fqdn).

Still need to implement commands to create weavedb and weavevolumes containers. Also, need to implement docker ps, which really just loads volume containers to delete, so maybe create a higher level command for that action. Probably should generalize some of the work done for weaveutil ask-version to launch weave container too.

dtshepherd · 2017-09-06T06:10:58Z

@bboreham Just committed the removal of the remaining docker ... commands except for the single complicated WEAVE_CONTAINER=$(docker run ...). The launch's docker run takes many options (some from user-defined WEAVE_DOCKER_ARGS) and we'll need to at least implement -d, -v, -e, --name, --restart, --privileged, --net=host, --pid=host, --volumes-from.

Thoughts on the implementation? Probably need to implement a near direct pass-through of the docker run with the limited options...

dtshepherd · 2017-09-07T05:31:25Z

OK, I tried a first cut at the run-container command. I'm not super happy with it because it doesn't use a true argument parser, but the smoke tests seem to work (although, I've noticed some fragility with some of the tests (e.g. 840_weave_kube_3_test.sh) even on master with none of my changes.

@bboreham Can you please kick off a CI build and test?

bboreham

I did an initial pass over the code; most of it looks great but I had some questions and comments.

prog/weaveutil/container.go

+		return fmt.Errorf("unable to connect to docker: %s", err)
+	}
+
+	container, err := c.InspectContainer(containerID)


prog/weaveutil/container.go

+	return nil
+}
+
+func listContainers(args []string) error {


prog/weaveutil/version.go

+	docker "github.com/fsouza/go-dockerclient"
+)
+
+func askVersion(args []string) error {


weave

-       protect_against_docker_hang
-       docker create -v /weavedb --name=$DB_CONTAINER_NAME \
-           --label=weavevolumes $WEAVEDB_IMAGE >/dev/null
+    if ! util_op container-state $DB_CONTAINER_NAME > /dev/null 2>&1 ; then


prog/weaveutil/container.go

+		fmt.Print(container.State.StateString())
+	} else {
+		image := args[1]
+		match1, _ := regexp.MatchString(image, container.Image)


prog/weaveutil/image.go

+	docker "github.com/fsouza/go-dockerclient"
+)
+
+func pullImage(args []string) error {


dtshepherd · 2017-09-10T19:02:42Z

@bboreham I think this is ready for a 2nd pass review. I just rebased with upstream master as well, since there was some conflicts with weave script.

I need to squash the commits down as well. Do you want a single commit or a few logically separated commits?

prog/weaveutil/container.go

+		fmt.Print(container.State.StateString())
+	} else {
+		image := args[1]
+		match1, _ := regexp.MatchString(image, container.Image)


prog/weaveutil/container.go

+	return nil
+}
+
+func listContainers(args []string) error {


prog/weaveutil/container.go

+
+func listContainers(args []string) error {
+	if len(args) < 1 {
+		cmdUsage("list-containers", "[<label>]")


weave

-        echo "DOCKER_HOST=$ORIG_DOCKER_HOST"
+# Handle special case $1 commands that run locally at the client end
+case "$1" in
+    help|--help)


weave

 # Note VOLUMES_CONTAINER which is for weavewait should change when you upgrade Weave
-VOLUMES_CONTAINER_NAME=weavevolumes-$IMAGE_VERSION
+VOLUMES_CONTAINER_NAME=$VOLUMES_LABEL-$IMAGE_VERSION


weave

-       protect_against_docker_hang
-       docker create -v /weavedb --name=$DB_CONTAINER_NAME \
-           --label=weavevolumes $WEAVEDB_IMAGE >/dev/null
+    if ! util_op container-state $DB_CONTAINER_NAME > /dev/null 2>&1 ; then


weave

-       docker create -v /weavedb --name=$DB_CONTAINER_NAME \
-           --label=weavevolumes $WEAVEDB_IMAGE >/dev/null
+    if ! util_op container-state $DB_CONTAINER_NAME > /dev/null 2>&1 ; then
+        protect_against_docker_hang


bboreham · 2017-09-12T12:22:58Z

This is great; I can see some potential further refactorings which would bring the code weight down, but happy to merge what you have and leave them for the future.

Regarding squashing commits, one would be fine, but if you see a few logically separate ones then that is fine too.

dtshepherd · 2017-09-13T02:26:22Z

@bboreham I think I responded to your review comments (some of the comments weren't on lines that needed to change so github didn't dismiss). I squashed to two commits (all of my changes + commit stolen from #2952 for try_bridge_create).

I think some of the code weight could be reduced if we merged some of prog/weaveutil/container.go with common/docker/client.go. Right now, the two are completely independent and I didn't want to change that in this branch. That refactor seemed like an effort that may cover the entire repository, not just weaveutil.

Hopefully this next review pass looks good for merging. I know this is a risky change, so I understand if its delayed for release to master. I'd like to build upon this PR to fix #3102.

bboreham

One small change in the Makefile please; then I will merge.

Makefile

@@ -406,7 +397,6 @@ integration-tests: all testrunner
 #     PLAYBOOK="<filename>" \
 #     RUNNER_ARGS="<...>" \
 #     TESTS="<...>" \  # Can be set to only run one or a few tests instead of the full test suite.
-#     DOCKER_VERSION=<...> \


Makefile

@@ -307,10 +302,6 @@ $(NETWORKTESTER_UPTODATE): test/images/network-tester/Dockerfile $(NETWORKTESTER
 $(WEAVE_EXPORT): $(IMAGES_UPTODATE) $(WEAVEDB_UPTODATE)
 	$(SUDO) DOCKER_HOST=$(DOCKER_HOST) docker save $(addsuffix :latest,$(IMAGES)) | gzip > $@

-$(DOCKER_DISTRIB):
-	curl -o $(DOCKER_DISTRIB) $(DOCKER_DISTRIB_URL)
-	cd $(shell dirname $@) && sha256sum -c $(shell pwd)/build/shasums/docker-tgz-$(WEAVEEXEC_DOCKER_ARCH).sha256sum


weave

-       docker create -v /weavedb --name=$DB_CONTAINER_NAME \
-           --label=weavevolumes $WEAVEDB_IMAGE >/dev/null
+    if ! util_op container-state $DB_CONTAINER_NAME > /dev/null 2>&1 ; then
+        protect_against_docker_hang


Fixes weaveworks#2957 - replace `docker stop` with `weaveuitl stop-container` - replace `docker kill` with `weaveutil kill-container` - replace `docker rm` with `weaveutil remove-container` - replace `docker run` with minimal `weaveutil container-run` - replace `docker inspect -f {{.ID}}` with `weaveutil container-id` - replace data-only container `docker create` with `weaveutil create-volume-container` - remove `docker inspect` dependencies from `weave` script `rewrite_etc_hosts` and `with_container_fqdn` - rework `weave` script `check_running` and `check_not_running` to use `weaveutil` - rework `weave version` to use `weaver` /report API or `weaver --version` if not running - rework `weave setup`'s `docker pull` to run locally rather than in `weaveexec` container

@bboreham

`weaveexec` no longer mounting '/' so we can't access the weavedb file to get persistent peer ID. If `weaver` was running, we couldn't have read the file anyway because BoltDB does not support concurrent access from two processes. Consequence is that if someone really does need the bridge creation on 'weave expose' the peer ID may change if created pre-1.9.0. We'll just live with that (per @bboreham).

dtshepherd · 2017-09-13T12:05:46Z

Fixed the Makefile and removed those shasum files, good catch. We'll see what the CI looks like after these changes...

bboreham · 2017-09-13T13:07:19Z

Nice! Image size comes down by ~34MB

dtshepherd · 2017-09-13T13:11:45Z

I didn't even check that! Yeah that's a good win.

dtshepherd mentioned this pull request Sep 4, 2017

Remove docker binary from weaveexec container #2957

Closed

dtshepherd force-pushed the issues/2957-rm-docker-binary-from-weaveexec branch from 4269a75 to 06a264d Compare September 4, 2017 06:31

dtshepherd force-pushed the issues/2957-rm-docker-binary-from-weaveexec branch 3 times, most recently from 8787f50 to 6bb8afd Compare September 5, 2017 04:40

bboreham reviewed Sep 7, 2017

View reviewed changes

dtshepherd commented Sep 8, 2017

View reviewed changes

prog/weaveutil/image.go Outdated

docker "github.com/fsouza/go-dockerclient"

)

func pullImage(args []string) error {

This comment was marked as abuse.

Sign in to view

This comment was marked as abuse.

Sign in to view

dtshepherd force-pushed the issues/2957-rm-docker-binary-from-weaveexec branch from ad7b122 to 9fcdce5 Compare September 10, 2017 19:01

bboreham suggested changes Sep 12, 2017

View reviewed changes

dtshepherd force-pushed the issues/2957-rm-docker-binary-from-weaveexec branch from 36c2351 to f63c9fa Compare September 13, 2017 02:13

dtshepherd changed the title ~~WIP: Remove docker binary from weaveexec container~~ Remove docker binary from weaveexec container Sep 13, 2017

bboreham added this to the 2.1 milestone Sep 13, 2017

bboreham approved these changes Sep 13, 2017

View reviewed changes

dtshepherd added 2 commits September 13, 2017 08:03

dtshepherd force-pushed the issues/2957-rm-docker-binary-from-weaveexec branch from f63c9fa to 38ca0f9 Compare September 13, 2017 12:04

bboreham merged commit 3c9c968 into weaveworks:master Sep 13, 2017

bboreham mentioned this pull request Sep 13, 2017

ARM64 weaveexec image has wrong docker client binary #2832

Closed

bboreham mentioned this pull request Sep 26, 2017

WIP: Download arm64 docker binary from Ubuntu repo #2878

Closed

bboreham mentioned this pull request Oct 5, 2017

cater for bleeding edge docker features in WEAVE[DNS]_DOCKER_ARGS #582

Open

berlic mentioned this pull request Nov 22, 2017

Releases v2.1.0 and v2.1.1 brake with WEAVE_DOCKER_ARGS defined #3182

Closed

Remove docker binary from weaveexec container #3110

Remove docker binary from weaveexec container #3110

Conversation

dtshepherd commented Sep 4, 2017

bboreham commented Sep 4, 2017

dtshepherd commented Sep 4, 2017

bboreham commented Sep 4, 2017

dtshepherd commented Sep 5, 2017

dtshepherd commented Sep 6, 2017

dtshepherd commented Sep 7, 2017

bboreham left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

dtshepherd commented Sep 10, 2017 • edited

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

bboreham commented Sep 12, 2017

dtshepherd commented Sep 13, 2017

bboreham left a comment

Choose a reason for hiding this comment

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

This comment was marked as abuse.

dtshepherd commented Sep 13, 2017

bboreham commented Sep 13, 2017

dtshepherd commented Sep 13, 2017

dtshepherd commented Sep 10, 2017 •

edited