feat(flow): Add event rule domain model#3881
Conversation
Summary by CodeRabbit
WalkthroughIntroduces the event-rule domain model, validation helpers, typed actions, policy and rule validation, task operation-code checks, and a built-in leakage rule that submits a force power-off task. ChangesEvent Rule Domain
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant LeakageRule
participant EventRule
participant Policy
participant SubmitTask
LeakageRule->>EventRule: construct enabled leakage rule
EventRule->>Policy: embed one action
Policy->>SubmitTask: validate force power-off operation
SubmitTask-->>Policy: return validation result
Policy-->>EventRule: return policy validation result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🔐 TruffleHog Secret Scan✅ No secrets or credentials found! Your code has been scanned for 700+ types of secrets and credentials. All clear! 🎉 🕐 Last updated: 2026-07-23 00:29:56 UTC | Commit: c077f76 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
rest-api/flow/internal/eventrule/event_test.go (1)
14-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse explicit test cases for payload validation.
Keep valid and invalid payloads as independent table entries instead of mutating one envelope. As per coding guidelines, “Write tests in table-driven style; group parser, validator, conversion, and similar input variants using the repository's scenario or explicit-case helpers where appropriate.”
Proposed refactor
func TestEnvelopeValidatePayload(t *testing.T) { - envelope := Envelope{ - ID: uuid.New(), - Type: "test.event", - Resource: Resource{Kind: ResourceKindRack}, - Payload: json.RawMessage(`{"value":42}`), + tests := map[string]struct { + payload json.RawMessage + wantErr string + }{ + "valid JSON": {payload: json.RawMessage(`{"value":42}`)}, + "invalid JSON": {payload: json.RawMessage(`{"value":`)}, wantErr: "payload must be valid JSON"}, } - require.NoError(t, envelope.Validate()) - envelope.Payload = json.RawMessage(`{"value":`) - require.ErrorContains(t, envelope.Validate(), "payload must be valid JSON") + for name, test := range tests { + t.Run(name, func(t *testing.T) { + envelope := Envelope{ + ID: uuid.New(), + Type: "test.event", + Resource: Resource{Kind: ResourceKindRack}, + Payload: test.payload, + } + if test.wantErr != "" { + require.ErrorContains(t, envelope.Validate(), test.wantErr) + return + } + require.NoError(t, envelope.Validate()) + }) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/flow/internal/eventrule/event_test.go` around lines 14 - 25, Refactor TestEnvelopeValidatePayload into explicit table-driven test cases, with separate valid and invalid payload entries rather than mutating a shared envelope. Keep the existing validation assertions and expected invalid-payload error message, using the repository’s established scenario or explicit-case helper if available.Source: Coding guidelines
rest-api/flow/internal/eventrule/leakage/leakage_test.go (1)
21-25: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin the operation code/type in the assertion.
The test verifies
TargetStrategybut notOperationType/OperationCode. Given this rule submits a forced hardware power-off in response to a leak, a regression that silently downgrades the operation (e.g., to a graceful power-off) would slip through undetected.✅ Proposed strengthening
spec, ok := rule.Actions[0].Spec.(eventrule.SubmitTask) require.True(t, ok) assert.Equal(t, eventrule.TargetStrategyAffectedComponents, spec.TargetStrategy) + assert.Equal(t, taskcommon.TaskTypePowerControl, spec.OperationType) + assert.Equal(t, taskcommon.OpCodePowerControlForcePowerOff, spec.OperationCode)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/flow/internal/eventrule/leakage/leakage_test.go` around lines 21 - 25, Strengthen the assertion block for the SubmitTask spec in the leakage rule test by verifying the expected forced hardware power-off OperationType/OperationCode in addition to TargetStrategy. Use the existing eventrule constants for that operation and preserve the current type and action-count assertions.rest-api/flow/internal/eventrule/leakage/leakage.go (1)
32-38: 🩺 Stability & Availability | 🔵 TrivialConfirm queue latency is acceptable for a safety-critical fallback.
The built-in leakage response uses
ConflictStrategyQueuefor its force-power-off task. SinceConflictStrategycurrently only offersqueueorreject(Lines 76-88 of action.go) with no priority/preemption option, a conflicting in-flight task would delay this emergency shutdown rather than pre-empt it.Rejectwould be worse (the safety action is dropped entirely), soQueueis the right choice today — but it's worth confirming with the Flow task-orchestration layer that queued conflict resolution doesn't introduce unacceptable delay for leak-driven power-offs, or whether a future priority/pre-emption strategy is needed.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/flow/internal/eventrule/leakage/leakage.go` around lines 32 - 38, Validate with the Flow task-orchestration layer that ConflictStrategyQueue in the leakage response’s SubmitTask preserves acceptable latency for the force-power-off safety action during conflicts. Keep the current queue strategy unless orchestration requirements mandate a supported priority or pre-emption strategy; do not switch to ConflictStrategyReject, which would drop the shutdown.rest-api/flow/internal/eventrule/action.go (1)
21-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated "non-nil but empty" slice guard.
The empty-slice check is duplicated verbatim for
Severities(Lines 22-24) andComponentTypes(Lines 36-38). A small generic helper would remove the duplication and make it trivial to apply the same rule to future condition fields.♻️ Proposed refactor
+func validateNonEmptyIfSet[T any](name string, values []T) error { + if values != nil && len(values) == 0 { + return fmt.Errorf("%s cannot be an empty array", name) + } + return nil +} + func (c ActionCondition) validate() error { - if c.Severities != nil && len(c.Severities) == 0 { - return fmt.Errorf("severities cannot be an empty array") + if err := validateNonEmptyIfSet("severities", c.Severities); err != nil { + return err } ... - if c.ComponentTypes != nil && len(c.ComponentTypes) == 0 { - return fmt.Errorf("component_types cannot be an empty array") + if err := validateNonEmptyIfSet("component_types", c.ComponentTypes); err != nil { + return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/flow/internal/eventrule/action.go` around lines 21 - 47, In ActionCondition.validate, extract the duplicated non-nil-but-empty slice check for Severities and ComponentTypes into a small generic helper, then reuse it for both fields while preserving their existing field-specific error messages and validation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rest-api/flow/internal/eventrule/action.go`:
- Around line 21-47: In ActionCondition.validate, extract the duplicated
non-nil-but-empty slice check for Severities and ComponentTypes into a small
generic helper, then reuse it for both fields while preserving their existing
field-specific error messages and validation behavior.
In `@rest-api/flow/internal/eventrule/event_test.go`:
- Around line 14-25: Refactor TestEnvelopeValidatePayload into explicit
table-driven test cases, with separate valid and invalid payload entries rather
than mutating a shared envelope. Keep the existing validation assertions and
expected invalid-payload error message, using the repository’s established
scenario or explicit-case helper if available.
In `@rest-api/flow/internal/eventrule/leakage/leakage_test.go`:
- Around line 21-25: Strengthen the assertion block for the SubmitTask spec in
the leakage rule test by verifying the expected forced hardware power-off
OperationType/OperationCode in addition to TargetStrategy. Use the existing
eventrule constants for that operation and preserve the current type and
action-count assertions.
In `@rest-api/flow/internal/eventrule/leakage/leakage.go`:
- Around line 32-38: Validate with the Flow task-orchestration layer that
ConflictStrategyQueue in the leakage response’s SubmitTask preserves acceptable
latency for the force-power-off safety action during conflicts. Keep the current
queue strategy unless orchestration requirements mandate a supported priority or
pre-emption strategy; do not switch to ConflictStrategyReject, which would drop
the shutdown.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 08910621-e569-4a92-bf57-e93403c25177
📒 Files selected for processing (15)
rest-api/flow/internal/eventrule/action.gorest-api/flow/internal/eventrule/action_test.gorest-api/flow/internal/eventrule/doc.gorest-api/flow/internal/eventrule/event.gorest-api/flow/internal/eventrule/event_test.gorest-api/flow/internal/eventrule/leakage/leakage.gorest-api/flow/internal/eventrule/leakage/leakage_test.gorest-api/flow/internal/eventrule/policy.gorest-api/flow/internal/eventrule/rule.gorest-api/flow/internal/eventrule/validation.gorest-api/flow/internal/task/common/operation_codes.gorest-api/flow/internal/task/common/operation_codes_test.gorest-api/flow/internal/task/operations/operations.gorest-api/flow/pkg/types/enums.gorest-api/flow/pkg/types/enums_test.go
| type OperationCode string | ||
|
|
||
| // Inject-expectation operation codes. | ||
| const OpCodeInjectExpectation = "inject_expectation" |
There was a problem hiding this comment.
Would better to make the type restriction stronger:
const OpCodeInjectExpectation OperationCode = "inject_expectation"
const (
OpCodePowerControlPowerOn OperationCode = "power_on"
...
)
There was a problem hiding this comment.
Don't enforce the type since the current operation codes are defined as string. Don't want to touch other codes in this PR. Will leave this enforcement and refactor of existing codes in a future PR.
| OperationType: taskcommon.TaskTypePowerControl, | ||
| OperationCode: taskcommon.OpCodePowerControlForcePowerOff, | ||
| TargetStrategy: eventrule.TargetStrategyAffectedComponents, | ||
| ConflictStrategy: eventrule.ConflictStrategyQueue, |
There was a problem hiding this comment.
In the current implementation, there are only two options: queue and reject. When a leak is detected, it chooses queue, meaning it waits for the current task to finish. Should we consider a more advanced approach where the current task should be interrupted immediately instead?
There was a problem hiding this comment.
Yes if achievable. This PR only lays out the foundation for event processing. This kind of fine-tunes and improvements will be done in the future after we complete the framework.
| return ActionTypeSendAlert | ||
| } | ||
|
|
||
| func (s SendAlert) validate() error { |
There was a problem hiding this comment.
SendAlert allows an empty severity, but ActionCondition rejects it — intentional, or should SendAlert.validate() require a severity too?
There was a problem hiding this comment.
This is intentional. SubmitTask is much heavier than SendAlert, so we don't allow users to define a rule to trigger SubmitTask for an event with unknown severity. For SendAlert, it is OK.
kunzhao-nv
left a comment
kunzhao-nv
left a comment
There was a problem hiding this comment.
Leaving #3881 (comment) as a TODO
- Introduce the focused internal/eventrule domain for event envelopes, resources, rules, policies, deduplication, typed actions, action conditions, and named target strategies. - Add immutable leakage defaults using the new rule model. Share component-type validation and define typed task operation codes with task-type-specific validation, including the inject-expectation operation code. - Add unit coverage for event-rule validation, leakage defaults, component types, and operation-code compatibility.
c077f76 to
7cb0c4c
Compare
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3881.docs.buildwithfern.com/infra-controller |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rest-api/flow/pkg/types/enums.go (1)
25-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsolidate the component-type validation contract.
ComponentType.Validate()duplicates the allowlist inrest-api/flow/internal/eventrule/event.goLines 26-38. Because these are distinct package types, a future component type could pass validation here but fail during event normalization. Make one type/constant set authoritative, or centralize validation and convert explicitly at the package boundary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@rest-api/flow/pkg/types/enums.go` around lines 25 - 38, Consolidate the supported component-type allowlist used by ComponentType.Validate and event normalization in rest-api/flow/internal/eventrule/event.go so only one definition is authoritative. Update the other validation path to reuse that authority, adding an explicit conversion at the package boundary where the types differ, and ensure newly supported types cannot pass one validator while failing the other.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@rest-api/flow/pkg/types/enums.go`:
- Around line 25-38: Consolidate the supported component-type allowlist used by
ComponentType.Validate and event normalization in
rest-api/flow/internal/eventrule/event.go so only one definition is
authoritative. Update the other validation path to reuse that authority, adding
an explicit conversion at the package boundary where the types differ, and
ensure newly supported types cannot pass one validator while failing the other.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 07bce9b0-6fdf-47cc-bd27-90c52d31e376
📒 Files selected for processing (15)
rest-api/flow/internal/eventrule/action.gorest-api/flow/internal/eventrule/action_test.gorest-api/flow/internal/eventrule/doc.gorest-api/flow/internal/eventrule/event.gorest-api/flow/internal/eventrule/event_test.gorest-api/flow/internal/eventrule/leakage/leakage.gorest-api/flow/internal/eventrule/leakage/leakage_test.gorest-api/flow/internal/eventrule/policy.gorest-api/flow/internal/eventrule/rule.gorest-api/flow/internal/eventrule/validation.gorest-api/flow/internal/task/common/operation_codes.gorest-api/flow/internal/task/common/operation_codes_test.gorest-api/flow/internal/task/operations/operations.gorest-api/flow/pkg/types/enums.gorest-api/flow/pkg/types/enums_test.go
🚧 Files skipped from review as they are similar to previous changes (13)
- rest-api/flow/internal/eventrule/rule.go
- rest-api/flow/internal/eventrule/action_test.go
- rest-api/flow/internal/task/operations/operations.go
- rest-api/flow/internal/eventrule/validation.go
- rest-api/flow/internal/eventrule/leakage/leakage.go
- rest-api/flow/internal/eventrule/event_test.go
- rest-api/flow/pkg/types/enums_test.go
- rest-api/flow/internal/task/common/operation_codes_test.go
- rest-api/flow/internal/task/common/operation_codes.go
- rest-api/flow/internal/eventrule/policy.go
- rest-api/flow/internal/eventrule/event.go
- rest-api/flow/internal/eventrule/action.go
- rest-api/flow/internal/eventrule/leakage/leakage_test.go
2 participants
Flow needs to respond to operational events such as hardware coolant leaks, so we
add a new feature for the users to define rules to handle various types of events.
This PR is the first one for the new feature, it introduces a small, typed "eventrule"
domain as the foundation for event processing.
Related issues
Type of Change
Breaking Changes
Testing
Additional Notes