xds: generate xds-routing config from XdsNameResolver

[dapengzhang0]

No description provided.

[voidzcy]

The conditions of this if-elseif-else is verbose and unclear. Use null value of fields in ConfigUpdate to make this cleaner:

if (update.clusterName != null) {
   // CDS
} else if (update.getRoutes.size() == 1) {
   // weighted-target
} else {
   // xds-routing
}

[voidzcy]

This whole logic of generating LB configs is complicated, given that each LB config can have recursively nested child policies. Same for config generation in EDS policy (TODO).

Previously generating service config with CDS LB config very simple (a raw JSON string, only ~10 lines). But now, it's bad to write all of this here (config generation in EDS policy is probably to be even more complicated). This is unreadable, with three levels of child policy nesting (maps with values of maps with values of maps). We would need to seek for a better approach:

Similar to how LB config is parsed (each LB config parser recursively calls child policy's own parser to parse child policy), we can implement a LB config generator for each LB policy so that each config generator generates its own config (recursively use child policy's generator to generate child policies) and put the output together.

[dapengzhang0]

Reorganized code. Seems a little better.

[dapengzhang0]

I changed the ConfigUpdate data, it has only routes field now. When path matching is not enabled, xdsClient will generate an configUpdate with only a single cluster route, the default route. XdsNameResolver does not need to know whether or not path matching is not enabled, it just reads the routes and generates the service config uniformly.

[voidzcy]

nit: this line should be put one line above.

[voidzcy]

Ah... I was wondering how existing test cases in XdsClientImplTest are not affected, this is why. ConfigUpdate intends to be POJO, it's a data class. Its getter should not involve any business logic, it's just for getting a property value. This method doesn't make sense when routes contains more than one items and it should not be called in that case.

So I would still leaning towards ConfigUpdate containing a String and a List, with "oneof" semantics. XdsClient will set clusterName (String) if path matching is not enabled and leave routes (List) being null, and vice versa for path matching enabled.

[dapengzhang0]

I agree that getter should not involve business logic, so I removed getClusterName(). It was just used as a helper method, mostly for making test easy. I'm not a fan of oneof with two nullalbe fields either, because it will add more awkward business logic for XdsNameResolver. Instead, I define a helper method in the test.

[voidzcy]

The logic for not enabling path matching is wired, as you can see in XdsClientImpl, it goes into Route in VirtualHost to populate clusterName but it puts Route into ConfigUpdate and the resolver goes into the Route to populate clusterName again.

[dapengzhang0]

as you can see in XdsClientImpl, it goes into Route in VirtualHost to populate clusterName but it puts Route into ConfigUpdate and the resolver goes into the Route to populate clusterName again.

The XdsClientImple goes into Route in VirtualHost NOT to populate clusterName, it populates the whole route. (Yeah, it reads clusterName, but that's only for validation and INFO level logging.)

    if (!enablePathMatching) {
        EnvoyProtoData.Route defaultRoute = Iterables.getLast(routes);
        configUpdate =
            ConfigUpdate.newBuilder()
                .addRoutes(ImmutableList.of(defaultRoute))
                .build();
        logger.log(
            XdsLogLevel.INFO,
            "Found cluster name (inlined in route config): {0}",
            defaultRoute.getRouteAction().getCluster());
      }

[dapengzhang0]

The con of this approach is:

• For path matching not enabled case, XdsNameResolver needs to dig one level deeper to get clusterName. However, for XdsNameResolver, there is no concept of path_matching_enabled.

The con of oneof approach is:

• The data structure of ConfigUpdate is ugly, and it is so only for the purpose of a usecase that might be migrated away.
• XdsNameResolver need to check null for the oneof fields.
• XdsNameResolver gets clusterName differently for "path_matching_enabled with single cluster route" and "path_matching_disabled" cases. That makes oneof essentially equivalent of the path_matching_enabled flag.

[voidzcy]

The data structure of ConfigUpdate is ugly, and it is so only for the purpose of a usecase that might be migrated away.

It's not "ugly". The two code paths use different fields in ConfigUpdate, separate code uses separate data, that's elegant. When eliminating one code path, just delete the field it uses.

• XdsNameResolver need to check null for the oneof fields.

That's the beauty of null here. Elegant and concise meaning of something unset and do not use.

[dapengzhang0]

This piece of logic is deleted in your last change, you really need to. revert it back.

I still have the logic in validateRoutes().

[voidzcy]

I missed that one. But, still, I think using "oneof" for different code paths makes implementation cleaner as I mentioned above.

[dapengzhang0]

I think we don't have to over-engineer for something that's temporary: the code path split for the enable path matching flag.
For me the code path split only in XdsClientImpl is easiest approach.

[voidzcy]

Err... I don't think that's over-engineering, it's about implementation integrity and code style. Alright, I failed to convince you. I am not going to block you on this.

[voidzcy]

Due to my recent experience making changes in XdsLoadBalancer, EdsLoadBalancer, FallbackLb, and their tests etc, I am gonna be stricter on code style and tests. 😢

Again, it rises my concern about ConfigUpdate's usage for two code paths. I am worrying things like this are gonna spread and we become addictive to easy but harmful (to code health/readability/style) helpers such as assertConfigUpdateContainsSingleClusterRoute.

[voidzcy]

nit: name assertCdsChildPolicy. Ditto below.

[voidzcy]

nit: delete this line.

[voidzcy]

nit: same for the change above. Name this to assertWeightedTargetPolicy.

[dapengzhang0]

The problem is I don't agree most of these. The oneof approach spreads the two code paths across XdsClientImpl, ConfigUdate and XdsNameResolver, if you were adding a new orthogonal flag, you would have four code paths across each of these three classes, that's very messy, not elegant. The current approach constraints the two code paths within XdsClientImpl, and it's easier to migrate and change. I'm not hoping my argument can convince you, but we do have different opinions.

You seem like tending to impose some opinionated dogmatic coding preferences that's not well-accepted/applied in google or necessary to other people. That's very counter-productive. The experience of XdsLoadBalancer, EdsLoadBalancer, FallbackL is whenever I tried to make a change, a lot of them are imposed to me, and a lot of time was spent to argue about or apply them, but those historical changes are totally gone today, we could have done the changes sooner. Similarly LoadReportClient was also making a lot of changes, and those historical changes, no matter how perfect to you or imperfect to me, are gone today.

Unlike other projects, for the XDS project, productivity is more important than "perfect" code style.

It's not "ugly". The two code paths use different fields in ConfigUpdate, separate code uses separate data, that's elegant. When eliminating one code path, just delete the field it uses.

Due to my recent experience making changes in XdsLoadBalancer, EdsLoadBalancer, FallbackLb, and their tests etc, I am gonna be stricter on code style and tests. 😢

Again, it rises my concern about ConfigUpdate's usage for two code paths. I am worrying things like this are gonna spread and we become addictive to easy but harmful (to code health/readability/style) helpers such as assertConfigUpdateContainsSingleClusterRoute.

[voidzcy]

Alright, I am not that severely uncomfortable with this approach and the scope it affects isn't big. I can accept it.