Fix for rdc fieldgroups not adding after creation (#3318)

yalmusaf · web-flow · commit efcd3abc4464 · 2026-02-24T13:49:51.000-08:00
Signed-off-by: yalmusaf_amdeng &lt;Yazen.ALMusaffar@amd.com&gt;
diff --git a/projects/rdc/include/rdc/rdc.h b/projects/rdc/include/rdc/rdc.h
@@ -1244,6 +1244,23 @@ rdc_status_t rdc_group_field_create(rdc_handle_t p_rdc_handle, uint32_t num_fiel
                                     rdc_field_t* field_ids, const char* field_group_name,
                                     rdc_field_grp_t* rdc_field_group_id);
 
+/**
+ *  @brief Add a field to an existing field group
+ *
+ *  @details Add a single field ID to an existing field group created by
+ *  rdc_group_field_create
+ *
+ *  @param[in] p_rdc_handle The RDC handler.
+ *
+ *  @param[in] rdc_field_group_id The field group ID to add the field to
+ *
+ *  @param[in] field_id The field ID to be added to the field group
+ *
+ *  @retval ::RDC_ST_OK is returned upon successful call.
+ */
+rdc_status_t rdc_group_field_add_field(rdc_handle_t p_rdc_handle, rdc_field_grp_t rdc_field_group_id,
+                                       rdc_field_t field_id);
+                                       
 /**
  *  @brief Get information about a field group
  *
diff --git a/projects/rdc/include/rdc_lib/RdcGroupSettings.h b/projects/rdc/include/rdc_lib/RdcGroupSettings.h
@@ -43,6 +43,8 @@ class RdcGroupSettings {
   virtual rdc_status_t rdc_group_field_create(uint32_t num_field_ids, rdc_field_t* field_ids,
                                               const char* field_group_name,
                                               rdc_field_grp_t* rdc_field_group_id) = 0;
+  virtual rdc_status_t rdc_group_field_add_field(rdc_field_grp_t rdc_field_group_id,
+                                                 rdc_field_t field_id) = 0;
   virtual rdc_status_t rdc_group_field_destroy(rdc_field_grp_t rdc_field_group_id) = 0;
   virtual rdc_status_t rdc_group_field_get_info(rdc_field_grp_t rdc_field_group_id,
                                                 rdc_field_group_info_t* field_group_info) = 0;
diff --git a/projects/rdc/include/rdc_lib/RdcHandler.h b/projects/rdc/include/rdc_lib/RdcHandler.h
@@ -55,6 +55,8 @@ class RdcHandler {
   virtual rdc_status_t rdc_group_field_create(uint32_t num_field_ids, rdc_field_t* field_ids,
                                               const char* field_group_name,
                                               rdc_field_grp_t* rdc_field_group_id) = 0;
+  virtual rdc_status_t rdc_group_field_add_field(rdc_field_grp_t rdc_field_group_id,
+                                                 rdc_field_t field_id) = 0;              
   virtual rdc_status_t rdc_group_field_get_info(rdc_field_grp_t rdc_field_group_id,
                                                 rdc_field_group_info_t* field_group_info) = 0;
   virtual rdc_status_t rdc_group_gpu_get_info(rdc_gpu_group_t p_rdc_group_id,
diff --git a/projects/rdc/include/rdc_lib/impl/RdcEmbeddedHandler.h b/projects/rdc/include/rdc_lib/impl/RdcEmbeddedHandler.h
@@ -65,6 +65,8 @@ class RdcEmbeddedHandler final : public RdcHandler {
   rdc_status_t rdc_group_field_create(uint32_t num_field_ids, rdc_field_t* field_ids,
                                       const char* field_group_name,
                                       rdc_field_grp_t* rdc_field_group_id) override;
+  rdc_status_t rdc_group_field_add_field(rdc_field_grp_t rdc_field_group_id,
+                                         rdc_field_t field_id) override;
   rdc_status_t rdc_group_field_get_info(rdc_field_grp_t rdc_field_group_id,
                                         rdc_field_group_info_t* field_group_info) override;
   rdc_status_t rdc_group_gpu_get_info(rdc_gpu_group_t p_rdc_group_id,
diff --git a/projects/rdc/include/rdc_lib/impl/RdcGroupSettingsImpl.h b/projects/rdc/include/rdc_lib/impl/RdcGroupSettingsImpl.h
@@ -46,6 +46,8 @@ class RdcGroupSettingsImpl : public RdcGroupSettings {
   rdc_status_t rdc_group_field_create(uint32_t num_field_ids, rdc_field_t* field_ids,
                                       const char* field_group_name,
                                       rdc_field_grp_t* rdc_field_group_id) override;
+  rdc_status_t rdc_group_field_add_field(rdc_field_grp_t rdc_field_group_id,
+                                         rdc_field_t field_id) override;
   rdc_status_t rdc_group_field_destroy(rdc_field_grp_t rdc_field_group_id) override;
   rdc_status_t rdc_group_field_get_info(rdc_field_grp_t rdc_field_group_id,
                                         rdc_field_group_info_t* field_group_info) override;
diff --git a/projects/rdc/include/rdc_lib/impl/RdcStandaloneHandler.h b/projects/rdc/include/rdc_lib/impl/RdcStandaloneHandler.h
@@ -59,6 +59,8 @@ class RdcStandaloneHandler : public RdcHandler {
   rdc_status_t rdc_group_field_create(uint32_t num_field_ids, rdc_field_t* field_ids,
                                       const char* field_group_name,
                                       rdc_field_grp_t* rdc_field_group_id) override;
+  rdc_status_t rdc_group_field_add_field(rdc_field_grp_t rdc_field_group_id,
+                                         rdc_field_t field_id) override;
   rdc_status_t rdc_group_field_get_info(rdc_field_grp_t rdc_field_group_id,
                                         rdc_field_group_info_t* field_group_info) override;
   rdc_status_t rdc_group_gpu_get_info(rdc_gpu_group_t p_rdc_group_id,
diff --git a/projects/rdc/protos/rdc.proto b/projects/rdc/protos/rdc.proto
@@ -73,6 +73,11 @@ service RdcAPI {
   //     rdc_field_grp_t* rdc_field_group_id)
   rpc CreateFieldGroup(CreateFieldGroupRequest) returns (CreateFieldGroupResponse) {}
 
+  // rdc_status_t rdc_group_field_add_field(
+  //     rdc_field_grp_t rdc_field_group_id, rdc_field_t field_id)
+  rpc AddFieldToFieldGroup(AddFieldToFieldGroupRequest)
+      returns (AddFieldToFieldGroupResponse) {}
+
   // rdc_status_t rdc_group_field_get_info(
   //     rdc_field_grp_t rdc_field_group_id,
   //     rdc_field_group_info_t* field_group_info)
@@ -291,6 +296,15 @@ message CreateFieldGroupResponse {
   uint32 field_group_id = 2;
 }
 
+message AddFieldToFieldGroupRequest {
+  uint32 field_group_id = 1;
+  uint32 field_id = 2;
+}
+
+message AddFieldToFieldGroupResponse {
+  uint32 status = 1;
+}
+
 message GetFieldGroupInfoRequest {
   uint32 field_group_id = 1;
 }
diff --git a/projects/rdc/rdc_libs/bootstrap/src/RdcBootStrap.cc b/projects/rdc/rdc_libs/bootstrap/src/RdcBootStrap.cc
@@ -206,6 +206,16 @@ rdc_status_t rdc_group_field_create(rdc_handle_t p_rdc_handle, uint32_t num_fiel
       ->rdc_group_field_create(num_field_ids, field_ids, field_group_name, rdc_field_group_id);
 }
 
+rdc_status_t rdc_group_field_add_field(rdc_handle_t p_rdc_handle, rdc_field_grp_t rdc_field_group_id,
+                                       rdc_field_t field_id) {
+  if (!p_rdc_handle) {
+    return RDC_ST_INVALID_HANDLER;
+  }
+
+  return static_cast<amd::rdc::RdcHandler*>(p_rdc_handle)
+      ->rdc_group_field_add_field(rdc_field_group_id, field_id);
+}
+
 rdc_status_t rdc_group_field_get_info(rdc_handle_t p_rdc_handle, rdc_field_grp_t rdc_field_group_id,
                                       rdc_field_group_info_t* field_group_info) {
   if (!p_rdc_handle) {
diff --git a/projects/rdc/rdc_libs/rdc/src/RdcEmbeddedHandler.cc b/projects/rdc/rdc_libs/rdc/src/RdcEmbeddedHandler.cc
@@ -323,6 +323,20 @@ rdc_status_t RdcEmbeddedHandler::rdc_group_field_create(uint32_t num_field_ids,
                                                  rdc_field_group_id);
 }
 
+rdc_status_t RdcEmbeddedHandler::rdc_group_field_add_field(rdc_field_grp_t rdc_field_group_id,
+                                                           rdc_field_t field_id) {
+  if (!group_settings_) {
+    return RDC_ST_NOT_SUPPORTED;
+  }
+
+  if (!is_field_valid(field_id)) {
+    RDC_LOG(RDC_INFO, "Fail to add field with unknown field id " << field_id);
+    return RDC_ST_NOT_SUPPORTED;
+  }
+
+  return group_settings_->rdc_group_field_add_field(rdc_field_group_id, field_id);
+}
+
 rdc_status_t RdcEmbeddedHandler::rdc_group_field_get_info(
     rdc_field_grp_t rdc_field_group_id, rdc_field_group_info_t* field_group_info) {
   if (!field_group_info) {
diff --git a/projects/rdc/rdc_libs/rdc/src/RdcGroupSettingsImpl.cc b/projects/rdc/rdc_libs/rdc/src/RdcGroupSettingsImpl.cc
@@ -188,6 +188,34 @@ rdc_status_t RdcGroupSettingsImpl::rdc_group_field_create(uint32_t num_field_ids
   return RDC_ST_OK;
 }
 
+rdc_status_t RdcGroupSettingsImpl::rdc_group_field_add_field(rdc_field_grp_t rdc_field_group_id,
+                                                             rdc_field_t field_id) {
+  std::lock_guard<std::mutex> guard(field_group_mutex_);
+
+  auto ite = field_group_.find(rdc_field_group_id);
+  if (ite == field_group_.end()) {
+    return RDC_ST_FLDGROUP_NOT_FOUND;
+  }
+
+  // Check if field already exists in the group
+  for (uint32_t i = 0; i < ite->second.count; i++) {
+    if (ite->second.field_ids[i] == field_id) {
+      return RDC_ST_BAD_PARAMETER;
+    }
+  }
+
+  // Check if we have room for another field
+  if (ite->second.count >= RDC_MAX_FIELD_IDS_PER_FIELD_GROUP) {
+    return RDC_ST_MAX_LIMIT;
+  }
+
+  // Add the field to the group
+  ite->second.field_ids[ite->second.count] = field_id;
+  ite->second.count++;
+
+  return RDC_ST_OK;
+}
+
 rdc_status_t RdcGroupSettingsImpl::rdc_group_field_destroy(rdc_field_grp_t rdc_field_group_id) {
   if (rdc_field_group_id == JOB_FIELD_ID) {
     RDC_LOG(RDC_INFO, "Cannot delete system JOB_FIELD_ID field group");
diff --git a/projects/rdc/rdc_libs/rdc_client/src/RdcStandaloneHandler.cc b/projects/rdc/rdc_libs/rdc_client/src/RdcStandaloneHandler.cc
@@ -346,6 +346,20 @@ rdc_status_t RdcStandaloneHandler::rdc_group_field_create(uint32_t num_field_ids
   return RDC_ST_OK;
 }
 
+rdc_status_t RdcStandaloneHandler::rdc_group_field_add_field(rdc_field_grp_t rdc_field_group_id,
+                                                             rdc_field_t field_id) {
+  ::rdc::AddFieldToFieldGroupRequest request;
+  ::rdc::AddFieldToFieldGroupResponse reply;
+  ::grpc::ClientContext context;
+
+  request.set_field_group_id(rdc_field_group_id);
+  request.set_field_id(field_id);
+  ::grpc::Status status = stub_->AddFieldToFieldGroup(&context, request, &reply);
+  rdc_status_t err_status = error_handle(status, reply.status());
+
+  return err_status;
+}
+
 rdc_status_t RdcStandaloneHandler::rdc_group_field_get_info(
     rdc_field_grp_t rdc_field_group_id, rdc_field_group_info_t* field_group_info) {
   if (!field_group_info) {
diff --git a/projects/rdc/rdci/include/RdciFieldGroupSubSystem.h b/projects/rdc/rdci/include/RdciFieldGroupSubSystem.h
@@ -42,6 +42,7 @@ class RdciFieldGroupSubSystem : public RdciSubSystem {
     FIELD_GROUP_UNKNOWN = 0,
     FIELD_GROUP_HELP,
     FIELD_GROUP_CREATE,
+    FIELD_GROUP_ADD,
     FIELD_GROUP_DELETE,
     FIELD_GROUP_LIST,
     FIELD_GROUP_INFO
diff --git a/projects/rdc/rdci/src/RdciFieldGroupSubSystem.cc b/projects/rdc/rdci/src/RdciFieldGroupSubSystem.cc
@@ -72,6 +72,10 @@ void RdciFieldGroupSubSystem::parse_cmd_opts(int argc, char** argv) {
         field_group_ops_ = FIELD_GROUP_LIST;
         break;
       case 'f':
+        if (optarg == nullptr || std::string(optarg).empty()) {
+          show_help();
+          throw RdcException(RDC_ST_BAD_PARAMETER, "Field id required with -f/--fieldids");
+        }
         field_ids_ = optarg;
         break;
       case 'g':
@@ -98,15 +102,29 @@ void RdciFieldGroupSubSystem::parse_cmd_opts(int argc, char** argv) {
         group_id_ = std::stoi(optarg);
         is_group_set_ = true;
         break;
+      case '?':
+        if ((optopt == 'f') ||
+            (optind > 0 && argv[optind - 1] != nullptr &&
+             std::string(argv[optind - 1]) == "--fieldids")) {
+          show_help();
+          throw RdcException(RDC_ST_BAD_PARAMETER, "Field id required with -f/--fieldids");
+        }
+        show_help();
+        throw RdcException(RDC_ST_BAD_PARAMETER, "Unknown command line options");
       default:
         show_help();
         throw RdcException(RDC_ST_BAD_PARAMETER, "Unknown command line options");
     }
   }
 
+  // Determine operation based on combination of options
   if (field_group_ops_ == FIELD_GROUP_UNKNOWN) {
-    show_help();
-    throw RdcException(RDC_ST_BAD_PARAMETER, "Must specify a valid operations");
+    if (is_group_set_ && !field_ids_.empty()) {
+      field_group_ops_ = FIELD_GROUP_ADD;
+    } else {
+      show_help();
+      throw RdcException(RDC_ST_BAD_PARAMETER, "Must specify a valid operations");
+    }
   }
 }
 
@@ -119,6 +137,8 @@ void RdciFieldGroupSubSystem::show_help() const {
             << " [--json] [-u] -l\n";
   std::cout << "    rdci fieldgroup [--host <IP/FQDN>:port] [--json]"
             << " [-u] -c <groupName> -f <filedIds>\n";
+  std::cout << "    rdci fieldgroup [--host <IP/FQDN>:port] [--json] [-u] "
+            << "-g <groupId> -f <fieldIds>\n";
   std::cout << "    rdci fieldgroup [--host <IP/FQDN>:port] [--json] [-u] "
             << "-g <groupId> -i\n";
   std::cout << "    rdci fieldgroup [--host <IP/FQDN>:port] [--json] [-u] "
@@ -134,7 +154,7 @@ void RdciFieldGroupSubSystem::show_help() const {
   std::cout << "  -c  --create groupName         "
             << "Create a field group on the remote host.\n";
   std::cout << "  -f  --fieldids fieldIds        Comma-separated "
-            << "list of the field ids to add to a field group\n";
+            << "list of the field ids (use with -c to create or -g to add to existing group)\n";
   std::cout << "  -i  --info                     "
             << "Display the information for the specified group Id\n";
   std::cout << "  -d  --delete groupId           "
@@ -150,6 +170,49 @@ void RdciFieldGroupSubSystem::process() {
     case FIELD_GROUP_HELP:
       show_help();
       break;
+    case FIELD_GROUP_ADD: {
+      if (!is_group_set_) {
+        show_help();
+        throw RdcException(RDC_ST_BAD_PARAMETER, "Must specify the group id to add fields");
+      }
+      if (field_ids_.empty()) {
+        show_help();
+        throw RdcException(RDC_ST_BAD_PARAMETER, "Field id required with -f/--fieldids");
+      }
+      
+      std::vector<std::string> fields = split_string(field_ids_, ',');
+      for (uint32_t i = 0; i < fields.size(); i++) {
+        rdc_field_t field_id;
+        if (!IsNumber(fields[i])) {
+          if (!get_field_id_from_name(fields[i], &field_id)) {
+            throw RdcException(RDC_ST_BAD_PARAMETER,
+                               "The field name " + fields[i] + " is not valid");
+          }
+        } else {
+          field_id = static_cast<rdc_field_t>(std::stoi(fields[i]));
+        }
+
+        // Validate field compatibility with available devices
+        if (!is_field_valid(field_id)) {
+          throw RdcException(RDC_ST_BAD_PARAMETER,
+                             "Field " + std::to_string(field_id) + " is not supported");
+        }
+
+        result = rdc_group_field_add_field(rdc_handle_, group_id_, field_id);
+        if (result != RDC_ST_OK) {
+          throw RdcException(result, "Failed to add field " + std::to_string(field_id) + 
+                             " to group " + std::to_string(group_id_));
+        }
+      }
+      
+      if (is_json_output()) {
+        std::cout << "\"field_group_id\": \"" << group_id_ << "\", \"status\": \"ok\"";
+      } else {
+        std::cout << "Successfully added " << fields.size() << " field(s) to group " 
+                  << group_id_ << std::endl;
+      }
+      return;
+    }
     case FIELD_GROUP_CREATE: {
       if (group_name_ == "") {
         show_help();
diff --git a/projects/rdc/server/include/rdc/rdc_api_service.h b/projects/rdc/server/include/rdc/rdc_api_service.h
@@ -74,6 +74,10 @@ class RdcAPIServiceImpl final : public ::rdc::RdcAPI::Service {
                                   const ::rdc::CreateFieldGroupRequest* request,
                                   ::rdc::CreateFieldGroupResponse* reply) override;
 
+  ::grpc::Status AddFieldToFieldGroup(::grpc::ServerContext* context,
+                                      const ::rdc::AddFieldToFieldGroupRequest* request,
+                                      ::rdc::AddFieldToFieldGroupResponse* reply) override;
+
   ::grpc::Status GetFieldGroupInfo(::grpc::ServerContext* context,
                                    const ::rdc::GetFieldGroupInfoRequest* request,
                                    ::rdc::GetFieldGroupInfoResponse* reply) override;
diff --git a/projects/rdc/server/src/rdc_api_service.cc b/projects/rdc/server/src/rdc_api_service.cc
@@ -278,6 +278,22 @@ ::grpc::Status RdcAPIServiceImpl::CreateFieldGroup(::grpc::ServerContext* contex
   return ::grpc::Status::OK;
 }
 
+::grpc::Status RdcAPIServiceImpl::AddFieldToFieldGroup(
+    ::grpc::ServerContext* context, const ::rdc::AddFieldToFieldGroupRequest* request,
+    ::rdc::AddFieldToFieldGroupResponse* reply) {
+  (void)(context);
+  if (!reply || !request) {
+    return ::grpc::Status(::grpc::StatusCode::INTERNAL, "Empty contents");
+  }
+
+  rdc_status_t result =
+      rdc_group_field_add_field(rdc_handle_, request->field_group_id(),
+                                static_cast<rdc_field_t>(request->field_id()));
+  reply->set_status(result);
+
+  return ::grpc::Status::OK;
+}
+
 ::grpc::Status RdcAPIServiceImpl::GetFieldGroupInfo(::grpc::ServerContext* context,
                                                     const ::rdc::GetFieldGroupInfoRequest* request,
                                                     ::rdc::GetFieldGroupInfoResponse* reply) {
