Skip to content

Commit

Permalink
fix(node): update-config in testnet environment (#3072)
Browse files Browse the repository at this point in the history
- Add a check in update-config preventing a "new" testnet config from
being overwritten. This check fixes errors in the [config
revamp](#1563) caused by update-config
erroneously overriding development config fields. This change will have
no impact on mainnet: cf825d2

- Allowing an empty hostname field in GuestOS fixes update-config errors
in the nested tests.
Example: https://github.com/dfinity/ic/actions/runs/12244488249
The hostname field will always be populated on mainnet, so this will
only impact testing: 764230f

- Switching nested testing to run with a "testnet" deployment
environment (54cde2e,
23d8824) fixes issues with deployment
configs being overwritten.
  • Loading branch information
andrewbattat authored Dec 13, 2024
1 parent 9064779 commit e9ff67e
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 28 deletions.
63 changes: 37 additions & 26 deletions rs/ic_os/config/src/update_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use macaddr::MacAddr6;

use crate::config_ini::{get_config_ini_settings, ConfigIniSettings};
use crate::deployment_json::get_deployment_settings;
use crate::serialize_and_write_config;
use crate::{deserialize_config, serialize_and_write_config};
use config_types::*;
use network::resolve_mgmt_mac;

Expand All @@ -26,6 +26,18 @@ pub fn update_guestos_config() -> Result<()> {
let network_conf_path = config_dir.join("network.conf");
let config_json_path = config_dir.join("config.json");

// If a config already exists and is Testnet, do not update.
if config_json_path.exists() {
if let Ok(existing_config) = deserialize_config::<GuestOSConfig, _>(&config_json_path) {
if existing_config.icos_settings.deployment_environment
== DeploymentEnvironment::Testnet
{
println!("A new GuestOSConfig already exists and the environment is Testnet. Skipping update.");
return Ok(());
}
}
}

let old_config_exists = network_conf_path.exists();

if old_config_exists {
Expand Down Expand Up @@ -185,19 +197,14 @@ fn read_reward_conf(config_dir: &Path) -> Result<Option<String>> {
}

fn derive_mgmt_mac_from_hostname(hostname: Option<&str>) -> Result<MacAddr6> {
if let Some(hostname) = hostname {
if let Some(unformatted_mac) = hostname.strip_prefix("guest-") {
unformatted_mac
.parse()
.map_err(|_| anyhow!("Unable to parse mac address: {}", unformatted_mac))
} else {
Err(anyhow::anyhow!(
"Hostname does not start with 'guest-': {}",
hostname
))
}
if let Some(unformatted_mac) = hostname.and_then(|h| h.strip_prefix("guest-")) {
unformatted_mac
.parse()
.map_err(|_| anyhow!("Unable to parse mac address: {}", unformatted_mac))
} else {
Err(anyhow::anyhow!("Hostname is not specified"))
"00:00:00:00:00:00"
.parse()
.map_err(|_| anyhow!("Unable to parse dummy mac address"))
}
}

Expand Down Expand Up @@ -253,6 +260,19 @@ pub fn update_hostos_config(
deployment_json_path: &Path,
hostos_config_json_path: &PathBuf,
) -> Result<()> {
// If a config already exists and is Testnet, do not update.
if hostos_config_json_path.exists() {
if let Ok(existing_config) = deserialize_config::<HostOSConfig, _>(&hostos_config_json_path)
{
if existing_config.icos_settings.deployment_environment
== DeploymentEnvironment::Testnet
{
println!("A new HostOSConfig already exists and the environment is Testnet. Skipping update.");
return Ok(());
}
}
}

let old_config_exists = config_ini_path.exists();

if old_config_exists {
Expand Down Expand Up @@ -366,19 +386,10 @@ mod tests {
let mac = derive_mgmt_mac_from_hostname(hostname)?;
assert_eq!(mac, expected_mac);

// Test with invalid hostname (wrong prefix)
let invalid_hostname = Some("host-001122334455");
let result = derive_mgmt_mac_from_hostname(invalid_hostname);
assert!(result.is_err());

// Test with invalid hostname (wrong length)
let invalid_hostname_length = Some("guest-00112233");
let result = derive_mgmt_mac_from_hostname(invalid_hostname_length);
assert!(result.is_err());

// Test with None
let result = derive_mgmt_mac_from_hostname(None);
assert!(result.is_err());
// Test empty hostname
let expected_mac: MacAddr6 = "00:00:00:00:00:00".parse().unwrap();
let mac = derive_mgmt_mac_from_hostname(None)?;
assert_eq!(mac, expected_mac);

Ok(())
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,9 @@ struct DeploymentConfig {

#[arg(long)]
mgmt_mac: Option<String>,

#[arg(long)]
deployment_environment: Option<String>,
}

#[tokio::main]
Expand Down Expand Up @@ -256,6 +259,10 @@ async fn update_deployment(path: &Path, cfg: &DeploymentConfig) -> Result<(), Er
deployment_json.resources.cpu = Some(cpu.to_owned());
}

if let Some(deployment_environment) = &cfg.deployment_environment {
deployment_json.deployment.name = deployment_environment.to_owned();
}

let mut f = File::create(path).context("failed to open deployment config file")?;
let output = serde_json::to_string_pretty(&deployment_json)?;
write!(&mut f, "{output}")?;
Expand Down
2 changes: 2 additions & 0 deletions rs/tests/driver/src/driver/bootstrap.rs
Original file line number Diff line number Diff line change
Expand Up @@ -651,6 +651,8 @@ fn configure_setupos_image(
let mut cmd = Command::new(setupos_inject_configs);
cmd.arg("--image-path")
.arg(&uncompressed_image)
.arg("--deployment-environment")
.arg("Testnet")
.arg("--mgmt-mac")
.arg(&mac)
.arg("--ipv6-prefix")
Expand Down
4 changes: 2 additions & 2 deletions rs/tests/driver/src/driver/nested.rs
Original file line number Diff line number Diff line change
Expand Up @@ -140,13 +140,13 @@ impl NestedVms for TestEnv {

let host_mac = calculate_deterministic_mac(
&seed_mac,
DeploymentEnvironment::Mainnet,
DeploymentEnvironment::Testnet,
IpVariant::V6,
NodeType::HostOS,
);
let guest_mac = calculate_deterministic_mac(
&seed_mac,
DeploymentEnvironment::Mainnet,
DeploymentEnvironment::Testnet,
IpVariant::V6,
NodeType::GuestOS,
);
Expand Down

0 comments on commit e9ff67e

Please sign in to comment.