xds: handle weighted cluster as route action

[menghanl]

This change adds support for route action being weighted cluster (instead of just one cluster). We still only support the default route (the last in the list).

Changes:

• xds_client: handle RDS response's weighted cluster fields, and propagate the update to xds resolver (by adding a fields to ServiceUpdate.
  • This makes ServiceUpdate not comparable by ==, thus the updates to use cmp.Equal
• xds_resolver: marshal weighted cluster into balancer config, which picks weighted_target as the top level balancer

---

This change is 

[menghanl]

We don't have e2e xds tests. We were relying on the integration tests. I will manually test this locally to make sure it works.

[menghanl]

Manually tested that it works. Found a expiry timer bug which will be fixed by #3615. This is ready for review.

[easwars]

Reviewed 8 of 13 files at r1, 1 of 1 files at r2.
Reviewable status: 6 of 14 files reviewed, 15 unresolved discussions (waiting on @easwars and @menghanl)

---

service_config.go, line 424 at r2 (raw file):

Quoted 8 lines of code…

	aaRaw := aa.rawJSONString
	aa.rawJSONString = ""
	bbRaw := bb.rawJSONString
	bb.rawJSONString = ""
	defer func() {
		aa.rawJSONString = aaRaw
		bb.rawJSONString = bbRaw
	}()

Is comparing just the rawJSONString field of a and b not sufficient? If so, why?

---

service_config.go, line 430 at r2 (raw file):

		bb.rawJSONString = bbRaw
	}()
	return reflect.DeepEqual(aa, bb)

Any specific reason not to use cmp.Equal here?

---

internal/internal.go, line 53 at r2 (raw file):

	// This function removes rawJSON before compareing, and adds back
	// afterwards.

Is this comment line about removing and adding back necessary here?

---

xds/internal/client/client_watchers_rds.go, line 27 at r2 (raw file):

	clusterName     string
	weightedCluster map[string]uint32

Will only one of these two be populated? If so, could you clarify with a comment.
And what is the key in the map? Could you clarify with a comment.

---

xds/internal/client/client_watchers_service.go, line 28 at r2 (raw file):

	Cluster         string
	WeightedCluster map[string]uint32

Comments here too please.

---

xds/internal/client/v2client_rds.go, line 109 at r2 (raw file):

		// The case sensitive is set to false. Not set or set to true are both
		// valid.
		return rdsUpdate{}, fmt.Errorf("matches virtual host's default route set case-sensitive to false")

s/matches/matched

---

xds/internal/client/v2client_rds.go, line 116 at r2 (raw file):

	}

	if wc := route.GetWeightedClusters(); wc != nil {

Is it a valid config if we get both weightedClusters and the singleton cluster?
Should we reject the config in that case?

---

xds/internal/client/v2client_rds.go, line 119 at r2 (raw file):

		m, err := weightedClustersProtoToMap(wc)
		if err != nil {
			return rdsUpdate{}, fmt.Errorf("match weighted cluster is invalid: %v", err)

s/match/matched

---

xds/internal/client/v2client_rds.go, line 130 at r2 (raw file):

	if t := wc.TotalWeight; t != nil {
		totalWeight = t.Value
	}

GetValue method on UInt32Value in generated code handles the latter being nil. So, you would simply use that and not check for nils.

---

xds/internal/client/v2client_rds.go, line 134 at r2 (raw file):

	}
	for _, cw := range wc.Clusters {
		w := cw.Weight.Value

Prefer using getters in the generated code rather than directly accessing fields.

---

xds/internal/client/v2client_rds_test.go, line 197 at r2 (raw file):

									}}}}}}}}},
			wantUpdate: rdsUpdate{weightedCluster: map[string]uint32{"a": 2, "b": 3, "c": 5}},
		},

A test case for weights not adding up?
And a case for totalWeight not being specified and making sure that weights add to 100.
Or if all cases for issues with weights are being handled in TestWeightedClustersProtoToMap, we should have one failure case at least here.

---

xds/internal/client/v2client_rds_test.go, line 429 at r2 (raw file):

}

func (s) TestWeightedClustersProtoToMap(t *testing.T) {

Why do we need the 's' receiver here.

---

xds/internal/client/v2client_rds_test.go, line 460 at r2 (raw file):

			},
			want:    map[string]uint32{"a": 2, "b": 3, "c": 5},
			wantErr: false,

Maybe not required to set wantErr to false since that is the default. Here and below.

---

xds/internal/client/v2client_rds_test.go, line 474 at r2 (raw file):

			want:    map[string]uint32{"a": 20, "b": 30, "c": 50},
			wantErr: false,
		},

A test case for default total weight not adding up to 100

---

xds/internal/client/v2client_rds_test.go, line 483 at r2 (raw file):

				return
			}
			if !reflect.DeepEqual(got, tt.want) {

Why not cmp.Equal?

[menghanl]

Reviewable status: 6 of 14 files reviewed, 15 unresolved discussions (waiting on @easwars and @menghanl)

---

service_config.go, line 424 at r2 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

	aaRaw := aa.rawJSONString
	aa.rawJSONString = ""
	bbRaw := bb.rawJSONString
	bb.rawJSONString = ""
	defer func() {
		aa.rawJSONString = aaRaw
		bb.rawJSONString = bbRaw
	}()

Is comparing just the rawJSONString field of a and b not sufficient? If so, why?

Because of white space..

---

service_config.go, line 430 at r2 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

Any specific reason not to use cmp.Equal here?

The service config structs contains a bunch of balancer configs, typed interface{}, with real type unexported.
For cmp.Equal to compare unexported fields, we need cmp.AllowUnexported, which needs a struct with the type. But the balancer config types aren't exported, this is becomes impossible.

---

internal/internal.go, line 53 at r2 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

	// This function removes rawJSON before compareing, and adds back
	// afterwards.

Is this comment line about removing and adding back necessary here?

Removed.
Replaced with "rawJSON strippd"

---

xds/internal/client/client_watchers_rds.go, line 27 at r2 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

	clusterName     string
	weightedCluster map[string]uint32

Will only one of these two be populated? If so, could you clarify with a comment.
And what is the key in the map? Could you clarify with a comment.

In fact clusterName is never set. I deleted the field, and corresponding field in other structs.

---

xds/internal/client/client_watchers_service.go, line 28 at r2 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

	Cluster         string
	WeightedCluster map[string]uint32

Comments here too please.

Field removed

---

xds/internal/client/v2client_rds.go, line 109 at r2 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

s/matches/matched

Done.

---

xds/internal/client/v2client_rds.go, line 116 at r2 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

Is it a valid config if we get both weightedClusters and the singleton cluster?
Should we reject the config in that case?

It's an oneof, will never happen.

---

xds/internal/client/v2client_rds.go, line 119 at r2 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

s/match/matched

Done.

---

xds/internal/client/v2client_rds.go, line 130 at r2 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

	if t := wc.TotalWeight; t != nil {
		totalWeight = t.Value
	}

GetValue method on UInt32Value in generated code handles the latter being nil. So, you would simply use that and not check for nils.

Still need to check nil. If total_weight is nil, total weight's default value is 100.

---

xds/internal/client/v2client_rds.go, line 134 at r2 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

Prefer using getters in the generated code rather than directly accessing fields.

Done.

---

xds/internal/client/v2client_rds_test.go, line 197 at r2 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

A test case for weights not adding up?
And a case for totalWeight not being specified and making sure that weights add to 100.
Or if all cases for issues with weights are being handled in TestWeightedClustersProtoToMap, we should have one failure case at least here.

Done.

---

xds/internal/client/v2client_rds_test.go, line 429 at r2 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

Why do we need the 's' receiver here.

We don't. It's probably added as part of the test logger refactor, all (most?) tests all have the s receiver now. So we have better logging when it fails?

---

xds/internal/client/v2client_rds_test.go, line 460 at r2 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

Maybe not required to set wantErr to false since that is the default. Here and below.

Done.

---

xds/internal/client/v2client_rds_test.go, line 474 at r2 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

A test case for default total weight not adding up to 100

Done.

---

xds/internal/client/v2client_rds_test.go, line 483 at r2 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

Why not cmp.Equal?

Done.

[easwars]

Reviewable status: 2 of 14 files reviewed, 4 unresolved discussions (waiting on @easwars and @menghanl)

---

service_config.go, line 424 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

Because of white space..

Maybe add a comment? Because otherwise it is not very clear.

---

service_config.go, line 430 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

The service config structs contains a bunch of balancer configs, typed interface{}, with real type unexported.
For cmp.Equal to compare unexported fields, we need cmp.AllowUnexported, which needs a struct with the type. But the balancer config types aren't exported, this is becomes impossible.

Same. A quick comment would be useful. Thanks.

---

xds/internal/client/client_watchers_service.go, line 28 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

Field removed

A comment here saying what is the key would still be useful. Thanks.

---

xds/internal/client/v2client_rds.go, line 130 at r2 (raw file):

Previously, menghanl (Menghan Li) wrote…

Still need to check nil. If total_weight is nil, total weight's default value is 100.

I might write it as:

	var totalWeight uint32 = 100
	if t := wc.GetTotalWeight().GetValue(); t != 0 {
		totalWeight = t.Value
	}

I find this cleaner. Although, could just be personal opinion.

[easwars]

Reviewed 2 of 13 files at r1, 10 of 10 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @menghanl)

---

xds/internal/client/v2client_rds.go, line 130 at r2 (raw file):

var totalWeight uint32 = 100
if t := wc.GetTotalWeight().GetValue(); t != 0 {
totalWeight = t.Value
}
I guess I wanted to say the following:

	var totalWeight uint32 = 100
	if t := wc.GetTotalWeight().GetValue(); t != 0 {
		totalWeight = t
	}

---

xds/internal/resolver/serviceconfig.go, line 76 at r4 (raw file):

	bs, err := json.Marshal(sc)
	if err != nil {
		panic(err.Error())

Do we really need to panic here?
Could we change this function to return (string, error) and have the resolver call cc.ReportError instead?

---

xds/internal/resolver/xds_resolver_test.go, line 348 at r4 (raw file):

			t.Errorf("ClientConn.UpdateState received different service config")
			t.Error("got: ", cmp.Diff(nil, rState.ServiceConfig.Config))
			t.Error("want: ", cmp.Diff(nil, wantSCParsed.Config))

Can't we have a single call to t.Errorf and do a cmp.Diff(gotConfig, wantConfig).
The same comment applied to the test in TestServiceUpdateToJSON

[menghanl]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @easwars)

---

service_config.go, line 424 at r2 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

Maybe add a comment? Because otherwise it is not very clear.

Done.

---

service_config.go, line 430 at r2 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

Same. A quick comment would be useful. Thanks.

Done.

---

xds/internal/client/client_watchers_service.go, line 28 at r2 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

A comment here saying what is the key would still be useful. Thanks.

Done.

---

xds/internal/client/v2client_rds.go, line 130 at r2 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

var totalWeight uint32 = 100
if t := wc.GetTotalWeight().GetValue(); t != 0 {
totalWeight = t.Value
}
I guess I wanted to say the following:

	var totalWeight uint32 = 100
	if t := wc.GetTotalWeight().GetValue(); t != 0 {
		totalWeight = t
	}

Done.

---

xds/internal/resolver/serviceconfig.go, line 76 at r4 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

Do we really need to panic here?
Could we change this function to return (string, error) and have the resolver call cc.ReportError instead?

Done.

---

xds/internal/resolver/xds_resolver_test.go, line 348 at r4 (raw file):

Previously, easwars (Easwar Swaminathan) wrote…

			t.Errorf("ClientConn.UpdateState received different service config")
			t.Error("got: ", cmp.Diff(nil, rState.ServiceConfig.Config))
			t.Error("want: ", cmp.Diff(nil, wantSCParsed.Config))

Can't we have a single call to t.Errorf and do a cmp.Diff(gotConfig, wantConfig).
The same comment applied to the test in TestServiceUpdateToJSON

We cannot cmp.Diff two service configs, because of the unexported balancer config types.
This cmp.Diff here is just to pretty print the content.

[easwars]

Is it really required that this function be exposed though the internal package?
I see that this only used by a couple of xds_tests. Could we rather move this to xds/testutils?

[menghanl]

To clear the rawJSONString field, it has to be here.
Or, we export a method to clear the field...