Fix raft of issues related to network listing#3508
Fix raft of issues related to network listing#3508AkihiroSuda merged 2 commits intocontainerd:mainfrom
Conversation
|
|
||
| // ParseMTU parses the mtu option | ||
| func ParseMTU(mtu string) (int, error) { | ||
| func parseMTU(mtu string) (int, error) { |
There was a problem hiding this comment.
No reason for this to be public.
| } | ||
| } | ||
| id, labels := nerdctlIDLabels(lcl.Bytes) | ||
| id, lbls := nerdctlIDLabels(lcl.Bytes) |
There was a problem hiding this comment.
Do not shadow imports.
|
|
||
| func (e *CNIEnv) CreateNetwork(opts types.NetworkCreateOptions) (*NetworkConfig, error) { //nolint:revive | ||
| var net *NetworkConfig | ||
| var netConf *NetworkConfig |
There was a problem hiding this comment.
Do not shadow imports.
| } | ||
|
|
||
| func (e *CNIEnv) FilterNetworks(filterf func(*NetworkConfig) bool) ([]*NetworkConfig, error) { | ||
| func (e *CNIEnv) filterNetworks(filterf func(*NetworkConfig) bool) ([]*NetworkConfig, error) { |
There was a problem hiding this comment.
No reason for this to be public.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| for _, net := range networkConfigs { |
There was a problem hiding this comment.
Do not shadow imports.
| log.L.Warnf("duplicate network name %q, %#v will get superseded by %#v", n.Name, original, n) | ||
| } | ||
| m[n.Name] = n | ||
| if n.NerdctlID != nil { |
There was a problem hiding this comment.
No reason for this. This is just encouraging collisions of unrelated namespaces (names and ids).
|
|
||
| type CNIEnvOpt func(e *CNIEnv) error | ||
|
|
||
| func (e *CNIEnv) ListNetworksMatch(reqs []string, allowPseudoNetwork bool) (list map[string][]*NetworkConfig, errs []error) { |
apostasie
force-pushed
the
chore-clean-network-walker
branch
from
1943696 to
c8bb50d
Compare
| limitations under the License. | ||
| */ | ||
|
|
||
| package netwalker |
There was a problem hiding this comment.
As exposed in the ticket about walkers, this pattern is introducing complexity with no clear upside.
Furthermore, for the specific case here of network, it is buggy.
Removing entirely and replacing with a simpler, shorter method in netutil.
apostasie
force-pushed
the
chore-clean-network-walker
branch
from
c8bb50d to
dc640e4
Compare
apostasie
changed the title
| return err | ||
| } | ||
| net, err = e.generateNetworkConfig(opts.Name, opts.Labels, plugins) | ||
| netConf, err = e.generateNetworkConfig(opts.Name, opts.Labels, plugins) |
There was a problem hiding this comment.
Do not shadow imports.
apostasie
force-pushed
the
chore-clean-network-walker
branch
2 times, most recently
from
94ca981 to
5e9762c
Compare
Current implementation of netwalker (henceforth network inspect and remove) suffers from many issues, rooting from mixing names and identifiers in the same namespace, faulty erroring logic, and overly complicated design making debugging and reasonning harder. This commit removes the walker and replaces it with a simpler approach. Next commit will add tests. Signed-off-by: apostasie <spam_blackhole@farcloser.world>
apostasie
force-pushed
the
chore-clean-network-walker
branch
from
5e9762c to
725482c
Compare
Signed-off-by: apostasie <spam_blackhole@farcloser.world>
apostasie
force-pushed
the
chore-clean-network-walker
branch
from
725482c to
1fe34f8
Compare
apostasie
changed the title
|
@AkihiroSuda this is ready to go. |
| { | ||
| Description: "invalid name network", | ||
| Command: test.RunCommand("network", "inspect", "∞"), | ||
| // FIXME: this is not even a valid identifier |
There was a problem hiding this comment.
Any reason to avoid ASCII here,?
There was a problem hiding this comment.
Not really (I just routinely use non-ascii when testing "wrong things" as it tends to wreak more havoc / uncover issues).
If you prefer ASCII, that is fine with me, as long as the identifier is not "valid".
There was a problem hiding this comment.
The problem here (and the reason for the FIXME) is that identifiers should be validated before looking up the network.
2 participants
Fix a number of bugs, oddities, and diverging behavior compared with docker, involving
network ls,remove,create.Notably:
nerdctl network createusing another network id[0:12] will fail #3507nerdctl networkmany issues when a network is using another network (substr) id as a name #3506nerdctl network rm bridgeshould fail instead of removing the network #3504conflist: no such file or directory(returns...) #3501walkers#3502And possibly others.
First commit is code, second commit is tests.
This is a small PR (besides tests).
Note that this is not a silver bullet for racyness / concurrency issues - a more indepth rewrite of netutil is required.