ROX-32316: New proto message for ACKs to Sensor#18215
Merged
Conversation
Contributor
Author
|
This change is part of the following stack: Change managed by git-spice. |
|
Skipping CI for Draft Pull Request. |
Contributor
|
Images are ready for the commit at 129b43e. To use with deploy scripts, first |
This was referenced Dec 15, 2025
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #18215 +/- ##
==========================================
+ Coverage 48.85% 48.89% +0.04%
==========================================
Files 2624 2626 +2
Lines 198125 198224 +99
==========================================
+ Hits 96789 96919 +130
+ Misses 93939 93912 -27
+ Partials 7397 7393 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
225c6d4 to
92246b0
Compare
9 tasks
92246b0 to
54faf82
Compare
…iting Scanner V4 can only process ~3 VM index reports/sec; exceeding this causes unbounded queue growth in Central leading to OOM. This commit adds the foundation for rate limiting with ACK/NACK feedback: - Added central.SensorACK message (Central → Sensor channel) - Added sensor.ComplianceACK message (Sensor → Compliance channel) - Deprecated central.NodeInventoryACK and sensor.NodeInventoryACK (kept for backward compatibility until 4.12) - Added SensorACKSupport capability for version negotiation - Added configuration: ROX_VM_INDEX_REPORT_RATE_LIMIT (default: 0/disabled) and ROX_VM_INDEX_REPORT_BURST_DURATION (default: 5s) No behavior changes in this commit. Rate limiting implementation follows in subsequent PRs. The two ACK messages are intentionally separate to avoid cross-imports between central and sensor protobuf packages. AI generated: Protobuf message definitions, capability constant, environment variable configuration, and protobuf regeneration. User corrected: Comment spacing, deprecation timeline (4.12), and deprecation notices for clarity.
54faf82 to
4395143
Compare
Contributor
Author
|
/retest |
guzalv
approved these changes
Jan 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR adds an unified proto message for ACK from Central to Sensor. So far, we have specialized ACK messages for Node Inventories and Node Indexes - named
NodeInventoryACK. In this PR, we introduce a new type of message -central.SensorACK- that is not bound to a particular feature and can be used for other ACK flows in the future.Additionally, we do the same for ACKs sent from Sensor to Compliance by adding message
sensor.ComplianceACK. Moreover, we add Sensor capabilitySensorACKSupport, so that Central can recognize whether the connected Sensor supports the new message format.Motivation: this is the foundation for rate limiting of VM index reports. Scanner V4 can only process ~3 VM index reports per second, and exceeding this causes unbounded queue growth in Central leading to OOM kills. This change introduces new generic ACK/NACK messages and configuration without changing behavior.
What changed:
central.SensorACKmessage for Central → Sensor acknowledgementssensor.ComplianceACKmessage for Sensor → Compliance acknowledgementscentral.NodeInventoryACKandsensor.NodeInventoryACK(will be kept until 4.12)SensorACKSupportcapability for version negotiationWhy separate ACK messages:
The two channels (Central↔Sensor vs Sensor↔Compliance) use separate protobuf packages. Having separate
SensorACKandComplianceACKmessages avoids cross-package imports while maintaining clean separation of concerns.Backward compatibility:
Central will check
conn.HasCapability(SensorACKSupport)and useSensorACKonly if present, otherwise fall back toNodeInventoryACK. This enables safe rolling upgrades with no crashes when Central 4.10 talks to Sensor 4.9.User-facing documentation
Testing and quality
Automated testing
No automated tests added. This is a foundation PR with only protobuf definitions and configuration. No runtime behavior changes.
How I validated my change
Manual inspection:
SensorACKandComplianceACKtypesNodeInventoryACKmessagescaps_list.goWhy no runtime testing:
This PR only adds unused protobuf definitions and configuration. Actual functionality will be tested in subsequent PRs when rate limiting is implemented and integrated.
AI contribution: Generated protobuf message definitions, capability constant, environment variable configuration, and performed protobuf regeneration.
User contribution: Refined comment formatting, added deprecation timeline (remove in 4.12), clarified that two separate ACK messages avoid cross-imports, and validated compilation.