Skip to content

Conversation

sozercan
Copy link

@sozercan sozercan commented Oct 14, 2025

Overview:

I am not sure what the intend of this code is but manager will exit if plannerClusterRoleName is not defined which is not defined in any recipes

Details:

Where should the reviewer start?

Related Issues: (use one of the action keywords Closes / Fixes / Resolves / Relates to)

  • closes GitHub issue: #xxx

Summary by CodeRabbit

  • Bug Fixes
    • Cluster-wide deployments no longer fail on startup due to a missing planner ClusterRole name, reducing configuration friction.
    • Improves reliability by preventing unnecessary validation errors and allowing deployments to proceed with default or existing RBAC settings.

@sozercan sozercan requested a review from a team as a code owner October 14, 2025 23:44
Copy link

copy-pr-bot bot commented Oct 14, 2025

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Copy link

👋 Hi sozercan! Thank you for contributing to ai-dynamo/dynamo.

Just a reminder: The NVIDIA Test Github Validation CI runs an essential subset of the testing framework to quickly catch errors.Your PR reviewers may elect to test the changes comprehensively before approving your changes.

🚀

@github-actions github-actions bot added fix external-contribution Pull request is from an external contributor labels Oct 14, 2025
Copy link
Contributor

coderabbitai bot commented Oct 14, 2025

Walkthrough

Removed the startup validation that enforced plannerClusterRoleName in cluster-wide mode, eliminating the error/exit path. Control flow now proceeds without this check; remaining validations (e.g., modelExpressURL) and RBAC configuration logic are unchanged.

Changes

Cohort / File(s) Change Summary
Operator CLI main
deploy/cloud/operator/cmd/main.go
Deleted conditional requiring plannerClusterRoleName when restrictedNamespace is empty. Removes early error/exit path; relaxes runtime validation. No exported API changes; subsequent URL validation and RBAC setup remain intact.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor User
  participant OperatorMain as Operator main()
  participant Config as Config/Flags
  participant Validator as Validator
  participant RBAC as RBAC Setup
  participant Runtime as Operator Runtime

  User->>OperatorMain: start
  OperatorMain->>Config: parse flags/env
  alt BEFORE (previous behavior)
    OperatorMain->>Validator: restrictedNamespace == ""?
    Validator-->>OperatorMain: Yes (cluster-wide)
    OperatorMain->>Validator: plannerClusterRoleName present?
    alt Missing
      Validator-->>OperatorMain: error
      OperatorMain-->>User: exit(1)
    else Present
      Validator-->>OperatorMain: ok
      OperatorMain->>Validator: validate modelExpressURL
      OperatorMain->>RBAC: configure via plannerClusterRoleName
      OperatorMain->>Runtime: run
    end
  else AFTER (current behavior)
    OperatorMain->>Validator: validate modelExpressURL
    Note right of OperatorMain: No check for\nplannerClusterRoleName\nin cluster-wide mode
    OperatorMain->>RBAC: configure (uses PlannerClusterRoleName if provided)
    OperatorMain->>Runtime: run
  end
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I nibbled the branch that blocked our trail,
A tiny check gone—no more fail.
Hop, hop, through startup’s clearer glade,
The flow now runs without charade.
Ears up, I ship with lightened load—
One less gate upon the road. 🐇✨

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The description follows the template structure but leaves the Details and Where should the reviewer start sections empty and continues to use a placeholder issue number without providing actual change details or file references, so it does not adequately inform reviewers of what was changed or where to look. Please complete the Details section with a clear summary of the code modifications, specify which files or functions the reviewer should examine under Where should the reviewer start, and replace the placeholder “#xxx” with the real issue number being closed.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title clearly states the primary fix—preventing the manager from exiting when plannerClusterRoleName is undefined—and uses concise, specific language without extraneous detail or noise, matching the actual change in the code.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c6e3db5 and 7152e0a.

📒 Files selected for processing (1)
  • deploy/cloud/operator/cmd/main.go (0 hunks)
💤 Files with no reviewable changes (1)
  • deploy/cloud/operator/cmd/main.go
⏰ 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). (1)
  • GitHub Check: Build and Test - dynamo

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@athreesh
Copy link
Contributor

Hi @tmonty12 @hhzhang16 (cc: @tedzhouhk )

This looks like a bug with operator / planner. Can we please take a look at the issue with plannerClusterRole and fix the root cause?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

external-contribution Pull request is from an external contributor fix size/XS

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants