Skip to content

OADP-7665: Pass --log-level to NodeAgent DaemonSet container args#2139

Open
kaovilai wants to merge 1 commit intoopenshift:oadp-devfrom
kaovilai:pass-log-level-to-nodeagent
Open

OADP-7665: Pass --log-level to NodeAgent DaemonSet container args#2139
kaovilai wants to merge 1 commit intoopenshift:oadp-devfrom
kaovilai:pass-log-level-to-nodeagent

Conversation

@kaovilai
Copy link
Copy Markdown
Member

@kaovilai kaovilai commented Mar 27, 2026

Summary

  • Pass --log-level from dpa.Spec.Configuration.Velero.LogLevel to NodeAgent DaemonSet container args
  • This allows the Velero exposer to inherit log level for data mover pods (via getInheritedPodInfo in pkg/exposer/image.go)
  • Previously, LogLevel was only passed to the Velero server deployment but not to NodeAgent

Request: https://redhat-internal.slack.com/archives/C0144ECKUJ0/p1772820634393599

Test plan

  • Unit test added for LogLevel=debug in NodeAgent DaemonSet build
  • go test ./internal/controller/ -run TestDPAReconciler_buildNodeAgentDaemonset passes
  • E2E: set logLevel: debug in DPA and verify node-agent pod has --log-level=debug in args
  • E2E: verify data mover pods spawned during backup/restore inherit the log level

🤖 Generated with Claude Code
via Happy

Copilot AI review requested due to automatic review settings March 27, 2026 13:42
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 27, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0c8ffb50-b820-4af5-a5ce-6c186e052fd0

📥 Commits

Reviewing files that changed from the base of the PR and between 61e70f4 and 6449f95.

📒 Files selected for processing (2)
  • internal/controller/nodeagent.go
  • internal/controller/nodeagent_test.go
✅ Files skipped from review due to trivial changes (1)
  • internal/controller/nodeagent_test.go

Walkthrough

Conditional injection of --log-level=<value> into the NodeAgent container CLI args when dpa.Spec.Configuration.Velero.LogLevel is non-empty; tests updated to cover the new behavior.

Changes

Cohort / File(s) Summary
NodeAgent log-level configuration
internal/controller/nodeagent.go
Appended conditional logic to add --log-level=<Velero.LogLevel> to NodeAgent container args when LogLevel is set, applied after --log-format handling and before server-args overrides.
NodeAgent test coverage
internal/controller/nodeagent_test.go
Added optional logLevel test field, updated test helper to include --log-level when provided, and added a test case asserting --log-level=debug appears when Configuration.Velero.LogLevel is "debug".

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 27, 2026
@weshayutin
Copy link
Copy Markdown
Contributor

@kaovilai isn't there a jira on this ???

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR propagates spec.configuration.velero.logLevel from the DataProtectionApplication (DPA) CR into the NodeAgent DaemonSet container args, aligning node-agent logging behavior with the Velero server deployment.

Changes:

  • Append --log-level=<value> to the node-agent container args when dpa.Spec.Configuration.Velero.LogLevel is set.
  • Extend node-agent daemonset unit test scaffolding to support asserting log-level.
  • Add a unit test case validating LogLevel=debug results in --log-level=debug on the NodeAgent DaemonSet.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
internal/controller/nodeagent.go Passes DPA Velero logLevel into the node-agent DaemonSet container args.
internal/controller/nodeagent_test.go Adds test builder support and a unit test case asserting node-agent receives --log-level.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +697 to +699
if dpa.Spec.Configuration.Velero.LogLevel != "" {
nodeAgentContainer.Args = append(nodeAgentContainer.Args, fmt.Sprintf("--log-level=%s", dpa.Spec.Configuration.Velero.LogLevel))
}
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR description references getInheritedPodInfo in pkg/exposer/image.go, but there is no pkg/exposer package (and no getInheritedPodInfo symbol) in this repository. Please update the PR description to point at the correct code path in this repo that consumes the node-agent --log-level arg, or clarify that it’s referring to an external component/repo.

Copilot uses AI. Check for mistakes.
@kaovilai
Copy link
Copy Markdown
Member Author

no jira yet.. thread

@weshayutin
Copy link
Copy Markdown
Contributor

/retest

Copy link
Copy Markdown
Contributor

@mpryc mpryc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaovilai is there any validation against improper log levels somewhere?

weshayutin
weshayutin previously approved these changes Mar 31, 2026
Copy link
Copy Markdown
Contributor

@weshayutin weshayutin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/LGTM

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 31, 2026
sseago
sseago previously approved these changes Mar 31, 2026
@kaovilai
Copy link
Copy Markdown
Member Author

is there any validation against improper log levels somewhere?

nodeagent will error out if that happens. Our CRD also have these markers to block invalid values.

// +kubebuilder:validation:Enum=trace;debug;info;warning;error;fatal;panic
LogLevel string `json:"logLevel,omitempty"`

@mpryc
Copy link
Copy Markdown
Contributor

mpryc commented Apr 1, 2026

Merge conflict in internal/controller/nodeagent_test.go
Automatic merge failed; fix conflicts and then commit the result.

@openshift-ci-robot
Copy link
Copy Markdown

/retest-required

Remaining retests: 0 against base HEAD 3cceb1e and 2 for PR HEAD 61e70f4 in total

@weshayutin
Copy link
Copy Markdown
Contributor

/retest

@weshayutin weshayutin changed the title Pass --log-level to NodeAgent DaemonSet container args OADP-7665 Pass --log-level to NodeAgent DaemonSet container args Apr 7, 2026
@kaovilai kaovilai changed the title OADP-7665 Pass --log-level to NodeAgent DaemonSet container args OADP-7665: Pass --log-level to NodeAgent DaemonSet container args Apr 7, 2026
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Apr 7, 2026
@openshift-ci-robot
Copy link
Copy Markdown

openshift-ci-robot commented Apr 7, 2026

@kaovilai: This pull request references OADP-7665 which is a valid jira issue.

Details

In response to this:

Summary

  • Pass --log-level from dpa.Spec.Configuration.Velero.LogLevel to NodeAgent DaemonSet container args
  • This allows the Velero exposer to inherit log level for data mover pods (via getInheritedPodInfo in pkg/exposer/image.go)
  • Previously, LogLevel was only passed to the Velero server deployment but not to NodeAgent

Request: https://redhat-internal.slack.com/archives/C0144ECKUJ0/p1772820634393599

Test plan

  • Unit test added for LogLevel=debug in NodeAgent DaemonSet build
  • go test ./internal/controller/ -run TestDPAReconciler_buildNodeAgentDaemonset passes
  • E2E: set logLevel: debug in DPA and verify node-agent pod has --log-level=debug in args
  • E2E: verify data mover pods spawned during backup/restore inherit the log level

🤖 Generated with Claude Code
via Happy

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.

@weshayutin
Copy link
Copy Markdown
Contributor

/cherry-pick oadp-1.6

@openshift-cherrypick-robot
Copy link
Copy Markdown
Contributor

@weshayutin: once the present PR merges, I will cherry-pick it on top of oadp-1.6 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick oadp-1.6

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 kubernetes-sigs/prow repository.

@kaovilai
Copy link
Copy Markdown
Member Author

kaovilai commented Apr 7, 2026

/retest

@kaovilai
Copy link
Copy Markdown
Member Author

kaovilai commented Apr 7, 2026

sounds like rebase needed..

The DPA LogLevel field was only passed to the Velero server deployment
but not to the NodeAgent DaemonSet. This meant data mover pods spawned
by the exposer could not inherit the log level from the node-agent,
since the exposer reads --log-level from the node-agent container args.

Generated with [Claude Code](https://claude.ai/code)
via [Happy](https://happy.engineering)

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Happy <yesreply@happy.engineering>
Signed-off-by: Tiger Kaovilai <tkaovila@redhat.com>
@kaovilai kaovilai dismissed stale reviews from sseago and weshayutin via 6449f95 April 7, 2026 21:27
@kaovilai kaovilai force-pushed the pass-log-level-to-nodeagent branch from 61e70f4 to 6449f95 Compare April 7, 2026 21:27
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Apr 7, 2026
@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Apr 7, 2026

New changes are detected. LGTM label has been removed.

@kaovilai kaovilai requested review from mpryc, sseago and weshayutin April 7, 2026 21:27
@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Apr 7, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kaovilai, weshayutin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

2 similar comments
@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Apr 7, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kaovilai, weshayutin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Apr 7, 2026

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kaovilai, weshayutin

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Apr 7, 2026

@kaovilai: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/4.23-e2e-test-aws 6449f95 link false /test 4.23-e2e-test-aws

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

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

Labels

ai-gen-bugfix ai-generated-test approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants