Fix: Deleting multiple networks stops at first delete fail #1618

yzxiu · 2022-12-07T14:54:36Z

 ~ nerdctl network create deleteme --gateway 10.10.11.1 --subnet 10.10.11.0/24

f9d6cec6deabf54691804bb7c1bf02ad7f461ab07f31a18398495b9de1cabf28

 ~ nerdctl  network ls                                                         
NETWORK ID      NAME               FILE
                k8s-pod-network    /etc/cni/net.d/10-calico.conflist
17f29b073143    bridge             /etc/cni/net.d/nerdctl-bridge.conflist
f9d6cec6deab    deleteme           /etc/cni/net.d/nerdctl-deleteme.conflist
                host               
                none

 ~ nerdctl network rm host k8s-pod-network test deleteme
ERRO[0000] pseudo network "host" cannot be removed      
ERRO[0000] k8s-pod-network is managed outside nerdctl and cannot be removed 
ERRO[0000] No such network: test                        
f9d6cec6deab

Signed-off-by: yaozhenxiu 946666800@qq.com

yzxiu · 2022-12-07T15:11:06Z

@fahedouch see #1592

fahedouch · 2022-12-07T15:18:49Z

cmd/nerdctl/network_rm.go

 		} else if n == 0 {
-			return fmt.Errorf("no such network %s", name)
+			fmt.Fprintf(cmd.OutOrStdout(), "Error: No such network: %s\n", name)


please use logrus.Error(err) or logrus.WithError(err).Errorf("failed to ... %s", var) to set the Error level for these logs

pkg/idutil/netwalker/netwalker.go

AkihiroSuda · 2022-12-08T02:44:45Z

Please squash commits

yzxiu · 2022-12-08T03:12:31Z

Please squash commits

Done

fahedouch · 2022-12-08T08:53:39Z

cmd/nerdctl/network_rm.go

@@ -93,14 +94,17 @@ func networkRmAction(cmd *cobra.Command, args []string) error {

 	for _, name := range args {
 		if name == "host" || name == "none" {
-			return fmt.Errorf("pseudo network %q cannot be removed", name)
+			logrus.Errorf("pseudo network %q cannot be removed\n", name)


\n is useless ( it is not interpreted)

cmd/nerdctl/network_rm.go

pkg/idutil/netwalker/netwalker.go

fahedouch · 2022-12-08T11:32:50Z

cc @yzxiu

    network_rm_linux_test.go:65: assertion failed: res.ExitCode is 0
--- FAIL: TestNetworkRemoveWhenLinkWithContainer (0.47s)

yzxiu · 2022-12-09T08:05:08Z

cc @yzxiu

    network_rm_linux_test.go:65: assertion failed: res.ExitCode is 0
--- FAIL: TestNetworkRemoveWhenLinkWithContainer (0.47s)

@fahedouch PTAL

cmd/nerdctl/network_rm.go

fahedouch · 2022-12-09T08:50:21Z

cmd/nerdctl/network_rm.go

+
+	// compatible with docker
+	if status != 0 {
+		return ExitCodeError{


return errors.New(..) is enough

ExitCodeError is to allow the program to exit with status code 1 without outputting an error message.

if return errors.New(..), will output redundant error lines, like

ERRO[0000] network "deleteme" is in use by container ["182ea93ddc15b187419a05cb10a4b8d3cdace1fc2b8592a10b345396ce91edc7"] FATA[0000]

There is no error message needs to be output here(err is always nil), but the program needs to exit with status 1, so use ExitCodeError

ok got the point, can we change status by code ( more explicit I guess) and error :nil is useless

Suggested change

return ExitCodeError{

return ExitCodeError{

exitCode: code,

}

ok got the point, can we change status by code ( more explicit I guess) and error :nil is useless

Done

ExitCodeError is to allow the program to exit with status code 1 without outputting an error message.

if return errors.New(..), will output redundant error lines, like

ERRO[0000] network "deleteme" is in use by container ["182ea93ddc15b187419a05cb10a4b8d3cdace1fc2b8592a10b345396ce91edc7"] FATA[0000]

Could you add that in the code comment?
Or just print warnings for errs[0]...errs[n-2] and return errs[n-1]?

ExitCodeError is to allow the program to exit with status code 1 without outputting an error message.

Add that in the code comment, like this?

// compatible with docker // ExitCodeError is to allow the program to exit with status code 1 without outputting an error message. if code != 0 { return ExitCodeError{ exitCode: code, } }

ExitCodeError is to allow the program to exit with status code 1 without outputting an error message.
if return errors.New(..), will output redundant error lines, like

ERRO[0000] network "deleteme" is in use by container ["182ea93ddc15b187419a05cb10a4b8d3cdace1fc2b8592a10b345396ce91edc7"] FATA[0000]

Could you add that in the code comment? Or just print warnings for errs[0]...errs[n-2] and return errs[n-1]?

return err will result in two different outputs, like ERRO[0000]... and FATA[0000]...

I think using ExitCodeError here is a better choice

Add that in the code comment, like this?

Yes

Add that in the code comment, like this?

Yes

Done

fahedouch

Thanks

Signed-off-by: yaozhenxiu <946666800@qq.com>

AkihiroSuda

Thanks

yzxiu mentioned this pull request Dec 7, 2022

Fix: Deleting multiple networks stops at first delete fail #1592

Closed

fahedouch reviewed Dec 7, 2022

View reviewed changes

pkg/idutil/netwalker/netwalker.go Outdated Show resolved Hide resolved

yzxiu force-pushed the fix-rm-network branch from b6acaf7 to 3b36a9d Compare December 8, 2022 03:02

fahedouch added this to the v1.1.0 milestone Dec 8, 2022

fahedouch reviewed Dec 8, 2022

View reviewed changes

yzxiu force-pushed the fix-rm-network branch from 256acf2 to 44f0b80 Compare December 8, 2022 09:29

Zheaoli added the area/network label Dec 8, 2022

AkihiroSuda modified the milestones: v1.1.0, v1.1.1 (tentative) Dec 9, 2022

yzxiu force-pushed the fix-rm-network branch 3 times, most recently from ae16016 to 7248cda Compare December 9, 2022 07:35

fahedouch reviewed Dec 9, 2022

View reviewed changes

yzxiu force-pushed the fix-rm-network branch from ed13cd3 to 5fd830e Compare December 9, 2022 10:46

fahedouch approved these changes Dec 9, 2022

View reviewed changes

AkihiroSuda self-requested a review December 9, 2022 14:26

fahedouch mentioned this pull request Dec 9, 2022

fix network walker filter #1633

Closed

Fix: Deleting multiple networks stops at first delete fail

8fab27b

Signed-off-by: yaozhenxiu <946666800@qq.com>

yzxiu force-pushed the fix-rm-network branch from 5fd830e to 8fab27b Compare December 9, 2022 15:43

AkihiroSuda modified the milestones: v1.1.1 (tentative), v1.1.0 Dec 9, 2022

AkihiroSuda approved these changes Dec 9, 2022

View reviewed changes

AkihiroSuda merged commit 4a71d00 into containerd:main Dec 9, 2022

yzxiu deleted the fix-rm-network branch December 9, 2022 20:00

fahedouch mentioned this pull request Dec 9, 2022

Deleting multiple networks stops at first delete fail #1587

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Deleting multiple networks stops at first delete fail #1618

Fix: Deleting multiple networks stops at first delete fail #1618

yzxiu commented Dec 7, 2022 •

edited

yzxiu commented Dec 7, 2022

fahedouch Dec 7, 2022

yzxiu Dec 7, 2022

AkihiroSuda commented Dec 8, 2022

yzxiu commented Dec 8, 2022

fahedouch Dec 8, 2022

fahedouch commented Dec 8, 2022

yzxiu commented Dec 9, 2022

fahedouch Dec 9, 2022

yzxiu Dec 9, 2022

yzxiu Dec 9, 2022 •

edited

fahedouch Dec 9, 2022

yzxiu Dec 9, 2022

AkihiroSuda Dec 9, 2022

yzxiu Dec 9, 2022

yzxiu Dec 9, 2022

AkihiroSuda Dec 9, 2022

yzxiu Dec 9, 2022

fahedouch left a comment

AkihiroSuda left a comment

Fix: Deleting multiple networks stops at first delete fail #1618

Fix: Deleting multiple networks stops at first delete fail #1618

Conversation

yzxiu commented Dec 7, 2022 • edited

yzxiu commented Dec 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AkihiroSuda commented Dec 8, 2022

yzxiu commented Dec 8, 2022

Choose a reason for hiding this comment

fahedouch commented Dec 8, 2022

yzxiu commented Dec 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yzxiu Dec 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fahedouch left a comment

Choose a reason for hiding this comment

AkihiroSuda left a comment

Choose a reason for hiding this comment

yzxiu commented Dec 7, 2022 •

edited

yzxiu Dec 9, 2022 •

edited