Skip to content

Commit

Permalink
Migrate off storing BackendName in Docker labels (#793)
Browse files Browse the repository at this point in the history
Instead of storing a label like:
 - `"dev.plane.backend"`: `"xjbgb03bhfxqgp"`

We will now store a label like:
 - `"dev.plane.backend"`: `"true"`

Critically, this allows us to continue relying on
`"dev.plane.backend"` as an _identifier_ for plane backends, while
deprecating the reliance on that label for carrying the `BackendName`.

The only place we relied on the `BackendName` carried by this label was
in the events loop, where as it turns out docker already applies a
`"name"` attribute to events.

Otherwise, introduce some helper functions to idiomatically convert
between `ContainerId` and `BackendName`.
  • Loading branch information
michaelsilver authored Jul 25, 2024
1 parent b0621f9 commit 7336dd9
Show file tree
Hide file tree
Showing 5 changed files with 64 additions and 35 deletions.
15 changes: 5 additions & 10 deletions plane/src/drone/runtime/docker/commands.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use super::{backend_id_to_container_id, types::ContainerId, DockerRuntime};
use super::{types::ContainerId, DockerRuntime};
use crate::{
names::BackendName,
protocol::AcquiredKey,
Expand Down Expand Up @@ -52,13 +52,8 @@ pub async fn pull_image(
Ok(())
}

pub fn create_labels(backend_id: &BackendName) -> HashMap<String, String> {
vec![(
super::PLANE_DOCKER_LABEL.to_string(),
backend_id.to_string(),
)]
.into_iter()
.collect()
fn create_labels() -> HashMap<String, String> {
HashMap::from([(super::PLANE_DOCKER_LABEL.to_string(), "true".to_string())])
}

fn create_port_bindings() -> HashMap<String, Option<Vec<PortBinding>>> {
Expand Down Expand Up @@ -208,7 +203,7 @@ pub fn get_container_config_from_executor_config(

Ok(bollard::container::Config {
image: Some(exec_config.image.clone()),
labels: backend_id.map(create_labels),
labels: Some(create_labels()),
env: Some(env),
exposed_ports: Some(
vec![(format!("{}/tcp", CONTAINER_PORT), HashMap::new())]
Expand Down Expand Up @@ -266,7 +261,7 @@ pub async fn run_container(
acquired_key: Option<&AcquiredKey>,
static_token: Option<&BearerToken>,
) -> Result<ContainerId> {
let container_id = backend_id_to_container_id(backend_id);
let container_id: ContainerId = backend_id.into();

let options = bollard::container::CreateContainerOptions {
name: container_id.to_string(),
Expand Down
4 changes: 2 additions & 2 deletions plane/src/drone/runtime/docker/metrics.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use super::{backend_id_to_container_id, types::ContainerId, MetricsCallback};
use super::{types::ContainerId, MetricsCallback};
use crate::{database::backend::BackendMetricsMessage, names::BackendName};
use bollard::{container::StatsOptions, Docker};
use futures_util::Stream;
Expand All @@ -22,7 +22,7 @@ pub async fn metrics_loop(
docker: Docker,
callback: Arc<Mutex<Option<MetricsCallback>>>,
) {
let container_id = backend_id_to_container_id(&backend_id);
let container_id = ContainerId::from(&backend_id);

let mut stream = stream_metrics(&docker, &container_id);

Expand Down
33 changes: 11 additions & 22 deletions plane/src/drone/runtime/docker/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -44,10 +44,6 @@ mod wait_backend;
/// The existence of this label is used to determine whether a container is managed by Plane.
const PLANE_DOCKER_LABEL: &str = "dev.plane.backend";

pub fn backend_id_to_container_id(backend_id: &BackendName) -> ContainerId {
ContainerId::from(format!("plane-{}", backend_id))
}

#[derive(Clone, Debug, Default, Serialize, Deserialize)]
pub struct DockerRuntimeConfig {
pub runtime: Option<String>,
Expand Down Expand Up @@ -109,18 +105,22 @@ async fn events_loop(
tracing::warn!("Received event without attributes.");
continue;
};
let Some(backend_id) = attributes.get(PLANE_DOCKER_LABEL) else {
if !attributes.contains_key(PLANE_DOCKER_LABEL) {
tracing::warn!(?e.actor, "Ignoring event without Plane backend ID label.");
continue;
};
let Some(container_id) = attributes.get("name").cloned().map(ContainerId::from) else {
tracing::warn!(?e.actor, "Ignoring event without name attribute.");
continue;
};
let Ok(backend_id) = BackendName::try_from(container_id.clone()) else {
tracing::warn!(?e.actor, ?container_id, "Ignoring event with invalid backend ID.");
continue;
};

if e.action.as_deref() == Some("start") {
tracing::info!(backend_id = backend_id.as_value(), "Received start event.");
tracing::info!(?backend_id, "Received start event.");

let Ok(backend_id) = BackendName::try_from(backend_id.to_string()) else {
tracing::warn!(?e.actor, "Ignoring start event with invalid backend ID.");
continue;
};
let docker = docker.clone();
let metrics_callback = metrics_callback.clone();
tracing::info!(%backend_id, "Spawning metrics loop.");
Expand All @@ -135,17 +135,6 @@ async fn events_loop(

let exit_code = attributes.get("exitCode");
let exit_code = exit_code.and_then(|s| s.parse::<i32>().ok());
let backend_id = match BackendName::try_from(backend_id.to_string()) {
Ok(backend_id) => backend_id,
Err(err) => {
tracing::warn!(
?err,
backend_id = backend_id.as_value(),
"Ignoring event with invalid backend ID."
);
continue;
}
};

tracing::info!(
exit_code,
Expand Down Expand Up @@ -203,7 +192,7 @@ impl Runtime for DockerRuntime {
}

async fn terminate(&self, backend_id: &BackendName, hard: bool) -> Result<bool, anyhow::Error> {
let container_id = backend_id_to_container_id(backend_id);
let container_id: ContainerId = backend_id.into();

let result = if hard {
self.docker
Expand Down
20 changes: 20 additions & 0 deletions plane/src/drone/runtime/docker/types.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::names::BackendName;
use serde::{Deserialize, Serialize};
use std::fmt::Display;

Expand All @@ -10,6 +11,12 @@ impl From<String> for ContainerId {
}
}

impl From<&BackendName> for ContainerId {
fn from(backend_id: &BackendName) -> Self {
Self(format!("plane-{}", backend_id))
}
}

impl ContainerId {
pub fn as_str(&self) -> &str {
&self.0
Expand All @@ -21,3 +28,16 @@ impl Display for ContainerId {
write!(f, "{}", &self.0)
}
}
#[cfg(test)]
mod tests {
use super::*;
use crate::names::{BackendName, Name};

#[test]
fn test_backend_name_container_id_roundtrip() {
let original_name = BackendName::new_random();
let container_id: ContainerId = ContainerId::from(&original_name);
let roundtrip_name = BackendName::try_from(container_id).expect("Should convert back");
assert_eq!(original_name, roundtrip_name);
}
}
27 changes: 26 additions & 1 deletion plane/src/names.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::types::NodeKind;
use crate::{drone::runtime::docker::types::ContainerId, types::NodeKind};
use clap::error::ErrorKind;
use serde::{Deserialize, Serialize};
use std::fmt::{Debug, Display};
Expand Down Expand Up @@ -163,6 +163,19 @@ entity_name!(DroneName, Some("dr"));
entity_name!(AcmeDnsServerName, Some("ns"));
entity_name!(BackendActionName, Some("ak"));

impl TryFrom<ContainerId> for BackendName {
type Error = NameError;

fn try_from(value: ContainerId) -> Result<Self, Self::Error> {
value
.as_str()
.strip_prefix("plane-")
.ok_or_else(|| NameError::InvalidPrefix(value.to_string(), "plane-".to_string()))?
.to_string()
.try_into()
}
}

pub trait NodeName: Name {
fn kind(&self) -> NodeKind;
}
Expand Down Expand Up @@ -278,4 +291,16 @@ mod tests {
let name = "co-".to_string() + &"a".repeat(100 - 3);
assert_eq!(Err(NameError::TooLong(100)), ControllerName::try_from(name));
}

#[test]
fn test_backend_name_from_invalid_container_id() {
let container_id = ContainerId::from("invalid-123".to_string());
assert_eq!(
Err(NameError::InvalidPrefix(
"invalid-123".to_string(),
"plane-".to_string()
)),
BackendName::try_from(container_id)
);
}
}

0 comments on commit 7336dd9

Please sign in to comment.