balancer/rls: allow maxAge to exceed 5m if staleAge is set #8137

shivaspeaks · 2025-03-01T14:30:11Z

Internal feature request: b/371591767

RELEASE NOTES:

balancer/rls: allow maxAge to exceed 5m if staleAge is set for the rls cache

codecov · 2025-03-01T14:35:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.23%. Comparing base (8ae4b7d) to head (bebde4c).
Report is 8 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8137      +/-   ##
==========================================
- Coverage   82.34%   82.23%   -0.12%     
==========================================
  Files         389      392       +3     
  Lines       39103    39153      +50     
==========================================
- Hits        32200    32197       -3     
- Misses       5579     5628      +49     
- Partials     1324     1328       +4

Files with missing lines	Coverage Δ
balancer/rls/config.go	`82.95% <100.00%> (+1.35%)`	⬆️

... and 31 files with indirect coverage changes

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

shivaspeaks · 2025-03-03T04:17:58Z

balancer/rls/config.go

 		logger.Infof("rls: max_age in route lookup config is %v, using %v", maxAge, maxMaxAge)
 		maxAge = maxMaxAge
 	}
+	if staleAge > maxAge {
+		logger.Infof("rls: stale_age %v is not less than max_age %v, ignoring it", staleAge, maxAge)
+		staleAge = maxAge


I assume, ignoring staleAge let us to set it to maxAge and that should be fine.

purnesh42H · 2025-03-04T05:45:26Z

balancer/rls/config.go

@@ -219,7 +219,7 @@ func parseRLSProto(rlsProto *rlspb.RouteLookupConfig) (*lbConfig, error) {
 	}

 	// Validations performed here:
-	// - if `max_age` > 5m, it should be set to 5 minutes
+	// - if `max_age` > 5m, it should be set to 5 minutes only if stale age is not set


nit: can you make sure to wrap the comments within 80 cols?

I don't see any checkstyle failures.

there is no checker to test but for go 80 cols is limit for comments

go/go-style/decisions#comment-line-length

purnesh42H · 2025-03-04T05:52:01Z

balancer/rls/config.go

 		logger.Infof("rls: max_age in route lookup config is %v, using %v", maxAge, maxMaxAge)
 		maxAge = maxMaxAge
 	}
+	if staleAge > maxAge {
+		logger.Infof("rls: stale_age %v is not less than max_age %v, ignoring it", staleAge, maxAge)
+		staleAge = maxAge


shouldn't staleAge be set to 0 because we are ignoring it? That's how its done before

#Comment23 talks about clamping the these values. I think this will be an improvement. Also rls_config.proto has been changed that talks about clamping the stale_age: grpc/grpc-proto@0462d4b

stale_age was set to 0 here, #3379. But now, as per rls_config it should be clamped. That's what java and core are doing.

CC: @easwars

I see the following logic in C-core

Internal representatino of stale_age and max_age both start off with default value of 5m, which is max allowed value

In the JSON configuration, check if stale_age and max_age are set in the configuration:

If stale_age is set, but max_age is not set, then throw an error

Clamp stale_age to the max allowed value

If stale_age is not set, clamp max_age to the max allowed value

If stale_age is greater than or equal to max_age, set stale_age to be the same as max_age

In Go, we marshal the JSON configuration into a proto message and perform all validation and build our internal representation from the proto message. Because the proto API in Go does not support checking if a value is set or not, I suggest going with the following approach.

Read staleAge from proto using the convertDuration helper function. If the read staleAge is non-zero, then set staleAgeSet to be true. If the read staleAge is zero, set staleAge to maxMaxAge which is 5m.

staleAgeSet := false staleAge, err := convertDuration(rlsProto.GetStaleAge()) if err != nil { return nil, fmt.Errorf("rls: failed to parse staleAge in route lookup config %+v: %v", rlsProto, err) } if staleAge == 0 { staleAge = maxMaxAge } else { staleAgeSet = true }

Do the same thing for maxAge

Then check whether both are set or not

If staleAgeSet && !maxAgeSet { return nil, fmt.Errorf("rls: stale_age is set, but max_age is not in route lookup config %+v", rlsProto) }

Clamp stale_age to the max allowed value

if staleAge > maxMaxAge { staleAge = maxMaxAge }

If stale_age is not set, clamp max_age to the max allowed value

if !staleAgeSet && maxAge > maxMaxAge { maxAge = maxMaxAge }

If stale_age is greater than or equal to max_age, set stale_age to be the same as max_age

if staleAge > maxAge { staleAge = maxAge }

Let me know what you think.

Yeah, this seems ditto logic-wise. The only diff is, we are always assigning some value to staleAge and that should be fine.
But looks good to use staleAgeSet and maxAgeSet . Makes it much easier to understand for the reader.

PS: In java we are not using these extra variables. We are playing with staleAge and maxAge only.

purnesh42H · 2025-03-04T06:06:04Z

balancer/rls/config.go

 	}
-	if maxAge == 0 || maxAge > maxMaxAge {
+	if staleAge == 0 && maxAge > maxMaxAge {


I think we only need to update this condition at line 239 and leave rest of the stuff as is. Just to make sure we are not doing any unintended changes here and only updating the validation to allow maxAge > 5 but only if staleAge is set. So, may be something like this should suffice?

if maxAge == 0 { logger.Infof("rls: max_age in route lookup config is %v, using %v", maxAge, maxMaxAge) maxAge = maxMaxAge } else if maxAge > maxMaxAge && staleAge == 0 { logger.Infof("rls: max_age in route lookup config is %v and stale_age is not set, using %v", maxAge, maxMaxAge) maxAge = maxMaxAge } ``` We update maxAge to maxMaxAge only if its either not set or its set but staleAge is not set. Otherwise we just keep the value assigned to it.

easwars · 2025-03-05T01:23:24Z

balancer/rls/config.go

@@ -219,7 +219,7 @@ func parseRLSProto(rlsProto *rlspb.RouteLookupConfig) (*lbConfig, error) {
 	}

 	// Validations performed here:
-	// - if `max_age` > 5m, it should be set to 5 minutes
+	// - if `max_age` > 5m, it should be set to 5 minutes only if stale age is not set


go/go-style/decisions#comment-line-length

easwars · 2025-03-05T18:18:57Z

balancer/rls/config.go

 		logger.Infof("rls: max_age in route lookup config is %v, using %v", maxAge, maxMaxAge)
 		maxAge = maxMaxAge
 	}
+	if staleAge > maxAge {
+		logger.Infof("rls: stale_age %v is not less than max_age %v, ignoring it", staleAge, maxAge)
+		staleAge = maxAge


I see the following logic in C-core

Internal representatino of stale_age and max_age both start off with default value of 5m, which is max allowed value

In the JSON configuration, check if stale_age and max_age are set in the configuration:

If stale_age is set, but max_age is not set, then throw an error

Clamp stale_age to the max allowed value

If stale_age is not set, clamp max_age to the max allowed value

If stale_age is greater than or equal to max_age, set stale_age to be the same as max_age

In Go, we marshal the JSON configuration into a proto message and perform all validation and build our internal representation from the proto message. Because the proto API in Go does not support checking if a value is set or not, I suggest going with the following approach.

Read staleAge from proto using the convertDuration helper function. If the read staleAge is non-zero, then set staleAgeSet to be true. If the read staleAge is zero, set staleAge to maxMaxAge which is 5m.

staleAgeSet := false staleAge, err := convertDuration(rlsProto.GetStaleAge()) if err != nil { return nil, fmt.Errorf("rls: failed to parse staleAge in route lookup config %+v: %v", rlsProto, err) } if staleAge == 0 { staleAge = maxMaxAge } else { staleAgeSet = true }

Do the same thing for maxAge

Then check whether both are set or not

If staleAgeSet && !maxAgeSet { return nil, fmt.Errorf("rls: stale_age is set, but max_age is not in route lookup config %+v", rlsProto) }

Clamp stale_age to the max allowed value

if staleAge > maxMaxAge { staleAge = maxMaxAge }

If stale_age is not set, clamp max_age to the max allowed value

if !staleAgeSet && maxAge > maxMaxAge { maxAge = maxMaxAge }

If stale_age is greater than or equal to max_age, set stale_age to be the same as max_age

if staleAge > maxAge { staleAge = maxAge }

Let me know what you think.

easwars · 2025-03-05T18:22:16Z

balancer/rls/config_test.go

+			},
+		},
+		{
+			desc: "with transformations 1",


The value for this field needs to be different in each of the sub tests.

Curious to know why it didn't fail even with same value with this field! And surprisingly it made it also through github checks!
Now it is getting caught and failing in the editor itself when I'm running the tests.

easwars · 2025-03-05T18:23:09Z

balancer/rls/config_test.go

+		{
+			desc: "with transformations 1",
+			input: []byte(`{
+				"top-level-unknown-field": "unknown-value",


This is not required for this sub-test.

easwars · 2025-03-05T18:23:18Z

balancer/rls/config_test.go

+			input: []byte(`{
+				"top-level-unknown-field": "unknown-value",
+				"routeLookupConfig": {
+					"unknown-field": "unknown-value",


Same with this field.

easwars · 2025-03-05T18:24:53Z

balancer/rls/config_test.go

+				maxAge:                 500 * time.Second, // Max age is not clamped when stale age is set.
+				staleAge:               300 * time.Second, // StaleAge is clamped because it was higher than maxMaxAge.


The comment at the top of this sub-test also needs to change.

easwars · 2025-03-05T18:25:36Z

balancer/rls/config_test.go

+					{"cds_experimental": {"Cluster": "my-fav-cluster"}},
+					{"unknown-policy": {"unknown-field": "unknown-value"}},


I think we can get rid of these two lines because they are not pertinent to this sub-test.

easwars · 2025-03-05T18:26:51Z

balancer/rls/config_test.go

+					"maxAge" : "500s",
+					"staleAge": "200s",
+					"cacheSizeBytes": 100000000,
+					"defaultTarget": "passthrough:///default"


Get rid of the defaultTarget as well. Not pertinent to this sub-test.

easwars · 2025-03-05T18:27:02Z

balancer/rls/config_test.go

+					"lookupService": ":///target",
+					"maxAge" : "500s",
+					"cacheSizeBytes": 100000000,
+					"defaultTarget": "passthrough:///default"


Get rid of the defaultTarget as well. Not pertinent to this sub-test.

easwars · 2025-03-05T18:27:12Z

balancer/rls/config_test.go

+					{"cds_experimental": {"Cluster": "my-fav-cluster"}},
+					{"unknown-policy": {"unknown-field": "unknown-value"}},


I think we can get rid of these two lines because they are not pertinent to this sub-test.

rls: allow maxAge to exceed 5m if staleAge is set

0140b27

shivaspeaks commented Mar 3, 2025

View reviewed changes

purnesh42H reviewed Mar 4, 2025

View reviewed changes

purnesh42H assigned shivaspeaks Mar 4, 2025

purnesh42H added Area: Resolvers/Balancers Includes LB policy & NR APIs, resolver/balancer/picker wrappers, LB policy impls and utilities. Type: Feature New features or improvements in behavior labels Mar 4, 2025

purnesh42H changed the title ~~rls: allow maxAge to exceed 5m if staleAge is set~~ balancer/rls: allow maxAge to exceed 5m if staleAge is set Mar 4, 2025

purnesh42H added Status: Requires Reporter Clarification and removed Status: Requires Reporter Clarification labels Mar 4, 2025

purnesh42H assigned easwars Mar 4, 2025

easwars reviewed Mar 5, 2025

View reviewed changes

easwars removed their assignment Mar 5, 2025

easwars added this to the 1.72 Release milestone Mar 5, 2025

shivaspeaks added 2 commits March 10, 2025 06:06

improved readability

dc0ace7

wrap the comments within 80 cols

bebde4c

shivaspeaks requested review from easwars and purnesh42H March 10, 2025 07:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

balancer/rls: allow maxAge to exceed 5m if staleAge is set #8137

balancer/rls: allow maxAge to exceed 5m if staleAge is set #8137

shivaspeaks commented Mar 1, 2025 •

edited by easwars

Loading

codecov bot commented Mar 1, 2025 •

edited

Loading

shivaspeaks Mar 3, 2025

purnesh42H Mar 4, 2025

shivaspeaks Mar 4, 2025

purnesh42H Mar 4, 2025

easwars Mar 5, 2025

purnesh42H Mar 4, 2025

shivaspeaks Mar 4, 2025

shivaspeaks Mar 4, 2025

easwars Mar 5, 2025

shivaspeaks Mar 10, 2025

purnesh42H Mar 4, 2025

easwars Mar 5, 2025

easwars Mar 5, 2025

easwars Mar 5, 2025

shivaspeaks Mar 10, 2025

easwars Mar 5, 2025

easwars Mar 5, 2025

easwars Mar 5, 2025

easwars Mar 5, 2025

easwars Mar 5, 2025

easwars Mar 5, 2025

easwars Mar 5, 2025

		maxAge: 500 * time.Second, // Max age is not clamped when stale age is set.
		staleAge: 300 * time.Second, // StaleAge is clamped because it was higher than maxMaxAge.

		{"cds_experimental": {"Cluster": "my-fav-cluster"}},
		{"unknown-policy": {"unknown-field": "unknown-value"}},

balancer/rls: allow maxAge to exceed 5m if staleAge is set #8137

Are you sure you want to change the base?

balancer/rls: allow maxAge to exceed 5m if staleAge is set #8137

Conversation

shivaspeaks commented Mar 1, 2025 • edited by easwars Loading

codecov bot commented Mar 1, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shivaspeaks commented Mar 1, 2025 •

edited by easwars

Loading

codecov bot commented Mar 1, 2025 •

edited

Loading