Skip to content

Conversation

@stevenzzzz
Copy link
Contributor

Commit Message: If rq_active is non-zero, we should send the locality stats even if rq_issued is zero(no requests have been issued in this poll window), some load balancer may need rq_active for load balancing.
Additional Description:
Risk Level: LOW (use or not completely decided by remote loadbalancer
Testing: unit test
Docs Changes: guarded by runtime envoy_reloadable_features_report_load_when_rq_active_is_non_zero
Release Notes: Added runtime guard envoy.reloadable_features.report_load_when_rq_active_is_non_zero, When enabled, LRS continues to send locality_stats reoprt to config server when there is no request_issued in the poll cycle.
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Signed-off-by: Xin Zhuang <stevenzzz@google.com>
Signed-off-by: Xin Zhuang <stevenzzz@google.com>
Signed-off-by: Xin Zhuang <stevenzzz@google.com>
@repokitteh-read-only
Copy link

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #41786 was opened by stevenzzzz.

see: more, trace.

@stevenzzzz
Copy link
Contributor Author

/assign @adisuissa

@stevenzzzz
Copy link
Contributor Author

/retest

Comment on lines 152 to 153
// rq_issued is zero(no requests have been issued in this poll
// window), some load balancer may need rq_active for load balancing.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// rq_issued is zero(no requests have been issued in this poll
// window), some load balancer may need rq_active for load balancing.
// rq_issued is zero (no new requests have been issued in this poll
// window). This is needed to report long-lived connections/requests (e.g., when web-sockets are used).
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines +551 to +552
scoped_runtime.mergeValues(
{{"envoy.reloadable_features.report_load_when_rq_active_is_non_zero", "true"}});
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be enabled by default, but it is still ok to enable this here so if a user disables it by default the test will still pass.
Please add a comment stating that when the runtime flag is deprecated this test should be kept, to avoid confusion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, done.

Signed-off-by: Xin Zhuang <stevenzzz@google.com>
Signed-off-by: Xin Zhuang <stevenzzz@google.com>
Copy link
Contributor Author

@stevenzzzz stevenzzzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

Comment on lines 152 to 153
// rq_issued is zero(no requests have been issued in this poll
// window), some load balancer may need rq_active for load balancing.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines +551 to +552
scoped_runtime.mergeValues(
{{"envoy.reloadable_features.report_load_when_rq_active_is_non_zero", "true"}});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, done.

adisuissa
adisuissa previously approved these changes Nov 3, 2025
Copy link
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@adisuissa
Copy link
Contributor

/assign-from @envoyproxy/senior-maintainers

@repokitteh-read-only
Copy link

@envoyproxy/senior-maintainers assignee is @RyanTheOptimist

🐱

Caused by: a #41786 (comment) was created by @adisuissa.

see: more, trace.

@adisuissa
Copy link
Contributor

Please fix CI issue.
/wait

Signed-off-by: Xin Zhuang <stevenzzz@google.com>
Signed-off-by: Xin Zhuang <stevenzzz@google.com>
@stevenzzzz
Copy link
Contributor Author

Thanks, done.

@stevenzzzz
Copy link
Contributor Author

/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

3 participants