mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 16:02:36 +00:00
* perf: resolve the worker external IP in the background `run_workers` awaited `external_ip::get_ip()` — an HTTPS GET to hub.windmill.dev — before spawning any worker, so every worker process paid that round trip before its first job pull. Measured on a CE debug build it was 120-450 ms of a ~200-500 ms startup, and behind a firewall the call does not fail fast: it burns its whole 5 s connect timeout, on every process start. That cost is per-job under EXIT_AFTER_N_JOBS. The value is informational (it is only written to `worker_ping.ip`, which the workers list displays so users can whitelist the address), so nothing needs to wait on it. It now resolves into a process-wide cache off the startup path, and `WORKER_EXTERNAL_IP` supplies it explicitly for deployments that know their egress address or have no egress at all. Until it resolves the ping carries no IP, which `insert_ping_query` now COALESCEs so a reclaimed row keeps the address the previous process wrote instead of being blanked. The main loop reports the IP as soon as it lands rather than on the next periodic tick, so a short-lived process still records it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: keep unknown worker IPs out of the whitelist alert Review follow-ups: - `WhitelistIp` filtered only the `'unretrievable IP'` sentinel, so the `'NO IP'` one a pending or failed lookup now leaves in the row would be offered as an address to whitelist. It filters both. - Register `WORKER_EXTERNAL_IP` in `ENV_SETTINGS` so operators can confirm from the instance settings view that it took effect. - The worker tracked whether it had reported the IP by re-reading the cache after each ping rather than remembering what the ping carried, so a lookup landing mid-ping marked it reported without it reaching the row. The value is read once and threaded through `insert_ping` / `update_worker_ping_full`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: report a sentinel IP once the lookup has definitively failed Keeping the previous process's address on a reclaimed `worker_ping` row is right while the lookup is still in flight, but not once it has failed: the row would advertise an address nothing has confirmed, and the whitelist alert would offer it. A failed lookup now reports `UNKNOWN_IP`, leaving NULL to mean "in flight". `WORKER_EXTERNAL_IP` is rejected when longer than the `varchar(50)` column rather than panicking the worker on its initial ping, which is a hard failure. Adds the regression guard for the `ON CONFLICT` semantics: reverting to `ip = EXCLUDED.ip` would compile and blank every reclaimed row. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: keep the agent initial ping acceptable to older servers An agent worker routinely runs against a server of a different version, and one predating the background lookup rejects an initial ping carrying no IP — which `run_worker` turns into a panic, so a newly upgraded agent would crash-loop against it. The not-resolved-yet case goes over the wire as the sentinel instead, and the server maps it back so a reclaimed row still keeps its address while resolution is pending. Also documents `ip` as the one conditional exception to `insert_ping_query`'s "only `started_at` and `jobs_executed` survive a restart", and adds `WORKER_EXTERNAL_IP` to the README env-var table. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: deliver the resolved IP to servers that only take it at registration A server predating the background lookup applies `ip` from the initial ping only, and ignores it on the periodic ones. An agent registering before its lookup resolves would therefore keep the sentinel forever on such a server, where it used to report its real address. It registers a second time once the address is known, skipping that when the address is still unknown, when the server is reached over SQL and needs no second registration, or once a job has run, since registering clears the row's current job. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: re-register the resolved IP even after a job has run Gating the second registration on "this process has not run a job yet" meant an agent that pulled queued work before its lookup resolved never delivered the address to a server that only takes one at registration. No job of the worker is in flight where that runs, so the gate bought nothing beyond the last job's id, which the next job refills. Documents the two cases where WORKER_EXTERNAL_IP stops being an optimisation and becomes the only way to report an address: an agent against such a server, and a process shorter-lived than the lookup. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * revert: drop the WORKER_EXTERNAL_IP escape hatch Supplying the address by hand skips the hub lookup, which is not something to make easy. Resolving it in the background is what keeps it off the startup path; opting out of it is a separate decision this does not need to take. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: distinguish an IP never established from one that could not be retrieved `NO IP` was doing double duty: the column default for a row whose lookup has not resolved, and the marker for one that failed. An operator reading the workers list could not tell "not resolved yet" from "this instance cannot reach the hub", and the latter is the actionable one. A failed lookup now reports `unretrievable IP`, which is also what it reported before the lookup moved off the startup path. That leaves `NO IP` meaning only "no address established", which is what an agent sends while its lookup is in flight and what the server maps back to "unresolved" — so the wire sentinel no longer collides with the failure marker, and an agent delivers the failure to a server that only reads an IP at registration. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
83 lines
3.2 KiB
Rust
83 lines
3.2 KiB
Rust
/*
|
|
* Author: Ruben Fiszel
|
|
* Copyright: Windmill Labs, Inc 2022
|
|
* This file and its contents are licensed under the AGPLv3 License.
|
|
* Please see the included NOTICE for copyright information and
|
|
* LICENSE-AGPL for a copy of the license.
|
|
*/
|
|
|
|
//! Used to determine the internet address that connections from workers will appear to come from.
|
|
//!
|
|
//! For users writing scripts to access their infrastructure with firewalls requiring incoming
|
|
//! connections to be from whitelisted IP addresses.
|
|
|
|
use crate::utils::configure_client;
|
|
use std::sync::OnceLock;
|
|
use std::time::Duration;
|
|
|
|
/// No address has ever been established for the row. Matches the `worker_ping.ip` column default,
|
|
/// and doubles as what an agent sends while its lookup is in flight, since a server that predates
|
|
/// the lookup being asynchronous rejects an initial ping carrying nothing.
|
|
pub const UNKNOWN_IP: &str = "NO IP";
|
|
|
|
/// The lookup ran and could not produce an address. Distinct from [`UNKNOWN_IP`] because it tells
|
|
/// an operator the difference between "never asked" and "asked, and this instance cannot reach the
|
|
/// hub", which is the actionable one. Both are filtered out of the addresses the frontend offers
|
|
/// for whitelisting.
|
|
pub const UNRETRIEVABLE_IP: &str = "unretrievable IP";
|
|
|
|
/// `worker_ping.ip` is `VARCHAR(50)`, and a failed initial ping takes the worker down, so an
|
|
/// overlong value must not reach the insert.
|
|
const MAX_IP_LEN: usize = 50;
|
|
|
|
static EXTERNAL_IP: OnceLock<String> = OnceLock::new();
|
|
|
|
/// The external IP of this process, [`UNRETRIEVABLE_IP`] once the lookup has failed, or `None`
|
|
/// while it is still in flight.
|
|
pub fn cached_ip() -> Option<&'static str> {
|
|
EXTERNAL_IP.get().map(String::as_str)
|
|
}
|
|
|
|
/// Resolves the external IP into the process-wide cache without blocking the caller. The value is
|
|
/// informational, and behind a firewall the lookup burns its whole 5s connect timeout on every
|
|
/// process start, so nothing on the worker startup path may wait on it.
|
|
pub fn resolve_ip_in_background() {
|
|
tokio::spawn(async {
|
|
let ip = get_ip()
|
|
.await
|
|
.map(|ip| {
|
|
if ip.len() > MAX_IP_LEN {
|
|
tracing::error!("external IP lookup returned an overlong value, ignoring it");
|
|
UNRETRIEVABLE_IP.to_string()
|
|
} else {
|
|
ip
|
|
}
|
|
})
|
|
.unwrap_or_else(|e| {
|
|
tracing::warn!(
|
|
error = e.to_string(),
|
|
"failed to get external IP, workers of this process will report it as unretrievable"
|
|
);
|
|
UNRETRIEVABLE_IP.to_string()
|
|
});
|
|
let _ = EXTERNAL_IP.set(ip);
|
|
});
|
|
}
|
|
|
|
pub async fn get_ip() -> anyhow::Result<String> {
|
|
tokio::select! {
|
|
biased;
|
|
_ = tokio::time::sleep(Duration::from_secs(10)) => {
|
|
return Err(anyhow::anyhow!("Expected to get ip under 10s"))
|
|
},
|
|
ip = configure_client(reqwest::ClientBuilder::new()
|
|
.connect_timeout(Duration::from_secs(5))
|
|
.timeout(Duration::from_secs(5)))
|
|
.build()?
|
|
.get("https://hub.windmill.dev/getip")
|
|
.send() => Ok(ip?
|
|
.error_for_status()?
|
|
.text().await?),
|
|
}
|
|
}
|