CARRY: refactor isopenshift check#128
Conversation
WalkthroughReplaces static OpenShift flag checks with runtime feature-flag utilities for routing decisions, adds OpenShift detection and routing utilities, and refactors existing OpenShift detection calls to use the new utilities across controllers. Changes
Sequence Diagram(s)sequenceDiagram
participant Reconciler as Reconciler\n(raycluster_controller)
participant Feature as ShouldCreateRoute()\n(utils)
participant OpenShift as IsOpenShiftCluster()\n(utils/discovery)
participant EnvVar as Environment\n(USE_INGRESS_ON_OPENSHIFT)
participant Route as OpenShift Route\nReconcile
participant Ingress as Kubernetes Ingress\nReconcile
Reconciler->>Feature: Ask ShouldCreateRoute()
Feature->>OpenShift: Call IsOpenShiftCluster()
OpenShift->>OpenShift: Query API groups
OpenShift-->>Feature: OpenShift? (true/false)
Feature->>EnvVar: Read USE_INGRESS_ON_OPENSHIFT
EnvVar-->>Feature: Env value
alt Feature returns true
Feature-->>Reconciler: true
Reconciler->>Route: reconcileRouteOpenShift()
else Feature returns false
Feature-->>Reconciler: false
Reconciler->>Ingress: reconcileIngress() (Kubernetes)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ray-operator/controllers/ray/raycluster_controller.go (1)
94-100: Confirmed:IsOpenShiftfield is unused and should be removed.The verification reveals that while
IsOpenShiftis initialized inmain.go:252, it is never accessed anywhere in the codebase (zero matches for.IsOpenShift). The refactored logic usesutils.ShouldCreateRoute()which independently callsIsOpenShiftCluster()andShouldUseIngressOnOpenShift(), making the field redundant. Remove theIsOpenShiftfield fromRayClusterReconcilerOptionsstruct and its initialization inmain.go:252to eliminate dead code.
🧹 Nitpick comments (3)
ray-operator/controllers/ray/raycluster_controller.go (1)
428-434: Good switch to centralized routing decision. Add traceability.Logic matches the PR intent. Consider logging which path was chosen and why (OpenShift detected + env override) to ease support.
Example:
logger.Info("Reconciling Ingress") +logger.Info("Routing strategy decision", + "isOpenShift", utils.IsOpenShiftCluster(), + "useIngressOnOpenShift", utils.ShouldUseIngressOnOpenShift()) if instance.Spec.HeadGroupSpec.EnableIngress == nil || !*instance.Spec.HeadGroupSpec.EnableIngress { return nil }ray-operator/controllers/ray/utils/openshift.go (1)
11-45: Tighten group match and avoid repeated discovery cost.
- Use equality (==) instead of Contains to prevent accidental matches.
- Cache detection with sync.Once to avoid creating a discovery client and calling ServerGroups on every reconcile.
-import ( +import ( "os" "strings" "k8s.io/client-go/discovery" ctrl "sigs.k8s.io/controller-runtime" + "sync" ) -// IsOpenShiftCluster detects if the cluster is OpenShift by checking for OpenShift-specific API groups -func IsOpenShiftCluster() bool { +var ( + ocpOnce sync.Once + ocpDetected bool +) + +// IsOpenShiftCluster detects if the cluster is OpenShift by checking for OpenShift-specific API groups +func IsOpenShiftCluster() bool { + ocpOnce.Do(func() { ocpDetected = detectOpenShift() }) + return ocpDetected +} + +func detectOpenShift() bool { // Check for OpenShift API groups config, err := ctrl.GetConfig() if err != nil || config == nil { return false } discoveryClient, err := discovery.NewDiscoveryClientForConfig(config) if err != nil || discoveryClient == nil { return false } apiGroups, err := discoveryClient.ServerGroups() - if err != nil { - return false - } + // If we got groups back, inspect them even if err != nil (partial discovery). + _ = err // Check for multiple OpenShift-specific API groups openshiftGroups := []string{ "route.openshift.io", "security.openshift.io", "config.openshift.io", } for _, group := range apiGroups.Groups { for _, openshiftGroup := range openshiftGroups { - if strings.Contains(group.Name, openshiftGroup) { + if group.Name == openshiftGroup { return true } } } return false }ray-operator/controllers/ray/utils/util.go (1)
673-677: Update call site at ray-operator/main.go:252 to use IsOpenShiftCluster() directly instead of the deprecated GetClusterType().The deprecation is correctly implemented with proper forwarding, but the remaining call site in
main.go:252should be migrated to the new function name as part of the deprecation cleanup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ray-operator/controllers/ray/raycluster_controller.go(1 hunks)ray-operator/controllers/ray/utils/openshift.go(1 hunks)ray-operator/controllers/ray/utils/util.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
ray-operator/controllers/ray/utils/util.go (1)
ray-operator/controllers/ray/utils/openshift.go (1)
IsOpenShiftCluster(12-45)
ray-operator/controllers/ray/utils/openshift.go (1)
ray-operator/controllers/ray/utils/constant.go (1)
USE_INGRESS_ON_OPENSHIFT(146-146)
ray-operator/controllers/ray/raycluster_controller.go (1)
ray-operator/controllers/ray/utils/openshift.go (1)
ShouldCreateRoute(53-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Build Apiserver and Docker Images
- GitHub Check: Lint (pre-commit)
- GitHub Check: Build security proxy Binaries and Docker Images
- GitHub Check: build
- GitHub Check: Build apiserversdk
- GitHub Check: Build Operator Binaries and Docker Images
- GitHub Check: Build Ray Kubectl plugin
🔇 Additional comments (1)
ray-operator/controllers/ray/utils/openshift.go (1)
52-55: Decision helper reads well.ShouldCreateRoute correctly combines detection + override.
| // ShouldUseIngressOnOpenShift determines if Ingress should be used instead of Route on OpenShift | ||
| func ShouldUseIngressOnOpenShift() bool { | ||
| return os.Getenv(USE_INGRESS_ON_OPENSHIFT) == "true" | ||
| } | ||
|
|
There was a problem hiding this comment.
Make env override case-insensitive.
Current check misses values like "TRUE"/"True". Use EqualFold.
-func ShouldUseIngressOnOpenShift() bool {
- return os.Getenv(USE_INGRESS_ON_OPENSHIFT) == "true"
-}
+func ShouldUseIngressOnOpenShift() bool {
+ return strings.EqualFold(os.Getenv(USE_INGRESS_ON_OPENSHIFT), "true")
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ShouldUseIngressOnOpenShift determines if Ingress should be used instead of Route on OpenShift | |
| func ShouldUseIngressOnOpenShift() bool { | |
| return os.Getenv(USE_INGRESS_ON_OPENSHIFT) == "true" | |
| } | |
| // ShouldUseIngressOnOpenShift determines if Ingress should be used instead of Route on OpenShift | |
| func ShouldUseIngressOnOpenShift() bool { | |
| return strings.EqualFold(os.Getenv(USE_INGRESS_ON_OPENSHIFT), "true") | |
| } |
🤖 Prompt for AI Agents
In ray-operator/controllers/ray/utils/openshift.go around lines 47 to 51, the
environment variable check for USE_INGRESS_ON_OPENSHIFT is case-sensitive and
misses values like "TRUE" or "True"; update the function to perform a
case-insensitive comparison (e.g., use
strings.EqualFold(os.Getenv(USE_INGRESS_ON_OPENSHIFT), "true")) and add the
strings import if it isn’t already imported.
b318b2a to
e21d131
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ray-operator/controllers/ray/networkpolicy_controller.go(1 hunks)ray-operator/controllers/ray/raycluster_controller.go(1 hunks)ray-operator/controllers/ray/utils/openshift.go(1 hunks)ray-operator/controllers/ray/utils/util.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ray-operator/controllers/ray/utils/util.go
- ray-operator/controllers/ray/utils/openshift.go
🧰 Additional context used
🧬 Code graph analysis (2)
ray-operator/controllers/ray/networkpolicy_controller.go (1)
ray-operator/controllers/ray/utils/openshift.go (1)
IsOpenShiftCluster(12-45)
ray-operator/controllers/ray/raycluster_controller.go (1)
ray-operator/controllers/ray/utils/openshift.go (1)
ShouldCreateRoute(53-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: helm-chart-verify-rbac
- GitHub Check: build
- GitHub Check: Build Operator Binaries and Docker Images
- GitHub Check: Build security proxy Binaries and Docker Images
- GitHub Check: Build apiserversdk
- GitHub Check: Build Ray Kubectl plugin
- GitHub Check: Lint (pre-commit)
- GitHub Check: Build Apiserver and Docker Images
|
|
||
| // On OpenShift, use stricter namespace detection | ||
| if r.isOpenShift() { | ||
| if utils.IsOpenShiftCluster() { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
LGTM: Centralized OpenShift detection.
Switching to utils.IsOpenShiftCluster() consolidates the OpenShift detection logic and eliminates code duplication.
Verify whether the RESTMapper field (line 31) is still needed after removing the local isOpenShift() helper:
If it's no longer used, consider removing it to reduce the controller's dependencies.
🏁 Script executed:
#!/bin/bash
# Search for RESTMapper usage in this file
rg -n "RESTMapper" ray-operator/controllers/ray/networkpolicy_controller.go -A3 -B3Length of output: 402
The output shows RESTMapper is declared and initialized, but I need to verify if it's actually used in any controller methods. Let me search more broadly:
🏁 Script executed:
#!/bin/bash
# Search for actual usage of RESTMapper in the file
rg -n "RESTMapper" ray-operator/controllers/ray/networkpolicy_controller.goLength of output: 68
🏁 Script executed:
#!/bin/bash
# Check full file for RESTMapper usage in method implementations
rg "\.RESTMapper|RESTMapper\s*," ray-operator/controllers/ray/networkpolicy_controller.go -nLength of output: 30
Remove unused RESTMapper field from controller struct.
Verification confirms RESTMapper (line 31) is never used in the controller—it's initialized at line 40 but not referenced anywhere. Remove it along with the initialization to clean up unused dependencies.
🤖 Prompt for AI Agents
In ray-operator/controllers/ray/networkpolicy_controller.go around line 104,
remove the unused RESTMapper field and its initialization: delete the RESTMapper
field declaration in the controller struct (around line 31), remove the code
that sets/initializes RESTMapper at line ~40, and update any constructor or New*
function signatures and assignments accordingly; also remove any now-unused
imports related to RESTMapper and run go build/go vet to ensure no remaining
references.
| if utils.ShouldCreateRoute() { | ||
| // Create OpenShift Route | ||
| return r.reconcileRouteOpenShift(ctx, instance) | ||
| } | ||
| // plain vanilla kubernetes - create ingress | ||
| // Create Kubernetes Ingress (also used on OpenShift when USE_INGRESS_ON_OPENSHIFT=true) | ||
| return r.reconcileIngressKubernetes(ctx, instance) |
There was a problem hiding this comment.
🧩 Analysis chain
Good refactor to runtime OpenShift detection with environment override.
The switch from a static flag to utils.ShouldCreateRoute() provides better flexibility and allows the USE_INGRESS_ON_OPENSHIFT environment variable to override the default behavior.
However, IsOpenShiftCluster() performs API discovery calls on every reconciliation. Consider caching the detection result at controller initialization to improve performance:
If called frequently in hot paths, you might want to cache the result at startup or use a sync.Once pattern.
🏁 Script executed:
#!/bin/bash
# Verify how frequently IsOpenShiftCluster() is called across the codebase
rg -n "IsOpenShiftCluster\(\)" --type=go -C2Length of output: 1817
I need to examine the actual implementation of IsOpenShiftCluster() in the codebase to verify whether the review comment's concern about API discovery on every call is valid.<!-- [scratchpad_update]
[task review_file_1/1 analyze_code_1/1]
[observations]
Web search confirms that ctrl.GetConfig() from controller-runtime gets a REST config but doesn't indicate expensive API calls directly. However, discovery operations can be expensive. Need to look at the actual IsOpenShiftCluster() implementation to see what discovery calls it makes.
[analysis]
Cannot determine from web search alone. Need direct code inspection to see:
- What IsOpenShiftCluster() actually calls
- Whether it makes API discovery calls on each invocation
- If there's any caching mechanism already in place
[pending]
- View IsOpenShiftCluster() implementation
- Check for caching (sync.Once, etc.)
- Determine call frequency in reconciliation loop
[actions]
- Use shell scripts to examine openshift.go implementation
[scratchpad_end] -->
Let me examine the actual implementation of IsOpenShiftCluster() to verify the review comment's performance concern.
Once I see the implementation, I can determine if the performance concern in the review comment is valid.
🏁 Script executed:
#!/bin/bash
# View the IsOpenShiftCluster() implementation
cat ray-operator/controllers/ray/utils/openshift.goLength of output: 1340
I can see the performance concern is valid. Let me verify if the original code (r.options.IsOpenShift) was a cached flag, and check the reconciliation pattern to understand the impact.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Search for r.options.IsOpenShift to understand the original cached approach
rg -n "r\.options\.IsOpenShift|IsOpenShift" ray-operator/controllers/ray/raycluster_controller.go -C3Length of output: 305
🏁 Script executed:
#!/bin/bash
# Check if there's a caching mechanism or init logic in the controller
rg -n "type.*Controller|func.*NewReconciler|func.*SetupWithManager" ray-operator/controllers/ray/raycluster_controller.go -A5 | head -50Length of output: 752
🏁 Script executed:
#!/bin/bash
# Search for sync.Once patterns in the codebase (common caching pattern)
rg -n "sync\.Once|cache|Cache" ray-operator/controllers/ray/utils/ --type=goLength of output: 0
Let me verify how the original IsOpenShift flag was being set during initialization.
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Search for where IsOpenShift was set in RayClusterReconcilerOptions
rg -n "IsOpenShift\s*=" ray-operator/controllers/ray/ --type=go -B2 -A2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the main.go or setup code to see how NewReconciler was called
rg -n "NewReconciler\(" ray-operator/controllers/ray/raycluster_controller.go -B3 -A10Length of output: 619
🏁 Script executed:
#!/bin/bash
# Search for where NewReconciler was invoked in the codebase
rg -n "NewReconciler\(" ray-operator/ --type=goLength of output: 480
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# View main.go around the NewReconciler call to understand rayClusterOptions initialization
sed -n '240,270p' ray-operator/main.goLength of output: 1577
🏁 Script executed:
#!/bin/bash
# Search for rayClusterOptions initialization
rg -n "rayClusterOptions\s*:=|rayClusterOptions\s*=" ray-operator/main.go -B5 -A10Length of output: 789
Let me verify if r.options.IsOpenShift is still available for use or if it was intentionally removed:
[/run_scripts]
🏁 Script executed:
#!/bin/bash
# Check if r.options.IsOpenShift is still used elsewhere in the controller
rg -n "r\.options\.IsOpenShift" ray-operator/controllers/ray/raycluster_controller.goLength of output: 0
Revert to cached r.options.IsOpenShift to avoid expensive per-reconciliation API discovery calls.
The change introduces a critical performance regression: the original code cached the OpenShift detection result at controller initialization (line 252 in main.go: IsOpenShift: utils.GetClusterType()), but the new code at lines 428-433 now calls utils.ShouldCreateRoute() on every reconciliation. This in turn calls IsOpenShiftCluster(), which performs an API discovery call via discoveryClient.ServerGroups() on each invocation—potentially thousands of times over the controller's lifetime.
Fix: Restore the cached approach by using r.options.IsOpenShift instead of calling utils.ShouldCreateRoute():
if r.options.IsOpenShift && !utils.ShouldUseIngressOnOpenShift() {
// Create OpenShift Route
return r.reconcileRouteOpenShift(ctx, instance)
}
// Create Kubernetes Ingress
return r.reconcileIngressKubernetes(ctx, instance)This avoids the expensive API call on every reconciliation while maintaining the environment variable override.
🤖 Prompt for AI Agents
In ray-operator/controllers/ray/raycluster_controller.go around lines 428 to
433, revert the runtime API-discovery call by replacing
utils.ShouldCreateRoute() with the cached cluster-type flag on the controller
options: use r.options.IsOpenShift combined with the ingress-on-openshift
override (call utils.ShouldUseIngressOnOpenShift()) so that you only hit the
expensive discovery path at startup; if r.options.IsOpenShift is true and
ShouldUseIngressOnOpenShift() is false, call reconcileRouteOpenShift, otherwise
call reconcileIngressKubernetes.
2 participants
This fixes up the openshift check to not be confused by the USE_INGRESS_ON_OPENSHIFT env var
Provides a vanilla openshift check with robust api checks.
Uses the new mechanism to decide about the route/ingress creation
Summary by CodeRabbit
New Features
Refactor