🌱 Add contextual logging for work agent #1242
Conversation
Signed-off-by: Jian Qiu <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qiujian16 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Warning Rate limit exceeded@qiujian16 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 45 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (10)
WalkthroughRefactors logging across pkg/work: replaces recorder-based event emission with context-derived klog structured logging; removes recorder parameters from several Apply variants and helper DeleteAppliedResources; introduces controller/finalizer name constants and small signature updates (workReconcile.reconcile, newWorkClientAndInformer). Tests updated accordingly. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1242 +/- ##
==========================================
- Coverage 62.19% 62.16% -0.03%
==========================================
Files 209 209
Lines 16968 16997 +29
==========================================
+ Hits 10554 10567 +13
- Misses 5297 5312 +15
- Partials 1117 1118 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/work/spoke/apply/create_only_apply.go (1)
41-49: Fix misplaced log statement.The log message "Resource created because of missing" at lines 46-47 is inside the
if err != nilblock, meaning it only logs when creation fails. This should be outside the error check to log successful creation.Apply this diff:
if apierrors.IsNotFound(err) { required.SetOwnerReferences([]metav1.OwnerReference{owner}) obj, err = c.client.Resource(gvr).Namespace(required.GetNamespace()).Create( ctx, resourcemerge.WithCleanLabelsAndAnnotations(required).(*unstructured.Unstructured), metav1.CreateOptions{}) - if err != nil { + if err == nil { logger.Info("Resource created because of missing", "kind", required.GetKind(), "resourceNamespace", required.GetNamespace(), "resourceName", required.GetName()) } }
🧹 Nitpick comments (2)
pkg/work/spoke/spokeagent.go (1)
64-66: Consider enriching logger with cluster-specific context.The contextual logger setup is correct and follows klog v2 best practices. However, adding identifying information would improve log traceability across distributed components.
Consider enriching the logger with cluster context:
// setting up contextual logger -logger := klog.NewKlogr() +logger := klog.NewKlogr().WithValues("cluster", o.agentOptions.SpokeClusterName) ctx = klog.NewContext(ctx, logger)Note: The
agentIDcould also be added, but it's not calculated until line 115. If you prefer to include it, the logger setup could be moved after the agentID calculation, or you could enrich the context later before passing it to controllers.pkg/work/spoke/apply/server_side_apply.go (1)
47-47: Consider removing the unused recorder parameter.The recorder parameter is now a blank identifier, effectively unused. If all callers in the codebase have been updated to not rely on event recording from this method, consider removing the parameter entirely to clean up the API surface.
If backward compatibility is required temporarily, this approach is acceptable but should be documented or tracked for future cleanup.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
pkg/work/helper/helper_test.go(1 hunks)pkg/work/helper/helpers.go(5 hunks)pkg/work/spoke/apply/create_only_apply.go(3 hunks)pkg/work/spoke/apply/read_only_apply.go(2 hunks)pkg/work/spoke/apply/server_side_apply.go(2 hunks)pkg/work/spoke/apply/update_apply.go(3 hunks)pkg/work/spoke/auth/basic/auth.go(2 hunks)pkg/work/spoke/auth/cache/auth.go(2 hunks)pkg/work/spoke/controllers/finalizercontroller/add_finalizer_controller.go(2 hunks)pkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.go(5 hunks)pkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.go(2 hunks)pkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller.go(4 hunks)pkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller_test.go(0 hunks)pkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler.go(2 hunks)pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go(3 hunks)pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go(1 hunks)pkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go(2 hunks)pkg/work/spoke/spokeagent.go(3 hunks)
💤 Files with no reviewable changes (1)
- pkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller_test.go
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-07-23T10:10:42.066Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1086
File: test/integration/util/grpc.go:146-146
Timestamp: 2025-07-23T10:10:42.066Z
Learning: In OCM codebase, there are two different GRPCServerOptions types: the local one in pkg/server/grpc/options.go (which only has GRPCServerConfig field) and the SDK one from open-cluster-management.io/sdk-go/pkg/cloudevents/server/grpc/options (which has ServerBindPort and other fields with default values). Test code uses the SDK version via grpcoptions import alias.
Applied to files:
pkg/work/helper/helper_test.go
📚 Learning: 2025-10-14T09:37:12.472Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1196
File: pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go:231-242
Timestamp: 2025-10-14T09:37:12.472Z
Learning: In the ocm repository, there are two different factory packages with different SyncContext.Queue() return types:
1. `github.com/openshift/library-go/pkg/controller/factory` returns `workqueue.RateLimitingInterface` (non-typed)
2. `open-cluster-management.io/sdk-go/pkg/basecontroller/factory` returns `workqueue.TypedRateLimitingInterface[string]` (typed)
When reviewing controller code, check which factory import is used to determine the correct queue interface type.
Applied to files:
pkg/work/spoke/controllers/finalizercontroller/add_finalizer_controller.gopkg/work/spoke/controllers/statuscontroller/availablestatus_controller.gopkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.gopkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go
📚 Learning: 2025-10-28T02:55:13.893Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1224
File: pkg/registration/register/grpc/spoke_driver.go:89-98
Timestamp: 2025-10-28T02:55:13.893Z
Learning: In pkg/registration/register/grpc/spoke_driver.go (Go), when calling cloudeventscsr.NewAgentClientHolder with GenericClientOptions, the watcher store does not need to be explicitly provided via WithClientWatcherStore. The GenericClientOptions.AgentClient() method automatically creates a default AgentInformerWatcherStore if none is provided, which satisfies the NewAgentClientHolder requirements.
Applied to files:
pkg/work/spoke/spokeagent.go
📚 Learning: 2025-09-08T06:10:32.388Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1164
File: test/integration/work/deleteoption_test.go:42-42
Timestamp: 2025-09-08T06:10:32.388Z
Learning: The startWorkAgent function in the OCM test suite uses variadic parameters for decorators (...agentOptionsDecorator), allowing it to be called with just ctx and clusterName without requiring an explicit nil decorator parameter.
Applied to files:
pkg/work/spoke/spokeagent.go
📚 Learning: 2025-07-01T02:27:10.927Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1053
File: vendor/open-cluster-management.io/sdk-go/pkg/cloudevents/clients/work/agent/client/manifestwork.go:237-237
Timestamp: 2025-07-01T02:27:10.927Z
Learning: In OCM ManifestWork agent client, when a work is being deleted (DeletionTimestamp set and finalizers removed), the agent publishes a status update event (types.UpdateRequestAction) with ResourceDeleted condition set to True to inform the hub that deletion is complete, rather than publishing a delete request event.
Applied to files:
pkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler.gopkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.gopkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.go
📚 Learning: 2025-09-03T08:43:34.751Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1158
File: test/integration/work/completedmanifestwork_test.go:216-256
Timestamp: 2025-09-03T08:43:34.751Z
Learning: In TTL=0 ManifestWork deletion tests, avoid waiting for WorkComplete condition because the controller deletes the resource immediately upon completion, creating a race condition where the test tries to check completion status on an already-deleted resource.
Applied to files:
pkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.gopkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.go
🧬 Code graph analysis (8)
pkg/work/helper/helpers.go (1)
vendor/k8s.io/klog/v2/klog.go (1)
V(1353-1355)
pkg/work/helper/helper_test.go (1)
pkg/work/helper/helpers.go (1)
DeleteAppliedResources(166-265)
pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go (1)
vendor/k8s.io/klog/v2/klog.go (1)
V(1353-1355)
pkg/work/spoke/spokeagent.go (1)
vendor/k8s.io/klog/v2/klogr.go (1)
NewKlogr(33-35)
pkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.go (2)
vendor/k8s.io/klog/v2/internal/serialize/keyvalues.go (1)
WithValues(35-52)vendor/k8s.io/klog/v2/klog.go (1)
V(1353-1355)
pkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.go (2)
vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)vendor/k8s.io/klog/v2/internal/serialize/keyvalues.go (1)
WithValues(35-52)
pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go (2)
vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)vendor/k8s.io/klog/v2/internal/serialize/keyvalues.go (1)
WithValues(35-52)
pkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller.go (3)
vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)vendor/k8s.io/klog/v2/internal/serialize/keyvalues.go (1)
WithValues(35-52)vendor/k8s.io/klog/v2/klog.go (1)
V(1353-1355)
⏰ 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: verify
- GitHub Check: integration
- GitHub Check: unit
- GitHub Check: build
- GitHub Check: cloudevents-integration
- GitHub Check: e2e
- GitHub Check: e2e-hosted
- GitHub Check: e2e-singleton
🔇 Additional comments (19)
pkg/work/spoke/spokeagent.go (3)
13-13: LGTM!The klog/v2 import is correctly added to support the contextual logging functionality.
110-110: LGTM!The function call correctly updated to match the simplified signature, removing the unused
restMapperparameter while keeping it available for the validator factory initialization at line 128.
204-206: LGTM!The function signature correctly simplified by removing the unused
restMapperparameter. The function body never used this parameter, so its removal is appropriate and improves clarity.pkg/work/spoke/auth/cache/auth.go (1)
104-104: LGTM! Clean transition to structured logging.The logger is properly initialized from the context, and the structured log at verbosity level 4 preserves all relevant information (executorKey, dimension, allowed) from the previous printf-style log.
Also applies to: 132-133
pkg/work/spoke/controllers/statuscontroller/availablestatus_controller.go (2)
34-38: Good practice: controller name as constant.Extracting the controller name as a constant improves maintainability and ensures consistency across the controller.
87-89: LGTM! Proper context-aware logging setup.The logger is correctly initialized from the context, enriched with controller name and manifestWorkName, and the updated context is propagated downstream. This ensures structured logging throughout the reconciliation flow.
pkg/work/spoke/apply/server_side_apply.go (1)
116-118: LGTM! Clean structured logging.The structured log captures all relevant information (kind, namespace, name, fieldManager) that would have been in the event, providing good observability.
pkg/work/spoke/apply/update_apply.go (1)
86-86: LGTM! Consistent structured logging pattern.The logger is properly initialized from context and used consistently for both create and update code paths with appropriate resource identifiers.
Also applies to: 94-95, 133-134
pkg/work/spoke/auth/basic/auth.go (1)
137-137: LGTM! Clean structured logging with proper error context.The logger is properly initialized from context, and the structured log includes all relevant information including the error as a structured field, which is excellent for log parsing and observability.
Also applies to: 155-156
pkg/work/helper/helpers.go (2)
175-176: LGTM! Clean refactoring from recorder to structured logging.The removal of the recorder parameter and transition to context-based logging is well-executed. The logger is initialized once at the function start and used consistently throughout with appropriate structured fields.
Also applies to: 193-194, 260-261
414-415: LGTM! Consistent structured logging in ApplyOwnerReferences.The logger initialization and structured logging at verbosity level 2 is appropriate for patch operations, providing good observability without excessive noise.
Also applies to: 439-440
pkg/work/spoke/controllers/manifestcontroller/appliedmanifestwork_reconciler.go (2)
61-61: Good: standardized log key naming.Changing "namespace" to "resourceNamespace" (and "name" to "resourceName") improves clarity and consistency with logging patterns used elsewhere in this PR.
93-93: LGTM! Properly updated for new DeleteAppliedResources signature.The function call correctly passes the dynamic client and owner reference, aligning with the helper function's new signature that uses context-based logging instead of a recorder.
pkg/work/helper/helper_test.go (1)
344-344: LGTM!The test correctly reflects the updated
DeleteAppliedResourcessignature which no longer requires an event recorder parameter.pkg/work/spoke/controllers/finalizercontroller/appliedmanifestwork_finalize_controller.go (2)
64-67: LGTM!Clean implementation of contextual logging with proper logger naming and structured fields.
104-105: LGTM!The call to
DeleteAppliedResourcescorrectly reflects the refactored signature without the recorder parameter, relying on context-based logging instead.pkg/work/spoke/controllers/finalizercontroller/add_finalizer_controller.go (1)
50-52: LGTM!Clean implementation of contextual logging with proper controller name constant and structured fields.
pkg/work/spoke/apply/read_only_apply.go (1)
28-30: LGTM!Clean refactoring from recorder-based events to structured logging with proper context-derived logger and fields.
pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go (1)
128-130: LGTM!Excellent implementation of contextual logging with proper controller name constant, structured fields, and context propagation.
| logger := klog.FromContext(ctx).WithName(appliedManifestWorkFinalizer). | ||
| WithValues(manifestWorkFinalizer, appliedManifestWorkName, "manifestWorkName", manifestWorkName) |
There was a problem hiding this comment.
Fix logger name and structured field key.
Two issues:
- The logger name is
appliedManifestWorkFinalizerbut this controller handles ManifestWork finalization, not AppliedManifestWork. It should usemanifestWorkFinalizer. - The first parameter to
WithValuesshould be a key string (e.g.,"finalizer"), not the constant valuemanifestWorkFinalizer.
Apply this diff:
- logger := klog.FromContext(ctx).WithName(appliedManifestWorkFinalizer).
- WithValues(manifestWorkFinalizer, appliedManifestWorkName, "manifestWorkName", manifestWorkName)
+ logger := klog.FromContext(ctx).WithName(manifestWorkFinalizer).
+ WithValues("appliedManifestWorkName", appliedManifestWorkName, "manifestWorkName", manifestWorkName)📝 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.
| logger := klog.FromContext(ctx).WithName(appliedManifestWorkFinalizer). | |
| WithValues(manifestWorkFinalizer, appliedManifestWorkName, "manifestWorkName", manifestWorkName) | |
| logger := klog.FromContext(ctx).WithName(manifestWorkFinalizer). | |
| WithValues("appliedManifestWorkName", appliedManifestWorkName, "manifestWorkName", manifestWorkName) |
🤖 Prompt for AI Agents
In
pkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.go
around lines 72 to 73, the logger name and structured field key are incorrect:
change the logger name from appliedManifestWorkFinalizer to
manifestWorkFinalizer, and call WithValues using a string key (e.g.,
"finalizer") followed by the manifestWorkFinalizer constant and then the
appliedManifestWorkName/value pair (e.g., WithValues("finalizer",
manifestWorkFinalizer, "manifestWorkName", manifestWorkName)) so the logger has
the correct name and uses a proper key/value pair for structured logging.
pkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller.go
Outdated
Show resolved
Hide resolved
| var authError *basic.NotAllowedError | ||
| if errors.As(result.Error, &authError) { | ||
| logger.V(2).Info("apply work failed", "name", manifestWork.Name, "error", result.Error) | ||
| logger.V(2).Info("apply work failed", "error", result.Error) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify logger context includes manifestWork.Name.
The manifestWork.Name was removed from this log entry. Please ensure that the logger initialized at line 48 includes the manifest work name in its context (similar to availablestatus_controller.go lines 87-89), otherwise this information will be lost from the logs.
🏁 Script executed:
#!/bin/bash
# Check if logger is initialized with manifestWorkName context in the sync method or constructor
rg -n -B5 -A2 'klog\.FromContext.*manifestwork_reconciler\.go|WithName.*manifestwork_reconciler\.go|WithValues.*manifestWorkName' --type=goLength of output: 4208
I need to examine the actual file to verify the logger initialization and the log statement in question.
🏁 Script executed:
#!/bin/bash
# Fetch the manifestwork_reconciler.go file around the lines of interest
sed -n '40,95p' pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.goLength of output: 115
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for the manifestwork_reconciler.go file
fd -type f -name '*manifestwork_reconciler*'
Length of output: 170
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for the manifestwork_reconciler.go file with correct syntax
fd 'manifestwork_reconciler'
Length of output: 315
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the manifestwork_reconciler.go file around the lines of interest
sed -n '40,95p' pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go
Length of output: 1976
Add manifestWorkName to logger context for consistent logging.
The logger at line 48 is initialized without the manifestWorkName context: logger := klog.FromContext(ctx). This differs from similar controllers (availablestatus_controller, manifestwork_controller, add_finalizer_controller), which include manifestWorkName via .WithValues("manifestWorkName", manifestWorkName). Since the function has access to the manifestWork parameter, you should add: .WithValues("manifestWorkName", manifestWork.Name) to maintain consistent logging context across controllers.
🤖 Prompt for AI Agents
In pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go
around line 88, the logger lacks the manifestWorkName context; change the logger
initialization from klog.FromContext(ctx) to
klog.FromContext(ctx).WithValues("manifestWorkName", manifestWork.Name) so all
subsequent logger calls (e.g., logger.V(2).Info("apply work failed", "error",
result.Error)) include the manifestWorkName for consistent logging with other
controllers.
There was a problem hiding this comment.
the ctx is passed from controller which already has manifestWorkName set.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how the manifestwork_controller.go sets up the logger and calls reconcile
sed -n '1,200p' pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go | head -150
Length of output: 6031
🏁 Script executed:
#!/bin/bash
# Look for where reconcile is called and how context is set up
rg -n -B10 -A5 'reconcile\(ctx' pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go
Length of output: 584
🏁 Script executed:
#!/bin/bash
# Check the sync method in manifestwork_controller.go where logger is set up
rg -n -B5 -A15 'func.*sync.*Context' pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go
Length of output: 855
@qiujian16, you're absolutely right! I verified that in manifestwork_controller.go at lines 128-130, the logger is initialized with manifestWorkName and then attached to the context:
logger := klog.FromContext(ctx).WithName(controllerName).WithValues("manifestWorkName", manifestWorkName)
logger.V(4).Info("Reconciling ManifestWork")
ctx = klog.NewContext(ctx, logger)This enriched context is then passed to the reconcile method, so when klog.FromContext(ctx) is called at line 48 in manifestwork_reconciler.go, it retrieves the logger with manifestWorkName already included. The removal of the explicit "name", manifestWork.Name field from the log statement at line 88 is correct and avoids duplication. Thank you for the clarification!
🐰✨
/> フ
| _ _ l
/` ミ_xノ
/ |
/ ヽ ﾉ
│ | | |
/ ￣| | | |
| (￣ヽ__ヽ_)__)
\二つ
🧬 Code Graph Analysis Results
[relevant_code_snippets]
Filename: pkg/work/helper/helpers.go
Lines: 347-354
func NewAppliedManifestWorkOwner(appliedWork *workapiv1.AppliedManifestWork) *metav1.OwnerReference {
return &metav1.OwnerReference{
APIVersion: workapiv1.GroupVersion.WithKind("AppliedManifestWork").GroupVersion().String(),
Kind: workapiv1.GroupVersion.WithKind("AppliedManifestWork").Kind,
Name: appliedWork.Name,
UID: appliedWork.UID,
}
}
Filename: pkg/work/spoke/auth/basic/auth.go
Lines: 21-24
NotAllowedError struct {
Err error
RequeueTime time.Duration
}
Filename: pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go
Lines: 39-39
ResyncInterval = 4 * time.Minute
Filename: pkg/common/helpers/error.go
Lines: 17-22
func NewRequeueError(msg string, requeueTime time.Duration) RequeueError {
return RequeueError{
RequeueTime: requeueTime,
Message: msg,
}
}
Filename: Any (context usage in reconcile path)
Lines: N/A (embedded in manifestwork_reconciler.go as part of reconcile signature)
func (m *manifestworkReconciler) reconcile(
ctx context.Context,
...
) ...
[End of snippets]
✏️ Learnings added
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1242
File: pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go:88-88
Timestamp: 2025-11-06T08:55:13.289Z
Learning: In pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go, the sync method initializes a logger with manifestWorkName and attaches it to the context before calling reconcile methods. Therefore, reconcile methods (like manifestworkReconciler.reconcile) that use klog.FromContext(ctx) automatically inherit the manifestWorkName context and do not need to add it again.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| recorder.Eventf(fmt.Sprintf( | ||
| "%s Created", required.GetKind()), "Created %s/%s because it was missing", required.GetNamespace(), required.GetName()) | ||
| logger.Info("Resource created because of missing", | ||
| "kind", required.GetKind(), "resourceNamespace", required.GetNamespace(), "resourceName", required.GetName()) |
There was a problem hiding this comment.
we may also use gvr instead of kind here for consistent?
| _ events.Recorder) (runtime.Object, error) { | ||
| logger := klog.FromContext(ctx) | ||
| logger.Info("Noop because its read-only", | ||
| "kind", required.GetKind(), "resourceNamespace", required.GetNamespace(), "resourceName", required.GetName()) |
There was a problem hiding this comment.
we may also use gvr instead of kind here?
| manifestWorkName := controllerContext.QueueKey() | ||
| logger.V(4).Info("Reconciling ManifestWork", "name", manifestWorkName) | ||
| logger := klog.FromContext(ctx).WithName(controllerName).WithValues("manifestWorkName", manifestWorkName) | ||
| logger.V(4).Info("Reconciling ManifestWork") |
There was a problem hiding this comment.
V(5) for reconciling info for consistent
| func (m *AddFinalizerController) sync(ctx context.Context, controllerContext factory.SyncContext) error { | ||
| manifestWorkName := controllerContext.QueueKey() | ||
| klog.V(5).Infof("Reconciling ManifestWork %q", manifestWorkName) | ||
| logger := klog.FromContext(ctx).WithName(manifestWorkAddFinalizerController). |
There was a problem hiding this comment.
do we have a chance to add the container name to the log context?
There was a problem hiding this comment.
we might be able to get podName from the env var.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/work/spoke/apply/create_only_apply.go (1)
41-49: Fix inverted logging condition.The logging condition is inverted. Lines 46-47 log "Resource created because of missing" inside an
if err != nilblock, meaning it logs a success message when creation fails. The log should only be emitted when creation succeeds.Apply this diff to fix the logic:
if apierrors.IsNotFound(err) { required.SetOwnerReferences([]metav1.OwnerReference{owner}) obj, err = c.client.Resource(gvr).Namespace(required.GetNamespace()).Create( ctx, resourcemerge.WithCleanLabelsAndAnnotations(required).(*unstructured.Unstructured), metav1.CreateOptions{}) - if err != nil { + if err == nil { logger.Info("Resource created because of missing", "gvr", gvr.String(), "resourceNamespace", required.GetNamespace(), "resourceName", required.GetName()) } }pkg/work/helper/helpers.go (1)
236-244: Use the passed context parameter instead ofcontext.TODO().The Delete call uses
context.TODO()while the rest of the function consistently uses thectxparameter (e.g., Get at line 191, ApplyOwnerReferences at line 214). This is inconsistent and will lose context information such as cancellation signals, deadlines, and structured logging fields.Apply this diff:
err = dynamicClient. Resource(gvr). Namespace(resource.Namespace). - Delete(context.TODO(), resource.Name, metav1.DeleteOptions{ + Delete(ctx, resource.Name, metav1.DeleteOptions{ Preconditions: &metav1.Preconditions{ UID: &uid, }, PropagationPolicy: &deletePolicy, })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
pkg/work/helper/helpers.go(5 hunks)pkg/work/spoke/apply/create_only_apply.go(3 hunks)pkg/work/spoke/apply/read_only_apply.go(2 hunks)pkg/work/spoke/auth/basic/auth.go(2 hunks)pkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.go(2 hunks)pkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller.go(4 hunks)pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go(3 hunks)pkg/work/spoke/spokeagent.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/work/spoke/controllers/finalizercontroller/unmanaged_appliedmanifestwork_controller.go
- pkg/work/spoke/controllers/finalizercontroller/manifestwork_finalize_controller.go
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1242
File: pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go:88-88
Timestamp: 2025-11-06T08:55:13.289Z
Learning: In pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go, the sync method initializes a logger with manifestWorkName and attaches it to the context before calling reconcile methods. Therefore, reconcile methods (like manifestworkReconciler.reconcile) that use klog.FromContext(ctx) automatically inherit the manifestWorkName context and do not need to add it again.
📚 Learning: 2025-11-06T08:55:13.289Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1242
File: pkg/work/spoke/controllers/manifestcontroller/manifestwork_reconciler.go:88-88
Timestamp: 2025-11-06T08:55:13.289Z
Learning: In pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go, the sync method initializes a logger with manifestWorkName and attaches it to the context before calling reconcile methods. Therefore, reconcile methods (like manifestworkReconciler.reconcile) that use klog.FromContext(ctx) automatically inherit the manifestWorkName context and do not need to add it again.
Applied to files:
pkg/work/spoke/spokeagent.gopkg/work/spoke/apply/create_only_apply.gopkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.gopkg/work/spoke/apply/read_only_apply.go
📚 Learning: 2025-10-28T02:55:13.893Z
Learnt from: skeeey
Repo: open-cluster-management-io/ocm PR: 1224
File: pkg/registration/register/grpc/spoke_driver.go:89-98
Timestamp: 2025-10-28T02:55:13.893Z
Learning: In pkg/registration/register/grpc/spoke_driver.go (Go), when calling cloudeventscsr.NewAgentClientHolder with GenericClientOptions, the watcher store does not need to be explicitly provided via WithClientWatcherStore. The GenericClientOptions.AgentClient() method automatically creates a default AgentInformerWatcherStore if none is provided, which satisfies the NewAgentClientHolder requirements.
Applied to files:
pkg/work/spoke/spokeagent.go
📚 Learning: 2025-09-08T06:10:32.388Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1164
File: test/integration/work/deleteoption_test.go:42-42
Timestamp: 2025-09-08T06:10:32.388Z
Learning: The startWorkAgent function in the OCM test suite uses variadic parameters for decorators (...agentOptionsDecorator), allowing it to be called with just ctx and clusterName without requiring an explicit nil decorator parameter.
Applied to files:
pkg/work/spoke/spokeagent.go
📚 Learning: 2025-10-14T09:37:12.472Z
Learnt from: qiujian16
Repo: open-cluster-management-io/ocm PR: 1196
File: pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go:231-242
Timestamp: 2025-10-14T09:37:12.472Z
Learning: In the ocm repository, there are two different factory packages with different SyncContext.Queue() return types:
1. `github.com/openshift/library-go/pkg/controller/factory` returns `workqueue.RateLimitingInterface` (non-typed)
2. `open-cluster-management.io/sdk-go/pkg/basecontroller/factory` returns `workqueue.TypedRateLimitingInterface[string]` (typed)
When reviewing controller code, check which factory import is used to determine the correct queue interface type.
Applied to files:
pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go
🧬 Code graph analysis (3)
pkg/work/helper/helpers.go (1)
vendor/k8s.io/klog/v2/klog.go (1)
V(1353-1355)
pkg/work/spoke/spokeagent.go (2)
vendor/k8s.io/klog/v2/klogr.go (1)
NewKlogr(33-35)vendor/k8s.io/klog/v2/internal/serialize/keyvalues.go (1)
WithValues(35-52)
pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go (3)
vendor/open-cluster-management.io/sdk-go/pkg/basecontroller/factory/interfaces.go (1)
SyncContext(33-37)vendor/k8s.io/klog/v2/internal/serialize/keyvalues.go (1)
WithValues(35-52)vendor/k8s.io/klog/v2/klog.go (1)
V(1353-1355)
⏰ 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: integration
- GitHub Check: unit
- GitHub Check: build
- GitHub Check: verify
- GitHub Check: e2e-singleton
- GitHub Check: cloudevents-integration
- GitHub Check: e2e
- GitHub Check: e2e-hosted
🔇 Additional comments (14)
pkg/work/spoke/apply/create_only_apply.go (2)
14-14: LGTM!The klog/v2 import addition supports the migration to context-based structured logging.
29-34: LGTM!The signature change appropriately ignores the recorder parameter while maintaining API compatibility, aligning with the PR's objective to replace event-recorder usage with klog-based logging.
pkg/work/spoke/apply/read_only_apply.go (2)
11-11: LGTM!The klog/v2 import addition supports the migration to context-based structured logging.
22-31: LGTM!The implementation correctly:
- Ignores the recorder parameter to maintain API compatibility
- Uses context-derived structured logging with consistent field names
- Logs an appropriate no-op message for read-only apply operations
pkg/work/spoke/auth/basic/auth.go (1)
137-137: LGTM! Structured logging improves observability.The refactoring from unstructured logging to contextual structured logging is a clear improvement. The logger extracted from context at line 137 will inherit any existing context (like manifestWorkName), and the structured fields at lines 155-156 provide well-labeled, queryable data for log aggregation systems.
Also applies to: 155-156
pkg/work/spoke/spokeagent.go (2)
5-5: LGTM!The new imports support the contextual logging setup added below.
Also applies to: 14-14
115-115: Good context propagation for structured logging.The change from passing
restMappertoctxenables the contextual logger to flow through the call chain, aligning with the PR's objective of adding structured, context-based logging across the work agent.Also applies to: 209-210
pkg/work/helper/helpers.go (2)
175-176: LGTM!The contextual logger initialization follows the standard pattern and will inherit structured fields from the parent context.
260-262: LGTM!The remaining logging changes are well-implemented:
- Successful deletion logging includes the reason parameter for better traceability (lines 260-262)
- Standard contextual logger initialization in
ApplyOwnerReferences(lines 414-415)- Appropriate use of verbose level logging with detailed patch data for debugging (lines 439-441)
Also applies to: 414-415, 439-441
pkg/work/spoke/controllers/manifestcontroller/manifestwork_controller.go (5)
42-42: LGTM!The constant centralizes the controller name, improving maintainability and consistency.
44-50: LGTM!The interface signature update to accept context as the first parameter enables context-aware logging throughout the reconciliation chain, aligning with Go best practices.
128-130: LGTM!The logger initialization correctly establishes the logging context with controller name and manifestWorkName, then propagates it via context to downstream reconcilers. The V(5) verbosity level is consistent with the codebase standards for reconciliation info. Based on learnings.
197-197: LGTM!The log statement correctly leverages the manifestWorkName already present in the logger context (from line 128), avoiding duplication. The V(2) verbosity level is appropriate for this operational information.
120-120: The review comment is incorrect—no controller name change from "ManifestWorkAgent" to "ManifestWorkController" exists in this codebase.The search found no references to "ManifestWorkAgent" as a controller name. The
controllerNameconstant (line 42) is already set to"ManifestWorkController", and line 120 correctly uses this constant viaToController(controllerName, recorder). The code is consistent and requires no observability updates.Likely an incorrect or invalid review comment.
qiujian16
force-pushed
the
contextual-logging
branch
from
202dc64 to
1c8f416
Compare
| "Server Side Applied %s %s", required.GetKind(), resourceKey), | ||
| "Patched with field manager %s, err %v", fieldManager, err) | ||
| logger.Info("Server side applied", | ||
| "kind", required.GetKind(), "resourceNamespace", required.GetNamespace(), |
There was a problem hiding this comment.
| "kind", required.GetKind(), "resourceNamespace", required.GetNamespace(), | |
| "gvr", gvr.String(), "resourceNamespace", required.GetNamespace(), |
pkg/work/spoke/apply/update_apply.go
Outdated
| recorder.Eventf(fmt.Sprintf( | ||
| "%s Created", required.GetKind()), "Created %s/%s because it was missing", required.GetNamespace(), required.GetName()) | ||
| logger.Info("Created resource because it was missing", | ||
| "kind", required.GetKind(), "resourceNamespace", required.GetNamespace(), "resourceName", required.GetName()) |
There was a problem hiding this comment.
| "kind", required.GetKind(), "resourceNamespace", required.GetNamespace(), "resourceName", required.GetName()) | |
| "gvr", gvr.String(), "resourceNamespace", required.GetNamespace(), "resourceName", required.GetName()) |
pkg/work/spoke/apply/update_apply.go
Outdated
| recorder.Eventf(fmt.Sprintf( | ||
| "%s Updated", required.GetKind()), "Updated %s/%s", required.GetNamespace(), required.GetName()) | ||
| logger.Info("Updated resource", | ||
| "kind", required.GetKind(), "resourceNamespace", required.GetNamespace(), "resourceName", required.GetName()) |
There was a problem hiding this comment.
| "kind", required.GetKind(), "resourceNamespace", required.GetNamespace(), "resourceName", required.GetName()) | |
| "gvr", gvr.String(), "resourceNamespace", required.GetNamespace(), "resourceName", required.GetName()) |
qiujian16
force-pushed
the
contextual-logging
branch
from
1c8f416 to
0cfcdeb
Compare
Signed-off-by: Jian Qiu <[email protected]>
qiujian16
force-pushed
the
contextual-logging
branch
from
0cfcdeb to
9bae123
Compare
|
/lgtm |
openshift-merge-bot
bot
merged commit 5528aff
into
open-cluster-management-io:main
2 participants
Summary
Related issue(s)
Fixes #
Summary by CodeRabbit
Refactor
Tests