-
Notifications
You must be signed in to change notification settings - Fork 45
Add bg task for collecting chicken switches from DB #8462
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -441,6 +441,8 @@ pub struct BackgroundTaskConfig { | |
pub webhook_deliverator: WebhookDeliveratorConfig, | ||
/// configuration for SP ereport ingester task | ||
pub sp_ereport_ingester: SpEreportIngesterConfig, | ||
/// reconfigurator runtime configuration | ||
pub chicken_switches: ChickenSwitchesConfig, | ||
} | ||
|
||
#[serde_as] | ||
|
@@ -594,9 +596,6 @@ pub struct PhantomDiskConfig { | |
#[serde_as] | ||
#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] | ||
pub struct BlueprintTasksConfig { | ||
/// background planner chicken switch | ||
pub disable_planner: bool, | ||
|
||
/// period (in seconds) for periodic activations of the background task that | ||
/// reads the latest target blueprint from the database | ||
#[serde_as(as = "DurationSeconds<u64>")] | ||
|
@@ -827,6 +826,20 @@ impl Default for SpEreportIngesterConfig { | |
} | ||
} | ||
|
||
#[serde_as] | ||
#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] | ||
pub struct ChickenSwitchesConfig { | ||
/// period (in seconds) for periodic activations of this background task | ||
#[serde_as(as = "DurationSeconds<u64>")] | ||
pub period_secs: Duration, | ||
} | ||
|
||
impl Default for ChickenSwitchesConfig { | ||
fn default() -> Self { | ||
Self { period_secs: Duration::from_secs(5) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this need a default? (If yes, why is it 5 here but 30 in the config files?) |
||
} | ||
} | ||
|
||
/// Configuration for a nexus server | ||
#[derive(Clone, Debug, Deserialize, PartialEq, Serialize)] | ||
pub struct PackageConfig { | ||
|
@@ -1079,7 +1092,6 @@ mod test { | |
physical_disk_adoption.period_secs = 30 | ||
decommissioned_disk_cleaner.period_secs = 30 | ||
phantom_disks.period_secs = 30 | ||
blueprints.disable_planner = true | ||
blueprints.period_secs_load = 10 | ||
blueprints.period_secs_plan = 60 | ||
blueprints.period_secs_execute = 60 | ||
|
@@ -1111,6 +1123,7 @@ mod test { | |
webhook_deliverator.first_retry_backoff_secs = 45 | ||
webhook_deliverator.second_retry_backoff_secs = 46 | ||
sp_ereport_ingester.period_secs = 47 | ||
chicken_switches.period_secs = 30 | ||
[default_region_allocation_strategy] | ||
type = "random" | ||
seed = 0 | ||
|
@@ -1247,7 +1260,6 @@ mod test { | |
period_secs: Duration::from_secs(30), | ||
}, | ||
blueprints: BlueprintTasksConfig { | ||
disable_planner: true, | ||
period_secs_load: Duration::from_secs(10), | ||
period_secs_plan: Duration::from_secs(60), | ||
period_secs_execute: Duration::from_secs(60), | ||
|
@@ -1333,6 +1345,9 @@ mod test { | |
sp_ereport_ingester: SpEreportIngesterConfig { | ||
period_secs: Duration::from_secs(47), | ||
}, | ||
chicken_switches: ChickenSwitchesConfig { | ||
period_secs: Duration::from_secs(30) | ||
} | ||
}, | ||
default_region_allocation_strategy: | ||
crate::nexus_config::RegionAllocationStrategy::Random { | ||
|
@@ -1396,7 +1411,6 @@ mod test { | |
physical_disk_adoption.period_secs = 30 | ||
decommissioned_disk_cleaner.period_secs = 30 | ||
phantom_disks.period_secs = 30 | ||
blueprints.disable_planner = true | ||
blueprints.period_secs_load = 10 | ||
blueprints.period_secs_plan = 60 | ||
blueprints.period_secs_execute = 60 | ||
|
@@ -1424,6 +1438,8 @@ mod test { | |
alert_dispatcher.period_secs = 42 | ||
webhook_deliverator.period_secs = 43 | ||
sp_ereport_ingester.period_secs = 44 | ||
chicken_switches.period_secs = 30 | ||
|
||
[default_region_allocation_strategy] | ||
type = "random" | ||
"##, | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -48,6 +48,7 @@ pub struct BackgroundTasks { | |||||
pub task_alert_dispatcher: Activator, | ||||||
pub task_webhook_deliverator: Activator, | ||||||
pub task_sp_ereport_ingester: Activator, | ||||||
pub task_chicken_switches_collector: Activator, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tiny nit - maybe
Suggested change
I'm not sure we've been consistent with this, but I'd expect "_collector" to be a task that goes out and collects things (e.g., inventory), vs "_loader" is a task that reads stuff from the DB. |
||||||
|
||||||
// Handles to activate background tasks that do not get used by Nexus | ||||||
// at-large. These background tasks are implementation details as far as | ||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -96,6 +96,7 @@ use super::tasks::blueprint_execution; | |||||
use super::tasks::blueprint_load; | ||||||
use super::tasks::blueprint_planner; | ||||||
use super::tasks::blueprint_rendezvous; | ||||||
use super::tasks::chicken_switches::ChickenSwitchesCollector; | ||||||
use super::tasks::crdb_node_id_collector; | ||||||
use super::tasks::decommissioned_disk_cleaner; | ||||||
use super::tasks::dns_config; | ||||||
|
@@ -230,6 +231,7 @@ impl BackgroundTasksInitializer { | |||||
task_alert_dispatcher: Activator::new(), | ||||||
task_webhook_deliverator: Activator::new(), | ||||||
task_sp_ereport_ingester: Activator::new(), | ||||||
task_chicken_switches_collector: Activator::new(), | ||||||
|
||||||
task_internal_dns_propagation: Activator::new(), | ||||||
task_external_dns_propagation: Activator::new(), | ||||||
|
@@ -306,6 +308,7 @@ impl BackgroundTasksInitializer { | |||||
task_alert_dispatcher, | ||||||
task_webhook_deliverator, | ||||||
task_sp_ereport_ingester, | ||||||
task_chicken_switches_collector, | ||||||
// Add new background tasks here. Be sure to use this binding in a | ||||||
// call to `Driver::register()` below. That's what actually wires | ||||||
// up the Activator to the corresponding background task. | ||||||
|
@@ -476,13 +479,26 @@ impl BackgroundTasksInitializer { | |||||
inventory_watcher | ||||||
}; | ||||||
|
||||||
let chicken_switches_collector = | ||||||
ChickenSwitchesCollector::new(datastore.clone()); | ||||||
let chicken_switches_watcher = chicken_switches_collector.watcher(); | ||||||
driver.register(TaskDefinition { | ||||||
name: "chickens_switches_watcher", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
description: "watch db for chicken switch changes", | ||||||
period: config.chicken_switches.period_secs, | ||||||
task_impl: Box::new(chicken_switches_collector), | ||||||
opctx: opctx.child(BTreeMap::new()), | ||||||
watchers: vec![], | ||||||
activator: task_chicken_switches_collector, | ||||||
}); | ||||||
|
||||||
// Background task: blueprint planner | ||||||
// | ||||||
// Replans on inventory collection and changes to the current | ||||||
// target blueprint. | ||||||
let blueprint_planner = blueprint_planner::BlueprintPlanner::new( | ||||||
datastore.clone(), | ||||||
config.blueprints.disable_planner, | ||||||
chicken_switches_watcher.clone(), | ||||||
inventory_watcher.clone(), | ||||||
rx_blueprint.clone(), | ||||||
); | ||||||
|
@@ -496,6 +512,7 @@ impl BackgroundTasksInitializer { | |||||
watchers: vec![ | ||||||
Box::new(inventory_watcher.clone()), | ||||||
Box::new(rx_blueprint.clone()), | ||||||
Box::new(chicken_switches_watcher), | ||||||
], | ||||||
activator: task_blueprint_planner, | ||||||
}); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,6 +12,7 @@ use nexus_db_queries::context::OpContext; | |
use nexus_db_queries::db::DataStore; | ||
use nexus_reconfigurator_planning::planner::Planner; | ||
use nexus_reconfigurator_preparation::PlanningInputFromDb; | ||
use nexus_types::deployment::ReconfiguratorChickenSwitches; | ||
use nexus_types::deployment::{Blueprint, BlueprintTarget}; | ||
use nexus_types::internal_api::background::BlueprintPlannerStatus; | ||
use omicron_common::api::external::LookupType; | ||
|
@@ -24,7 +25,7 @@ use tokio::sync::watch::{self, Receiver, Sender}; | |
/// Background task that runs the update planner. | ||
pub struct BlueprintPlanner { | ||
datastore: Arc<DataStore>, | ||
disabled: bool, | ||
rx_chicken_switches: Receiver<Option<ReconfiguratorChickenSwitches>>, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand why this is an Oh, as I write that I guess we don't have a default for |
||
rx_inventory: Receiver<Option<CollectionUuid>>, | ||
rx_blueprint: Receiver<Option<Arc<(BlueprintTarget, Blueprint)>>>, | ||
tx_blueprint: Sender<Option<Arc<(BlueprintTarget, Blueprint)>>>, | ||
|
@@ -33,12 +34,18 @@ pub struct BlueprintPlanner { | |
impl BlueprintPlanner { | ||
pub fn new( | ||
datastore: Arc<DataStore>, | ||
disabled: bool, | ||
rx_chicken_switches: Receiver<Option<ReconfiguratorChickenSwitches>>, | ||
rx_inventory: Receiver<Option<CollectionUuid>>, | ||
rx_blueprint: Receiver<Option<Arc<(BlueprintTarget, Blueprint)>>>, | ||
) -> Self { | ||
let (tx_blueprint, _) = watch::channel(None); | ||
Self { datastore, disabled, rx_inventory, rx_blueprint, tx_blueprint } | ||
Self { | ||
datastore, | ||
rx_chicken_switches, | ||
rx_inventory, | ||
rx_blueprint, | ||
tx_blueprint, | ||
} | ||
} | ||
|
||
pub fn watcher( | ||
|
@@ -51,7 +58,8 @@ impl BlueprintPlanner { | |
/// If it is different from the current target blueprint, | ||
/// save it and make it the current target. | ||
pub async fn plan(&mut self, opctx: &OpContext) -> BlueprintPlannerStatus { | ||
if self.disabled { | ||
let switches = self.rx_chicken_switches.borrow_and_update().clone(); | ||
if switches.is_none_or(|s| !s.planner_enabled) { | ||
debug!(&opctx.log, "blueprint planning disabled, doing nothing"); | ||
return BlueprintPlannerStatus::Disabled; | ||
} | ||
|
@@ -251,6 +259,7 @@ mod test { | |
use super::*; | ||
use crate::app::background::tasks::blueprint_load::TargetBlueprintLoader; | ||
use crate::app::background::tasks::inventory_collection::InventoryCollector; | ||
use nexus_inventory::now_db_precision; | ||
use nexus_test_utils_macros::nexus_test; | ||
|
||
type ControlPlaneTestContext = | ||
|
@@ -291,10 +300,18 @@ mod test { | |
let rx_collector = collector.watcher(); | ||
collector.activate(&opctx).await; | ||
|
||
// Enable the planner | ||
let (_tx, chicken_switches_collector_rx) = | ||
watch::channel(Some(ReconfiguratorChickenSwitches { | ||
version: 1, | ||
planner_enabled: true, | ||
time_modified: now_db_precision(), | ||
})); | ||
|
||
// Finally, spin up the planner background task. | ||
let mut planner = BlueprintPlanner::new( | ||
datastore.clone(), | ||
false, | ||
chicken_switches_collector_rx, | ||
rx_collector, | ||
rx_loader.clone(), | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small nit - should these live under
BlueprintTasksConfig
? So in the TOML, it'd be something likeblueprint.chicken_switches.loader_period_secs = N
or if we only expect this to ever really have the period, maybe eliminate the
ChickenSwitchesConfig
struct and make itblueprints.chicken_switches_loader_period_secs = N