Usage update of absl::variant in xds module to ensure better compatibility & simplicity

[jiminj]

Is your feature request related to a problem? Please describe.

(Please visit #30118 first)

Grpc >= 1.46.x build fails with gcc 9.3 & c++17 setup. After some investigation, I found that it is related to the newly introduced xds core module and a bug in std::variant gcc standard implementation when it has the same type more than once, which is fixed in gcc 9.4, gcc 10.2 and gcc 11.1.

Although this is from a bug from gcc and I respect your decision in the last issue, simply closing that. I still think there is room to reconsider.

1. gcc bug is not as a simple thing as bugs in any other third-party libraries, especially Some dev environments with old Linux distributions are not always allowed to upgrade (or apply a patch and rebuild tools) their dev tool sometimes, forced to stick to old versions. This impacts the users in these environments.

2. gcc 9.3, and other releases with this bug are still in your support range according to your official document.

3. Another aspect; although std/absl::variant is allowed to have the same type more than once, I think the drawbacks of holding the same type multiple times outweigh the benefits of it because it limits the usage of type-based accessors, like std/absl::visit and std/absl::holds_alternative.

Describe the solution you'd like

The one possible approach I can think now is using wrappers for two strings to distinguish them.

  struct ClusterNameWrapper { std::string str; ...}
  struct PluginNameWrapper { std::string str; ...}
  absl::variant<ClusterNameWrapper, std::vector<ClusterWeight>, PluginNameWrapper> action;

Not only it can be a workaround to the gcc bug mentioned above. This change can bring more simplicity and Intuitiveness to the related codebase. For example, with this change the below code can be updated to a simpler form, with the help of Overload pattern,

grpc/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc

Lines 478 to 516 in 85059e9

	if (route_action->action.index() ==
	XdsRouteConfigResource::Route::RouteAction::kClusterIndex) {
	*error = CreateMethodConfig(route_entry.route, nullptr,
	&route_entry.method_config);
	MaybeAddCluster(absl::StrCat(
	"cluster:",
	absl::get<
	XdsRouteConfigResource::Route::RouteAction::kClusterIndex>(
	route_action->action)));
	} else if (route_action->action.index() ==
	XdsRouteConfigResource::Route::RouteAction::
	kWeightedClustersIndex) {
	auto& action_weighted_clusters = absl::get<
	XdsRouteConfigResource::Route::RouteAction::kWeightedClustersIndex>(
	route_action->action);
	uint32_t end = 0;
	for (const auto& weighted_cluster : action_weighted_clusters) {
	Route::ClusterWeightState cluster_weight_state;
	*error = CreateMethodConfig(route_entry.route, &weighted_cluster,
	&cluster_weight_state.method_config);
	if (!GRPC_ERROR_IS_NONE(*error)) return;
	end += weighted_cluster.weight;
	cluster_weight_state.range_end = end;
	cluster_weight_state.cluster = weighted_cluster.name;
	route_entry.weighted_cluster_state.push_back(
	std::move(cluster_weight_state));
	MaybeAddCluster(absl::StrCat("cluster:", weighted_cluster.name));
	}
	} else if (route_action->action.index() ==
	XdsRouteConfigResource::Route::RouteAction::
	kClusterSpecifierPluginIndex) {
	// cluster_specifier_plugin case:
	*error = CreateMethodConfig(route_entry.route, nullptr,
	&route_entry.method_config);
	MaybeAddCluster(absl::StrCat(
	"cluster_specifier_plugin:",
	absl::get<XdsRouteConfigResource::Route::RouteAction::
	kClusterSpecifierPluginIndex>(route_action->action)));
	}

std::visit(overload {
  [](const ClusterNameWrapper & v) {
   *error = CreateMethodConfig(route_entry.route, nullptr, 
                               &route_entry.method_config); 
    MaybeAddCluster(absl::StrCat( "cluster:", v.str));
  },
  [](const std::vector<ClusterWeight> & v) { ... },
  [](const PluginNameWrapper & v) { ... }
}, route_action->action);

Describe alternatives you've considered

If you do not think this is the required change to be considerable, I think it is needed to be mentioned about the information of "unsupported environments" with c++17 standard, (gcc 9 < 9.4, gcc10 < 10.2 and gcc11 < 11.1 without a related patch), not just GCC 6.3+.

Additional context

Issue #30118

[veblush]

@jiminj Thanks for the detailed information. Although it's a buggy gcc issue, it's a bit concerning that fixed version of gcc isn't available on certain linux distributions such as CentOS 7. Would you share your environment and whether fixed gcc versions (such as gcc 9.4) are available?

[jiminj]

@veblush My client uses CentOS 7, (7.4) which has gcc 4.8.5 as its embedded one. Since our solution is based on C++17 and built and tested with gcc9, we guided them to use gcc 9.3 using the package devtoolset-9-gcc-9.3.1, which is the latest gcc9 version available on the package manager. gcc9.4 is not a feasible option unless building them individually by our own, and we do not see it as an available option for our client.

They provide gcc-10.2 and gcc-11.2, which does not seem to have the issue (didn't test though). Updating environments will take many steps and time for us though.

[veblush]

@jiminj Thanks for the input!

[veblush]

@jiminj We've decided to support this environment by having the workaround which will follow. Thanks!

[jiminj]

Awesome. Thank you!