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(())