From 52e5627b66f78947a957173d80d6eeb866779931 Mon Sep 17 00:00:00 2001 From: Paco Huelsz Prince Date: Fri, 27 Mar 2026 17:23:57 -0700 Subject: [PATCH 1/3] Promote Commit --- .../trident/src/grpc_client/tridentclient.rs | 23 +- crates/trident/src/server/mod.rs | 21 +- .../src/server/tridentserver/datastore.rs | 217 ++++++++++-------- .../trident/src/server/tridentserver/mod.rs | 4 +- .../server/tridentserver/services/commit.rs | 37 +-- .../src/server/tridentserver/services/mod.rs | 3 +- proto/trident/v1/commit_service.proto | 15 ++ proto/trident/v1preview/commit_service.proto | 5 - 8 files changed, 183 insertions(+), 142 deletions(-) create mode 100644 proto/trident/v1/commit_service.proto diff --git a/crates/trident/src/grpc_client/tridentclient.rs b/crates/trident/src/grpc_client/tridentclient.rs index f8764391d..8bfc3cd3b 100644 --- a/crates/trident/src/grpc_client/tridentclient.rs +++ b/crates/trident/src/grpc_client/tridentclient.rs @@ -8,18 +8,18 @@ use tonic::{ use url::Url; use trident_proto::v1::{ - servicing_response::Response as ResponseBody, streaming_service_client::StreamingServiceClient, - version_service_client::VersionServiceClient, RebootHandling as ProtoRebootHandling, - RebootManagement, RebootStatus, ServicingResponse, StatusCode, StreamDiskRequest, - VersionRequest, + commit_service_client::CommitServiceClient, servicing_response::Response as ResponseBody, + streaming_service_client::StreamingServiceClient, version_service_client::VersionServiceClient, + CommitRequest, RebootHandling as ProtoRebootHandling, RebootManagement, RebootStatus, + ServicingResponse, StatusCode, StreamDiskRequest, VersionRequest, }; #[cfg(feature = "grpc-preview")] use trident_proto::v1preview::{ - commit_service_client::CommitServiceClient, install_service_client::InstallServiceClient, + install_service_client::InstallServiceClient, rebuild_raid_service_client::RebuildRaidServiceClient, rollback_service_client::RollbackServiceClient, status_service_client::StatusServiceClient, update_service_client::UpdateServiceClient, validation_service_client::ValidationServiceClient, - CommitRequest, FinalizeInstallRequest, HostConfiguration, InstallRequest, StageInstallRequest, + FinalizeInstallRequest, HostConfiguration, InstallRequest, StageInstallRequest, }; use crate::ExitKind; @@ -55,10 +55,12 @@ impl From for i32 { pub struct TridentClient { version_client: VersionServiceClient, streaming_client: StreamingServiceClient, + + #[allow(dead_code)] + commit_client: CommitServiceClient, + #[cfg(feature = "grpc-preview")] install_client: InstallServiceClient, - #[cfg(feature = "grpc-preview")] - commit_client: CommitServiceClient, #[expect(dead_code)] #[cfg(feature = "grpc-preview")] @@ -97,8 +99,6 @@ impl TridentClient { #[cfg(feature = "grpc-preview")] install_client: InstallServiceClient::new(channel.clone()), #[cfg(feature = "grpc-preview")] - commit_client: CommitServiceClient::new(channel.clone()), - #[cfg(feature = "grpc-preview")] update_client: UpdateServiceClient::new(channel.clone()), #[cfg(feature = "grpc-preview")] rollback_client: RollbackServiceClient::new(channel.clone()), @@ -110,6 +110,7 @@ impl TridentClient { validation_client: ValidationServiceClient::new(channel.clone()), // Prod clients + commit_client: CommitServiceClient::new(channel.clone()), version_client: VersionServiceClient::new(channel.clone()), streaming_client: StreamingServiceClient::new(channel), }) @@ -184,7 +185,7 @@ impl TridentClient { } /// Perform a commit on the Trident server. - #[cfg(feature = "grpc-preview")] + #[allow(dead_code)] pub async fn commit(&mut self) -> Result { let response = self .commit_client diff --git a/crates/trident/src/server/mod.rs b/crates/trident/src/server/mod.rs index fb0d5b51e..161e0e981 100644 --- a/crates/trident/src/server/mod.rs +++ b/crates/trident/src/server/mod.rs @@ -18,12 +18,14 @@ use tonic::transport::Server; use tonic_middleware::MiddlewareFor; use trident_proto::v1::{ - streaming_service_server::StreamingServiceServer, version_service_server::VersionServiceServer, + commit_service_server::CommitServiceServer, streaming_service_server::StreamingServiceServer, + version_service_server::VersionServiceServer, }; #[cfg(feature = "grpc-preview")] use trident_proto::v1preview::{ - commit_service_server::CommitServiceServer, install_service_server::InstallServiceServer, + commit_service_server::CommitServiceServer as CommitServiceServerPreview, + install_service_server::InstallServiceServer, rebuild_raid_service_server::RebuildRaidServiceServer, rollback_service_server::RollbackServiceServer, status_service_server::StatusServiceServer, update_service_server::UpdateServiceServer, validation_service_server::ValidationServiceServer, @@ -209,16 +211,21 @@ async fn server_main_inner( activity_tracker.middleware(), )); - router = router.add_service(MiddlewareFor::new( - StreamingServiceServer::from_arc(trident_server.clone()), - activity_tracker.middleware(), - )); + router = router + .add_service(MiddlewareFor::new( + StreamingServiceServer::from_arc(trident_server.clone()), + activity_tracker.middleware(), + )) + .add_service(MiddlewareFor::new( + CommitServiceServer::from_arc(trident_server.clone()), + activity_tracker.middleware(), + )); #[cfg(feature = "grpc-preview")] { router = router .add_service(MiddlewareFor::new( - CommitServiceServer::from_arc(trident_server.clone()), + CommitServiceServerPreview::from_arc(trident_server.clone()), activity_tracker.middleware(), )) .add_service(MiddlewareFor::new( diff --git a/crates/trident/src/server/tridentserver/datastore.rs b/crates/trident/src/server/tridentserver/datastore.rs index 6cd8895ed..defeae477 100644 --- a/crates/trident/src/server/tridentserver/datastore.rs +++ b/crates/trident/src/server/tridentserver/datastore.rs @@ -1,29 +1,13 @@ -use trident_api::{config::ImageSha384, primitives::hash::Sha384Hash, status::ServicingState}; -use trident_proto::v1preview::ServicingState as ProtoServicingState; +use trident_api::{config::ImageSha384, primitives::hash::Sha384Hash}; use crate::DataStore; -/// Maps the servicing state from the internal datastore representation to the Proto API representation. -pub(super) fn servicing_state_from_datastore(datastore: &DataStore) -> ProtoServicingState { - match datastore.host_status().servicing_state { - ServicingState::NotProvisioned => ProtoServicingState::NotProvisioned, - ServicingState::CleanInstallStaged => ProtoServicingState::InstallStaged, - ServicingState::AbUpdateStaged => ProtoServicingState::UpdateAbStaged, - ServicingState::ManualRollbackAbStaged => ProtoServicingState::ManualRollbackAbStaged, - ServicingState::ManualRollbackRuntimeStaged => { - ProtoServicingState::ManualRollbackRuntimeStaged - } - ServicingState::RuntimeUpdateStaged => ProtoServicingState::UpdateRuntimeStaged, - ServicingState::CleanInstallFinalized => ProtoServicingState::InstallFinalized, - ServicingState::AbUpdateFinalized => ProtoServicingState::UpdateAbFinalized, - ServicingState::ManualRollbackAbFinalized => ProtoServicingState::ManualRollbackAbFinalized, - ServicingState::Provisioned => ProtoServicingState::Provisioned, - ServicingState::AbUpdateHealthCheckFailed => ProtoServicingState::UpdateAbHealthCheckFailed, - } -} +// Re-export preview functions for convenience and keeping the path stable once +// they graduate to stable. +#[cfg(feature = "grpc-preview")] +pub(super) use preview::servicing_state_from_datastore; /// Returns the stored image hash from the datastore, if it exists. -#[cfg(feature = "grpc-preview")] pub(super) fn stored_image_hash(datastore: &DataStore) -> Option { match datastore .host_status() @@ -37,93 +21,126 @@ pub(super) fn stored_image_hash(datastore: &DataStore) -> Option { } } -#[cfg(test)] -mod tests { +#[cfg(feature = "grpc-preview")] +mod preview { use super::*; - use strum::IntoEnumIterator; - use tempfile::TempDir; + use trident_api::status::ServicingState; + use trident_proto::v1preview::ServicingState as ProtoServicingState; - /// An overly-paranoid test to ensure that all variants of ServicingState - /// are covered in servicing_state_from_datastore function and that the - /// mapping is correct. It is essentially a re-implementation of the mapping - /// in the function, but in different form, meaning that any error would - /// need to be typed twice for this test to pass. - #[test] - fn test_servicing_state_from_datastore() { - let test_cases = vec![ - ( - ServicingState::NotProvisioned, - ProtoServicingState::NotProvisioned, - ), - ( - ServicingState::CleanInstallStaged, - ProtoServicingState::InstallStaged, - ), - ( - ServicingState::AbUpdateStaged, - ProtoServicingState::UpdateAbStaged, - ), - ( - ServicingState::ManualRollbackAbStaged, - ProtoServicingState::ManualRollbackAbStaged, - ), - ( - ServicingState::ManualRollbackRuntimeStaged, - ProtoServicingState::ManualRollbackRuntimeStaged, - ), - ( - ServicingState::RuntimeUpdateStaged, - ProtoServicingState::UpdateRuntimeStaged, - ), - ( - ServicingState::CleanInstallFinalized, - ProtoServicingState::InstallFinalized, - ), - ( - ServicingState::AbUpdateFinalized, - ProtoServicingState::UpdateAbFinalized, - ), - ( - ServicingState::ManualRollbackAbFinalized, - ProtoServicingState::ManualRollbackAbFinalized, - ), - ( - ServicingState::Provisioned, - ProtoServicingState::Provisioned, - ), - ( - ServicingState::AbUpdateHealthCheckFailed, - ProtoServicingState::UpdateAbHealthCheckFailed, - ), - ]; + /// Maps the servicing state from the internal datastore representation to the Proto API representation. + pub(in crate::server::tridentserver) fn servicing_state_from_datastore( + datastore: &DataStore, + ) -> ProtoServicingState { + match datastore.host_status().servicing_state { + ServicingState::NotProvisioned => ProtoServicingState::NotProvisioned, + ServicingState::CleanInstallStaged => ProtoServicingState::InstallStaged, + ServicingState::AbUpdateStaged => ProtoServicingState::UpdateAbStaged, + ServicingState::ManualRollbackAbStaged => ProtoServicingState::ManualRollbackAbStaged, + ServicingState::ManualRollbackRuntimeStaged => { + ProtoServicingState::ManualRollbackRuntimeStaged + } + ServicingState::RuntimeUpdateStaged => ProtoServicingState::UpdateRuntimeStaged, + ServicingState::CleanInstallFinalized => ProtoServicingState::InstallFinalized, + ServicingState::AbUpdateFinalized => ProtoServicingState::UpdateAbFinalized, + ServicingState::ManualRollbackAbFinalized => { + ProtoServicingState::ManualRollbackAbFinalized + } + ServicingState::Provisioned => ProtoServicingState::Provisioned, + ServicingState::AbUpdateHealthCheckFailed => { + ProtoServicingState::UpdateAbHealthCheckFailed + } + } + } + + #[cfg(test)] + mod tests { + use super::*; + + use strum::IntoEnumIterator; + use tempfile::TempDir; - // Track which ServicingState variants have been tested for coverage. - let mut coverage_assertions = ServicingState::iter().collect::>(); + /// An overly-paranoid test to ensure that all variants of ServicingState + /// are covered in servicing_state_from_datastore function and that the + /// mapping is correct. It is essentially a re-implementation of the mapping + /// in the function, but in different form, meaning that any error would + /// need to be typed twice for this test to pass. + #[test] + fn test_servicing_state_from_datastore() { + let test_cases = vec![ + ( + ServicingState::NotProvisioned, + ProtoServicingState::NotProvisioned, + ), + ( + ServicingState::CleanInstallStaged, + ProtoServicingState::InstallStaged, + ), + ( + ServicingState::AbUpdateStaged, + ProtoServicingState::UpdateAbStaged, + ), + ( + ServicingState::ManualRollbackAbStaged, + ProtoServicingState::ManualRollbackAbStaged, + ), + ( + ServicingState::ManualRollbackRuntimeStaged, + ProtoServicingState::ManualRollbackRuntimeStaged, + ), + ( + ServicingState::RuntimeUpdateStaged, + ProtoServicingState::UpdateRuntimeStaged, + ), + ( + ServicingState::CleanInstallFinalized, + ProtoServicingState::InstallFinalized, + ), + ( + ServicingState::AbUpdateFinalized, + ProtoServicingState::UpdateAbFinalized, + ), + ( + ServicingState::ManualRollbackAbFinalized, + ProtoServicingState::ManualRollbackAbFinalized, + ), + ( + ServicingState::Provisioned, + ProtoServicingState::Provisioned, + ), + ( + ServicingState::AbUpdateHealthCheckFailed, + ProtoServicingState::UpdateAbHealthCheckFailed, + ), + ]; - for (input, expected) in test_cases { - // Remove input from coverage assertions - coverage_assertions.retain(|item| *item != input); + // Track which ServicingState variants have been tested for coverage. + let mut coverage_assertions = ServicingState::iter().collect::>(); - let temp_dir = TempDir::new().unwrap(); - let mut datastore = - DataStore::open_or_create(&temp_dir.path().join("datastore.sqlite")).unwrap(); - datastore - .with_host_status(|hs| hs.servicing_state = input) - .unwrap(); + for (input, expected) in test_cases { + // Remove input from coverage assertions + coverage_assertions.retain(|item| *item != input); - let result = servicing_state_from_datastore(&datastore); - assert_eq!( - result, expected, - "Failed for input: {input:?}, result: {result:?}, expected: {expected:?}" + let temp_dir = TempDir::new().unwrap(); + let mut datastore = + DataStore::open_or_create(&temp_dir.path().join("datastore.sqlite")).unwrap(); + datastore + .with_host_status(|hs| hs.servicing_state = input) + .unwrap(); + + let result = servicing_state_from_datastore(&datastore); + assert_eq!( + result, expected, + "Failed for input: {input:?}, result: {result:?}, expected: {expected:?}" + ); + } + + // Ensure all ServicingState variants were tested, coverage_assertions should be empty. + assert!( + coverage_assertions.is_empty(), + "Not all ServicingState variants were tested: {:?}", + coverage_assertions ); } - - // Ensure all ServicingState variants were tested, coverage_assertions should be empty. - assert!( - coverage_assertions.is_empty(), - "Not all ServicingState variants were tested: {:?}", - coverage_assertions - ); } } diff --git a/crates/trident/src/server/tridentserver/mod.rs b/crates/trident/src/server/tridentserver/mod.rs index 2f7bcbd93..39869b0d1 100644 --- a/crates/trident/src/server/tridentserver/mod.rs +++ b/crates/trident/src/server/tridentserver/mod.rs @@ -37,12 +37,10 @@ use tonic::Code; #[cfg(feature = "grpc-preview")] use trident_api::error::ErrorKind; +mod datastore; mod services; mod servicingmgr; -#[cfg(feature = "grpc-preview")] -mod datastore; - use servicingmgr::RebootDecision; pub(super) use servicingmgr::ServicingManager; diff --git a/crates/trident/src/server/tridentserver/services/commit.rs b/crates/trident/src/server/tridentserver/services/commit.rs index 007ca3ee6..317e1bd77 100644 --- a/crates/trident/src/server/tridentserver/services/commit.rs +++ b/crates/trident/src/server/tridentserver/services/commit.rs @@ -1,8 +1,13 @@ use tonic::{async_trait, Request, Response, Status}; -use trident_api::error::{InternalError, TridentError, TridentResultExt}; +use trident_api::error::TridentResultExt; +use trident_proto::v1::{commit_service_server::CommitService, CommitRequest}; + +#[cfg(feature = "grpc-preview")] +use trident_api::error::{InternalError, TridentError}; +#[cfg(feature = "grpc-preview")] use trident_proto::v1preview::{ - commit_service_server::CommitService, CheckRootRequest, CommitRequest, + commit_service_server::CommitService as CommitServicePreview, CheckRootRequest, }; use crate::{ @@ -15,18 +20,6 @@ use crate::{ #[async_trait] impl CommitService for TridentServer { - type CheckRootStream = ServicingResponseStream; - async fn check_root( - &self, - _request: Request, - ) -> Result, Status> { - self.servicing_request("check_root", RebootDecision::Error, || { - Err(TridentError::new(InternalError::Internal( - "Not implemented: check_root", - ))) - }) - } - type CommitStream = ServicingResponseStream; async fn commit( &self, @@ -51,3 +44,19 @@ impl CommitService for TridentServer { }) } } + +#[cfg(feature = "grpc-preview")] +#[async_trait] +impl CommitServicePreview for TridentServer { + type CheckRootStream = ServicingResponseStream; + async fn check_root( + &self, + _request: Request, + ) -> Result, Status> { + self.servicing_request("check_root", RebootDecision::Error, || { + Err(TridentError::new(InternalError::Internal( + "Not implemented: check_root", + ))) + }) + } +} diff --git a/crates/trident/src/server/tridentserver/services/mod.rs b/crates/trident/src/server/tridentserver/services/mod.rs index ab3792bf8..b4cb584ab 100644 --- a/crates/trident/src/server/tridentserver/services/mod.rs +++ b/crates/trident/src/server/tridentserver/services/mod.rs @@ -2,11 +2,10 @@ use trident_proto::v1::{RebootHandling, RebootManagement}; use crate::server::tridentserver::RebootDecision; +mod commit; mod streaming; mod version; -#[cfg(feature = "grpc-preview")] -mod commit; #[cfg(feature = "grpc-preview")] mod install; #[cfg(feature = "grpc-preview")] diff --git a/proto/trident/v1/commit_service.proto b/proto/trident/v1/commit_service.proto new file mode 100644 index 000000000..2952147af --- /dev/null +++ b/proto/trident/v1/commit_service.proto @@ -0,0 +1,15 @@ +// Proto file defining the CommitService and related messages. + +syntax = "proto3"; + +package trident.v1; + +import "trident/v1/servicing.proto"; + +// CommitService provides methods for performing OS commit operations. +service CommitService { + // Commit attempts the commit operation to finalize a servicing. + rpc Commit(CommitRequest) returns (stream ServicingResponse); +} + +message CommitRequest {} diff --git a/proto/trident/v1preview/commit_service.proto b/proto/trident/v1preview/commit_service.proto index 6477d2b53..d18bdbaa6 100644 --- a/proto/trident/v1preview/commit_service.proto +++ b/proto/trident/v1preview/commit_service.proto @@ -11,11 +11,6 @@ service CommitService { // CheckRoot validates that the host booted using the expected root // filesystem. rpc CheckRoot(CheckRootRequest) returns (stream trident.v1.ServicingResponse); - - // Commit attempts the commit operation to finalize a servicing. - rpc Commit(CommitRequest) returns (stream trident.v1.ServicingResponse); } message CheckRootRequest {} - -message CommitRequest {} From 3a437e4d86ca8682cfad5e96b1d6475970a2d9b9 Mon Sep 17 00:00:00 2001 From: Paco Huelsz Prince Date: Thu, 2 Apr 2026 17:50:22 -0700 Subject: [PATCH 2/3] Add reboot management --- proto/trident/v1/commit_service.proto | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/proto/trident/v1/commit_service.proto b/proto/trident/v1/commit_service.proto index 2952147af..c83f78e82 100644 --- a/proto/trident/v1/commit_service.proto +++ b/proto/trident/v1/commit_service.proto @@ -12,4 +12,7 @@ service CommitService { rpc Commit(CommitRequest) returns (stream ServicingResponse); } -message CommitRequest {} +message CommitRequest { + // Reboot handling configuration. + RebootManagement reboot = 1; +} From 7f59ae1f0e0fa3bf3b7b92a1e81354d5e77d889f Mon Sep 17 00:00:00 2001 From: Paco Huelsz Prince Date: Thu, 2 Apr 2026 18:00:50 -0700 Subject: [PATCH 3/3] Merge fixes --- crates/trident/src/grpc_client/tridentclient.rs | 11 ++++++----- .../src/server/tridentserver/services/commit.rs | 13 +++++++++---- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/crates/trident/src/grpc_client/tridentclient.rs b/crates/trident/src/grpc_client/tridentclient.rs index b7f28fc9a..016b087d0 100644 --- a/crates/trident/src/grpc_client/tridentclient.rs +++ b/crates/trident/src/grpc_client/tridentclient.rs @@ -58,13 +58,10 @@ impl From for i32 { pub struct TridentClient { version_client: VersionServiceClient, streaming_client: StreamingServiceClient, - + update_client: UpdateServiceClient, #[allow(dead_code)] commit_client: CommitServiceClient, - #[expect(dead_code)] - update_client: UpdateServiceClient, - #[cfg(feature = "grpc-preview")] install_client: InstallServiceClient, #[expect(dead_code)] @@ -258,7 +255,11 @@ impl TridentClient { pub async fn commit(&mut self) -> Result { let response = self .commit_client - .commit(Request::new(CommitRequest {})) + .commit(Request::new(CommitRequest { + reboot: Some(RebootManagement { + handling: RebootHandling::Trident.into(), + }), + })) .await .map_err(|e| TridentClientError::RequestError("commit".to_string(), e))? .into_inner(); diff --git a/crates/trident/src/server/tridentserver/services/commit.rs b/crates/trident/src/server/tridentserver/services/commit.rs index 317e1bd77..eef60a3ab 100644 --- a/crates/trident/src/server/tridentserver/services/commit.rs +++ b/crates/trident/src/server/tridentserver/services/commit.rs @@ -12,25 +12,30 @@ use trident_proto::v1preview::{ use crate::{ server::{ - tridentserver::{datastore, RebootDecision, ServicingResponseStream}, + tridentserver::{datastore, ServicingResponseStream}, TridentServer, }, DataStore, Trident, }; +#[cfg(feature = "grpc-preview")] +use crate::server::tridentserver::RebootDecision; + #[async_trait] impl CommitService for TridentServer { type CommitStream = ServicingResponseStream; async fn commit( &self, - _request: Request, + request: Request, ) -> Result, Status> { + let req = request.into_inner(); + let data_store_path = self.agent_config.datastore_path().to_owned(); let logstream = self.logstream.clone(); let tracestream = self.tracestream.clone(); - self.servicing_request("commit", RebootDecision::Error, move || { - let mut trident = Trident::new(None, &data_store_path, logstream, tracestream) + self.servicing_request("commit", super::reboot_allowed(&req.reboot), move || { + let mut trident: Trident = Trident::new(None, &data_store_path, logstream, tracestream) .message("Failed to initialize Trident")?; let mut datastore =