refactor: use govendor to depend on libnetwork #1445

idealhack · 2018-05-31T10:32:30Z

Ⅰ. Describe what this PR did

Use govendor to depend on alibaba/libnetwork.

Ⅱ. Does this pull request fix one issue?

this pull request should fixes #1344, but some issues with vendor.json raised there were not addressed yet, see #1344 (comment)

Ⅲ. Describe how you did it

As the commits show:

refactor: remove extra logrus import and initNetworkLog function
refactor: remove extra/libnetwork
refactor: add alibaba/libnetwork as dependency with govendor
refactor: fix api call for new version of libnetwork
refactor: cleanup scripts related to extra/libnetwork
refactor: govendor libnetwork without -tree (with great help of @rudyfly)
refactor: update ablibaba/libnetwork

Ⅳ. Describe how to verify it

make
CI

Ⅴ. Special notes for reviews

I'm new to pouch, so please tell me if I missing anything.

allencloud · 2018-05-31T13:49:09Z

Thanks a lot for your work. This is quite important for pouch team how it will work with the latest libnetwork.

This branch is not squashed yet.

I think for the quite huge pull request, squashing is not necessary, and splitting the work into several commits would make it much easier to code review.

Currently the fix for contiv/executor is on my fork.

Could you explain more on the point above.

also involve @rudyfly for communication.

idealhack · 2018-05-31T14:05:03Z

Currently the fix for contiv/executor is on my fork.

Could you explain more on the point above.

@allencloud Please take a look at contiv/executor and commit 4649d1d. We may send the patch to them or drop this package (to implement the feature we need by ourselves) later if you want.

Also, I think I could use some help with the (CI) tests, thanks.

rudyfly · 2018-06-01T06:38:16Z

daemon/mgr/network.go

@@ -537,18 +536,16 @@ func (nm *NetworkManager) getNetworkSandbox(id string) libnetwork.Sandbox {
 	return sb
 }

-// libnetwork's logrus version is different from pouchd,
-// so we need to set libnetwork's logrus addintionly.
 func initNetworkLog(cfg *config.Config) {


If pouchd use the lastest libnetwork, initialize logrus is unuse.

Thank you, I thought about this but was not sure then.

rudyfly · 2018-06-01T07:40:28Z

@idealhack Thanks a lot for your work. Change libnetwork into vendor is also my next job, my plan is fork the libnetwork into Alibaba group, because we need to change the libnetwork. Can you wait me to complete forking the libnetwork.

idealhack · 2018-06-01T07:50:04Z

@rudyfly I see. It's reasonable to maintain a fork if you want change many things. So after you select a suitable version of libnetwork and forked it, I will make changes in this PR.

@rudyfly

address @rudyfly’s [comment](AliyunContainerService#1445 (comment))

rudyfly · 2018-06-06T13:52:27Z

@idealhack I have fork libnetwork into github.com/alibaba/libnetwork, can you continue doing your next job?

@rudyfly

address @rudyfly’s [comment](AliyunContainerService#1445 (comment)) Signed-off-by: Yang Li <idealhack@gmail.com>

idealhack · 2018-06-07T03:54:48Z

@rudyfly I've rebased the branch. Could you check the tests?

rudyfly · 2018-06-07T04:09:06Z

@idealhack Thanks for your job! You need to modify Makefile and hack/build.

idealhack · 2018-06-07T06:35:28Z

@rudyfly Thanks, PTAL.

rudyfly · 2018-06-07T07:34:41Z

docs/test/test.md

@@ -100,7 +100,7 @@ The following steps are also needed to make sure libnetwork package could be fou
 ```
 BUILDPATH=/tmp/pouchbuild
 mkdir -p $BUILDPATH/src/github.com/docker
-cp -r /go/src/github.com/alibaba/pouch/extra/libnetwork $BUILDPATH/src/github.com/docker/libnetwork
+cp -r /go/src/github.com/alibaba/pouch/vendor/github.com/docker/libnetwork $BUILDPATH/src/github.com/docker/libnetwork


No need to add libnetwork directory into GOPATH, it will vendor to build. I think just keep $BUILDPATH in GOPATH.

OK. I was not sure about this.

rudyfly · 2018-06-07T07:36:57Z

Makefile

@@ -2,7 +2,7 @@
 GOBUILD=go build
 GOCLEAN=go clean
 GOTEST=go test
-GOPACKAGES=$(shell go list ./... | grep -v /vendor/ | grep -v /extra/ | sed 's/^_//')
+GOPACKAGES=$(shell go list ./... | grep -v /vendor/ | sed 's/^_//')

 # Binary name of CLI and Daemon
 BINARY_NAME=pouchd


make pre in hack/build should be deleted libnetwork into GOPATH

Sorry, but what do you suggest for this line in Makefile?

idealhack · 2018-06-07T07:55:28Z

hack/build

@@ -26,7 +26,7 @@ function pre()

    mkdir -p $BUILDPATH/src/github.com/docker
    [ -L $BUILDPATH/src/github.com/docker/libnetwork ] && rm -f $BUILDPATH/src/github.com/docker/libnetwork
-    ln -s $DIR/extra/libnetwork $BUILDPATH/src/github.com/docker/libnetwork
+    ln -s $DIR/vendor/github.com/docker/libnetwork $BUILDPATH/src/github.com/docker/libnetwork


@rudyfly Is there any need for ln?

idealhack · 2018-06-07T07:57:28Z

hack/make.sh

@@ -247,7 +247,7 @@ function target
 				 rich container related tests will be skipped"

 		docker run --rm -v "$(pwd):$SOURCEDIR" \
-			-e GOPATH=/go:$SOURCEDIR/extra/libnetwork/Godeps/_workspace \
+			-e GOPATH=/go:$SOURCEDIR/vendor/github.com/docker/libnetwork/Godeps/_workspace \


@rudyfly Also, please take a look at this line.

rudyfly · 2018-06-07T08:40:13Z

This is my modify for delete extra/libnetwork,
rudyfly@fd3f09a#diff-3e1bf1973faaf0555bce85518aa3ac52

rudyfly@fd3f09a#diff-c4b8d2fa793507a7ce5835e7ca780464

idealhack · 2018-06-07T09:06:43Z

@rudyfly I've checked your commit and made some modifications.

rudyfly · 2018-06-07T09:52:05Z

@idealhack I think your modification is ok, but there are some ci fails, do you have ci environment to fix ci fails?

idealhack · 2018-06-07T10:00:12Z

@rudyfly Yes this is what I'm confusing about.

Could anyone who contributing this project give some guidance on how to fix that?

rudyfly · 2018-06-07T10:08:15Z

@idealhack OK, can you rebase master code, because I have changed network configure.

rudyfly · 2018-06-08T03:30:10Z

@idealhack I have push my modification to your fork. 😃

idealhack · 2018-06-08T03:32:12Z

@rudyfly Well done! Did you resolve those dependencies by hand?

rudyfly · 2018-06-08T03:39:06Z

Well done! Did you resolve those dependencies by hand?

yes, it makes me crazy. 😭

And can you do rebase squash about these commits？

idealhack · 2018-06-08T03:47:15Z

@rudyfly I was just about to said how can we do this in a less tedious way... If we have a script we can check it in here. Hope it will not make us more trouble in the future.

I see some commits are meanningless now. After the CI passed, I can do some squash work.

idealhack · 2018-06-08T06:35:49Z

@rudyfly I've squashed the commits, it's much cleaner now. Also, I backed up the old commits at refactor/govendor-libnetwork-backup just in case.

The CI still fails.

fuweid · 2018-06-09T01:39:45Z

@idealhack could you provide the information about how to use govendor? I check the vendor/vendor.json and found that the github.com/Microsoft/go-winio is useless package. Thanks

idealhack · 2018-06-09T04:41:40Z

@fuweid As I metioned in #1344 (comment) I found there are useless packages too.

That said, AFAIK go-winio is a dependency of libnetwork and containerd.

rudyfly · 2018-06-12T06:41:09Z

@idealhack I'm sorry for delaying this work, I have modified libnetwork in alibaba, Can you update the latest commit on master again? Thank you very much.

idealhack · 2018-06-12T07:10:01Z

@rudyfly Sure, here it is.

Since I want to leave the record of it, I added a new commit instead of amending.

codecov-io · 2018-06-12T07:39:35Z

Codecov Report

Merging #1445 into master will decrease coverage by 0.11%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1445      +/-   ##
==========================================
- Coverage   40.56%   40.45%   -0.12%     
==========================================
  Files         254      254              
  Lines       16535    16527       -8     
==========================================
- Hits         6708     6686      -22     
- Misses       8965     8976      +11     
- Partials      862      865       +3

Impacted Files	Coverage Δ
daemon/mgr/network.go	`68.32% <0%> (-1.11%)`	⬇️
daemon/containerio/container_io.go	`58.33% <0%> (-3.21%)`	⬇️
ctrd/watch.go	`75% <0%> (-3.13%)`	⬇️
daemon/logger/jsonfile/utils.go	`70% <0%> (-1.67%)`	⬇️
ctrd/container.go	`49.45% <0%> (-0.73%)`	⬇️
daemon/mgr/container.go	`49.2% <0%> (-0.09%)`	⬇️

Signed-off-by: Yang Li <idealhack@gmail.com>

The adding command is `govendor add github.com/docker/libnetwork/^::github.com/alibaba/libnetwork` Signed-off-by: Yang Li <idealhack@gmail.com>

Signed-off-by: Yang Li <idealhack@gmail.com>

Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>

1. `go get -u github.com/alibaba/libnetwork` 2. `govendor update github.com/docker/libnetwork/...::github.com/alibaba/libnetwork@04a77bdbfb6f51185791ee3d61e3644f058e6b21` Signed-off-by: Yang Li <idealhack@gmail.com>

idealhack · 2018-06-12T08:42:31Z

CI tests passing after I rebased on master 🎉

We can now verify if this is what we need. Also, I've updated the description, PTAL.

rudyfly · 2018-06-12T12:03:36Z

@idealhack Fantastic! 🍺🍺

LGTM

allencloud · 2018-06-12T13:22:38Z

Thanks for your work, and it is quite important for PouchContainer @idealhack
While I think this is quite huge, I would like to invite @rudyfly to take a thorough test of this.

pouchrobot added areas/network kind/refactor size/XXL labels May 31, 2018

This was referenced May 31, 2018

[WIP] refactor: add docker/pkg/plugingetter as govendor dependency #1436

Closed

[WIP] refactor: add ishidawataru/sctp as govendor dependency #1435

Closed

refactor: remove extra logrus import #1434

Closed

allencloud assigned rudyfly May 31, 2018

rudyfly reviewed Jun 1, 2018

View reviewed changes

idealhack added a commit to idealhack/pouch that referenced this pull request Jun 1, 2018

refactor: remove useless initNetworkLog function

627d187

address @rudyfly’s [comment](AliyunContainerService#1445 (comment))

pouchrobot added the conflict/needs-rebase label Jun 7, 2018

idealhack force-pushed the refactor/govendor-libnetwork branch from 627d187 to 67f98e3 Compare June 7, 2018 03:50

idealhack added a commit to idealhack/pouch that referenced this pull request Jun 7, 2018

refactor: remove useless initNetworkLog function

19b6914

address @rudyfly’s [comment](AliyunContainerService#1445 (comment)) Signed-off-by: Yang Li <idealhack@gmail.com>

pouchrobot removed the conflict/needs-rebase label Jun 7, 2018

rudyfly reviewed Jun 7, 2018

View reviewed changes

idealhack commented Jun 7, 2018

View reviewed changes

idealhack force-pushed the refactor/govendor-libnetwork branch from 8771c6d to 429f4e1 Compare June 7, 2018 09:03

idealhack force-pushed the refactor/govendor-libnetwork branch from 3af70b8 to 5eb0ea3 Compare June 8, 2018 04:35

idealhack force-pushed the refactor/govendor-libnetwork branch 2 times, most recently from 02d67e9 to a8c1cf2 Compare June 12, 2018 07:15

idealhack and others added 7 commits June 12, 2018 16:01

refactor: remove extra logrus import and initNetworkLog function

6b8f64b

Signed-off-by: Yang Li <idealhack@gmail.com>

refactor: remove extra/libnetwork

3258b6b

Signed-off-by: Yang Li <idealhack@gmail.com>

refactor: add alibaba/libnetwork as dependency with govendor

94d2fcd

The adding command is `govendor add github.com/docker/libnetwork/^::github.com/alibaba/libnetwork` Signed-off-by: Yang Li <idealhack@gmail.com>

refactor: fix api call for new version of libnetwork

53bc5c1

Signed-off-by: Yang Li <idealhack@gmail.com>

refactor: cleanup scripts related to extra/libnetwork

3a8d184

Signed-off-by: Yang Li <idealhack@gmail.com>

refactor: govendor libnetwork without -tree

1c302b8

Signed-off-by: Rudy Zhang <rudyflyzhang@gmail.com>

refactor: update ablibaba/libnetwork

a9fa255

1. `go get -u github.com/alibaba/libnetwork` 2. `govendor update github.com/docker/libnetwork/...::github.com/alibaba/libnetwork@04a77bdbfb6f51185791ee3d61e3644f058e6b21` Signed-off-by: Yang Li <idealhack@gmail.com>

idealhack force-pushed the refactor/govendor-libnetwork branch from a8c1cf2 to a9fa255 Compare June 12, 2018 08:02

pouchrobot added the LGTM one maintainer or community participant agrees to merge the pull reuqest. label Jun 12, 2018

rudyfly merged commit bd4fa8d into AliyunContainerService:master Jun 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use govendor to depend on libnetwork #1445

refactor: use govendor to depend on libnetwork #1445

idealhack commented May 31, 2018 •

edited

Loading

allencloud commented May 31, 2018

idealhack commented May 31, 2018 •

edited

Loading

rudyfly Jun 1, 2018

idealhack Jun 1, 2018

rudyfly commented Jun 1, 2018

idealhack commented Jun 1, 2018 •

edited

Loading

rudyfly commented Jun 6, 2018

idealhack commented Jun 7, 2018

rudyfly commented Jun 7, 2018

idealhack commented Jun 7, 2018

rudyfly Jun 7, 2018

idealhack Jun 7, 2018

rudyfly Jun 7, 2018 •

edited

Loading

idealhack Jun 7, 2018

idealhack Jun 7, 2018

idealhack Jun 7, 2018

rudyfly commented Jun 7, 2018 •

edited

Loading

idealhack commented Jun 7, 2018

rudyfly commented Jun 7, 2018

idealhack commented Jun 7, 2018 •

edited

Loading

rudyfly commented Jun 7, 2018

rudyfly commented Jun 8, 2018

idealhack commented Jun 8, 2018

rudyfly commented Jun 8, 2018 •

edited

Loading

idealhack commented Jun 8, 2018

idealhack commented Jun 8, 2018

fuweid commented Jun 9, 2018

idealhack commented Jun 9, 2018

rudyfly commented Jun 12, 2018

idealhack commented Jun 12, 2018 •

edited

Loading

codecov-io commented Jun 12, 2018 •

edited

Loading

idealhack commented Jun 12, 2018

rudyfly commented Jun 12, 2018

allencloud commented Jun 12, 2018

refactor: use govendor to depend on libnetwork #1445

refactor: use govendor to depend on libnetwork #1445

Conversation

idealhack commented May 31, 2018 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Describe how you did it

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

allencloud commented May 31, 2018

idealhack commented May 31, 2018 • edited Loading

rudyfly Jun 1, 2018

Choose a reason for hiding this comment

idealhack Jun 1, 2018

Choose a reason for hiding this comment

rudyfly commented Jun 1, 2018

idealhack commented Jun 1, 2018 • edited Loading

rudyfly commented Jun 6, 2018

idealhack commented Jun 7, 2018

rudyfly commented Jun 7, 2018

idealhack commented Jun 7, 2018

rudyfly Jun 7, 2018

Choose a reason for hiding this comment

idealhack Jun 7, 2018

Choose a reason for hiding this comment

rudyfly Jun 7, 2018 • edited Loading

Choose a reason for hiding this comment

idealhack Jun 7, 2018

Choose a reason for hiding this comment

idealhack Jun 7, 2018

Choose a reason for hiding this comment

idealhack Jun 7, 2018

Choose a reason for hiding this comment

rudyfly commented Jun 7, 2018 • edited Loading

idealhack commented Jun 7, 2018

rudyfly commented Jun 7, 2018

idealhack commented Jun 7, 2018 • edited Loading

rudyfly commented Jun 7, 2018

rudyfly commented Jun 8, 2018

idealhack commented Jun 8, 2018

rudyfly commented Jun 8, 2018 • edited Loading

idealhack commented Jun 8, 2018

idealhack commented Jun 8, 2018

fuweid commented Jun 9, 2018

idealhack commented Jun 9, 2018

rudyfly commented Jun 12, 2018

idealhack commented Jun 12, 2018 • edited Loading

codecov-io commented Jun 12, 2018 • edited Loading

Codecov Report

idealhack commented Jun 12, 2018

rudyfly commented Jun 12, 2018

allencloud commented Jun 12, 2018

idealhack commented May 31, 2018 •

edited

Loading

idealhack commented May 31, 2018 •

edited

Loading

idealhack commented Jun 1, 2018 •

edited

Loading

rudyfly Jun 7, 2018 •

edited

Loading

rudyfly commented Jun 7, 2018 •

edited

Loading

idealhack commented Jun 7, 2018 •

edited

Loading

rudyfly commented Jun 8, 2018 •

edited

Loading

idealhack commented Jun 12, 2018 •

edited

Loading

codecov-io commented Jun 12, 2018 •

edited

Loading