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.

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

Fix: inspect multiple networks stops at first inspect fail #1870

Merged
merged 1 commit into from
Jan 25, 2023

Conversation

yzxiu
Copy link
Contributor

@yzxiu yzxiu commented Jan 18, 2023

n: aliased to sudo nerdctl

 ~ n network ls                                         
NETWORK ID      NAME               FILE
                k8s-pod-network    /etc/cni/net.d/10-calico.conflist
44262d4ba0d3    58de8b120c68       /etc/cni/net.d/nerdctl-58de8b120c68.conflist
17f29b073143    bridge             /etc/cni/net.d/nerdctl-bridge.conflist
f9d6cec6deab    deleteme           /etc/cni/net.d/nerdctl-deleteme.conflist
58de8b120c68    nerdctl_default    /etc/cni/net.d/nerdctl-nerdctl_default.conflist
84584c571686    xiu_default        /etc/cni/net.d/nerdctl-xiu_default.conflist
                host               
                none 

1、Add the option to inspect a network by id

before:

~ n network inspect f9d6cec6deab                                   
FATA[0000] no such network: f9d6cec6deab

after:

n network inspect f9d6cec6deab
[
    {
        "Name": "deleteme",
        "Id": "f9d6cec6deabf54691804bb7c1bf02ad7f461ab07f31a18398495b9de1cabf28",
        "IPAM": {
            "Config": [
                {
                    "Subnet": "10.10.11.0/24",
                    "Gateway": "10.10.11.1"
                }
            ]
        },
        "Labels": {}
    }
]

2、Fix: inspect multiple networks stops at first inspect fail

before:

n network inspect notfound bridge f9d6cec6deab k8s-pod-network notfound2
FATA[0000] no such network: notfound 

after:

n network inspect notfound bridge f9d6cec6deab k8s-pod-network notfound2
[
    {
        "Name": "bridge",
        "Id": "17f29b073143d8cd97b5bbe492bdeffec1c5fee55cc1fe2112c8b9335f8b6121",
        "IPAM": {
            "Config": [
                {
                    "Subnet": "10.4.0.0/24",
                    "Gateway": "10.4.0.1"
                }
            ]
        },
        "Labels": {}
    },
    {
        "Name": "deleteme",
        "Id": "f9d6cec6deabf54691804bb7c1bf02ad7f461ab07f31a18398495b9de1cabf28",
        "IPAM": {
            "Config": [
                {
                    "Subnet": "10.10.11.0/24",
                    "Gateway": "10.10.11.1"
                }
            ]
        },
        "Labels": {}
    },
    {
        "Name": "k8s-pod-network",
        "IPAM": {},
        "Labels": null
    }
]
ERRO[0000] No such network: notfound                    
ERRO[0000] No such network: notfound2

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

@yzxiu yzxiu force-pushed the network-inspect branch 2 times, most recently from d1f3a24 to 6c43b42 Compare January 18, 2023 03:19

walker := netwalker.NetworkWalker{
Client: e,
OnFound: func(ctx context.Context, found netwalker.Found) error {
Copy link
Member

Choose a reason for hiding this comment

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

Probably we need to check if found.MatchCount == 1 and return error if not. Otherwise a prefix (e.g., nerdctl network inspect a) corresponding to multiple ids will print all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@yzxiu yzxiu force-pushed the network-inspect branch 4 times, most recently from ce40a8b to 4bd3de9 Compare January 18, 2023 04:56
if err != nil {
return err
}
return formatter.FormatSlice(format, cmd.OutOrStdout(), result)
for _, e := range errs {
logrus.Errorln(e)
Copy link
Contributor

Choose a reason for hiding this comment

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

logrus.WithError(err).Errorf

Copy link
Contributor Author

Choose a reason for hiding this comment

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

logrus.WithError(err).Errorf

PTAL

}

// compatible with docker
// ExitCodeError is to allow the program to exit with status code 1 without outputting an error message.
Copy link
Contributor

Choose a reason for hiding this comment

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

Would not logrus print log anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would not logrus print log anyway?

sorry, i can't get your point

Copy link
Contributor

@manugupt1 manugupt1 Jan 19, 2023

Choose a reason for hiding this comment

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

Sorry! I think this comment does not make sense; since you will probably log an error when it occurs.

Copy link
Contributor Author

@yzxiu yzxiu Jan 19, 2023

Choose a reason for hiding this comment

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

comment removed

@yzxiu yzxiu force-pushed the network-inspect branch 8 times, most recently from 4fd4a21 to aab0d95 Compare January 19, 2023 02:52
Copy link
Member

@djdongjin djdongjin left a comment

Choose a reason for hiding this comment

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

left some nits, otherwise LGTM, thanks

n, err := walker.Walk(cmd.Context(), name)
if err != nil {
errs = append(errs, err)
continue
Copy link
Member

Choose a reason for hiding this comment

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

nit: this continue is not necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 125 to 127
for _, err := range errs {
logrus.Error(err)
}
Copy link
Member

Choose a reason for hiding this comment

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

nit: this can be moved into the if len(errs) > 0 since they're all code only related to errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@yzxiu yzxiu force-pushed the network-inspect branch 2 times, most recently from 6dcfbf2 to 4901595 Compare January 19, 2023 05:15
Copy link
Member

@fahedouch fahedouch left a comment

Choose a reason for hiding this comment

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

Thanks

@fahedouch fahedouch added this to the v1.2.0 milestone Jan 19, 2023
@yzxiu yzxiu force-pushed the network-inspect branch 7 times, most recently from 87aa240 to 38e69b2 Compare January 21, 2023 12:44
@yzxiu yzxiu force-pushed the network-inspect branch 2 times, most recently from 8541625 to 9791701 Compare January 21, 2023 15:22
@yzxiu yzxiu closed this Jan 23, 2023
@yzxiu yzxiu reopened this Jan 23, 2023
@yzxiu yzxiu force-pushed the network-inspect branch 2 times, most recently from 5503928 to 5a19d30 Compare January 23, 2023 19:04
Signed-off-by: yaozhenxiu <946666800@qq.com>
Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

@AkihiroSuda AkihiroSuda merged commit fa5b5ca into containerd:main Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants