Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions crates/api-core/src/dhcp/expire.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ pub async fn expire_dhcp_lease(
mac_address,
} = request.into_inner();
let ip_address: IpAddr = ip_address.parse()?;

let mac_address: Option<MacAddress> = mac_address
.as_deref()
.map(|m| m.parse::<MacAddress>().map_err(CarbideError::from))
Expand All @@ -45,6 +46,28 @@ pub async fn expire_dhcp_lease(
// to still exist, so we must do this before the delete.
let interface = db::machine_interface::find_by_ip(&mut txn, ip_address).await?;

if ip_address.is_ipv4() || ip_address.is_ipv6() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of if ip_address.is_ipv4() || ip_address.is_ipv6() {, how about we add a config bool handle_lease_expiration_callback, defaults to false, and then this just becomes:

if !handle_lease_expiration_callback

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done.

// This is just a dummy check which will be `true` always.
// The problem with lease expiry handling is that
// 1. If a BMC IP is released, there is no way to update it in machine_topologies table,
// which causes a mismatch between machine_interface and topology entry.
// 2. Since this might cauase BMC IP to change, DPF right now does not support BMC IP
// change. Again a mismatch between DPF and NICo.
// 3. State machine can't process the host since there is no address attached to a interface.
// Blocking this handling for now and will revisit once DPF releases the fix.
tracing::info!("Expire lease handling for DHCP is blocked.");
txn.rollback().await?;
return Ok(Response::new(rpc::ExpireDhcpLeaseResponse {
ip_address: ip_address.to_string(),
status: if interface.is_none() {
rpc::ExpireDhcpLeaseStatus::NotFound
} else {
rpc::ExpireDhcpLeaseStatus::NotHandled
}
.into(),
}));
}

// When the caller provides the MAC, scope the delete to the (ip, mac)
// pair. Otherwise, just call the address-only variant, which would
// be something we would see from an admin-cli call used for deleting
Expand Down
46 changes: 21 additions & 25 deletions crates/api-core/src/tests/dhcp_lease_expiration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ async fn test_expire_releases_allocation(
let ip = interface.addresses[0];
txn.commit().await?;

// Expire the lease via the RPC endpoint
// Expire the lease via the RPC endpoint — currently blocked, returns NotHandled.
let response = env
.api
.expire_dhcp_lease(Request::new(ExpireDhcpLeaseRequest {
Expand All @@ -59,20 +59,20 @@ async fn test_expire_releases_allocation(

let resp = response.into_inner();
assert_eq!(resp.ip_address, ip.to_string());
assert_eq!(resp.status(), ExpireDhcpLeaseStatus::Released);
assert_eq!(resp.status(), ExpireDhcpLeaseStatus::NotHandled);

// Verify the address was deleted but the interface preserved
// Address and interface must both still exist (expiry is blocked).
let mut txn = env.pool.begin().await?;
let result =
db::machine_interface_address::find_ipv4_for_interface(&mut txn, interface.id).await;
assert!(result.is_err(), "address should have been deleted");

let iface = db::machine_interface::find_one(&mut *txn, interface.id).await?;
let addr =
db::machine_interface_address::find_ipv4_for_interface(&mut txn, interface.id).await?;
assert_eq!(
iface.id, interface.id,
"interface should still exist (only the address is removed)"
addr.address, ip,
"address should still exist while expiry is blocked"
);

let iface = db::machine_interface::find_one(&mut *txn, interface.id).await?;
assert_eq!(iface.id, interface.id, "interface should still exist");

Ok(())
}

Expand Down Expand Up @@ -157,9 +157,7 @@ async fn test_discover_reallocates_after_expiration(
"should get an IP on first discover"
);

// Now, expire the lease by actually sending an `expire_dhcp_lease()`
// API call. This deletes the address, BUT keeps the interface (which
// now doesn't have an address).
// Expire the lease — currently blocked, so the address is NOT deleted.
let expire_response = env
.api
.expire_dhcp_lease(Request::new(ExpireDhcpLeaseRequest {
Expand All @@ -168,7 +166,7 @@ async fn test_discover_reallocates_after_expiration(
}))
.await?
.into_inner();
assert_eq!(expire_response.status(), ExpireDhcpLeaseStatus::Released);
assert_eq!(expire_response.status(), ExpireDhcpLeaseStatus::NotHandled);

// And finally, DHCP discover again! This should see the interface
// exists, but doesn't have an IP, so it will [re]allocate an IP to
Expand Down Expand Up @@ -232,7 +230,7 @@ async fn test_expire_does_not_delete_static_allocation(
.into_inner();
assert_eq!(
response.status(),
ExpireDhcpLeaseStatus::NotFound,
ExpireDhcpLeaseStatus::NotHandled,
"static allocation should not be expired"
);

Expand Down Expand Up @@ -284,7 +282,7 @@ async fn test_static_address_survives_expiration_and_rediscover(
.into_inner();
assert_eq!(
expire_response.status(),
ExpireDhcpLeaseStatus::NotFound,
ExpireDhcpLeaseStatus::NotHandled,
"static address should not be expired"
);

Expand Down Expand Up @@ -339,7 +337,7 @@ async fn test_expire_with_matching_mac_releases(
}))
.await?
.into_inner();
assert_eq!(response.status(), ExpireDhcpLeaseStatus::Released);
assert_eq!(response.status(), ExpireDhcpLeaseStatus::NotHandled);

Ok(())
}
Expand Down Expand Up @@ -376,7 +374,7 @@ async fn test_expire_resets_hostname_and_discover_restores_it(
);
drop(txn);

// Expire the lease.
// Expire the lease — currently blocked, hostname is not changed.
let expire_response = env
.api
.expire_dhcp_lease(Request::new(ExpireDhcpLeaseRequest {
Expand All @@ -385,16 +383,14 @@ async fn test_expire_resets_hostname_and_discover_restores_it(
}))
.await?
.into_inner();
assert_eq!(expire_response.status(), ExpireDhcpLeaseStatus::Released);
assert_eq!(expire_response.status(), ExpireDhcpLeaseStatus::NotHandled);

// After expiry the hostname must be the dormant no-IP placeholder.
// Hostname must be unchanged since expiry is blocked.
let mut txn = env.pool.begin().await?;
let iface_after_expiry = db::machine_interface::find_one(&mut *txn, interface_id).await?;
let expected_dormant = format!("noip-{}", mac_address.replace(':', "-"));
assert_eq!(
iface_after_expiry.hostname.to_lowercase(),
expected_dormant.to_lowercase(),
"hostname should be reset to dormant placeholder after lease expiry"
iface_after_expiry.hostname, expected_hostname,
"hostname should be unchanged while expiry is blocked"
);
drop(txn);

Expand Down Expand Up @@ -456,7 +452,7 @@ async fn test_expire_with_mismatched_mac_is_no_op(
.into_inner();
assert_eq!(
response.status(),
ExpireDhcpLeaseStatus::NotFound,
ExpireDhcpLeaseStatus::NotHandled,
"late expire hook with previous MAC must not delete new record"
);

Expand Down
3 changes: 3 additions & 0 deletions crates/dhcp/src/lease_expiration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,9 @@ fn expire_lease_at(
rpc::ExpireDhcpLeaseStatus::NotFound => {
log::info!("No allocation found for expired lease {ip_str}");
}
rpc::ExpireDhcpLeaseStatus::NotHandled => {
log::info!("{ip_str} found but not handled by api.");
}
Comment thread
abvarshney-nv marked this conversation as resolved.
}
LeaseExpirationResult::Success
}
Expand Down
1 change: 1 addition & 0 deletions crates/rpc/proto/forge.proto
Original file line number Diff line number Diff line change
Expand Up @@ -3928,6 +3928,7 @@ message ExpireDhcpLeaseRequest {
enum ExpireDhcpLeaseStatus {
EXPIRE_DHCP_LEASE_STATUS_RELEASED = 0;
EXPIRE_DHCP_LEASE_STATUS_NOT_FOUND = 1;
EXPIRE_DHCP_LEASE_STATUS_NOT_HANDLED = 2;
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}

message ExpireDhcpLeaseResponse {
Expand Down
1 change: 1 addition & 0 deletions rest-api/flow/internal/nicoapi/nicoproto/nico.proto
Original file line number Diff line number Diff line change
Expand Up @@ -3804,6 +3804,7 @@ message ExpireDhcpLeaseRequest {
enum ExpireDhcpLeaseStatus {
EXPIRE_DHCP_LEASE_STATUS_RELEASED = 0;
EXPIRE_DHCP_LEASE_STATUS_NOT_FOUND = 1;
EXPIRE_DHCP_LEASE_STATUS_NOT_HANDLED = 2;
}

message ExpireDhcpLeaseResponse {
Expand Down
Loading