Add warning message for the failure of deleting link device by luzhipeng-zte · Pull Request #2085 · moby/libnetwork

luzhipeng-zte · 2018-02-23T01:05:37Z

We can know the reason for the failure of deleting the link device.

Signed-off-by: ZhiPeng Lu [email protected]

fcrisciani · 2018-02-23T21:37:27Z

@luzhipeng-zte thanks for the contributions, right now the CI fails because of: please format Go code with 'gofmt -s -w'

fcrisciani · 2018-02-23T21:38:45Z

 		if link, err := d.nlh.LinkByName(ep.srcName); err == nil {
-			d.nlh.LinkDel(link)
+			if dellinkerr := d.nlh.LinkDel(link); dellinkerr != nil {
+				logrus.Warnf("Failed to delete interface (%s)'s link on endpoint (%s) delete: %v", ep.srcName, ep.id, dellinkerr)


can you use the format with: logrus.WithError(dellinkerr).Warnf

you maybe want this to be Errorf

Thanks. i think using the same formats better

fcrisciani · 2018-02-23T21:39:50Z

 	if link, err := ns.NlHandle().LinkByName(ep.srcName); err == nil {
-		ns.NlHandle().LinkDel(link)
+		if dellinkerr := ns.NlHandle().LinkDel(link); dellinkerr != nil {
+			logrus.Warnf("Failed to delete interface (%s)'s link on endpoint (%s) delete: %v", ep.srcName, ep.id, dellinkerr)


maybe instead of (%s) you can use %q that will keep the string "quoted"

fcrisciani · 2018-02-23T21:40:27Z

 		}
 		if link, err := d.nlh.LinkByName(ep.srcName); err == nil {
-			d.nlh.LinkDel(link)
+			if dellinkerr := d.nlh.LinkDel(link); dellinkerr != nil {


you can also simply use err := d.nlh.LinkDel(link) err will shadows the other err but is fine

selansen

you can reuse err instead of adding new variable dellinkerr. This can be applicable all the places.

selansen · 2018-02-26T03:05:58Z

Did you fix CI failure ?

codecov-io · 2018-02-27T11:18:18Z

Codecov Report

Merging #2085 into master will increase coverage by 0.02%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #2085      +/-   ##
==========================================
+ Coverage   40.38%   40.41%   +0.02%     
==========================================
  Files         139      139              
  Lines       22356    22361       +5     
==========================================
+ Hits         9029     9037       +8     
+ Misses      12000    11995       -5     
- Partials     1327     1329       +2

Impacted Files	Coverage Δ
drivers/macvlan/macvlan_network.go	`0% <0%> (ø)`	⬆️
drivers/macvlan/macvlan_endpoint.go	`0% <0%> (ø)`	⬆️
drivers/ipvlan/ipvlan_network.go	`0% <0%> (ø)`	⬆️
drivers/ipvlan/ipvlan_endpoint.go	`0% <0%> (ø)`	⬆️
drivers/overlay/ov_network.go	`2.8% <0%> (-0.01%)`	⬇️
drivers/bridge/bridge.go	`55% <0%> (-0.42%)`	⬇️
cmd/proxy/tcp_proxy.go	`82.97% <0%> (-2.13%)`	⬇️
service_common.go	`7.1% <0%> (+0.13%)`	⬆️
networkdb/delegate.go	`75.09% <0%> (+0.73%)`	⬆️
networkdb/cluster.go	`63.3% <0%> (+1.97%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f6d309...3141524. Read the comment docs.

luzhipeng-zte · 2018-02-27T11:20:21Z

@selansen @fcrisciani , i have updated the PR.
thanks

fcrisciani · 2018-02-27T16:41:02Z

 		if link, err := d.nlh.LinkByName(ep.srcName); err == nil {
-			d.nlh.LinkDel(link)
+			if err := d.nlh.LinkDel(link); err != nil {
+				logrus.WithError(err).Errorf("Failed to delete interface (%q)'s link on endpoint (%q) delete", ep.srcName, ep.id)


sorry probably I was not clear in the previous comment, what I was suggesting is that if you use the formatter %q you can remove the brackets.
See here the difference: https://play.golang.com/p/HBUIUuAx7UE
(%q) seems too much

GordonTheTurtle · 2018-03-01T00:43:25Z

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "linkdel" [email protected]:luzhipeng-zte/libnetwork.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354473656
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

luzhipeng-zte · 2018-03-02T00:19:57Z

ping @fcrisciani @selansen， i have updated the PR. could you take a look ?
Thanks.

selansen · 2018-03-02T03:41:44Z

 	defer func() {
 		if err != nil {
-			d.nlh.LinkDel(host)
+			if dellinkerr := d.nlh.LinkDel(host); dellinkerr != nil {


Can we reuse err instead of dellinkerr like above change ?

I worry that this will overwrite the previous errorand the error of LinkDel can be recorded by log

selansen · 2018-03-02T03:43:09Z

 	defer func() {
 		if err != nil {
-			d.nlh.LinkDel(sbox)
+			if dellinkerr := d.nlh.LinkDel(sbox); dellinkerr != nil {


Can we reuse err instead of dellinkerr like above change ?

luzhipeng-zte · 2018-03-03T14:19:27Z

@selansen， i have updated the PR.
Thanks.

selansen · 2018-03-04T02:12:07Z

LGTM

fcrisciani · 2018-03-05T17:34:54Z

 		if err != nil {
-			d.nlh.LinkDel(sbox)
+			if err := d.nlh.LinkDel(sbox); err != nil {
+				logrus.WithError(err).Warnf("Failed to del sandbox side interface (%s)'s link", containerIfName)


here should still be delete

luzhipeng-zte · 2018-03-06T07:05:40Z

ping @fcrisciani ， i have updated the PR.
Thanks

Signed-off-by: ZhiPeng Lu <[email protected]>

fcrisciani

LGTM

fcrisciani · 2018-03-06T17:53:49Z

Thanks @luzhipeng-zte

fcrisciani reviewed Feb 23, 2018

View reviewed changes

fcrisciani requested a review from selansen February 23, 2018 21:42

selansen reviewed Feb 26, 2018

View reviewed changes

luzhipeng-zte force-pushed the linkdel branch 2 times, most recently from f68c909 to feac87d Compare February 27, 2018 10:58

fcrisciani reviewed Feb 27, 2018

View reviewed changes

luzhipeng-zte force-pushed the linkdel branch 3 times, most recently from 165c2c5 to cbfd981 Compare March 1, 2018 00:25

GordonTheTurtle added the dco/no label Mar 1, 2018

luzhipeng-zte force-pushed the linkdel branch from 0af6fb2 to b65b5a6 Compare March 1, 2018 00:55

GordonTheTurtle removed the dco/no label Mar 1, 2018

selansen reviewed Mar 2, 2018

View reviewed changes

luzhipeng-zte force-pushed the linkdel branch from b65b5a6 to 9a4ee3a Compare March 3, 2018 13:37

fcrisciani reviewed Mar 5, 2018

View reviewed changes

luzhipeng-zte force-pushed the linkdel branch from 9a4ee3a to 3141524 Compare March 6, 2018 00:38

Add warning message for the failure of deleting link device

3141524

Signed-off-by: ZhiPeng Lu <[email protected]>

fcrisciani approved these changes Mar 6, 2018

View reviewed changes

fcrisciani merged commit 37b12fb into moby:master Mar 6, 2018

thaJeztah mentioned this pull request Mar 13, 2018

Fix automatic removal of ingress sandbox when last service leaves moby/moby#36538

Merged

Conversation

luzhipeng-zte commented Feb 23, 2018

Uh oh!

fcrisciani commented Feb 23, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

selansen left a comment

Choose a reason for hiding this comment

Uh oh!

selansen commented Feb 26, 2018

Uh oh!

codecov-io commented Feb 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

luzhipeng-zte commented Feb 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GordonTheTurtle commented Mar 1, 2018

Uh oh!

luzhipeng-zte commented Mar 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luzhipeng-zte commented Mar 3, 2018

Uh oh!

selansen commented Mar 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luzhipeng-zte commented Mar 6, 2018

Uh oh!

fcrisciani left a comment

Choose a reason for hiding this comment

Uh oh!

fcrisciani commented Mar 6, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

codecov-io commented Feb 27, 2018 •

edited

Loading