Upgrade to mozim 0.3.0 #1343
Upgrade to mozim 0.3.0 #1343
Conversation
Reviewer's GuideThis PR upgrades the DHCP proxy implementation to mozim 0.3.0 by adopting its new Tokio-based async API and state machine (replacing Sequence diagram for DHCP lease acquisition with new mozim async APIsequenceDiagram
participant Client
participant NetavarkProxyService
participant DhcpV4Service
participant DhcpV4Client
Client->>NetavarkProxyService: setup(NetworkConfig)
NetavarkProxyService->>DhcpV4Service: new(NetworkConfig, timeout)
DhcpV4Service->>DhcpV4Client: init(config)
NetavarkProxyService->>DhcpV4Service: get_lease()
loop until lease acquired or error
DhcpV4Service->>DhcpV4Client: run()
DhcpV4Client-->>DhcpV4Service: DhcpV4State (Done(lease) or other)
end
DhcpV4Service-->>NetavarkProxyService: NetavarkLease
NetavarkProxyService-->>Client: Lease response
Sequence diagram for releasing DHCP lease with async releasesequenceDiagram
participant Client
participant NetavarkProxyService
participant DhcpV4Service
participant DhcpV4Client
Client->>NetavarkProxyService: teardown(NetworkConfig)
NetavarkProxyService->>DhcpV4Service: release_lease()
DhcpV4Service->>DhcpV4Client: release(lease)
DhcpV4Client-->>DhcpV4Service: (async result)
DhcpV4Service-->>NetavarkProxyService: Result
NetavarkProxyService-->>Client: OperationResponse
ER diagram for updated Lease and DhcpV4Lease fieldserDiagram
NetavarkLease {
t1 u32
t2 u32
lease_time u32
mtu u32
domain_name string
mac_address string
}
MozimV4Lease {
t1_sec u32
t2_sec u32
lease_time_sec u32
siaddr string
yiaddr string
srv_id string
subnet_mask string
broadcast_addr string
}
NetavarkLease ||--o{ MozimV4Lease : "from() conversion"
MozimV4Lease {
t1_sec u32
t2_sec u32
lease_time_sec u32
}
NetavarkLease {
t1 u32
t2 u32
lease_time u32
}
Class diagram for updated DhcpV4Service and related typesclassDiagram
class DhcpV4Service {
+DhcpV4Client client
+NetworkConfig network_config
+Option<MozimV4Lease> previous_lease
+async new(nc: NetworkConfig, timeout: u32) Result<Self, DhcpServiceError>
+async get_lease() Result<NetavarkLease, DhcpServiceError>
+async release_lease() Result<(), DhcpServiceError>
}
class DhcpV4Client {
+async run() Result<DhcpV4State, mozim::Error>
+async release(lease: MozimV4Lease)
}
class MozimV4Lease {
+t1_sec: u32
+t2_sec: u32
+lease_time_sec: u32
+siaddr: Ipv4Addr
+yiaddr: Ipv4Addr
+srv_id: Ipv4Addr
+subnet_mask: Ipv4Addr
+broadcast_addr: Ipv4Addr
}
class NetavarkLease {
+t1: u32
+t2: u32
+lease_time: u32
+mtu: u32
+domain_name: String
+mac_address: String
+add_domain_name(domain: &str)
+add_mac_address(mac: &str)
}
DhcpV4Service --> DhcpV4Client
DhcpV4Service --> NetworkConfig
DhcpV4Service --> MozimV4Lease
NetavarkLease <.. MozimV4Lease : from()
DhcpV4Client --> MozimV4Lease
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
I have habit of run I don't know how to test it, please review carefully. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/dhcp_proxy/dhcp_service.rs:53-62` </location>
<code_context>
impl DhcpV4Service {
- pub fn new(nc: NetworkConfig, timeout: u32) -> Result<Self, DhcpServiceError> {
- let mut config = DhcpV4Config::new_proxy(&nc.host_iface, &nc.container_mac_addr);
- config.set_timeout(timeout);
+ pub async fn new(
+ nc: NetworkConfig,
+ timeout: u32,
+ ) -> Result<Self, DhcpServiceError> {
+ let mut config =
+ DhcpV4Config::new_proxy(&nc.host_iface, &nc.container_mac_addr)
+ .map_err(|e| {
+ DhcpServiceError::new(InvalidArgument, e.to_string())
+ })?;
+ config.set_timeout_sec(timeout);
- // Sending the hostname to the DHCP server is optional but it can be useful
</code_context>
<issue_to_address>
**issue (bug_risk):** set_timeout_sec replaces set_timeout; verify units and semantic equivalence.
Ensure the timeout value is correctly converted to seconds to prevent unintended changes in behavior.
</issue_to_address>
### Comment 2
<location> `src/dhcp_proxy/dhcp_service.rs:96` </location>
<code_context>
+ /// * `client`: a IPv4 mozim dhcp client. When this method is called, it
+ /// takes ownership of client.
///
- pub async fn get_lease(&mut self) -> Result<NetavarkLease, DhcpServiceError> {
- if let Some(lease_result) = self.client.next().await {
- match lease_result {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** get_lease now loops on client.run; check for infinite loop or starvation.
Consider adding a maximum iteration count or timeout to prevent infinite loops or starvation if the DHCP server is unresponsive or returns unexpected states.
Suggested implementation:
```rust
/// Performs a DHCP DORA on a ipv4 network configuration.
/// # Arguments
///
/// * `client`: a IPv4 mozim dhcp client. When this method is called, it
/// takes ownership of client.
/// * The method will attempt to obtain a lease up to MAX_ATTEMPTS times to prevent infinite loops or starvation.
///
/// returns: Result<Lease, DhcpSearchError>. Either finds a lease
/// successfully, finds no lease, or fails
pub async fn get_lease(
&mut self,
) -> Result<NetavarkLease, DhcpServiceError> {
const MAX_ATTEMPTS: usize = 100;
let mut attempts = 0;
loop {
if attempts >= MAX_ATTEMPTS {
return Err(DhcpServiceError::Timeout);
}
attempts += 1;
let state = self.client.run().await;
match state {
Ok(DhcpV4State::Done(lease)) => {
```
You may need to ensure that `DhcpServiceError::Timeout` exists. If not, add a suitable variant to the `DhcpServiceError` enum, e.g.:
```rust
Timeout,
```
and implement its `Display`/`Error` traits as needed.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Thanks! I was looking at this yesterday, the new API does indeed look nicer and using tokio makes a lot of sense.
You need to cargo fmt the code here, at least validate is complaining, I need to look at the integration test failure to see what is wrong with that.
| client.network_config.container_mac_addr | ||
| ); | ||
| } | ||
| Err(err) => { |
There was a problem hiding this comment.
Based on your docs
Repeat run() after error will emit the same error again until [DhcpV4Client::clean_up] been invoked.
So should the error branch do this here? Otherwise we loop on the error forever which seems pointless and could that busy spin then?
There was a problem hiding this comment.
Ok that is causing the integration test failure in the "release after timeout" test as it get stucked into the error loop here and needs the cleanup, when adding it locally it passes.
FYI, for testing the dhcp parts sudo bats test-dhcp/ should work.
There was a problem hiding this comment.
Changed the patch to run clean up and retry the DHCPv4 process. Is it correct?
There was a problem hiding this comment.
the sudo bats test-dhcp/ passed in my VM.
|
We don't have My preference: max_width = 80
wrap_comments = true
reorder_imports = true
format_strings = true
group_imports = "StdExternalCrate"
imports_granularity = "Crate"
edition = "2024" |
We use whatever the defaults are without a config, we have never really cared about customizing the formatting here. |
|
If you don't have config, it will inherent the one in your $HOME folder. Anyway, let me amend the patch. |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
5 similar comments
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
|
Ephemeral COPR build failed. @containers/packit-build please check. |
Highlights for mozim 0.3: * Full tokio based rewrite which does not require standalone thread for Future wakeup. * Removed `nispor` as dependency. Signed-off-by: Gris Ge <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cathay4t, Luap99, sourcery-ai[bot] The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
3 participants
Highlights for mozim 0.3:
Future wakeup.
nisporas dependency.Summary by Sourcery
Upgrade the DHCP proxy to use mozim v0.3.0 with its async client API, switch to a full Tokio-based runtime for timeouts and signal handling, remove nispor, and update build features accordingly.
Enhancements:
Build:
Chores: