feature: Promote gRPC Commit to v1 stable

crates/trident/src/grpc_client/tridentclient.rs

@@ -8,20 +8,21 @@ use tonic::{
 use url::Url;
 
 use trident_proto::v1::{
-    servicing_response::Response as ResponseBody, streaming_service_client::StreamingServiceClient,
-    update_service_client::UpdateServiceClient, version_service_client::VersionServiceClient,
-    FinalizeUpdateRequest, HostConfiguration, RebootHandling as ProtoRebootHandling,
-    RebootManagement, RebootStatus, ServicingResponse, StageUpdateRequest, StatusCode,
-    StreamDiskRequest, UpdateRequest, VersionRequest,
+    commit_service_client::CommitServiceClient, servicing_response::Response as ResponseBody,
+    streaming_service_client::StreamingServiceClient, update_service_client::UpdateServiceClient,
+    version_service_client::VersionServiceClient, CommitRequest, FinalizeUpdateRequest,
+    HostConfiguration, RebootHandling as ProtoRebootHandling, RebootManagement, RebootStatus,
+    ServicingResponse, StageUpdateRequest, StatusCode, StreamDiskRequest, UpdateRequest,
+    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,
-    validation_service_client::ValidationServiceClient, CommitRequest, FinalizeInstallRequest,
-    InstallRequest, StageInstallRequest,
+    validation_service_client::ValidationServiceClient, FinalizeInstallRequest, InstallRequest,
+    StageInstallRequest,
 };
 
 use crate::ExitKind;
@@ -58,11 +59,11 @@ pub struct TridentClient {
     version_client: VersionServiceClient<Channel>,
     streaming_client: StreamingServiceClient<Channel>,
     update_client: UpdateServiceClient<Channel>,
+    #[allow(dead_code)]
+    commit_client: CommitServiceClient<Channel>,
 
     #[cfg(feature = "grpc-preview")]
     install_client: InstallServiceClient<Channel>,
-    #[cfg(feature = "grpc-preview")]
-    commit_client: CommitServiceClient<Channel>,
     #[expect(dead_code)]
     #[cfg(feature = "grpc-preview")]
     rollback_client: RollbackServiceClient<Channel>,
@@ -97,8 +98,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")]
             rollback_client: RollbackServiceClient::new(channel.clone()),
             #[cfg(feature = "grpc-preview")]
             rebuild_raid_client: RebuildRaidServiceClient::new(channel.clone()),
@@ -108,6 +107,7 @@ impl TridentClient {
             validation_client: ValidationServiceClient::new(channel.clone()),
 
             // Prod clients
+            commit_client: CommitServiceClient::new(channel.clone()),
             update_client: UpdateServiceClient::new(channel.clone()),
             version_client: VersionServiceClient::new(channel.clone()),
             streaming_client: StreamingServiceClient::new(channel),
@@ -251,11 +251,15 @@ impl TridentClient {
     }
 
     /// Perform a commit on the Trident server.
-    #[cfg(feature = "grpc-preview")]
+    #[allow(dead_code)]
     pub async fn commit(&mut self) -> Result<ExitKind, TridentClientError> {
         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();

crates/trident/src/server/mod.rs

@@ -18,13 +18,14 @@ use tonic::transport::Server;
 use tonic_middleware::MiddlewareFor;
 
 use trident_proto::v1::{
-    streaming_service_server::StreamingServiceServer, update_service_server::UpdateServiceServer,
-    version_service_server::VersionServiceServer,
+    commit_service_server::CommitServiceServer, streaming_service_server::StreamingServiceServer,
+    update_service_server::UpdateServiceServer, 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,
     validation_service_server::ValidationServiceServer,
@@ -218,13 +219,17 @@ async fn server_main_inner(
         .add_service(MiddlewareFor::new(
             UpdateServiceServer::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(

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<Sha384Hash> {
     match datastore
         .host_status()
@@ -37,93 +21,126 @@ pub(super) fn stored_image_hash(datastore: &DataStore) -> Option<Sha384Hash> {
     }
 }
 
-#[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::<Vec<_>>();
+        /// 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::<Vec<_>>();
 
-            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
-        );
     }
 }

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;