TRT-2618: Add env var support to selectively enable resource monitor tests and event collection#31420
TRT-2618: Add env var support to selectively enable resource monitor tests and event collection#31420openshift-trt-agent[bot] wants to merge 7 commits into
Conversation
…tests and event collection Add ENABLE_RESOURCE_MONITOR_TESTS and ENABLE_RESOURCE_EVENT_COLLECTION environment variables to control resource watch functionality. Both default to disabled so resource watch remains off unless explicitly opted in. Jobs can enable one or both independently. ENABLE_RESOURCE_MONITOR_TESTS gates: pod-lifecycle, machine-lifecycle, node-lifecycle, and clusteroperator-collector monitors. ENABLE_RESOURCE_EVENT_COLLECTION gates: event-collector monitor. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use strconv.ParseBool instead of != "" so that setting the env var to "0", "false", or "FALSE" correctly disables the feature. - Group all resourceMonitorTestsEnabled() gated tests together in newUniversalMonitorTests instead of scattering them across the function. - Add test cases for "0", "1", and "FALSE" env var values. - Use t.Setenv for automatic cleanup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Apply ENABLE_RESOURCE_MONITOR_TESTS and ENABLE_RESOURCE_EVENT_COLLECTION env vars to the run-resourcewatch command as well, so the resource watch system is gated consistently with the monitor test framework. When neither env var is set, RunResourceWatch returns immediately without initializing git storage or starting watchers. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci
Bot
added
the
ready-for-human-review
|
@openshift-trt-agent[bot]: This pull request references TRT-2618 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
needs-ok-to-test
|
Hi @openshift-trt-agent[bot]. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: openshift-trt-agent[bot] The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughTwo environment variables independently gate resource monitor tests, watched resources, event collection, and resource-watch startup. Monitor and event resource selection are separated, with observer completion tracked for selected resources. ChangesResource observation feature gates
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| if resourceMonitorTestsEnabled() { | ||
| monitorTestRegistry.AddMonitorTestOrDie("pod-lifecycle", "Node / Kubelet", watchpods.NewPodWatcher()) | ||
| monitorTestRegistry.AddMonitorTestOrDie("machine-lifecycle", "Cluster-Lifecycle / machine-api", watchmachines.NewMachineWatcher()) | ||
| monitorTestRegistry.AddMonitorTestOrDie("clusteroperator-collector", "Test Framework", watchclusteroperators.NewOperatorWatcher()) | ||
| } |
There was a problem hiding this comment.
Can we get all the tests enabled by this marker in the same block? Is there a reason why
monitorTestRegistry.AddMonitorTestOrDie("node-lifecycle", "Node / Kubelet", watchnodes.NewNodeWatcher())
can't be in here too?
There was a problem hiding this comment.
Done — moved node-lifecycle into the resourceMonitorTestsEnabled() block in newUniversalMonitorTests so all four gated tests are together.
| const ( | ||
| EnvEnableResourceMonitorTests = "ENABLE_RESOURCE_MONITOR_TESTS" | ||
| EnvEnableResourceEventCollection = "ENABLE_RESOURCE_EVENT_COLLECTION" | ||
| ) |
There was a problem hiding this comment.
Is there a common place that these vars can live so they aren't duplicated?
There was a problem hiding this comment.
Done — removed the duplicated constants from types.go. The single source of truth is now observe.EnvEnableResourceMonitorTests and observe.EnvEnableResourceEventCollection in pkg/resourcewatch/observe/source.go. Both types.go and starter.go import from there. Also consolidated the strconv.ParseBool logic into a shared observe.ParseBoolEnv() helper that logs a warning for malformed values.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@pkg/defaultmonitortests/types.go`:
- Around line 82-89: Handle strconv.ParseBool errors consistently at
pkg/defaultmonitortests/types.go lines 82-89,
pkg/resourcewatch/observe/source.go lines 21-23, and
pkg/resourcewatch/operator/starter.go lines 20-24: surface or clearly log
malformed environment values before defaulting to disabled behavior, while
preserving intentional false handling. Add coverage for malformed boolean values
across the affected monitor registration, observation selection, and
run-resourcewatch paths.
In `@pkg/resourcewatch/observe/source_test.go`:
- Around line 91-100: Update TestEventResourcesContainOnlyEvents to compare
resources[0].GroupVersionResource against the existing eventsGVR value, rather
than checking only the Resource field. Keep the assertion that exactly one event
resource is returned.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: b2433bed-e0d9-4572-abb6-cbb2d645afaf
📒 Files selected for processing (5)
pkg/defaultmonitortests/types.gopkg/defaultmonitortests/types_test.gopkg/resourcewatch/observe/source.gopkg/resourcewatch/observe/source_test.gopkg/resourcewatch/operator/starter.go
…rove assertions - Move env var constants and ParseBool logic to a single ParseBoolEnv helper in pkg/resourcewatch/observe, removing duplication across types.go, source.go, and starter.go. Malformed values now log a warning instead of silently defaulting to false. - Move node-lifecycle registration into the resourceMonitorTestsEnabled block in newUniversalMonitorTests alongside the other gated tests. - Strengthen TestEventResourcesContainOnlyEvents to assert the full GroupVersionResource instead of just the Resource field. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/resourcewatch/operator/starter.go (1)
19-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the disabled-state log describe the evaluated flags.
The variables may be set to
falseor an invalid value, so “neither ... is set” is misleading. Use wording such as “neither feature is enabled” instead.Proposed fix
- klog.Infof("Resource watch disabled: neither %s nor %s is set", observe.EnvEnableResourceMonitorTests, observe.EnvEnableResourceEventCollection) + klog.Infof("Resource watch disabled: neither resource monitor tests nor resource event collection is enabled")🤖 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 `@pkg/resourcewatch/operator/starter.go` around lines 19 - 24, Update the disabled-state klog.Infof message in the starter initialization flow to describe the evaluated monitorEnabled and eventEnabled flags, using wording such as “neither feature is enabled” instead of implying the environment variables are unset. Preserve the existing condition and return 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.
Outside diff comments:
In `@pkg/resourcewatch/operator/starter.go`:
- Around line 19-24: Update the disabled-state klog.Infof message in the starter
initialization flow to describe the evaluated monitorEnabled and eventEnabled
flags, using wording such as “neither feature is enabled” instead of implying
the environment variables are unset. Preserve the existing condition and return
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e806705-1466-439a-931b-7981f288160f
📒 Files selected for processing (5)
pkg/defaultmonitortests/types.gopkg/defaultmonitortests/types_test.gopkg/resourcewatch/observe/source.gopkg/resourcewatch/observe/source_test.gopkg/resourcewatch/operator/starter.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/resourcewatch/observe/source_test.go
- pkg/defaultmonitortests/types_test.go
- pkg/resourcewatch/observe/source.go
…ed-state message Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Addressed the remaining CodeRabbit feedback in b9ff1a9:
|
|
/ok-to-test |
openshift-ci
Bot
added
ok-to-test
|
@openshift-trt-agent, looks like the |
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Fixed in 2ef5328 — |
|
We need to document this behavior. I think it should go in |
| configResource("proxies"), | ||
| configResource("schedulers"), | ||
|
|
||
| // operator resources provide low level details about how what operators are doing |
There was a problem hiding this comment.
you've removed a bunch of comments that you shouldn't have in code that wasn't even touched...
There was a problem hiding this comment.
Done — restored all the section-heading comments (// provide high level details of configuration..., // operator resources provide low level details..., // machine resources are required to..., etc.) that were inadvertently removed during the refactoring. Sorry about that.
|
Scheduling required tests: |
…ation Restores section-heading comments in the resource list that were inadvertently removed during refactoring, and adds documentation for the ENABLE_RESOURCE_MONITOR_TESTS and ENABLE_RESOURCE_EVENT_COLLECTION environment variable gating behavior to pkg/monitortests/README.md. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Done — added documentation for the |
|
Regarding the failing e2e jobs: the failures are unrelated to this PR. The |
|
@openshift-trt-agent[bot]: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Summary
TRT-2618: Add env var support to selectively enable resource monitor tests and event collection.
This PR adds two environment variables to control resource watch functionality, which is now disabled by default:
ENABLE_RESOURCE_MONITOR_TESTS— When set totrue/1, enables resource monitor tests (pod-lifecycle, machine-lifecycle, node-lifecycle, clusteroperator-collector) and resource watching in therun-resourcewatchcommand.ENABLE_RESOURCE_EVENT_COLLECTION— When set totrue/1, enables the event-collector monitor test and event watching in therun-resourcewatchcommand.Both default to disabled. Jobs can opt in to one or both independently. Values are parsed with
strconv.ParseBool, so"true","1","TRUE"all enable, while"false","0","FALSE", or unset all disable.Changes
pkg/defaultmonitortests/types.go: Gatepod-lifecycle,machine-lifecycle,node-lifecycle, andclusteroperator-collectorbehindENABLE_RESOURCE_MONITOR_TESTS; gateevent-collectorbehindENABLE_RESOURCE_EVENT_COLLECTION.pkg/resourcewatch/observe/source.go: Split resource list into monitor resources and event resources, filtered by the same env vars. Returns a no-op source when neither is set.pkg/resourcewatch/operator/starter.go: Early return fromRunResourceWatchwhen neither env var is set, avoiding unnecessary git storage initialization.pkg/defaultmonitortests/types_test.go: Table-driven tests for monitor test registration gating with various env var values.pkg/resourcewatch/observe/source_test.go: Table-driven tests for resource list filtering logic.Test plan
go vet ./...passesgo test ./pkg/resourcewatch/...— all 8 tests pass (5 existing + 3 new source tests)go test ./pkg/defaultmonitortests/...— all 8 test cases pass for env var gatingmake buildsucceedsJira
TRT-2618
Generated with Claude Code
Summary by CodeRabbit
ENABLE_RESOURCE_MONITOR_TESTSandENABLE_RESOURCE_EVENT_COLLECTIONto independently control resource monitor tests and event collection.eventsresource.