Introduce netavark create command#1394
Conversation
|
Container libs part lives here as a draft, but the tests won't pass until this PR merges, unless there's another way to throw it at tests? The config tests are updated there and pass locally, but if there's good ideas for testing, I'm all ears. |
|
@ashley-cui you have some test unhappiness |
ashley-cui
force-pushed
the
create
branch
2 times, most recently
from
ea6d595 to
5047c27
Compare
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
@containers/netavark-maintainers PTAL |
|
LGTM |
Luap99
left a comment
Luap99
left a comment
There was a problem hiding this comment.
not a full review but some comments
I did not carefully check all the validation logic matches common but I guess we can run the common libnetwork unit tests with your new code to ensure we have proper coverage if you haven't done already
| let mut child = Command::new(&plugin_path) | ||
| .arg("create") | ||
| .stdin(Stdio::piped()) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::inherit()) | ||
| .spawn()?; | ||
|
|
||
| let stdin = child.stdin.take().unwrap(); | ||
| serde_json::to_writer(&stdin, &input)?; | ||
| // Close stdin here to avoid that the plugin waits forever for an EOF. | ||
| // And then we would wait for the child to exit which would cause a hang. | ||
| drop(stdin); | ||
|
|
||
| // Note: We need to buffer the output and then deserialize into the correct type after | ||
| // the plugin exits, since the plugin can return two different json types depending on | ||
| // the exit code. | ||
| let mut buffer: Vec<u8> = Vec::new(); | ||
|
|
||
| let mut stdout = child.stdout.take().unwrap(); | ||
| // Do not handle error here, we have to wait for the child first. | ||
| let result = stdout.read_to_end(&mut buffer); | ||
|
|
||
| let exit_status = wrap!(child.wait(), "wait for plugin to exit")?; | ||
| if let Some(rc) = exit_status.code() { | ||
| // make sure the buffer is correct | ||
| wrap!(result, "read into buffer")?; | ||
| if rc == 0 { | ||
| // read the modified network config | ||
| let updated_network: Network = serde_json::from_slice(&buffer)?; | ||
| // Update the network with the plugin's response | ||
| *network = updated_network; | ||
| Ok(()) | ||
| } else { | ||
| // exit code not 0 => error | ||
| let err: JsonError = serde_json::from_slice(&buffer)?; | ||
| Err(NetavarkError::msg(format!( | ||
| "plugin {:?} failed with exit code {}, message: {}", | ||
| plugin_path.file_name().unwrap_or_default(), | ||
| rc, | ||
| err.error | ||
| ))) | ||
| } | ||
| } else { | ||
| // If we could not get the exit code then the process was killed by a signal. | ||
| // I don't think it is necessary to read and return the signal so we just return a generic error. | ||
| Err(NetavarkError::msg(format!( | ||
| "plugin {:?} killed by signal", | ||
| plugin_path.file_name().unwrap_or_default() | ||
| ))) | ||
| } | ||
| } |
There was a problem hiding this comment.
this feels like copy pasted from the other location so this must be shared as well
There was a problem hiding this comment.
There are some differences in how the code is used - the other driver plugin code requires fields such as container_name and container_id which we don't have since we're only filling out the config, I've re-pushed to consolidate some of the code using helper functions, but that was heavily LLM-aided. Let me know if the changes are theoretically okay, or if i should leave it as two separate functions.
|
re-pushed with some fixes, and split out some of the commits for easier review, I'll squash them later. |
|
@containers/netavark-maintainers PTAL |
|
LGTM |
ashley-cui
force-pushed
the
create
branch
4 times, most recently
from
8629a75 to
4279e31
Compare
| validate_routes(&mut network)?; | ||
| } | ||
|
|
||
| network.created = Some(Utc::now()); |
There was a problem hiding this comment.
Is UTC correct, looking at existing code we get the local time in the config not UTC?
I think this need to be the local time Local::now() unless I am missing something
There was a problem hiding this comment.
I think UTC is correct because Go marshall/unmarshalls into utc?
There was a problem hiding this comment.
podman network inspect podman shows me "created": "2026-03-19T14:50:37.000932454+01:00", so that is my local CET offset not UTC.
If the configs are still generated with that same offset despite being calling UTC here sure otherwise I think we need local. That said I don't think we need to block this right now on that, I will have to look at the rest again but I would ok merging it and we can fix smaller things up in follow ups so we have at least the large part merged.
|
|
||
| let bridge_opts = parse_bridge_opts(&network.options, true)?; | ||
| if bridge_opts.vlan.is_some() | ||
| || (bridge_opts.mode.is_some() && bridge_opts.mode.unwrap() != "managed") |
There was a problem hiding this comment.
using is_some() followed by unwrap() is a bit of an anti pattern I would say.
generally speaking in this function both bools always act the same so one bool is enough. Then there is no need to declare them as mut since we could write this as early return code,
if bridge_opts.vlan.is_some() {return false}
if let Some(mode) = bridge_opts.mode {
if mode != "manged" {return false}
}
return true
It also seems like the function never returns an error, so remove the result and just return the single bool
There was a problem hiding this comment.
38 has a ? that can result in an error, so I left the result
Signed-off-by: Ashley Cui <acui@redhat.com>
ashley-cui
force-pushed
the
create
branch
2 times, most recently
from
b63a5dd to
0320eeb
Compare
The netavark create command is a new command that takes a partially filled out network config, and some options, via a json on stdin. The command will validate the network and fill out incompleted fields in the config. It then will return a complete network config json. The behavior is to match and replace https://github.com/containers/container-libs/blob/1e46b0756b39f76f287c64aa46c41107ad1110bb/common/libnetwork/netavark/config.go#L118 Signed-off-by: Ashley Cui <acui@redhat.com>
Signed-off-by: Ashley Cui <acui@redhat.com>
|
@Luap99 PTAnotherL, thanks! |
|
I try to review this as first thing tomorrow. |
Luap99
left a comment
Luap99
left a comment
There was a problem hiding this comment.
LGTM, I think looking at container-libs test there is still same issue since the go boolean parser accepts 1/0 as true/false while the rust on only uses the true/false strings but we can fix this in a follow up.
|
@Luap99 Thanks for the reviews! I was thinking of requiring true/false and dropping support for 1/0 as options, do you think this would be okay, or should we keep the 1/0? Also, I containers/libs testing will be an issue, do we have any workarounds or do we need to just wait |
|
given it is podman 6 I think would be fine to always force true/false and block 1/0, if users complain later we can re-add that support for testing in container-libs we will need to create a custom CI VM image with a new netavark build off main, Matt needs that to so I look into doing that later |
4 participants
The netavark create command is a new command that takes a partially filled out network config, and some options, via a json on stdin. The command will validate the network and fill out incompleted fields in the config. It then will return a complete network config json. The behavior is to match and replace https://github.com/containers/container-libs/blob/1e46b0756b39f76f287c64aa46c41107ad1110bb/common/libnetwork/netavark/config.go#L118