Update rule oom handler

[Tzvonimir]

[Title]

📚 Description of Changes

Provide an overview of your changes and why they’re needed. Link to any related issues (e.g., "Fixes #123"). If your PR fixes a bug, resolves a feature request, or updates documentation, please explain how.

• What Changed:
  (Describe the modifications, additions, or removals.)

• Why This Change:
  (Explain the problem this PR addresses or the improvement it provides.)

• Affected Components:
  (Which component does this change affect? - put x for all components)

• Compose

• K8s

• Other (please specify)

❓ Motivation and Context

Why is this change required? What problem does it solve?

• Context:
  (Provide background information or link to related discussions/issues.)

• Relevant Tasks/Issues:
  (e.g., Fixes: #GitHub Issue)

🔍 Types of Changes

Indicate which type of changes your code introduces (check all that apply):

• BUGFIX: Non-breaking fix for an issue.
• NEW FEATURE: Non-breaking addition of functionality.
• BREAKING CHANGE: Fix or feature that causes existing functionality to not work as expected.
• ENHANCEMENT: Improvement to existing functionality.
• CHORE: Changes that do not affect production (e.g., documentation, build tooling, CI).

🔬 QA / Verification Steps

Describe the steps a reviewer should take to verify your changes:

1. (Step one: e.g., "Run make test to verify all tests pass.")
2. (Step two: e.g., "Deploy to a Kind cluster with make create-kind && make deploy.")
3. (Additional steps as needed.)

✅ Global Checklist

Please check all boxes that apply:

• I have read and followed the CONTRIBUTING guidelines.
• My code follows the code style of this project.
• I have updated the documentation as needed.
• I have added tests that cover my changes.
• All new and existing tests have passed locally.
• I have run this code in a local environment to verify functionality.
• I have considered the security implications of this change.

[gitar-bot]

⚠️ Bug: Race: concurrent Stop() calls can double-close batchChan

The Stop() method can be called concurrently from the background goroutine (lines 127-136) and from an external caller. The batchChan nil-check-and-close at lines 187-191 is not protected by a mutex, so two goroutines can both see c.batchChan != nil and both attempt close(c.batchChan), causing a panic: close of closed channel.

Additionally, the informer's event handler (line 150) can still be executing c.batchChan <- ... after Stop() closes the channel, causing a panic: send on closed channel. There's no synchronization between the informer shutdown (async via stopCh) and the immediate close(c.batchChan).

Impact: Under graceful shutdown or context cancellation, the collector can panic and crash the process.

Suggested fix: Add a sync.Once to protect the channel close, or use a mutex around both the send and close paths. For example:

type WorkloadRuleCollector struct {
    // ... existing fields
    stopOnce sync.Once
}

func (c *WorkloadRuleCollector) Stop() error {
    c.stopOnce.Do(func() {
        close(c.stopCh)
        // wait briefly for informer handlers to drain
        close(c.batchChan)
    })
    if c.batcher != nil {
        c.batcher.stop()
    }
    return nil
}

Note: The WorkloadRecommendationCollector has the same issue, but that's pre-existing.

_{Was this helpful? React with 👍 / 👎}

[gitar-bot]

🔍 CI failure analysis for 3fddc6c: The build-installer CI check failed because dist/install.yaml was not regenerated after RBAC changes. Running make build-installer locally and committing the result will fix this.

Issue

The CI job failed because running make build-installer produced changes to dist/install.yaml that weren't committed to the repository.

Root Cause

The PR adds new RBAC permissions for the WorkloadRule CRD in config/rbac/role.yaml. The make build-installer command uses kustomize to generate dist/install.yaml from source files including the RBAC configuration. When the RBAC changes were committed, the corresponding regenerated dist/install.yaml was not updated and committed.

Details

The CI detected this uncommitted line in dist/install.yaml:

+  - workloadrules

This corresponds to the new RBAC rule added in config/rbac/role.yaml for the workloadrules resource under the dakr.devzero.io API group. The repository requires that all generated files (like dist/install.yaml) be kept in sync with their source files and committed together.

Code Review ⚠️ Changes requested 1 resolved / 3 findings

The new WorkloadRuleCollector has a race condition in its shutdown goroutine that diverges from the WorkloadRecommendationCollector template — the <-stopCh branch can re-enter Stop(), and concurrent Stop() calls still lack mutex protection for batchChan close.

⚠️ Bug: Goroutine can re-enter Stop() causing double-close of batchChan

📄 internal/collector/workload_rule_collector.go:126

The background goroutine in Start() (lines 127-136) differs from the template WorkloadRecommendationCollector in a problematic way. When stopCh is closed by an external Stop() call, the goroutine checks ctx.Err() != nil and calls Stop() again (line 133). This creates a guaranteed re-entrant call path:

1. External caller invokes Stop() → closes stopCh, closes batchChan, sets it to nil
2. Goroutine receives on <-stopCh (line 131)
3. If context is also cancelled (ctx.Err() != nil), goroutine calls Stop() again (line 133)
4. Second Stop() reads c.batchChan as nil (set in step 1), skips the close — this specific path is safe

However, there's a race window: if Stop() is called externally and context cancels simultaneously, both the goroutine's ctx.Done() branch and the external caller run Stop() concurrently. Neither call is protected by a mutex, so both can see batchChan != nil, and both can attempt close(batchChan), causing a panic.

The WorkloadRecommendationCollector avoids this by simply returning when stopCh fires (line 154-155 of that file), without calling Stop() again. The fix is to either:

1. Match the template pattern: remove the re-entrant Stop() call on the <-stopCh branch
2. Add a sync.Once to guard the teardown logic in Stop()

Suggested fix

	stopCh := c.stopCh
	go func() {
		select {
		case <-ctx.Done():
			_ = c.Stop()
		case <-stopCh:
			// Channel was closed by Stop() method
		}
	}()

⚠️ Bug: Race: concurrent Stop() calls can double-close batchChan

📄 internal/collector/workload_rule_collector.go:176

The Stop() method can be called concurrently from the background goroutine (lines 127-136) and from an external caller. The batchChan nil-check-and-close at lines 187-191 is not protected by a mutex, so two goroutines can both see c.batchChan != nil and both attempt close(c.batchChan), causing a panic: close of closed channel.

Additionally, the informer's event handler (line 150) can still be executing c.batchChan <- ... after Stop() closes the channel, causing a panic: send on closed channel. There's no synchronization between the informer shutdown (async via stopCh) and the immediate close(c.batchChan).

Impact: Under graceful shutdown or context cancellation, the collector can panic and crash the process.

Suggested fix: Add a sync.Once to protect the channel close, or use a mutex around both the send and close paths. For example:

type WorkloadRuleCollector struct {
    // ... existing fields
    stopOnce sync.Once
}

func (c *WorkloadRuleCollector) Stop() error {
    c.stopOnce.Do(func() {
        close(c.stopCh)
        // wait briefly for informer handlers to drain
        close(c.batchChan)
    })
    if c.batcher != nil {
        c.batcher.stop()
    }
    return nil
}

Note: The WorkloadRecommendationCollector has the same issue, but that's pre-existing.

✅ 1 resolved

✅ Edge Case: oomEventsChanged misses mutations when event count is unchanged

📄 internal/collector/workload_rule_collector.go:165
The oomEventsChanged method only compares len(oldEvents) != len(newEvents). This means if an OOM event is removed and a different one is added in the same update (count stays the same), or if an existing event's fields are modified, the change is silently dropped.

While the 60-second resync partially mitigates this (it re-sends if any OOM events exist), there's a window where the control plane has stale data. More importantly, if the CRD controller ever implements event rotation (removing old events while adding new ones to cap the list), this logic would permanently miss those updates until a resync.

Consider using reflect.DeepEqual for a content comparison, consistent with how workloadRecommendationChanged compares status maps in the sibling collector:

func (c *WorkloadRuleCollector) oomEventsChanged(oldWR, newWR *unstructured.Unstructured) bool {
	oldEvents, _, _ := unstructured.NestedSlice(oldWR.Object, "status", "oomEvents")
	newEvents, _, _ := unstructured.NestedSlice(newWR.Object, "status", "oomEvents")
	return !reflect.DeepEqual(oldEvents, newEvents)
}

This is cheap for small slices and catches all mutations.

Tip

Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on

Options

Auto-apply is off → Gitar will not commit updates to this branch.
Display: compact → Showing less information.

Comment with these commands to change:

Auto-apply	Compact
gitar auto-apply:on	gitar display:verbose

_{Was this helpful? React with 👍 / 👎 | Gitar}

[gitar-bot]

⚠️ Bug: Goroutine can re-enter Stop() causing double-close of batchChan

The background goroutine in Start() (lines 127-136) differs from the template WorkloadRecommendationCollector in a problematic way. When stopCh is closed by an external Stop() call, the goroutine checks ctx.Err() != nil and calls Stop() again (line 133). This creates a guaranteed re-entrant call path:

1. External caller invokes Stop() → closes stopCh, closes batchChan, sets it to nil
2. Goroutine receives on <-stopCh (line 131)
3. If context is also cancelled (ctx.Err() != nil), goroutine calls Stop() again (line 133)
4. Second Stop() reads c.batchChan as nil (set in step 1), skips the close — this specific path is safe

However, there's a race window: if Stop() is called externally and context cancels simultaneously, both the goroutine's ctx.Done() branch and the external caller run Stop() concurrently. Neither call is protected by a mutex, so both can see batchChan != nil, and both can attempt close(batchChan), causing a panic.

The WorkloadRecommendationCollector avoids this by simply returning when stopCh fires (line 154-155 of that file), without calling Stop() again. The fix is to either:

1. Match the template pattern: remove the re-entrant Stop() call on the <-stopCh branch
2. Add a sync.Once to guard the teardown logic in Stop()

Suggested fix:

	stopCh := c.stopCh
	go func() {
		select {
		case <-ctx.Done():
			_ = c.Stop()
		case <-stopCh:
			// Channel was closed by Stop() method
		}
	}()

_{Was this helpful? React with 👍 / 👎}