Add support for policy alerts #3168
Conversation
- SetStatus now accepts labels that go along with the alert. Amongst the labels, a special label called "severity" controls the severity of the alert
- concurrency-scheduler - load-scheduler - quota-scheduler - scheduler
WalkthroughThe recent updates primarily focus on enhancing the status tracking system across various components of the project. A notable change is the introduction of an additional argument in Changes
TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Configuration used: CodeRabbit UI
Files selected for processing (23)
- pkg/jobs/job-tracker.go (1 hunks)
- pkg/jobs/job.go (2 hunks)
- pkg/labelstatus/labelstatus.go (1 hunks)
- pkg/notifiers/fx-driver.go (3 hunks)
- pkg/otelcollector/otelcollector.go (1 hunks)
- pkg/platform/platform.go (1 hunks)
- pkg/platform/readiness.go (1 hunks)
- pkg/policies/autoscale/kubernetes/actuators/podscaler/pod-scaler.go (2 hunks)
- pkg/policies/controlplane/components/alerter.go (1 hunks)
- pkg/policies/controlplane/policy-factory.go (2 hunks)
- pkg/policies/controlplane/runtime/circuit.go (1 hunks)
- pkg/policies/flowcontrol/actuators/concurrency-limiter/concurrency-limiter.go (14 hunks)
- pkg/policies/flowcontrol/actuators/concurrency-scheduler/concurrency-scheduler.go (14 hunks)
- pkg/policies/flowcontrol/actuators/load-scheduler/load-scheduler.go (14 hunks)
- pkg/policies/flowcontrol/actuators/quota-scheduler/quota-scheduler.go (12 hunks)
- pkg/policies/flowcontrol/actuators/rate-limiter/rate-limiter.go (13 hunks)
- pkg/policies/flowcontrol/actuators/sampler/sampler.go (2 hunks)
- pkg/policies/flowcontrol/actuators/workload-scheduler/scheduler.go (6 hunks)
- pkg/policies/flowcontrol/resources/fluxmeter/flux-meter.go (1 hunks)
- pkg/status/registry.go (2 hunks)
- pkg/status/status_test.go (4 hunks)
- pkg/watchdog/watchdog.go (1 hunks)
- test/aperture_suite_test.go (2 hunks)
Files skipped from review due to trivial changes (2)
- pkg/platform/readiness.go
- pkg/policies/controlplane/components/alerter.go
Additional comments: 68
pkg/labelstatus/labelstatus.go (7)
- 18-21: The
LabelStatusFactorystruct is well-defined with a clear purpose. It holds a reference to astatus.Registry, which is essential for creatingLabelStatusinstances with specific status management capabilities.- 23-28: The
LabelStatusModulefunction correctly provides anfx.Optionfor dependency injection, ensuring that theLabelStatusFactorycan be easily integrated into an application using thefxframework.- 30-34: The
NewLabelStatusFactoryfunction is correctly implemented to create and return a new instance ofLabelStatusFactory. It properly initializes theregistryfield with a child registry specific to label status management.- 37-46: The
Newmethod inLabelStatusFactorycorrectly creates a newLabelStatusinstance with the provided label key, policy name, and component ID. It ensures that eachLabelStatushas its own registry for status management.- 48-56: The
LabelStatusstruct is well-defined, encapsulating the necessary fields for managing the status of labels. It includes synchronization primitives for thread safety, which is a good practice.- 58-79: The
Setupmethod inLabelStatuscorrectly sets up lifecycle hooks for the label status, registering jobs for setting lookup status. It properly handles job registration and deregistration in theOnStartandOnStophooks, respectively.- 81-86: The
SetMissingmethod inLabelStatuscorrectly sets the status to missing with the current timestamp. It properly uses a lock to ensure thread safety when updating the timestamp.pkg/jobs/job-tracker.go (1)
- 72-72: The modification to the
updateStatusmethod to include an additionalnilargument in theSetStatuscall aligns with the updated method signature. This change is consistent and correctly implemented.pkg/platform/platform.go (2)
- 167-167: The addition of an extra
nilargument in theSetStatusmethod call within theRunfunction is correctly implemented, matching the updated method signature.- 172-172: The modification to include an additional
nilargument in theSetStatuscall during the platform stopping phase is correctly implemented and aligns with the updated method signature.pkg/jobs/job.go (2)
- 137-137: The addition of an extra
nilargument in theSetStatusmethod call within thedoJobmethod is correctly implemented, matching the updated method signature.- 141-141: The modification to include an additional
nilargument in theSetStatuscall after job completion is correctly implemented and aligns with the updated method signature.pkg/otelcollector/otelcollector.go (1)
- 166-166: The modification to include an additional
nilargument in theSetStatuscall within thesetReadinessStatusfunction is correctly implemented, aligning with the updated method signature.pkg/status/status_test.go (2)
- 38-38: The test cases have been updated to include an additional
nilargument in theSetStatuscalls, reflecting the changes in the method signature. This ensures that the tests remain valid and accurate.- 101-101: The test cases correctly include an additional
nilargument in theSetStatuscalls, ensuring that the tests are aligned with the updated method signature.pkg/notifiers/fx-driver.go (8)
- 63-63: The addition of an extra
nilargument in theSetStatusmethod call within theinitAppfunction is correctly implemented, matching the updated method signature.- 101-101: The modification to include an additional
nilargument in theSetStatuscall when handling errors during app initialization is correctly implemented and aligns with the updated method signature.- 108-108: The addition of an extra
nilargument in theSetStatusmethod call within theinitAppfunction, indicating the start of the policy runner, is correctly implemented.- 114-114: The modification to include an additional
nilargument in theSetStatuscall when handling errors during app start is correctly implemented and aligns with the updated method signature.- 120-120: The addition of an extra
nilargument in theSetStatusmethod call within theinitAppfunction, indicating the successful start of the policy runner, is correctly implemented.- 122-122: The inclusion of an additional
nilargument in theSetStatuscall when thefxRunneris not initialized is correctly implemented, aligning with the updated method signature.- 128-128: The addition of an extra
nilargument in theSetStatusmethod call within thedeinitAppfunction, indicating the stopping of the policy runner, is correctly implemented.- 141-141: The modification to include an additional
nilargument in theSetStatuscall when the policy runner has stopped is correctly implemented and aligns with the updated method signature.pkg/status/registry.go (3)
- 18-18: The update to the
SetStatusmethod signature to include an additionalmap[string]stringparameter for labels is a significant change. This allows for more detailed status updates with contextual information.- 180-180: The implementation of the updated
SetStatusmethod correctly handles the newlabelsparameter, ensuring that status updates can include additional contextual information.- 190-216: The
createAlertmethod has been updated to accept and process an additionallabelsparameter. This enhancement allows for more detailed alerts with contextual information. The method correctly handles theseveritylabel and removes it from the labels map before passing it to the alert options, ensuring that the severity is processed separately from other labels.pkg/policies/controlplane/policy-factory.go (2)
- 149-149: The addition of a
nilargument to theSetStatusmethod call aligns with the updated method signature. Ensure that thenilargument's purpose (likely representing additional context or labels) is correctly handled or documented if it's intended for future use.- 159-159: Similar to the previous comment, the inclusion of a
nilargument in theSetStatusmethod call is consistent with the method's new signature. It's important to verify that this change is properly documented, especially regarding thenilargument's intended use.test/aperture_suite_test.go (2)
- 199-199: The addition of a
nilargument to theSetStatusmethod call is consistent with the updated method signature. This change should be documented, particularly the rationale behind thenilargument and how it might be utilized in the future.- 209-209: As with the previous instances, adding a
nilargument to theSetStatusmethod call aligns with the updated method signature. Ensure this change is well-documented, focusing on thenilargument's intended purpose.pkg/policies/flowcontrol/resources/fluxmeter/flux-meter.go (1)
- 123-123: The inclusion of a
nilargument in theSetStatusmethod call adheres to the updated method signature. It's crucial to document this change, especially thenilargument's role and potential future applications.pkg/watchdog/watchdog.go (1)
- 178-178: Adding a
nilargument to theSetStatusmethod call is in line with the method's new signature. This modification should be documented, with a particular focus on explaining thenilargument's intended use.pkg/policies/flowcontrol/actuators/sampler/sampler.go (2)
- 172-172: The addition of a
nilargument to theSetStatusmethod call aligns with the updated method signature. This change should be documented, especially thenilargument's purpose and how it might be used in the future.- 272-272: Similar to previous instances, the inclusion of a
nilargument in theSetStatusmethod call is consistent with the method's new signature. Ensure this change is well-documented, focusing on thenilargument's intended purpose.pkg/policies/autoscale/kubernetes/actuators/podscaler/pod-scaler.go (2)
- 165-165: The
SetStatusmethod now includes an additionalnilargument. Ensure that the updated method signature is correctly implemented across all calls to this method.- 299-299: The
SetStatusmethod call within theOnStopfunction of thepodScalerstruct has been updated to include an additionalnilargument. This change aligns with the modifications made to theSetStatusmethod signature. Ensure consistency in the usage of this updated method across the codebase.pkg/policies/controlplane/runtime/circuit.go (1)
- 285-285: The
SetStatusmethod call within theExecutemethod now includes an additionalnilargument. This change should be verified for consistency across all usages of theSetStatusmethod within the codebase.Verification successful
The attempt to verify the consistent use of the updated
SetStatusmethod signature with an additionalnilargument across the codebase did not yield any results. This outcome suggests that the specific patternSetStatus($_, $_, nil)was not found elsewhere in the codebase, or there was an issue with the search parameters used.Given the lack of results, it's prudent to adjust the search parameters to ensure a comprehensive search. This adjustment will aim to capture any usage of
SetStatusregardless of the number of arguments, to better understand its usage patterns across the codebase.
Given the lack of results from the adjusted search parameters, it appears that the
ast-greptool did not find any instances of theSetStatusmethod being used in the codebase, according to the patterns provided. This outcome could be due to several reasons, such as limitations in the search pattern or the absence of the method usage within the accessible code during the search.To ensure a thorough verification, a different approach using a more general search tool like
rg(ripgrep) might provide insight into the usage ofSetStatuswithin the codebase. This tool can help identify any occurrences ofSetStatus, regardless of the method signature.
The attempt to use
rg(ripgrep) with the--goflag resulted in an error, indicating a misuse of the tool's options. The correct approach does not require specifying the language with--gosincergsearches through text files based on the patterns provided without language-specific filtering. To accurately search for occurrences ofSetStatuswithin the codebase, the command should be adjusted to omit the incorrect flag.Let's correct the command to search for any usage of
SetStatuswithout specifying the language, ensuring a comprehensive search across all Go files by default, given the file extensions.
The search results indicate multiple usages of the
SetStatusmethod across various files in the codebase, with a variety of signatures and contexts. Specifically, the method is used in both Go and C# files, reflecting its application across different parts of the project. The Go usages include calls withnilas an argument, aligning with the change observed in thecircuit.gofile, as well as other patterns.Given the diversity in the usage of
SetStatusacross the codebase, including the presence of thenilargument in several instances, it can be concluded that the change in theExecutemethod of theCircuitstruct to include an additionalnilargument is consistent with other usages within the codebase. This suggests that the method's signature may have been updated to accommodate a broader range of scenarios, including those that do not require a specific second argument, hence thenil.Based on the information gathered, the review comment regarding the need to verify the consistency of the
SetStatusmethod's usage across the codebase has been addressed. The usage ofSetStatuswith anilargument in the specified context is consistent with other parts of the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Ensure the updated SetStatus method signature is used consistently. ast-grep --lang go --pattern $'SetStatus($_, $_, nil)'Length of output: 55
Script:
#!/bin/bash # Broaden the search to capture any usage of SetStatus to understand its usage patterns. ast-grep --lang go --pattern $'SetStatus($$$)'Length of output: 47
Script:
#!/bin/bash # Use rg to search for any usage of SetStatus to ensure comprehensive coverage. rg 'SetStatus' --goLength of output: 448
Script:
#!/bin/bash # Correcting the command to search for any usage of SetStatus without the --go flag. rg 'SetStatus'Length of output: 11123
pkg/policies/flowcontrol/actuators/concurrency-limiter/concurrency-limiter.go (8)
- 24-26: Imports for
jobsandlabelstatushave been added, while the import forerrorshas been removed. Ensure that these changes are necessary and that the removed import is not used elsewhere in the file.Verification successful
The verification scripts have confirmed that the
errorspackage is not used withinconcurrency-limiter.go, aligning with the removal of its import. Additionally, the new imports forjobsandlabelstatusare utilized within the file, as evidenced by references tojobs.JobGroup,labelstatus.LabelStatusFactory, andlabelstatus.LabelStatusin the provided output. This confirms the necessity of the changes and the correct removal of the unused import.* 76-83: The `concurrencyLimiterFactory` struct now includes `labelStatusJobGroup` and `labelStatusFactory` fields. Confirm that these new fields are properly initialized and used within the factory's methods.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining usage of the `errors` package. rg --type go 'errors\.' pkg/policies/flowcontrol/actuators/concurrency-limiter/concurrency-limiter.go # Verify that the new imports are utilized within the file. rg --type go '(jobs|labelstatus)\.' pkg/policies/flowcontrol/actuators/concurrency-limiter/concurrency-limiter.goLength of output: 626
Verification successful
The verification process has confirmed that both
labelStatusJobGroupandlabelStatusFactoryfields within theconcurrencyLimiterFactorystruct are not only properly initialized but also actively used within the factory's methods. ThelabelStatusJobGroupis involved in operations such as starting and stopping job groups, as well as being passed to other components for setup. Similarly,labelStatusFactoryis utilized to create new instances related to label status management. This demonstrates that the addition of these fields is functional and integrated into the broader logic of the concurrency limiter's implementation.* 96-96: The `setupConcurrencyLimiterFactory` function now accepts a `labelStatusFactory` parameter. Verify that this parameter is correctly passed and utilized within the function. * 113-117: Creation of a `labelStatusJobGroup` within `setupConcurrencyLimiterFactory`. Ensure error handling is consistent and that the job group is correctly utilized for label status management. * 201-202: Initialization of `tokensLabelKeyStatus` and `limitByLabelKeyStatus` within the `concurrencyLimiter` struct. Confirm that these label statuses are correctly set up and updated based on limiter decisions. * 254-256: The setup calls for `tokensLabelKeyStatus` and `limitByLabelKeyStatus` within the `setup` function. Ensure that these label statuses are correctly integrated into the concurrency limiter's lifecycle. * 333-339: The decision logic now considers `tokensLabelKey` from request labels. Verify that the `tokensLabelKeyStatus` is correctly updated based on the presence or absence of the `tokensLabelKey` in the request labels.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Ensure that labelStatusJobGroup and labelStatusFactory are initialized and used. ast-grep --lang go --pattern $'concurrencyLimiterFactory { $$$, labelStatusJobGroup: $_, labelStatusFactory: $_, $$$ }'Length of output: 1322
Script:
#!/bin/bash # Search for usages of labelStatusJobGroup and labelStatusFactory within concurrencyLimiterFactory methods rg "labelStatusJobGroup" ./pkg/policies/flowcontrol/actuators/concurrency-limiter/ rg "labelStatusFactory" ./pkg/policies/flowcontrol/actuators/concurrency-limiter/Length of output: 2207
Verification successful
Given the absence of output from the
ast-grepcommand searching fortokensLabelKeyStatus.SetMissing($$$), it suggests that either the methodSetMissingis not called in the context oftokensLabelKeyStatuswithin the Go codebase, or the pattern used in theast-grepcommand did not match the actual implementation due to syntax variations or inaccuracies in the pattern.To ensure a thorough verification, it's prudent to adjust the search strategy to capture any potential usage of
SetMissingrelated totokensLabelKeyStatuswithout relying on the specific pattern that might have been too restrictive or incorrect. Additionally, verifying the presence oftokensLabelKeyStatusand its updates in a broader context could provide a clearer picture of howtokensLabelKeyinfluences its status.
The attempt to use
rgwith an incorrect flag (--go) resulted in an error, preventing the retrieval of relevant information abouttokensLabelKeyStatus. To correct this and effectively search for references totokensLabelKeyStatusin the Go codebase, the correct approach is to remove the incorrect flag and execute the search again.
The search results confirm that
tokensLabelKeyStatusis indeed updated based on the presence or absence of thetokensLabelKeyin various components of the codebase, including theconcurrency-limiter. Specifically, the methodSetMissing()is called ontokensLabelKeyStatusin the context of theconcurrency-limiteras well as in other related components (workload-scheduler,rate-limiter, etc.), indicating that the logic for handling the absence oftokensLabelKeyis implemented as described.Based on this evidence, the review comment regarding the verification of
tokensLabelKeyStatusbeing correctly updated based on the presence or absence of thetokensLabelKeyin request labels is confirmed to be accurate within the provided context.* 475-476: The `getLimitLabelFromLabels` function now updates `limitByLabelKeyStatus` if the `limitByLabelKey` is not found in the labels. Confirm that this status update is correctly implemented.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for correct updates to tokensLabelKeyStatus based on tokensLabelKey presence. ast-grep --lang go --pattern $'tokensLabelKeyStatus.SetMissing($$$)'Length of output: 69
Script:
#!/bin/bash # Broaden the search to find any references to tokensLabelKeyStatus and its method calls. rg "tokensLabelKeyStatus" --goLength of output: 459
Script:
#!/bin/bash # Correcting the search for any references to tokensLabelKeyStatus and its method calls without the incorrect flag. rg "tokensLabelKeyStatus"Length of output: 2916
pkg/policies/flowcontrol/actuators/rate-limiter/rate-limiter.go (7)
- 28-28: The import of the
labelstatuspackage aligns with the new functionality for label status management.- 86-87: The addition of
labelStatusJobGroupandlabelStatusFactoryfields to therateLimiterFactorystruct is necessary for supporting label status management.- 100-100: The inclusion of
labelStatusFactoryas a parameter insetupRateLimiterFactoryis appropriate for initializing label status management within the rate limiter.- 125-129: The initialization of
labelStatusJobGroupwithinsetupRateLimiterFactoryis correctly implemented, ensuring label status tracking is set up properly.- 255-256: The addition of
tokensLabelKeyStatusandlimitByLabelKeyStatusfields to therateLimiterstruct supports the new label status tracking functionality.- 385-391: The modifications in the
Decidemethod to handle missing label values usingtokensLabelKeyStatus.SetMissing()are correctly implemented, enhancing the label handling logic.- 462-462: The update to
takeIfAvailableto handle missing label values withlimitByLabelKeyStatus.SetMissing()is correctly applied, ensuring consistent label status management.pkg/policies/flowcontrol/actuators/concurrency-scheduler/concurrency-scheduler.go (6)
- 26-26: The import of the
labelstatuspackage is necessary for the new label status management functionality in the concurrency scheduler.- 82-83: The addition of
labelStatusJobGroupandlabelStatusFactoryfields to theconcurrencySchedulerFactorystruct is essential for supporting label status management.- 97-97: Including
labelStatusFactoryas a parameter insetupConcurrencySchedulerFactorycorrectly integrates label status management into the concurrency scheduler setup.- 116-120: The initialization of
labelStatusJobGroupwithinsetupConcurrencySchedulerFactoryis properly implemented, ensuring label status tracking is set up correctly.- 237-241: The addition of label status fields like
limitByLabelKeyStatusandtokensLabelKeyStatusto theconcurrencySchedulerstruct supports the new label status tracking functionality effectively.- 268-273: The setup of label status instances within the
setupmethod ofconcurrencyScheduleris correctly implemented, ensuring proper label status management.pkg/policies/flowcontrol/actuators/quota-scheduler/quota-scheduler.go (5)
- 28-28: The import of the
labelstatuspackage is necessary for the new label status management functionality in the quota scheduler.- 85-86: The addition of
labelStatusJobGroupandlabelStatusFactoryfields to thequotaSchedulerFactorystruct is essential for supporting label status management.- 100-100: Including
labelStatusFactoryas a parameter insetupQuotaSchedulerFactorycorrectly integrates label status management into the quota scheduler setup.- 125-129: The initialization of
labelStatusJobGroupwithinsetupQuotaSchedulerFactoryis properly implemented, ensuring label status tracking is set up correctly.- 256-259: The addition of label status fields like
tokensLabelKeyStatusandpriorityLabelKeyStatusto thequotaSchedulerstruct supports the new label status tracking functionality effectively.pkg/policies/flowcontrol/actuators/load-scheduler/load-scheduler.go (5)
- 22-24: The addition of imports for
jobsandlabelstatuspackages aligns with the new functionality introduced in theloadSchedulerFactoryandloadSchedulerstructs for managing label statuses. This change is necessary for the integration of label status management within the load scheduler component.- 83-84: The addition of
labelStatusJobGroupandlabelStatusFactoryfields to theloadSchedulerFactorystruct is a significant enhancement. It enables the load scheduler to manage job groups and label statuses, which is crucial for the new policy alert system. This change aligns with the PR's objective to enhance policy alert capabilities and status handling mechanisms across the system.- 187-190: Starting the
labelStatusJobGroupin theOnStartlifecycle hook is essential for ensuring that the job group is operational before the load scheduler begins processing. This ensures that label status management is active and ready to handle updates as soon as the load scheduler starts, aligning with the system's enhanced policy alert capabilities.- 207-210: Stopping the
labelStatusJobGroupin theOnStoplifecycle hook is crucial for gracefully shutting down the job group when the load scheduler component is stopped. This ensures that all label status management activities are cleanly terminated, preventing potential resource leaks or incomplete status updates.- 264-269: The instantiation of
LabelStatusobjects for various label keys (tokensLabelKeyStatus,priorityLabelKeyStatus,workloadLabelKeyStatus,fairnessLabelKeyStatus) within theloadSchedulerstruct is a key enhancement. It enables detailed status management for different aspects of the load scheduling process, such as token allocation and priority handling. This addition significantly improves the system's ability to monitor and respond to policy-related conditions in a granular manner.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- pkg/labelstatus/labelstatus.go (1 hunks)
- pkg/policies/flowcontrol/actuators/concurrency-scheduler/concurrency-scheduler.go (14 hunks)
- pkg/policies/flowcontrol/provide.go (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- pkg/labelstatus/labelstatus.go
- pkg/policies/flowcontrol/actuators/concurrency-scheduler/concurrency-scheduler.go
Additional comments: 2
pkg/policies/flowcontrol/provide.go (2)
- 7-7: The addition of the
labelstatusimport aligns with the PR's objective to enhance policy alerts and status management across the system. Ensure that thelabelstatusmodule's functionalities are utilized effectively within this package.- 26-26: The inclusion of
labelstatus.Module()in theModulefunction'sfx.Optionsis a crucial step for integrating label status management into the flow control policies. This change ensures that the label status functionalities are available for the flow control components, supporting the PR's enhancements for policy alerts and dynamic configuration.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- pkg/labelstatus/labelstatus.go (1 hunks)
- pkg/policies/flowcontrol/actuators/workload-scheduler/scheduler.go (7 hunks)
Files skipped from review as they are similar to previous changes (2)
- pkg/labelstatus/labelstatus.go
- pkg/policies/flowcontrol/actuators/workload-scheduler/scheduler.go
Description of change
Checklist
Summary by CodeRabbit
DynamicConfigUpdatemethod for more efficient configuration updates.