Use netavark create to fill out network config#610
Conversation
ashley-cui
force-pushed
the
netavarkcreate
branch
2 times, most recently
from
fa4eabd to
0a9721c
Compare
|
Packit jobs failed. @containers/packit-build please check. |
ashley-cui
force-pushed
the
netavarkcreate
branch
2 times, most recently
from
aaf14f5 to
c34e22b
Compare
ashley-cui
force-pushed
the
netavarkcreate
branch
3 times, most recently
from
23535d7 to
dac307f
Compare
|
Tests are passing here, but would love to do a vendor into podman before this merges, but that depends on new images which are currently failing. Hold tight.. |
ashley-cui
force-pushed
the
netavarkcreate
branch
2 times, most recently
from
c896fb5 to
e635760
Compare
Luap99
added
the
podman 6
ashley-cui
force-pushed
the
netavarkcreate
branch
from
e635760 to
188ff19
Compare
Signed-off-by: Ashley Cui <acui@redhat.com>
ashley-cui
force-pushed
the
netavarkcreate
branch
from
188ff19 to
cbe9f9a
Compare
Signed-off-by: Ashley Cui <acui@redhat.com>
ashley-cui
force-pushed
the
netavarkcreate
branch
from
cbe9f9a to
f5e09b0
Compare
|
Ready to review. Tests are passing in containers/podman#28570 except for one, which will be fixed in containers/netavark#1440. Please merge the netavark PR first to unblock podman vendoring. @containers/container-libs-maintainers PTAL |
ashley-cui
force-pushed
the
netavarkcreate
branch
from
f5e09b0 to
df86146
Compare
For containers/container-libs#610 Signed-off-by: Ashley Cui <acui@redhat.com>
From containers/automation_images#443 Signed-off-by: Ashley Cui <acui@redhat.com>
ashley-cui
force-pushed
the
netavarkcreate
branch
from
df86146 to
0c8ff08
Compare
For containers/container-libs#610 Signed-off-by: Ashley Cui <acui@redhat.com>
|
Tests are passing in containers/podman#28570, this should be good to go. |
Luap99
left a comment
Luap99
left a comment
There was a problem hiding this comment.
I don't really have time to do full thorough review, just some small comments I guess. overall seems fine
| var result *types.Network | ||
| err = n.execNetavark([]string{"create"}, needsPlugin, &opts, &result) | ||
| if err != nil { | ||
| if strings.Contains(err.Error(), "already exists") { |
There was a problem hiding this comment.
generally this is rather poor since this text could be triggered by a lot of unexpected future code paths. Is there a way we can match the exact code path and then comment here on the exact nv code and make a comment in nv code that we depend on string matches for now.
There was a problem hiding this comment.
The code path comes from
https://github.com/containers/netavark/blob/0043f76d1a54aac6aface5c5363149a2ae3cc9a2/src/network/create_config/validation.rs#L29
I can do a little more accurate match, and then comment in nv?
There was a problem hiding this comment.
yes lets match the full string network already exists , though the longer I think about it do we actually want to do that check on the rust side? feels like the check could be done much easier on the go side before we call nv? like the IgnoreIfExists check
There was a problem hiding this comment.
I think used_names still needs to be passed to netavark anyway, so we don't collide on device names when generating device names? So we could add this check in later, no rush to remove the used_names from the input json in netavark
There was a problem hiding this comment.
moving it would make sense but maybe lets not delays this further, we have some freedom to move things around later as well so lets continue with what we have
Netavark now supports a new command, netavark create, which will take incomplete network configs, validate options, and return a completed config. Migrate c/common/libnetwork to use the netavark create command Requires a new netavark, but mostly non-breaking, other than now we enforce true/false for no default route - 0/1 is no longer supported Signed-off-by: Ashley Cui <acui@redhat.com>
ashley-cui
force-pushed
the
netavarkcreate
branch
from
0c8ff08 to
fabe506
Compare
|
Light weight netavark reviewer, but the changes LGTM |
3 participants
Netavark now supports a new command, netavark create, which will take incomplete network configs, validate options, and return a completed config.
Migrate c/common/libnetwork to use the netavark create command
Requires a new netavark, but mostly non-breaking, other than now we enforce true/false for no default route - 0/1 is no longer supported