Disambiguate probed configs sharing the same template name in tlBMC When multiple distinct entity config files share identical unexpanded template names (e.g. "Name": "board_i2c_$bus" or "Card_$index"), tlBMC stored probed configurations in `ProbedConfigMap` keyed by the unexpanded template name string. When subsequent configs with distinct probes (e.g., matching bus 0 vs bus 8) probed true, the latter config's JSON object overwrote earlier matching configs in the map. During `ProcessProbedConfigs`, only the last config's "Exposes" array (and ComputerSystem associations) was applied across all matched FRUs. Key Changes: 1. Updated `ProbedConfigMap` to `absl::flat_hash_map<std::string, std::vector<ProbedConfigData>>`, storing each probed config's matched FRUs alongside its original JSON configuration. 2. Updated `ProbeFru`, `AddReadyFruConfig`, and `ProcessConfigs` to append and iterate over probed config entries in the map. 3. Updated `ProcessProbedConfigs` to process each `ProbedConfigData` with its corresponding JSON configuration while maintaining continuous `$index` numbering across configs sharing the template name. 4. Added unit test cases in `entity_config_json_impl_test.cc` verifying `$bus` and `$index` substitution when multiple config files share template names. Google-Bug-Id:548667378,552599743 PiperOrigin-RevId: 972010029 Change-Id: I67cc5c64450595cdfd508fcc5dfa1e388d435911
diff --git a/tlbmc/configs/entity_config_json_impl.cc b/tlbmc/configs/entity_config_json_impl.cc index 77ecd4a..564f7e1 100644 --- a/tlbmc/configs/entity_config_json_impl.cc +++ b/tlbmc/configs/entity_config_json_impl.cc
@@ -996,7 +996,7 @@ LOG(INFO) << "IpmiFru field is not an array, converting to array"; ipmi_fru_object = nlohmann::json::array({ipmi_fru_object}); } - bool is_fru_probed = false; + std::vector<FruKey> matched_fru_keys; for (const auto& [key, fru] : fru_table.key_to_fru()) { // We should only be probing raw fru which have the pattern $bus:$address if (!RE2::FullMatch(key, R"(^\d+:0x[0-9a-f]+$)") && @@ -1059,7 +1059,7 @@ // 2. Multiple configs cannot probe true with the same name const bool* compound_fru_ptr = GetValueAsBool(config, "CompoundFru"); bool compound_fru = compound_fru_ptr != nullptr && *compound_fru_ptr; - if (probed_config_map.contains(name) && + if ((!matched_fru_keys.empty() || probed_config_map.contains(name)) && !absl::StrContains(name, "$index") && !absl::StrContainsIgnoreCase(name, "$bus") && !compound_fru) { return absl::InvalidArgumentError( @@ -1069,20 +1069,19 @@ name)); } // If we reach here, the FRU matches the probe and is unique. - is_fru_probed = true; - auto& probed_data = probed_config_map[name]; - // If the key is in the format of "bus:address", we will parse the bus // and address and use them to construct the FruKey object. Otherwise, // default to the string representation of the FruKey. - probed_data.fru_keys.push_back(FruKey(key)); - probed_data.config = config; + matched_fru_keys.push_back(FruKey(key)); LOG(INFO) << "Matched config: " << name << " with FRU: " << key; } - if (is_fru_probed) { - auto& probed_data = probed_config_map[name]; - std::sort(probed_data.fru_keys.begin(), probed_data.fru_keys.end()); + if (!matched_fru_keys.empty()) { + std::sort(matched_fru_keys.begin(), matched_fru_keys.end()); + ProbedConfigData probed_data; + probed_data.fru_keys = std::move(matched_fru_keys); + probed_data.config = config; + probed_config_map[std::string(name)].push_back(std::move(probed_data)); return true; } @@ -1101,9 +1100,10 @@ " probe have the same name: ", name)); } - auto& probed_data = probed_config_map[name]; + ProbedConfigData probed_data; probed_data.fru_keys.push_back(FruKey(std::string(name))); probed_data.config = config; + probed_config_map[std::string(name)].push_back(std::move(probed_data)); Fru entity_object; entity_object.mutable_attributes()->set_key(name); entity_object.mutable_attributes()->set_status(status); @@ -1278,7 +1278,7 @@ // have different asset fields. if (probe.contains("AttachToExistingFru")) { std::vector<FruKey> new_fru_keys; - auto& probed_data = probed_config_map[name]; + auto& probed_data = probed_config_map[name].back(); for (size_t i = 0; i < probed_data.fru_keys.size(); i++) { std::string config_name_with_index = name; absl::StrReplaceAll({{"$index", std::to_string(i + 1)}}, @@ -1292,7 +1292,7 @@ new_fru_keys.push_back(FruKey(config_name_with_index)); } - probed_config_map[name].fru_keys = std::move(new_fru_keys); + probed_data.fru_keys = std::move(new_fru_keys); } } } @@ -1313,36 +1313,38 @@ // rule is fulfilled. This is to avoid the recursion approach in // https://github.com/openbmc/entity-manager/blob/7f51d32fb69f3bb6c4eabf685a27b57f345445cb/src/entity_manager/perform_scan.cpp#L582 std::queue<std::string> found_target_candidates; - for (const auto& [name, config_data] : probed_config_map) { - if (pending_config_name_to_probed_configs.contains(name)) { - found_target_candidates.push(name); - continue; - } + for (const auto& [name, config_data_list] : probed_config_map) { + for (const auto& config_data : config_data_list) { + if (pending_config_name_to_probed_configs.contains(name)) { + found_target_candidates.push(name); + continue; + } - if (absl::StrContains(name, "$index")) { - for (size_t i = 0; i < config_data.fru_keys.size(); ++i) { - std::string config_name_with_index = name; - absl::StrReplaceAll({{"$index", std::to_string(i + 1)}}, - &config_name_with_index); - if (pending_config_name_to_probed_configs.contains( - config_name_with_index)) { - found_target_candidates.push(config_name_with_index); + if (absl::StrContains(name, "$index")) { + for (size_t i = 0; i < config_data.fru_keys.size(); ++i) { + std::string config_name_with_index = name; + absl::StrReplaceAll({{"$index", std::to_string(i + 1)}}, + &config_name_with_index); + if (pending_config_name_to_probed_configs.contains( + config_name_with_index)) { + found_target_candidates.push(config_name_with_index); + } } } - } - if (absl::StrContainsIgnoreCase(name, "$bus")) { - for (const auto& fru_key : config_data.fru_keys) { - std::string config_name_with_bus = name; - absl::StrReplaceAll({{"_", " "}}, &config_name_with_bus); - absl::StatusOr<std::string> bus_value_substitute_result = - SubstituteExpressionVariables(config_name_with_bus, "$bus", - fru_key.bus); - if (bus_value_substitute_result.ok()) { - config_name_with_bus = *bus_value_substitute_result; - absl::StrReplaceAll({{" ", "_"}}, &config_name_with_bus); - if (pending_config_name_to_probed_configs.contains( - config_name_with_bus)) { - found_target_candidates.push(config_name_with_bus); + if (absl::StrContainsIgnoreCase(name, "$bus")) { + for (const auto& fru_key : config_data.fru_keys) { + std::string config_name_with_bus = name; + absl::StrReplaceAll({{"_", " "}}, &config_name_with_bus); + absl::StatusOr<std::string> bus_value_substitute_result = + SubstituteExpressionVariables(config_name_with_bus, "$bus", + fru_key.bus); + if (bus_value_substitute_result.ok()) { + config_name_with_bus = *bus_value_substitute_result; + absl::StrReplaceAll({{" ", "_"}}, &config_name_with_bus); + if (pending_config_name_to_probed_configs.contains( + config_name_with_bus)) { + found_target_candidates.push(config_name_with_bus); + } } } } @@ -1364,7 +1366,7 @@ config_name)); } found_target_candidates.push(config_name); - probed_config_map[config_name] = found_config_data; + probed_config_map[config_name].push_back(found_config_data); Fru entity_object; entity_object.mutable_attributes()->set_key(config_name); entity_object.mutable_attributes()->set_status(STATUS_READY); @@ -1373,8 +1375,10 @@ // separate Fru objects are modeled from same physical board. // TODO b/483380675 - If multiple FOUND is supported, we need to handle if (auto it = probed_config_map.find(name); - it != probed_config_map.end() && !it->second.fru_keys.empty()) { - const std::string source_fru_key = it->second.fru_keys[0].ToString(); + it != probed_config_map.end() && !it->second.empty() && + !it->second[0].fru_keys.empty()) { + const std::string source_fru_key = + it->second[0].fru_keys[0].ToString(); if (auto fru_it = fru_table.key_to_fru().find(source_fru_key); fru_it != fru_table.key_to_fru().end()) { *entity_object.mutable_data() = fru_it->second.data(); @@ -2568,108 +2572,115 @@ const ProbedConfigMap& probed_config_map, ReloadType reload_type, absl::flat_hash_map<std::string, absl::flat_hash_set<std::string>>& fru_key_to_sub_fru_config) { - for (const auto& [name, config_data] : probed_config_map) { - // Check if config belongs to a sub FRU. - // RESOURCE_TYPE_ASSEMBLY indicates a config that is a sub FRU. - ECCLESIA_ASSIGN_OR_RETURN(ResourceType resource_type, - ParseResourceType(config_data.config)); - ECCLESIA_ASSIGN_OR_RETURN(const nlohmann::json::array_t elements, - GetExposesElements(config_data.config, name)); + for (const auto& [name, config_data_list] : probed_config_map) { + size_t base_fru_group_index = 0; + for (const auto& config_data : config_data_list) { + // Check if config belongs to a sub FRU. + // RESOURCE_TYPE_ASSEMBLY indicates a config that is a sub FRU. + ECCLESIA_ASSIGN_OR_RETURN(ResourceType resource_type, + ParseResourceType(config_data.config)); + ECCLESIA_ASSIGN_OR_RETURN(const nlohmann::json::array_t elements, + GetExposesElements(config_data.config, name)); - ECCLESIA_RETURN_IF_ERROR(ValidateExposesPorts(elements)); + ECCLESIA_RETURN_IF_ERROR(ValidateExposesPorts(elements)); - const bool* compound_frus_ptr = - GetValueAsBool(config_data.config, "CompoundFru"); - bool is_compound_fru = compound_frus_ptr != nullptr && *compound_frus_ptr; + const bool* compound_frus_ptr = + GetValueAsBool(config_data.config, "CompoundFru"); + bool is_compound_fru = compound_frus_ptr != nullptr && *compound_frus_ptr; - const bool* spawn_assembly_ptr = - GetValueAsBool(config_data.config, "SpawnAssembly"); - bool spawn_assembly = spawn_assembly_ptr != nullptr && *spawn_assembly_ptr; + const bool* spawn_assembly_ptr = + GetValueAsBool(config_data.config, "SpawnAssembly"); + bool spawn_assembly = + spawn_assembly_ptr != nullptr && *spawn_assembly_ptr; - std::vector<std::vector<FruKey>> fru_groups; - if (is_compound_fru) { - if (!config_data.fru_keys.empty()) { - absl::flat_hash_map< - std::tuple<std::string, std::string, std::string, std::string>, - std::vector<FruKey>> - pn_sn_groups; - for (const auto& fru_key : config_data.fru_keys) { - const Fru& fru = - mutable_data.fru_table.key_to_fru().at(fru_key.ToString()); - auto key = - std::make_tuple(fru.data().fru_info().board_part_number(), - fru.data().fru_info().board_serial_number(), - fru.data().fru_info().product_part_number(), - fru.data().fru_info().product_serial_number()); - pn_sn_groups[key].push_back(fru_key); + std::vector<std::vector<FruKey>> fru_groups; + if (is_compound_fru) { + if (!config_data.fru_keys.empty()) { + absl::flat_hash_map< + std::tuple<std::string, std::string, std::string, std::string>, + std::vector<FruKey>> + pn_sn_groups; + for (const auto& fru_key : config_data.fru_keys) { + const Fru& fru = + mutable_data.fru_table.key_to_fru().at(fru_key.ToString()); + auto key = + std::make_tuple(fru.data().fru_info().board_part_number(), + fru.data().fru_info().board_serial_number(), + fru.data().fru_info().product_part_number(), + fru.data().fru_info().product_serial_number()); + pn_sn_groups[key].push_back(fru_key); + } + for (auto const& [key, frus] : pn_sn_groups) { + fru_groups.push_back(frus); + } + std::sort( + fru_groups.begin(), fru_groups.end(), + [](const std::vector<FruKey>& a, const std::vector<FruKey>& b) { + if (a.empty()) { + return false; + } + if (b.empty()) { + return true; + } + return a[0] < b[0]; + }); } - for (auto const& [key, frus] : pn_sn_groups) { - fru_groups.push_back(frus); - } - std::sort( - fru_groups.begin(), fru_groups.end(), - [](const std::vector<FruKey>& a, const std::vector<FruKey>& b) { - if (a.empty()) { - return false; - } - if (b.empty()) { - return true; - } - return a[0] < b[0]; - }); - } - } else { - for (const auto& fru_key : config_data.fru_keys) { - fru_groups.push_back({fru_key}); - } - } - - // For each FRU, substitute config variables with FRU info and parse the - // config. - // `ProcessConfigs` is already checked to guarantee that only configs with - // $index will have multiple Fru keys matching. - for (size_t i = 0; i < fru_groups.size(); i++) { - // Create a new topology config node for each FRU. - TopologyConfigNode topology_config_node; - std::string config_name_with_index = name; - absl::StrReplaceAll({{"$index", std::to_string(i + 1)}}, - &config_name_with_index); - if (!fru_groups[i].empty() && - absl::StrContainsIgnoreCase(config_name_with_index, "$bus")) { - absl::StrReplaceAll({{"_", " "}}, &config_name_with_index); - absl::StatusOr<std::string> bus_value_substitute_result = - SubstituteExpressionVariables(config_name_with_index, "$bus", - fru_groups[i][0].bus); - if (bus_value_substitute_result.ok()) { - config_name_with_index = bus_value_substitute_result.value(); - } - absl::StrReplaceAll({{" ", "_"}}, &config_name_with_index); - } - topology_config_node.set_name(config_name_with_index); - if (spawn_assembly) { - ECCLESIA_RETURN_IF_ERROR(ProcessSpawnAssemblyFru( - immutable_data, mutable_data, config_data, elements, resource_type, - fru_groups[i], i, config_name_with_index, reload_type, - topology_config_node)); } else { - ECCLESIA_RETURN_IF_ERROR(ProcessNonSpawnAssemblyFru( - immutable_data, mutable_data, config_data, elements, resource_type, - fru_groups[i], i, config_name_with_index, reload_type, - is_compound_fru, topology_config_node, fru_key_to_sub_fru_config)); + for (const auto& fru_key : config_data.fru_keys) { + fru_groups.push_back({fru_key}); + } } - // If the config has port configs, add the topology config node to the - // topology config. - if (GetTlbmcConfig() - .fru_collector_module() - .no_associations_based_topology() || - !topology_config_node.upstream_port_configs().empty() || - !topology_config_node.port_configs().empty()) { - topology_config_node.set_config_key(config_name_with_index); - mutable_data.topology_config.mutable_topology_config_nodes()->insert( - {std::move(config_name_with_index), - std::move(topology_config_node)}); + // For each FRU, substitute config variables with FRU info and parse the + // config. + // `ProcessConfigs` is already checked to guarantee that only configs with + // $index will have multiple Fru keys matching. + for (size_t i = 0; i < fru_groups.size(); i++) { + size_t fru_group_index = base_fru_group_index + i; + // Create a new topology config node for each FRU. + TopologyConfigNode topology_config_node; + std::string config_name_with_index = name; + absl::StrReplaceAll({{"$index", std::to_string(fru_group_index + 1)}}, + &config_name_with_index); + if (!fru_groups[i].empty() && + absl::StrContainsIgnoreCase(config_name_with_index, "$bus")) { + absl::StrReplaceAll({{"_", " "}}, &config_name_with_index); + absl::StatusOr<std::string> bus_value_substitute_result = + SubstituteExpressionVariables(config_name_with_index, "$bus", + fru_groups[i][0].bus); + if (bus_value_substitute_result.ok()) { + config_name_with_index = bus_value_substitute_result.value(); + } + absl::StrReplaceAll({{" ", "_"}}, &config_name_with_index); + } + topology_config_node.set_name(config_name_with_index); + if (spawn_assembly) { + ECCLESIA_RETURN_IF_ERROR(ProcessSpawnAssemblyFru( + immutable_data, mutable_data, config_data, elements, + resource_type, fru_groups[i], fru_group_index, + config_name_with_index, reload_type, topology_config_node)); + } else { + ECCLESIA_RETURN_IF_ERROR(ProcessNonSpawnAssemblyFru( + immutable_data, mutable_data, config_data, elements, + resource_type, fru_groups[i], fru_group_index, + config_name_with_index, reload_type, is_compound_fru, + topology_config_node, fru_key_to_sub_fru_config)); + } + + // If the config has port configs, add the topology config node to the + // topology config. + if (GetTlbmcConfig() + .fru_collector_module() + .no_associations_based_topology() || + !topology_config_node.upstream_port_configs().empty() || + !topology_config_node.port_configs().empty()) { + topology_config_node.set_config_key(config_name_with_index); + mutable_data.topology_config.mutable_topology_config_nodes()->insert( + {std::move(config_name_with_index), + std::move(topology_config_node)}); + } } + base_fru_group_index += fru_groups.size(); } } return absl::OkStatus(); @@ -3131,7 +3142,7 @@ bool ignore_zero_index_name) { absl::flat_hash_map<std::string, std::string> label_to_name; for (std::size_t i = 0; i < labels.size(); ++i) { - std::string label = labels[i]; + const std::string& label = labels[i]; std::string key = absl::Substitute("$0_Name", absl::StrReplaceAll(label, {{" ", "_"}})); const std::string* name_str = GetValueAsString(config, key);
diff --git a/tlbmc/configs/entity_config_json_impl.h b/tlbmc/configs/entity_config_json_impl.h index 8197908..36b0751 100644 --- a/tlbmc/configs/entity_config_json_impl.h +++ b/tlbmc/configs/entity_config_json_impl.h
@@ -99,7 +99,8 @@ std::vector<FruKey> fru_keys; }; -using ProbedConfigMap = absl::flat_hash_map<std::string, ProbedConfigData>; +using ProbedConfigMap = + absl::flat_hash_map<std::string, std::vector<ProbedConfigData>>; struct ProcessedConfigData { ProbedConfigMap probed_config_map;