Add missing URI schemes #51
Conversation
d4a90e4 to
c69cfd8
Compare
c69cfd8 to
995c17e
Compare
|
@nirs let me know if this works for the use case you were asking for. Thanks! |
995c17e to
f9bce2c
Compare
f9bce2c to
b1d2735
Compare
5f23366 to
1f290b8
Compare
|
https://github.com/containers/krunkit/blob/main/docs/usage.md should be updated to describe the new schemes. |
Depends on containers/krunkit#51 adding `unix` and `none` schemes. Since we don't use the listener yet we can disable it.
This removes the limit of using a single machine, removing the need to
manage tcp ports. We use:
--restful-uri unix://$MINIKUBE_HOKE/machines/NAME/krunkit.sock
The socket is created and deleted by krunkit.
NOTES:
- Depends on containers/krunkit#51.
To test this change build and install krunkit from this PR.
|
Testing with nirs/vmnet-helper#86, krunkit panics. From krunkit.log: The krunkit command: This diff fixes the panic: --- a/src/status.rs
+++ b/src/status.rs
@@ -79,14 +79,10 @@ impl FromStr for RestfulUri {
}
Ok(Self::Unix(postfix))
}
- _ => unreachable!(),
+ UriScheme::None => Ok(Self::None),
}
} else {
- if UriScheme::None != UriScheme::from_str(s)? {
- return Err(anyhow!("invalid scheme input"));
- }
-
- Ok(Self::None)
+ Err(anyhow!("invalid resful-uri"))
}
}
}With the fix I tested starting 2 vms with --restful-uri=none:// and running iperf3 between the vms. I also tested with current minikube krunkit driver using tcp://localhost:8981 Testing show that sending Stop command does nothing, and krukit is not terminated. I can reproduce using curl: In krubnkit log we don't see any error from the the listener. I guess this is an issue in libkrun, not handling the event fd. Finally I change minikube krunkit driver to use unix:// uri: I see the same behavior, krunkit does not terminate. I could also reproduce the issue with curl: In summary, this PR is almost right, but shutting down the guest does not work and users need to use signals. I guess podman does not use --restul-uri? |
|
I did another test starting krukit from this command: This vm is using Fedora 42. Previous test use a minikube vm. This time sending a POST request works and krunkit was terminated. curl commands: krunkit stderr: So it seems that krunkit is doing the right thing, but maybe the minikube vm is missing something that ignores the termination request. The interesting thing is that the same vm does terminate with vfkit. Maybe Apple virtualization framework uses another way to terminate the VM. |
|
<snip>
<snip>
Hi @nirs, thank you very much for the thorough testing. I tried this with podman by doing the following: ❯ bin/darwin/podman machine init --now --log-level=debug
# get the port for the restful service from debug console
❯ curl 'http://localhost:49748/vm/state' -o -
{"state": "VirtualMachineStateRunning"}%
❯ curl -X POST 'http://localhost:49748/vm/state' -o -
{"state": "VirtualMachineStateStopping"}%
❯ curl 'http://localhost:49748/vm/state' -o -
curl: (7) Failed to connect to localhost port 49748 after 0 ms: Couldn't connect to server
❯ bin/darwin/podman machine inspect --format '{{ .State }}'
stoppedSo it seems to work with Podman at least |
|
@jakecorrenti I tested it with a modified minikube kernel using default configuration for the architecture and it works. |
1f290b8 to
d8834e6
Compare
d8834e6 to
e7f138f
Compare
There was a problem hiding this comment.
The code looks right but the tests are broken.
This diff fixes the tests:
--- a/src/status.rs
+++ b/src/status.rs
@@ -70,7 +70,12 @@ impl FromStr for RestfulUri {
let (ip_addr, port) = parse_tcp_input(value)?;
Ok(Self::Tcp(ip_addr, port))
}
- UriScheme::Unix => Ok(Self::Unix(value.to_string())),
+ UriScheme::Unix => {
+ if value == "" {
+ return Err(anyhow!("empty unix socket path"));
+ }
+ Ok(Self::Unix(value.to_string()))
+ }
UriScheme::None => Ok(Self::None),
}
}
@@ -201,7 +206,7 @@ mod tests {
assert_eq!(true, RestfulUri::from_str("tcp:///tmp/path").is_err(),);
// none valid
- assert_eq!(RestfulUri::None, RestfulUri::from_str("none").unwrap(),);
+ assert_eq!(RestfulUri::None, RestfulUri::from_str("none://").unwrap(),);
// random string
assert_eq!(true, RestfulUri::from_str("foobar").is_err(),);| // unix valid | ||
| assert_eq!( | ||
| RestfulUri::Unix("/tmp/path".to_string()), | ||
| RestfulUri::from_str("unix:///tmp/path").unwrap() |
There was a problem hiding this comment.
If this errors, the test panics and we don't get any info on the failure.
I tested this change, simulating a bug in the code:
diff --git a/src/status.rs b/src/status.rs
index c0e889e..b8520b0 100644
--- a/src/status.rs
+++ b/src/status.rs
@@ -179,7 +179,7 @@ mod tests {
// unix valid
assert_eq!(
RestfulUri::Unix("/tmp/path".to_string()),
- RestfulUri::from_str("unix:///tmp/path").unwrap()
+ RestfulUri::from_str("uni:///tmp/path").unwrap()
);
// unix missing path
% cargo test
...
test status::tests::uri_input_parsing ... FAILED
failures:
---- status::tests::uri_input_parsing stdout ----
thread 'status::tests::uri_input_parsing' panicked at src/status.rs:182:53:
called `Result::unwrap()` on an `Err` value: invalid scheme input
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
failures:
status::tests::uri_input_parsing
test result: FAILED. 9 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.01s
error: test failed, to rerun pass `--bin krunkit`
The test cannot assume that the code does not return an error, it must validate this.
There was a problem hiding this comment.
I'm not sure I fully understand. Are you suggesting not using .unwrap in the test when trying to assert a successful parse?
There was a problem hiding this comment.
Yes, if we fail to parse we want to report an error. If we parsed and got the wrong results we want to report the wrong result. If in both case we stop running the current test this will not make much difference.
There was a problem hiding this comment.
I thinks is fine now since we have single assert per test, and the assert reports the failure with the error message or the mismatched value.
| // tcp valid | ||
| assert_eq!( | ||
| RestfulUri::Tcp(Ipv4Addr::new(127, 0, 0, 1), 8080), | ||
| RestfulUri::from_str("tcp://localhost:8080").unwrap(), |
There was a problem hiding this comment.
Same - error will break the test instead of failing a single test.
|
Looks like we are missing basic CI running the tests for every PR. We can assume that developers run the tests but it is much easier when CI runs the tests for you. |
e7f138f to
9c2787c
Compare
9c2787c to
f8cec2c
Compare
|
@nirs I updated the tests so that if a Let me know what you think! |
src/status.rs
Outdated
|
|
||
| #[test] | ||
| fn parse_valid_unix_scheme() -> Result<(), String> { | ||
| let scheme = "unix:///tmp/path"; |
src/status.rs
Outdated
| let uri = RestfulUri::from_str(scheme); | ||
| assert_ok!(scheme, uri)?; | ||
| assert_eq!(RestfulUri::Unix("/tmp/path".to_string()), uri.unwrap(),); | ||
| Ok(()) |
There was a problem hiding this comment.
Why do we have to return a Result? This makes the tests more verbose and complicated.
I think the previous tests were better. They were clear, short, and direct. Now we have temporary variables, hard to follow macros, and unclear Result.
Lets look at the previous test output and see if something is missing:
Simulating wrong path:
--- a/src/status.rs
+++ b/src/status.rs
@@ -182,7 +182,7 @@ mod tests {
#[test]
fn parse_valid_unix_scheme() {
assert_eq!(
- RestfulUri::Unix("/tmp/path".to_string()),
+ RestfulUri::Unix("/tmp/pathx".to_string()),
RestfulUri::from_str("unix:///tmp/path").unwrap()
);
}Result:
---- status::tests::parse_valid_unix_scheme stdout ----
thread 'status::tests::parse_valid_unix_scheme' panicked at src/status.rs:184:9:
assertion `left == right` failed
left: Unix("/tmp/pathx")
right: Unix("/tmp/path")
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Looks good, no change needed.
Simulating failed parse:
diff --git a/src/status.rs b/src/status.rs
index 0f925e7..f50ce58 100644
--- a/src/status.rs
+++ b/src/status.rs
@@ -183,7 +183,7 @@ mod tests {
fn parse_valid_unix_scheme() {
assert_eq!(
RestfulUri::Unix("/tmp/path".to_string()),
- RestfulUri::from_str("unix:///tmp/path").unwrap()
+ RestfulUri::from_str("uni:///tmp/path").unwrap()
);
} Result:
---- status::tests::parse_valid_unix_scheme stdout ----
thread 'status::tests::parse_valid_unix_scheme' panicked at src/status.rs:186:53:
called `Result::unwrap()` on an `Err` value: invalid scheme input
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
We see the unexpected error message, good enough.
src/status.rs
Outdated
| anyhow!(expected_err).to_string(), | ||
| uri.err().unwrap().to_string() | ||
| ); | ||
| Ok(()) |
There was a problem hiding this comment.
Simulating unexpected successful parse with previous version:
diff --git a/src/status.rs b/src/status.rs
index 0f925e7..cc0bb3d 100644
--- a/src/status.rs
+++ b/src/status.rs
@@ -191,7 +191,7 @@ mod tests {
fn parse_unix_scheme_missing_path() {
assert_eq!(
anyhow!("empty unix socket path").to_string(),
- RestfulUri::from_str("unix://").err().unwrap().to_string()
+ RestfulUri::from_str("unix://foo").err().unwrap().to_string()
);
}
Result:
---- status::tests::parse_unix_scheme_missing_path stdout ----
thread 'status::tests::parse_unix_scheme_missing_path' panicked at src/status.rs:194:54:
called `Option::unwrap()` on a `None` value
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
Not very helpful, the error message is about wrong usage instead of describing that parsing did not fail.
We can simplify the negative tests like this:
diff --git a/src/status.rs b/src/status.rs
index 0f925e7..59cabd3 100644
--- a/src/status.rs
+++ b/src/status.rs
@@ -188,11 +188,9 @@ mod tests {
}
#[test]
+ #[should_panic(expected = "empty unix socket path")]
fn parse_unix_scheme_missing_path() {
- assert_eq!(
- anyhow!("empty unix socket path").to_string(),
- RestfulUri::from_str("unix://").err().unwrap().to_string()
- );
+ RestfulUri::from_str("unix://foo").unwrap();
}
The code is much simpler and we get a reasonable error:
---- status::tests::parse_unix_scheme_missing_path stdout ----
note: test did not panic as expected
What do you think?
Vfkit supports the schemes `unix` and `none` in addition to `tcp`. Krunkit currently only implements `tcp`, so implement the remaining schemes to meet vfkit parity. Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>
Signed-off-by: Jake Correnti <jakecorrenti+github@proton.me>
f8cec2c to
8ab2816
Compare
|
@nirs since you approved the previous revision I reverted back to that. I'll come back in with another PR to address your comments on the tests. I think we should just move this PR through if it was in an ok state prior. |
Agree, time to merge. |
Depends on containers/krunkit#51 adding `unix` and `none` schemes. Since we don't use the listener yet we can disable it.
Vfkit supports the schemes
unixandnonein addition totcp.Krunkit currently only implements
tcp, so implement the remainingschemes to meet vfkit parity.
Resolves #47