From cd9bcd1cdf2d526a6be46c710ebb4628e98306da Mon Sep 17 00:00:00 2001 From: Aleksei Sviridkin Date: Wed, 30 Sep 2026 02:18:20 +0300 Subject: [PATCH] fix(lb): retry reconcile changes rejected as busy Adding targets already retried when Hetzner answered locked, conflict, robot_unavailable or 5xx, but the other changes a reconcile makes to a balancer failed the whole reconcile on the first such answer. A reconcile that changes the type and then the services is likely to hit the lock the type change just took, and then waits for the next retry of the service. During a reconcile, removing targets, changing the type or the algorithm, attaching or detaching a network, and adding, updating or deleting a service now go through the same short retry. The retry constants lose their add-target names since they now cover all of these calls. Deleting a balancer when its service goes away is left as it was. A target Hetzner reports as already defined now counts as live. That answer comes when an earlier attempt was applied but answered with a 5xx, and without it a balancer with a single target reported that no target could be added although the target was there. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin --- src/error.rs | 50 ++++++++-- src/lb.rs | 255 +++++++++++++++++++++++++++------------------------ 2 files changed, 177 insertions(+), 128 deletions(-) diff --git a/src/error.rs b/src/error.rs index 15616df..35e1f9d 100644 --- a/src/error.rs +++ b/src/error.rs @@ -163,16 +163,27 @@ pub fn is_temporary_rejection(error: &hcloud::apis::Error) -> bool { let hcloud::apis::Error::ResponseError(response) = error else { return false; }; - let code = k8s_openapi::serde_json::from_str::( - &response.content, - ) - .map(|body| { - body.pointer("/error/code") - .and_then(|code| code.as_str()) - .map(str::to_owned) - }); - let retryable = matches!(code, Ok(Some(code)) if matches!(code.as_str(), "locked" | "conflict" | "robot_unavailable")); - retryable || response.status.is_server_error() + matches!( + error_code(error).as_deref(), + Some("locked" | "conflict" | "robot_unavailable") + ) || response.status.is_server_error() +} + +/// Whether Hetzner refused to add a target because the balancer already has it, which +/// is what a retry gets after a call that Hetzner applied but answered with a 5xx. +#[must_use] +pub fn is_target_already_defined(error: &hcloud::apis::Error) -> bool { + error_code(error).as_deref() == Some("target_already_defined") +} + +fn error_code(error: &hcloud::apis::Error) -> Option { + let hcloud::apis::Error::ResponseError(response) = error else { + return None; + }; + let body = + k8s_openapi::serde_json::from_str::(&response.content) + .ok()?; + body.pointer("/error/code")?.as_str().map(str::to_owned) } /// Replace the API token, or any prefix of it long enough to identify it, with a marker. @@ -318,6 +329,25 @@ mod tests { assert!(!super::is_temporary_rejection(&response_error(404, ""))); } + #[test] + fn an_already_defined_target_is_recognised() { + let body = r#"{"error": {"code": "target_already_defined", "message": "already added"}}"#; + assert!(super::is_target_already_defined(&response_error(409, body))); + assert!(super::is_target_already_defined(&response_error(422, body))); + } + + #[test] + fn other_rejections_are_not_an_already_defined_target() { + let body = r#"{"error": {"code": "locked", "message": "item is locked"}}"#; + assert!(!super::is_target_already_defined(&response_error( + 423, body + ))); + assert!(!super::is_target_already_defined(&response_error(500, ""))); + let error: Error = + Error::Serde(k8s_openapi::serde_json::from_str::<()>("x").unwrap_err()); + assert!(!super::is_target_already_defined(&error)); + } + #[test] fn a_transport_failure_is_not_temporary() { let error: Error = diff --git a/src/lb.rs b/src/lb.rs index 70ddd1a..f3cf34e 100644 --- a/src/lb.rs +++ b/src/lb.rs @@ -27,8 +27,8 @@ use crate::{ }; /// Retries after the first attempt, sleeping 1 unit, 2 units, ... between them. -const ADD_TARGET_RETRIES: u32 = 2; -const ADD_TARGET_RETRY_UNIT: std::time::Duration = std::time::Duration::from_secs(1); +const BUSY_RETRIES: u32 = 2; +const BUSY_RETRY_UNIT: std::time::Duration = std::time::Duration::from_secs(1); #[derive(Debug)] pub struct LBService { @@ -199,46 +199,38 @@ impl LoadBalancer { // Here we check that all the services are configured correctly. // If the service is not configured correctly, we update it. if let Some(destination_port) = self.services.get(&service.listen_port) { - if service.destination_port == *destination_port - && service.health_check.port == *destination_port - && service.health_check.interval == self.check_interval - && service.health_check.retries == self.retries - && service.health_check.timeout == self.timeout - && service.proxyprotocol == self.proxy_mode - && service.http.is_none() - && service.health_check.protocol - == hcloud::models::load_balancer_service_health_check::Protocol::Tcp - { - // The desired configuration matches the current configuration. + if self.service_is_current(service, *destination_port) { continue; } tracing::info!( "Desired service configuration for port {} does not match current configuration. Updating ...", service.listen_port, ); - hcloud::apis::load_balancers_api::update_service( + retry_temporary(BUSY_RETRIES, BUSY_RETRY_UNIT, || { + hcloud::apis::load_balancers_api::update_service( &self.hcloud_config, - UpdateServiceParams { - id: hcloud_balancer.id, - body: Some(UpdateLoadBalancerService { - http: None, - protocol: Some(hcloud::models::update_load_balancer_service::Protocol::Tcp), - listen_port: service.listen_port, - destination_port: Some(*destination_port), - proxyprotocol: Some(self.proxy_mode), - health_check: Some(Box::new( - hcloud::models::UpdateLoadBalancerServiceHealthCheck { - protocol: Some(hcloud::models::update_load_balancer_service_health_check::Protocol::Tcp), - http: None, - interval: Some(self.check_interval), - port: Some(*destination_port), - retries: Some(self.retries), - timeout: Some(self.timeout), - }, - )), - }), - }, - ) + UpdateServiceParams { + id: hcloud_balancer.id, + body: Some(UpdateLoadBalancerService { + http: None, + protocol: Some(hcloud::models::update_load_balancer_service::Protocol::Tcp), + listen_port: service.listen_port, + destination_port: Some(*destination_port), + proxyprotocol: Some(self.proxy_mode), + health_check: Some(Box::new( + hcloud::models::UpdateLoadBalancerServiceHealthCheck { + protocol: Some(hcloud::models::update_load_balancer_service_health_check::Protocol::Tcp), + http: None, + interval: Some(self.check_interval), + port: Some(*destination_port), + retries: Some(self.retries), + timeout: Some(self.timeout), + }, + )), + }), + }, + ) + }) .await?; } else { tracing::info!( @@ -246,15 +238,17 @@ impl LoadBalancer { service.listen_port, hcloud_balancer.name, ); - hcloud::apis::load_balancers_api::delete_service( - &self.hcloud_config, - DeleteServiceParams { - id: hcloud_balancer.id, - delete_service_request: Some(DeleteServiceRequest { - listen_port: service.listen_port, - }), - }, - ) + retry_temporary(BUSY_RETRIES, BUSY_RETRY_UNIT, || { + hcloud::apis::load_balancers_api::delete_service( + &self.hcloud_config, + DeleteServiceParams { + id: hcloud_balancer.id, + delete_service_request: Some(DeleteServiceRequest { + listen_port: service.listen_port, + }), + }, + ) + }) .await?; } } @@ -269,34 +263,48 @@ impl LoadBalancer { "Found missing service. Adding service that listens for port {}", listen_port ); - hcloud::apis::load_balancers_api::add_service( - &self.hcloud_config, - AddServiceParams { - id: hcloud_balancer.id, - body: Some(LoadBalancerService { - http: None, - listen_port: *listen_port, - destination_port: *destination_port, - protocol: hcloud::models::load_balancer_service::Protocol::Tcp, - proxyprotocol: self.proxy_mode, - health_check: Box::new(LoadBalancerServiceHealthCheck { - http: None, - interval: self.check_interval, - port: *destination_port, - protocol: - hcloud::models::load_balancer_service_health_check::Protocol::Tcp, - retries: self.retries, - timeout: self.timeout, - }), - }), - }, - ) - .await?; + retry_temporary(BUSY_RETRIES, BUSY_RETRY_UNIT, || { + hcloud::apis::load_balancers_api::add_service( + &self.hcloud_config, + AddServiceParams { + id: hcloud_balancer.id, + body: Some(LoadBalancerService { + http: None, + listen_port: *listen_port, + destination_port: *destination_port, + protocol: hcloud::models::load_balancer_service::Protocol::Tcp, + proxyprotocol: self.proxy_mode, + health_check: Box::new(LoadBalancerServiceHealthCheck { + http: None, + interval: self.check_interval, + port: *destination_port, + protocol: + hcloud::models::load_balancer_service_health_check::Protocol::Tcp, + retries: self.retries, + timeout: self.timeout, + }), + }), + }, + ) + }) + .await?; } } Ok(()) } + fn service_is_current(&self, service: &LoadBalancerService, destination_port: i32) -> bool { + service.destination_port == destination_port + && service.health_check.port == destination_port + && service.health_check.interval == self.check_interval + && service.health_check.retries == self.retries + && service.health_check.timeout == self.timeout + && service.proxyprotocol == self.proxy_mode + && service.http.is_none() + && service.health_check.protocol + == hcloud::models::load_balancer_service_health_check::Protocol::Tcp + } + /// Reconcile the targets of the load balancer. /// This method will compare the desired configuration of the targets /// with the current configuration of the targets in the load balancer. @@ -328,23 +336,25 @@ impl LoadBalancer { }; if !planned.contains(&target_ip.ip.as_str()) { tracing::info!("Removing target {}", target_ip.ip); - hcloud::apis::load_balancers_api::remove_target( - &self.hcloud_config, - RemoveTargetParams { - id: hcloud_balancer.id, - remove_target_request: Some(RemoveTargetRequest { - ip: Some(target_ip), - ..Default::default() - }), - }, - ) + retry_temporary(BUSY_RETRIES, BUSY_RETRY_UNIT, || { + hcloud::apis::load_balancers_api::remove_target( + &self.hcloud_config, + RemoveTargetParams { + id: hcloud_balancer.id, + remove_target_request: Some(RemoveTargetRequest { + ip: Some(target_ip.clone()), + ..Default::default() + }), + }, + ) + }) .await?; } } let mut live = 0_usize; let mut last_error = None; - let mut retries = ADD_TARGET_RETRIES; + let mut retries = BUSY_RETRIES; for ip in &planned { if hcloud_balancer .targets @@ -355,7 +365,7 @@ impl LoadBalancer { continue; } tracing::info!("Adding target {}", ip); - let added = retry_temporary(retries, ADD_TARGET_RETRY_UNIT, || { + let added = retry_temporary(retries, BUSY_RETRY_UNIT, || { hcloud::apis::load_balancers_api::add_target( &self.hcloud_config, AddTargetParams { @@ -383,6 +393,7 @@ impl LoadBalancer { // which must not keep the remaining nodes out of the load balancer. match added { Ok(_) => live += 1, + Err(error) if crate::error::is_target_already_defined(&error) => live += 1, // Every further call would be rejected too and only drain the budget. Err(error) if crate::error::is_rate_limit_response(&error) => { return Err(error.into()); @@ -421,13 +432,15 @@ impl LoadBalancer { hcloud_balancer.algorithm, self.algorithm ); - hcloud::apis::load_balancers_api::change_algorithm( - &self.hcloud_config, - ChangeAlgorithmParams { - id: hcloud_balancer.id, - body: Some(self.algorithm.clone().into()), - }, - ) + retry_temporary(BUSY_RETRIES, BUSY_RETRY_UNIT, || { + hcloud::apis::load_balancers_api::change_algorithm( + &self.hcloud_config, + ChangeAlgorithmParams { + id: hcloud_balancer.id, + body: Some(self.algorithm.clone().into()), + }, + ) + }) .await?; Ok(()) } @@ -445,15 +458,17 @@ impl LoadBalancer { hcloud_balancer.load_balancer_type.name, self.balancer_type ); - hcloud::apis::load_balancers_api::change_type_of_load_balancer( - &self.hcloud_config, - ChangeTypeOfLoadBalancerParams { - id: hcloud_balancer.id, - change_type_of_load_balancer_request: Some(ChangeTypeOfLoadBalancerRequest { - load_balancer_type: self.balancer_type.clone(), - }), - }, - ) + retry_temporary(BUSY_RETRIES, BUSY_RETRY_UNIT, || { + hcloud::apis::load_balancers_api::change_type_of_load_balancer( + &self.hcloud_config, + ChangeTypeOfLoadBalancerParams { + id: hcloud_balancer.id, + change_type_of_load_balancer_request: Some(ChangeTypeOfLoadBalancerRequest { + load_balancer_type: self.balancer_type.clone(), + }), + }, + ) + }) .await?; Ok(()) } @@ -498,17 +513,19 @@ impl LoadBalancer { } } tracing::info!("Detaching balancer from network {}", private_net_id); - hcloud::apis::load_balancers_api::detach_load_balancer_from_network( - &self.hcloud_config, - DetachLoadBalancerFromNetworkParams { - id: hcloud_balancer.id, - detach_load_balancer_from_network_request: Some( - DetachLoadBalancerFromNetworkRequest { - network: private_net_id, - }, - ), - }, - ) + retry_temporary(BUSY_RETRIES, BUSY_RETRY_UNIT, || { + hcloud::apis::load_balancers_api::detach_load_balancer_from_network( + &self.hcloud_config, + DetachLoadBalancerFromNetworkParams { + id: hcloud_balancer.id, + detach_load_balancer_from_network_request: Some( + DetachLoadBalancerFromNetworkRequest { + network: private_net_id, + }, + ), + }, + ) + }) .await?; } } @@ -517,18 +534,20 @@ impl LoadBalancer { return Ok(()); }; tracing::info!("Attaching balancer to network {}", network_id); - hcloud::apis::load_balancers_api::attach_load_balancer_to_network( - &self.hcloud_config, - AttachLoadBalancerToNetworkParams { - id: hcloud_balancer.id, - attach_load_balancer_to_network_request: Some( - AttachLoadBalancerToNetworkRequest { - ip: self.private_ip.clone(), - network: network_id, - }, - ), - }, - ) + retry_temporary(BUSY_RETRIES, BUSY_RETRY_UNIT, || { + hcloud::apis::load_balancers_api::attach_load_balancer_to_network( + &self.hcloud_config, + AttachLoadBalancerToNetworkParams { + id: hcloud_balancer.id, + attach_load_balancer_to_network_request: Some( + AttachLoadBalancerToNetworkRequest { + ip: self.private_ip.clone(), + network: network_id, + }, + ), + }, + ) + }) .await?; } Ok(())