Skip to content

Commit

Permalink
Merge branch 'eero/hostos-followups' into 'master'
Browse files Browse the repository at this point in the history 
[NODE-1191] Assorted `ic-admin` improvements

Various followups from HostOS Upgrades, around `ic-admin`:
- Bring back a helper function that was cut from a previous MR to reduce complexity.
- Add a new `--clear-hostos-version` flag to `ic-admin` for discoverability.
- Simplify some logic under `GetReplicaVersion`, that has been panicking for as long as I can remember. 

See merge request dfinity-lab/public/ic!16058
  • Loading branch information
@Bownairo
Bownairo committed Nov 14, 2023
2 parents a954abf + 215c1d1 commit 886ae3b
Show file tree
Hide file tree
Showing 2 changed files with 71 additions and 62 deletions.
109 changes: 48 additions & 61 deletions rs/registry/admin/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
use anyhow::anyhow;
use async_trait::async_trait;
use candid::{CandidType, Decode, Encode, Principal};
use clap::Parser;
use clap::{Args, Parser};
use cycles_minting_canister::{
ChangeSubnetTypeAssignmentArgs, SetAuthorizedSubnetworkListArgs, SubnetListWithType,
UpdateSubnetTypeArgs,
Expand All @@ -14,7 +14,7 @@ use ic_config::subnet_config::SchedulerConfig;
use ic_crypto_utils_threshold_sig_der::{
parse_threshold_sig_key, parse_threshold_sig_key_from_der,
};
use ic_http_utils::file_downloader::{check_file_hash, extract_tar_gz_into_dir, FileDownloader};
use ic_http_utils::file_downloader::{check_file_hash, FileDownloader};
use ic_ic00_types::{CanisterInstallMode, EcdsaKeyId};
use ic_interfaces_registry::RegistryClient;
use ic_nervous_system_clients::{
Expand Down Expand Up @@ -69,7 +69,6 @@ use ic_protobuf::registry::{
crypto::v1::{PublicKey, X509PublicKeyCert},
dc::v1::{AddOrRemoveDataCentersProposalPayload, DataCenterRecord},
firewall::v1::{FirewallConfig, FirewallRule, FirewallRuleSet},
hostos_version::v1::HostosVersionRecord,
node::v1::NodeRecord,
node_operator::v1::{NodeOperatorRecord, RemoveNodeOperatorsPayload},
node_rewards::v2::{NodeRewardRate, UpdateNodeRewardsTableProposalPayload},
Expand All @@ -82,7 +81,7 @@ use ic_protobuf::registry::{
use ic_registry_client::client::RegistryClientImpl;
use ic_registry_client_helpers::{
crypto::CryptoRegistry, deserialize_registry_value, ecdsa_keys::EcdsaKeysRegistry,
subnet::SubnetRegistry,
hostos_version::HostosRegistry, subnet::SubnetRegistry,
};
use ic_registry_keys::{
get_node_operator_id_from_record_key, get_node_record_node_id, is_node_operator_record_key,
Expand All @@ -93,8 +92,8 @@ use ic_registry_keys::{
make_node_operator_record_key, make_node_record_key, make_provisional_whitelist_record_key,
make_replica_version_key, make_routing_table_record_key, make_subnet_list_record_key,
make_subnet_record_key, make_unassigned_nodes_config_record_key, FirewallRulesScope,
API_BOUNDARY_NODE_RECORD_KEY_PREFIX, HOSTOS_VERSION_KEY_PREFIX,
NODE_OPERATOR_RECORD_KEY_PREFIX, NODE_REWARDS_TABLE_KEY, ROOT_SUBNET_ID_KEY,
API_BOUNDARY_NODE_RECORD_KEY_PREFIX, NODE_OPERATOR_RECORD_KEY_PREFIX, NODE_REWARDS_TABLE_KEY,
ROOT_SUBNET_ID_KEY,
};
use ic_registry_local_store::{
Changelog, ChangelogEntry, KeyMutation, LocalStoreImpl, LocalStoreWriter,
Expand Down Expand Up @@ -4127,17 +4126,40 @@ struct ProposeToUpdateNodesHostosVersionCmd {
#[clap(name = "NODE_ID", multiple_values(true), required = true)]
pub node_ids: Vec<PrincipalId>,

#[clap(flatten)]
pub hostos_version_flag: HostosVersionFlag,
}

#[derive(Args)]
struct HostosVersionFlag {
/// Version ID. This should correspond to a HostOS version previously added
/// to the registry.
#[clap(long)]
pub hostos_version_id: Option<String>,
/// When this flag is passed, remove the HostOS version from this set of
/// Nodes. This will take the same action as excluding the version id flag.
#[clap(long)]
pub clear_hostos_version: bool,
}

impl HostosVersionFlag {
// TODO: If we upgrade clap, this can be replaced with `group` attributes.
fn simplify(&self) -> &Option<String> {
if self.hostos_version_id.is_some() && self.clear_hostos_version {
panic!("Only one of --hostos-version-id or --clear-hostos-version can be specified at once.");
}

// When `--clear-hostos-version` is set, `--hostos-version-id` must be
// unset and `None`, so we can always return it directly.
&self.hostos_version_id
}
}

impl ProposalTitle for ProposeToUpdateNodesHostosVersionCmd {
fn title(&self) -> String {
match &self.proposal_title {
Some(title) => title.clone(),
None => match &self.hostos_version_id {
None => match &self.hostos_version_flag.simplify() {
Some(hostos_version_id) => format!(
"Set HostOS version: '{}' on nodes: '{}'",
hostos_version_id,
Expand All @@ -4164,7 +4186,7 @@ impl ProposalPayload<UpdateNodesHostosVersionPayload> for ProposeToUpdateNodesHo

UpdateNodesHostosVersionPayload {
node_ids,
hostos_version_id: self.hostos_version_id.clone(),
hostos_version_id: self.hostos_version_flag.simplify().clone(),
}
}
}
Expand Down Expand Up @@ -4657,13 +4679,19 @@ async fn main() {
// Download the IC-OS upgrade, do not check sha256 yet, we will do that
// explicitly later
let file_downloader = FileDownloader::new(None).follow_redirects();
if version.release_package_urls.iter().all(|url| {
tokio::runtime::Handle::current()
.block_on(file_downloader.download_file(url, &tmp_file, None))
.is_err()
}) {
panic!("Download of release package failed.");

let mut result = Err(anyhow!("Download of release package failed."));
for url in version.release_package_urls.iter() {
result = file_downloader
.download_file(url, &tmp_file, None)
.await
.map_err(|v| v.into());

if result.is_ok() {
break;
}
}
result.unwrap();

println!("OK Download success");

Expand All @@ -4677,40 +4705,6 @@ async fn main() {
}
};

// Check version number.
eprintln!("Extracting .. ");
match extract_tar_gz_into_dir(&tmp_file, &tmp_dir) {
Ok(()) => {
println!("OK extracting tar gz archive");
let mut version_file = tmp_dir.clone();
version_file.push("VERSION.TXT");

if !version_file.exists() {
// Older versions of the IC-OS had version.txt as a file name
version_file = tmp_dir.clone();
version_file.push("version.txt");
}

let archive_version = read_to_string(version_file)
.expect("Could not read version in extracted version file");
let archive_version = archive_version.trim();

if archive_version == get_replica_version_cmd.replica_version_id {
println!("OK correct version number in archived version file");
} else {
println!(
"FAIL incorrect version number in archived version file ({} vs {})",
archive_version, get_replica_version_cmd.replica_version_id
);
success = false;
}
}
Err(e) => {
println!("FAIL extracting tar gz archive: {:?}", e);
success = false;
}
}

if !success {
exit(1);
}
Expand Down Expand Up @@ -5640,21 +5634,14 @@ async fn main() {
.try_polling_latest_version(usize::MAX)
.unwrap();

let keys = registry_client
.get_key_family(
HOSTOS_VERSION_KEY_PREFIX,
registry_client.get_latest_version(),
)
let hostos_versions = registry_client
.get_hostos_versions(registry_client.get_latest_version())
.unwrap();

for key in keys {
let bytes = registry_client
.get_value(&key, registry_client.get_latest_version())
.unwrap()
.unwrap();
let hostos_version_record = HostosVersionRecord::decode(&bytes[..])
.expect("Error decoding HostosVersionRecord from registry");
println!("{}", hostos_version_record.hostos_version_id);
if let Some(hostos_versions) = hostos_versions {
for version in hostos_versions {
println!("{}", version.hostos_version_id);
}
}
}
SubCommand::ProposeToAddApiBoundaryNode(cmd) => {
Expand Down
24 changes: 23 additions & 1 deletion rs/registry/helpers/src/hostos_version.rs
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
use crate::deserialize_registry_value;
use ic_interfaces_registry::{RegistryClient, RegistryClientResult};
use ic_protobuf::registry::hostos_version::v1::HostosVersionRecord;
use ic_registry_keys::make_hostos_version_key;
use ic_registry_keys::{make_hostos_version_key, HOSTOS_VERSION_KEY_PREFIX};
pub use ic_types::hostos_version::HostosVersion;
pub use ic_types::{NodeId, RegistryVersion, SubnetId};

pub trait HostosRegistry {
fn get_hostos_versions(
&self,
version: RegistryVersion,
) -> RegistryClientResult<Vec<HostosVersionRecord>>;

fn get_hostos_version_record(
&self,
hostos_version_id: &HostosVersion,
Expand All @@ -14,6 +19,23 @@ pub trait HostosRegistry {
}

impl<T: RegistryClient + ?Sized> HostosRegistry for T {
fn get_hostos_versions(
&self,
version: RegistryVersion,
) -> RegistryClientResult<Vec<HostosVersionRecord>> {
let keys = self.get_key_family(HOSTOS_VERSION_KEY_PREFIX, version)?;

let mut records = Vec::new();
for key in keys {
let bytes = self.get_value(&key, version);
let hostos_version_proto =
deserialize_registry_value::<HostosVersionRecord>(bytes)?.unwrap_or_default();
records.push(hostos_version_proto)
}

Ok(Some(records))
}

fn get_hostos_version_record(
&self,
hostos_version_id: &HostosVersion,
Expand Down

0 comments on commit 886ae3b

Please sign in to comment.