Skip to content

Conversation

@flpanbin
Copy link
Contributor

@flpanbin flpanbin commented Oct 12, 2025

Overview:

Moves ctrl.SetLogger(zap.New(zap.UseFlagOptions(&opts))) initialization earlier to ensure logs are properly captured from the beginning.

Details:

This fixes missing log entries that occurred before the logger was initialized. like this:

setupLog.Error(nil, "planner-cluster-role-name is required in cluster-wide mode")

setupLog.Info("Model Express URL configured", "url", modelExpressURL)

setupLog.Error(nil, "mpi-run-ssh-secret-name is required")

setupLog.Error(nil, "mpi-run-ssh-secret-namespace is required")

Where should the reviewer start?

deploy/cloud/operator/cmd/main.go

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

  • closes GitHub issue: #xxx

Summary by CodeRabbit

  • Refactor
    • Adjusted logging initialization to occur earlier during startup, ensuring logs are available sooner in the application lifecycle.
  • Chores
    • Removed a duplicate logging setup to streamline initialization and reduce redundancy.

No user-facing functionality has changed. Users may notice more consistent log output during early startup phases, improving observability without altering behavior.

@flpanbin flpanbin requested a review from a team as a code owner October 12, 2025 14:36
@copy-pr-bot
Copy link

copy-pr-bot bot commented Oct 12, 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.

@github-actions
Copy link

👋 Hi flpanbin! 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 12, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 12, 2025

Walkthrough

The main function in deploy/cloud/operator/cmd/main.go moves controller-runtime logger initialization to occur immediately after flag parsing and removes the later, duplicate SetLogger call after SetupSignalHandler.

Changes

Cohort / File(s) Summary
Logger initialization order
deploy/cloud/operator/cmd/main.go
SetLogger is now called right after flag parsing with flag-derived options; the later SetLogger call after SetupSignalHandler is removed to prevent duplicate initialization.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant Proc as Process Start
  participant Flags as Flag Parsing
  participant Log as Logger
  participant Ctx as Signal Handler
  participant RT as Runtime Setup

  note over Proc,RT: Startup sequence (updated)
  Proc->>Flags: Parse CLI flags
  Flags->>Log: Configure logger (SetLogger)
  note right of Log: Earlier initialization
  Proc->>Ctx: SetupSignalHandler
  Ctx->>RT: Initialize controllers/runtime
  RT-->>Log: Use configured logger
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

I twitched my whiskers, logs aligned,
Moved the setup up in time.
No double echoes in the burrow’s night,
One crisp voice, the levels right.
Hoppity-hop, the operator sings—
Early logs sprout tidy things.

Pre-merge checks

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly describes the primary change of moving logger initialization earlier to capture startup logs and matches the core modifications in main.go, making it clear to reviewers and accurately reflecting the main intent of the pull request.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Description Check ✅ Passed The pull request description includes all required sections from the template—Overview, Details, Where should the reviewer start, and Related Issues—with clear content and proper formatting, satisfying the repository’s description guidelines.

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.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 12, 2025

Walkthrough

Logger initialization in deploy/cloud/operator/cmd/main.go is moved to immediately after flag.Parse using Zap with UseFlagOptions. The later, duplicate logger setup is removed. This changes only the timing of logger setup so it occurs before namespace and role validation.

Changes

Cohort / File(s) Summary
Startup logging init
deploy/cloud/operator/cmd/main.go
Moved logger initialization to immediately after flag.Parse; removed later duplicate initialization to avoid reconfiguring the logger; no changes to exported APIs.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    participant Main
    participant Flags as Flag Parser
    participant Logger as Zap Logger
    participant Validate as Namespace/Role Validation
    participant Runtime as Operator Runtime

    Main->>Flags: flag.Parse()
    Flags-->>Main: parsed options
    Main->>Logger: init logger (UseFlagOptions)
    Note right of Logger: Logger configured earlier than before
    Main->>Validate: validate restrictedNamespace / plannerClusterRoleName
    Validate-->>Main: ok or error
    alt ok
        Main->>Runtime: start operator runtime
    else error
        Main-->>Main: exit with error
    end

    %% Previously: a second, duplicate logger init occurred here (removed)
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~7 minutes

Poem

I twitch my whiskers at logs that arrive,
Early to the burrow, the traces thrive.
No duplicate thumps, just one clean hop,
Flags parsed, lights on—then off we pop.
In quiet bytes, I cheer and grin:
“Initialize once, let the run begin!” 🐇✨

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The description includes the Overview and Related Issues sections but leaves the Details and Where should the reviewer start? headings empty, meaning the specific code changes and reviewer guidance are not documented as required by the repository template. Please populate the Details section with a summary of what was changed in the code and add specific file paths or functions under Where should the reviewer start? to guide reviewers to the relevant changes.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title succinctly and accurately describes the core change of moving logger initialization earlier to capture startup logs, clearly reflecting the main intent of the pull request without unnecessary detail or noise.
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 60975b5 and c333d84.

📒 Files selected for processing (1)
  • deploy/cloud/operator/cmd/main.go (1 hunks)
⏰ 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
🔇 Additional comments (1)
deploy/cloud/operator/cmd/main.go (1)

188-188: Approve logger initialization placement
Positioned immediately after flag parsing to capture all startup logs; no duplicate ctrl.SetLogger calls remain.


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.

@flpanbin
Copy link
Contributor Author

Hi @julienmancuso , Could you please help review this PR?

@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the Stale label Nov 23, 2025
@flpanbin
Copy link
Contributor Author

/no-stale

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 Stale

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant