CNTRLPLANE-3701: simplify logging, ensure proper fields in reoncilers#8802
CNTRLPLANE-3701: simplify logging, ensure proper fields in reoncilers#8802stevekuznetsov wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
📝 WalkthroughWalkthroughThis pull request introduces On the controller side, stored ✨ Finishing Touches🧪 Generate unit tests (beta)
|
openshift-ci
Bot
added
area/cli
stevekuznetsov
force-pushed
the
skuznets/controller-logs
branch
2 times, most recently
from
6b7e682 to
3992c0f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go (1)
65-75:⚠️ Potential issue | 🟡 MinorUse the informer lister in Reconcile instead of direct API calls.
SetupWithManager creates a shared informer and watches it (lines 102-128), but Reconcile bypasses this by making direct API calls via
r.clientset.CoreV1().Services(req.Namespace).Get(). This defeats the caching benefit of the informer.Store the informer's lister during setup and use
services.Lister().Services(req.Namespace).Get(req.Name)in Reconcile to read from the cache.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go` around lines 65 - 75, The PrivateServiceObserver struct makes direct API calls in the Reconcile method using r.clientset.CoreV1().Services(req.Namespace).Get() instead of leveraging the informer lister that is set up in SetupWithManager. To fix this, add a services lister field to the PrivateServiceObserver struct to store the informer's lister, initialize this lister in SetupWithManager when the shared informer is created, and then update the Reconcile method to use services.Lister().Services(req.Namespace).Get() instead of the direct client API call to benefit from caching.
🧹 Nitpick comments (10)
product-cli/cmd/cluster/aws/destroy.go (1)
32-41: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse the pre-initialized logger from
opts.Loginstead of allocating a new one.Line 39 creates a new logger instance for every error that occurs. The
optsparameter carries a pre-initializedlogr.Logger(set by the caller incluster.go) that should be reused for consistency and efficiency. This follows the established pattern elsewhere (e.g.,cmd/cluster/core/destroy.go).♻️ Proposed fix
if err = hypershiftaws.DestroyCluster(cmd.Context(), opts); err != nil { - cmdutil.NewLogger().Error(err, "Failed to destroy cluster") + opts.Log.Error(err, "Failed to destroy cluster") return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@product-cli/cmd/cluster/aws/destroy.go` around lines 32 - 41, The RunE function in the destroy command is creating a new logger instance with cmdutil.NewLogger() on each error instead of reusing the pre-initialized logger available in opts.Log. Replace the cmdutil.NewLogger().Error() call with opts.Log.Error() to use the existing logger that was set by the caller, ensuring consistency and avoiding unnecessary allocations following the established pattern in similar files like cmd/cluster/core/destroy.go.product-cli/cmd/cluster/agent/destroy.go (1)
18-22: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse the pre-initialized logger from
opts.Loginstead of allocating a new one.Line 20 creates a new logger instance for every error that occurs. The
optsparameter carries a pre-initializedlogr.Logger(set by the caller incluster.go) that should be reused for consistency and efficiency. This follows the established pattern elsewhere (e.g.,cmd/cluster/core/destroy.go).♻️ Proposed fix
cmd.RunE = func(cmd *cobra.Command, args []string) error { if err := agent.DestroyCluster(cmd.Context(), opts); err != nil { - cmdutil.NewLogger().Error(err, "Failed to destroy cluster") + opts.Log.Error(err, "Failed to destroy cluster") return err } return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@product-cli/cmd/cluster/agent/destroy.go` around lines 18 - 22, In the RunE function of the destroy command, replace the inefficient `cmdutil.NewLogger()` call in the error handling block with the pre-initialized logger from `opts.Log`. Instead of creating a new logger instance each time an error occurs when `agent.DestroyCluster` fails, use the logger that was already initialized and passed in through the options parameter. This maintains consistency with the established pattern used elsewhere in the codebase and avoids unnecessary logger allocations.product-cli/cmd/cluster/openstack/destroy.go (1)
16-39: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse the pre-initialized logger from
opts.Loginstead of allocating a new one.Line 23 creates a new logger instance and stores it in a local variable. The
optsparameter carries a pre-initializedlogr.Logger(set by the caller incluster.go) that should be reused for consistency and efficiency. This follows the established pattern elsewhere (e.g.,cmd/cluster/core/destroy.go).♻️ Proposed fix
cmd.Run = func(cmd *cobra.Command, args []string) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() sigs := make(chan os.Signal, 1) signal.Notify(sigs, syscall.SIGINT) go func() { <-sigs cancel() }() if err := openstack.DestroyCluster(ctx, opts); err != nil { - logger.Error(err, "Failed to destroy cluster") + opts.Log.Error(err, "Failed to destroy cluster") os.Exit(1) } } - logger := cmdutil.NewLogger()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@product-cli/cmd/cluster/openstack/destroy.go` around lines 16 - 39, In the NewDestroyCommand function, replace the local logger initialization (logger := cmdutil.NewLogger()) with direct use of the pre-initialized logger available in the opts parameter. Update the logger.Error call within the Run function to use opts.Log instead of the local logger variable. This ensures consistency with the established pattern in the codebase and reuses the logger that was already initialized by the caller.product-cli/cmd/cluster/kubevirt/destroy.go (1)
18-24: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse the pre-initialized logger from
opts.Loginstead of allocating a new one.Line 20 creates a new logger instance for every error that occurs. The
optsparameter carries a pre-initializedlogr.Logger(set by the caller incluster.go) that should be reused for consistency and efficiency. This follows the established pattern elsewhere (e.g.,cmd/cluster/core/destroy.go).♻️ Proposed fix
cmd.RunE = func(cmd *cobra.Command, args []string) error { if err := none.DestroyCluster(cmd.Context(), opts); err != nil { - cmdutil.NewLogger().Error(err, "Failed to destroy cluster") + opts.Log.Error(err, "Failed to destroy cluster") return err } return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@product-cli/cmd/cluster/kubevirt/destroy.go` around lines 18 - 24, The RunE function in the destroy command is creating a new logger instance with cmdutil.NewLogger() for the error log, which is inefficient and inconsistent with the pattern used elsewhere. Instead, replace the cmdutil.NewLogger().Error() call with opts.Log.Error() to reuse the pre-initialized logger that is already available in the opts parameter (which is passed by the caller and carries the context-aware logger configuration). This ensures consistency, efficiency, and proper log context propagation throughout the cluster destruction operation.cmd/cluster/azure/destroy.go (1)
49-49: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winRedundant logger creation.
opts.Logis already initialized incluster.goline 51. Creating a separateloggerhere for error reporting incmd.Runmeans this command uses two different logger instances. Consider usingopts.Logconsistently:- logger := util.NewLogger() cmd.Run = func(cmd *cobra.Command, args []string) { ctx, cancel := context.WithCancel(context.Background()) defer cancel() sigs := make(chan os.Signal, 1) signal.Notify(sigs, syscall.SIGINT) go func() { <-sigs cancel() }() if err := DestroyCluster(ctx, opts); err != nil { - logger.Error(err, "Failed to destroy cluster") + opts.Log.Error(err, "Failed to destroy cluster") os.Exit(1) } }This pattern appears across all destroy commands in this layer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/cluster/azure/destroy.go` at line 49, The code in the destroy command is creating a redundant logger instance using util.NewLogger() when opts.Log is already initialized and available. Replace the logger assignment on line 49 to use opts.Log instead of creating a new logger, and update all references to the logger variable throughout the cmd.Run function to consistently use opts.Log. This ensures the destroy command uses a single, consistent logger instance rather than maintaining two separate logger instances.cmd/infra/azure/rbac.go (1)
219-219: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winReuse one logger instance inside
assignRole.Lines 219–275 repeatedly call
cmdutil.NewLogger()in the same request path. Create one local logger once and reuse it across branches to avoid repeated setup and keep log context consistent.Proposed fix
func (r *RBACManager) assignRole(ctx context.Context, client roleAssignmentClient, infraID, component, assigneeID, role, scope string) error { + logger := cmdutil.NewLogger().WithValues("infraID", infraID, "component", component) // Generate the role assignment name roleAssignmentName := cmdutil.GenerateRoleAssignmentName(infraID, component, scope) @@ - cmdutil.NewLogger().Info("Skipping role assignment creation, matching assignment already exists.", "role", role, "assigneeID", assigneeID, "scope", scope) + logger.Info("Skipping role assignment creation, matching assignment already exists.", "role", role, "assigneeID", assigneeID, "scope", scope) @@ - cmdutil.NewLogger().Info("Skipping role assignment creation, role assignment already exists.", "role", role, "assigneeID", assigneeID, "scope", scope) + logger.Info("Skipping role assignment creation, role assignment already exists.", "role", role, "assigneeID", assigneeID, "scope", scope) @@ - cmdutil.NewLogger().Info("Deleting stale role assignment with mismatched principal", + logger.Info("Deleting stale role assignment with mismatched principal", @@ - cmdutil.NewLogger().Info("Get not permitted; will attempt create and rely on 409 for idempotency.", "role", role, "assigneeID", assigneeID, "scope", scope) + logger.Info("Get not permitted; will attempt create and rely on 409 for idempotency.", "role", role, "assigneeID", assigneeID, "scope", scope) @@ - cmdutil.NewLogger().Info("Failed role assignment creation, role assignment already exists.", "role", role, "assigneeID", assigneeID, "scope", scope) + logger.Info("Failed role assignment creation, role assignment already exists.", "role", role, "assigneeID", assigneeID, "scope", scope) @@ - cmdutil.NewLogger().Info("successfully created role assignment", "role", role, "assigneeID", assigneeID, "scope", scope) + logger.Info("successfully created role assignment", "role", role, "assigneeID", assigneeID, "scope", scope)Also applies to: 235-243, 256-257, 270-275
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/infra/azure/rbac.go` at line 219, The assignRole function repeatedly calls cmdutil.NewLogger() across multiple branches (lines 219-275), which creates unnecessary logger instances and can be inefficient. Create a single logger instance once at the beginning of the assignRole function and reuse it throughout all the branches and conditional blocks instead of calling cmdutil.NewLogger() each time. Replace all occurrences of cmdutil.NewLogger().Info() and similar calls with the reused local logger instance to maintain consistent log context and avoid repeated setup overhead.cmd/infra/powervs/create.go (1)
230-237: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winBind logger name after flag parsing.
On Line 230,
opts.InfraIDis still at its default whenNewCreateCommand()runs, so.WithName(opts.InfraID)can end up empty even when--infra-idis provided. Initialize the named logger insideRunEafter flags are applied.Proposed fix
- logger := cmdutil.NewLogger().WithName(opts.InfraID) cmd.RunE = func(cmd *cobra.Command, args []string) error { + logger := cmdutil.NewLogger().WithName(opts.InfraID) if err := opts.Run(cmd.Context(), logger); err != nil { logger.Error(err, "Failed to create infrastructure") return err }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/infra/powervs/create.go` around lines 230 - 237, The logger is being initialized with opts.InfraID before the command flags are parsed, causing opts.InfraID to be at its default value. Move the logger initialization line `logger := cmdutil.NewLogger().WithName(opts.InfraID)` from outside the RunE function into the beginning of the RunE anonymous function, after flag parsing has occurred. This ensures opts.InfraID contains the actual value from the --infra-id flag when the named logger is created.hypershift-operator/controllers/nodepool/nodepool_controller.go (1)
156-156: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winExtract hardcoded controller name to a constant for consistency.
The main controller uses
ControllerNameconstant at Line 134, but the secretjanitor controller uses a hardcoded string. For consistency and maintainability, extract this to a package-level constant.♻️ Proposed refactor
const ControllerName = "nodepool" +const SecretJanitorControllerName = "secretjanitor"Then update the usage:
if err := ctrl.NewControllerManagedBy(mgr). - Named("secretjanitor"). + Named(SecretJanitorControllerName). For(&corev1.Secret{}, builder.WithPredicates(supportutil.PredicatesForHostedClusterAnnotationScoping(mgr.GetClient()))).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hypershift-operator/controllers/nodepool/nodepool_controller.go` at line 156, The Named() method call uses a hardcoded string "secretjanitor" instead of a constant, inconsistent with how the main controller uses the ControllerName constant. Extract the hardcoded "secretjanitor" string to a package-level constant (similar to ControllerName) and replace the hardcoded string in the Named() call with this new constant to maintain consistency across the controller definitions.control-plane-operator/main.go (1)
536-536: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse the exported controller-name constant here instead of a duplicated literal.
This avoids drift if the controller name changes in one place but not the other.
Suggested change
- }).SetupWithManager(mgr, upsert.New(enableCIDebugOutput).CreateOrUpdate, hcp, mgr.GetLogger().WithName("hostedcontrolplane")); err != nil { + }).SetupWithManager( + mgr, + upsert.New(enableCIDebugOutput).CreateOrUpdate, + hcp, + mgr.GetLogger().WithName(hostedcontrolplane.ControllerName), + ); err != nil {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane-operator/main.go` at line 536, Replace the hardcoded string literal "hostedcontrolplane" in the mgr.GetLogger().WithName() call on line 536 of the SetupWithManager method invocation with an exported controller-name constant. This ensures that if the controller name needs to be updated in the future, it only needs to be changed in one place, preventing drift between different usages of the same controller name throughout the codebase.control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go (1)
605-605: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winUse structured fields for the release-image mismatch log.
Line 605 still builds one long message string, which makes filtering harder in structured log backends.
Suggested change
- log.Info("releaseImage is " + util.HCPControlPlaneReleaseImage(hostedControlPlane) + ", but this operator is configured for " + r.OperateOnReleaseImage + ", skipping reconciliation") + log.Info( + "Skipping reconciliation due to release image mismatch", + "hcpReleaseImage", util.HCPControlPlaneReleaseImage(hostedControlPlane), + "operatorReleaseImage", r.OperateOnReleaseImage, + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go` at line 605, The log.Info statement at line 605 builds a single concatenated string message using the "+" operator, which prevents structured log backends from efficiently filtering and processing the log data. Replace the string concatenation with structured logging fields by passing the release image values and operator configuration as separate key-value pairs to the log.Info call instead of concatenating them into a single message string. The values util.HCPControlPlaneReleaseImage(hostedControlPlane) and r.OperateOnReleaseImage should be passed as structured fields alongside a descriptive log message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/infra/powervs/destroy.go`:
- Line 120: The logger initialization with WithName(opts.InfraID) on line 120
occurs before Cobra parses the command-line flags, so opts.InfraID contains its
default value instead of the runtime --infra-id flag value. Move the logger
creation statement to after the Cobra flag parsing completes (typically after
opts.Complete() or similar flag binding method is called) so that opts.InfraID
contains the actual value provided by the user at runtime.
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Line 1559: The logr.Error function does not perform printf-style formatting on
the message parameter, so the `%w` verb in the message string at line 1559 will
be logged literally instead of substituting the actual error value. Remove the
`%w` printf verb from the message string in the log.Error call and any other
occurrences at lines 1766 and 1799 in the same file. The error value should be
passed as a separate parameter to logr.Error if needed for structured logging,
not embedded in the message string itself.
---
Outside diff comments:
In
`@control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go`:
- Around line 65-75: The PrivateServiceObserver struct makes direct API calls in
the Reconcile method using r.clientset.CoreV1().Services(req.Namespace).Get()
instead of leveraging the informer lister that is set up in SetupWithManager. To
fix this, add a services lister field to the PrivateServiceObserver struct to
store the informer's lister, initialize this lister in SetupWithManager when the
shared informer is created, and then update the Reconcile method to use
services.Lister().Services(req.Namespace).Get() instead of the direct client API
call to benefit from caching.
---
Nitpick comments:
In `@cmd/cluster/azure/destroy.go`:
- Line 49: The code in the destroy command is creating a redundant logger
instance using util.NewLogger() when opts.Log is already initialized and
available. Replace the logger assignment on line 49 to use opts.Log instead of
creating a new logger, and update all references to the logger variable
throughout the cmd.Run function to consistently use opts.Log. This ensures the
destroy command uses a single, consistent logger instance rather than
maintaining two separate logger instances.
In `@cmd/infra/azure/rbac.go`:
- Line 219: The assignRole function repeatedly calls cmdutil.NewLogger() across
multiple branches (lines 219-275), which creates unnecessary logger instances
and can be inefficient. Create a single logger instance once at the beginning of
the assignRole function and reuse it throughout all the branches and conditional
blocks instead of calling cmdutil.NewLogger() each time. Replace all occurrences
of cmdutil.NewLogger().Info() and similar calls with the reused local logger
instance to maintain consistent log context and avoid repeated setup overhead.
In `@cmd/infra/powervs/create.go`:
- Around line 230-237: The logger is being initialized with opts.InfraID before
the command flags are parsed, causing opts.InfraID to be at its default value.
Move the logger initialization line `logger :=
cmdutil.NewLogger().WithName(opts.InfraID)` from outside the RunE function into
the beginning of the RunE anonymous function, after flag parsing has occurred.
This ensures opts.InfraID contains the actual value from the --infra-id flag
when the named logger is created.
In
`@control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go`:
- Line 605: The log.Info statement at line 605 builds a single concatenated
string message using the "+" operator, which prevents structured log backends
from efficiently filtering and processing the log data. Replace the string
concatenation with structured logging fields by passing the release image values
and operator configuration as separate key-value pairs to the log.Info call
instead of concatenating them into a single message string. The values
util.HCPControlPlaneReleaseImage(hostedControlPlane) and r.OperateOnReleaseImage
should be passed as structured fields alongside a descriptive log message.
In `@control-plane-operator/main.go`:
- Line 536: Replace the hardcoded string literal "hostedcontrolplane" in the
mgr.GetLogger().WithName() call on line 536 of the SetupWithManager method
invocation with an exported controller-name constant. This ensures that if the
controller name needs to be updated in the future, it only needs to be changed
in one place, preventing drift between different usages of the same controller
name throughout the codebase.
In `@hypershift-operator/controllers/nodepool/nodepool_controller.go`:
- Line 156: The Named() method call uses a hardcoded string "secretjanitor"
instead of a constant, inconsistent with how the main controller uses the
ControllerName constant. Extract the hardcoded "secretjanitor" string to a
package-level constant (similar to ControllerName) and replace the hardcoded
string in the Named() call with this new constant to maintain consistency across
the controller definitions.
In `@product-cli/cmd/cluster/agent/destroy.go`:
- Around line 18-22: In the RunE function of the destroy command, replace the
inefficient `cmdutil.NewLogger()` call in the error handling block with the
pre-initialized logger from `opts.Log`. Instead of creating a new logger
instance each time an error occurs when `agent.DestroyCluster` fails, use the
logger that was already initialized and passed in through the options parameter.
This maintains consistency with the established pattern used elsewhere in the
codebase and avoids unnecessary logger allocations.
In `@product-cli/cmd/cluster/aws/destroy.go`:
- Around line 32-41: The RunE function in the destroy command is creating a new
logger instance with cmdutil.NewLogger() on each error instead of reusing the
pre-initialized logger available in opts.Log. Replace the
cmdutil.NewLogger().Error() call with opts.Log.Error() to use the existing
logger that was set by the caller, ensuring consistency and avoiding unnecessary
allocations following the established pattern in similar files like
cmd/cluster/core/destroy.go.
In `@product-cli/cmd/cluster/kubevirt/destroy.go`:
- Around line 18-24: The RunE function in the destroy command is creating a new
logger instance with cmdutil.NewLogger() for the error log, which is inefficient
and inconsistent with the pattern used elsewhere. Instead, replace the
cmdutil.NewLogger().Error() call with opts.Log.Error() to reuse the
pre-initialized logger that is already available in the opts parameter (which is
passed by the caller and carries the context-aware logger configuration). This
ensures consistency, efficiency, and proper log context propagation throughout
the cluster destruction operation.
In `@product-cli/cmd/cluster/openstack/destroy.go`:
- Around line 16-39: In the NewDestroyCommand function, replace the local logger
initialization (logger := cmdutil.NewLogger()) with direct use of the
pre-initialized logger available in the opts parameter. Update the logger.Error
call within the Run function to use opts.Log instead of the local logger
variable. This ensures consistency with the established pattern in the codebase
and reuses the logger that was already initialized by the caller.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3788e3e5-969f-465f-b6e9-9f38631bdf8a
📒 Files selected for processing (76)
cmd/bastion/aws/create.gocmd/bastion/aws/destroy.gocmd/cluster/agent/destroy.gocmd/cluster/aws/destroy.gocmd/cluster/azure/destroy.gocmd/cluster/azure/destroy_test.gocmd/cluster/cluster.gocmd/cluster/core/create.gocmd/cluster/core/destroy_test.gocmd/cluster/core/dump.gocmd/cluster/gcp/destroy.gocmd/cluster/kubevirt/destroy.gocmd/cluster/none/destroy.gocmd/cluster/openstack/destroy.gocmd/cluster/powervs/destroy.gocmd/consolelogs/aws/getlogs.gocmd/infra/aws/create.gocmd/infra/aws/create_cli_role.gocmd/infra/aws/create_iam.gocmd/infra/aws/destroy.gocmd/infra/aws/destroy_iam.gocmd/infra/azure/create.gocmd/infra/azure/create_iam.gocmd/infra/azure/destroy.gocmd/infra/azure/destroy_iam.gocmd/infra/azure/rbac.gocmd/infra/gcp/create_iam.gocmd/infra/gcp/create_infra.gocmd/infra/gcp/destroy_iam.gocmd/infra/gcp/destroy_infra.gocmd/infra/powervs/create.gocmd/infra/powervs/destroy.gocmd/log/log.gocmd/nodepool/core/create.gocmd/nodepool/powervs/create.gocmd/oadp/backup.gocmd/oadp/restore.gocmd/oadp/schedule.gocmd/oadp/schedule_test.gocmd/util/azure_test.gocmd/util/log.gocontrol-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.gocontrol-plane-operator/controllers/azureprivatelinkservice/controller.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/observer.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/cmca/configmap_observer.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/cmca/setup.gocontrol-plane-operator/main.gohypershift-operator/controllers/auditlogpersistence/configmap_webhook.gohypershift-operator/controllers/auditlogpersistence/configmap_webhook_test.gohypershift-operator/controllers/auditlogpersistence/pod_webhook.gohypershift-operator/controllers/auditlogpersistence/pod_webhook_test.gohypershift-operator/controllers/auditlogpersistence/snapshot_controller.gohypershift-operator/controllers/auditlogpersistence/snapshot_controller_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/platform/aws/controller.gohypershift-operator/controllers/platform/azure/controller.gohypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.gohypershift-operator/controllers/platform/gcp/privateserviceconnect_controller_test.gohypershift-operator/controllers/sharedingress/sharedingress_controller.gohypershift-operator/controllers/supportedversion/reconciler.gohypershift-operator/controllers/uwmtelemetry/uwm_telemetry.gohypershift-operator/main.goproduct-cli/cmd/cluster/agent/destroy.goproduct-cli/cmd/cluster/aws/destroy.goproduct-cli/cmd/cluster/cluster.goproduct-cli/cmd/cluster/kubevirt/destroy.goproduct-cli/cmd/cluster/openstack/destroy.goproduct-cli/cmd/iam/azure/create.goproduct-cli/cmd/iam/azure/destroy.goproduct-cli/cmd/infra/azure/create.goproduct-cli/cmd/infra/azure/destroy.goproduct-cli/cmd/nodepool/destroy.go
💤 Files with no reviewable changes (4)
- cmd/log/log.go
- hypershift-operator/controllers/auditlogpersistence/snapshot_controller_test.go
- hypershift-operator/main.go
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8802 +/- ##
==========================================
+ Coverage 42.11% 42.13% +0.01%
==========================================
Files 767 768 +1
Lines 95069 95095 +26
==========================================
+ Hits 40039 40067 +28
+ Misses 52216 52214 -2
Partials 2814 2814
... and 1 file with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.go`:
- Line 77: The logger.Info call that logs "Initialized GCP platform information"
includes the raw projectID field, which is customer-identifying data that should
not be exposed in shared logs. Remove the projectID field from the logger.Info
call to avoid leaking tenant context, or alternatively redact the identifier
with a masked or hashed value instead of logging the raw project ID.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 2cae6c22-dd7d-49c2-a615-11b65355bb5a
📒 Files selected for processing (76)
cmd/bastion/aws/create.gocmd/bastion/aws/destroy.gocmd/cluster/agent/destroy.gocmd/cluster/aws/destroy.gocmd/cluster/azure/destroy.gocmd/cluster/azure/destroy_test.gocmd/cluster/cluster.gocmd/cluster/core/create.gocmd/cluster/core/destroy_test.gocmd/cluster/core/dump.gocmd/cluster/gcp/destroy.gocmd/cluster/kubevirt/destroy.gocmd/cluster/none/destroy.gocmd/cluster/openstack/destroy.gocmd/cluster/powervs/destroy.gocmd/consolelogs/aws/getlogs.gocmd/infra/aws/create.gocmd/infra/aws/create_cli_role.gocmd/infra/aws/create_iam.gocmd/infra/aws/destroy.gocmd/infra/aws/destroy_iam.gocmd/infra/azure/create.gocmd/infra/azure/create_iam.gocmd/infra/azure/destroy.gocmd/infra/azure/destroy_iam.gocmd/infra/azure/rbac.gocmd/infra/gcp/create_iam.gocmd/infra/gcp/create_infra.gocmd/infra/gcp/destroy_iam.gocmd/infra/gcp/destroy_infra.gocmd/infra/powervs/create.gocmd/infra/powervs/destroy.gocmd/log/log.gocmd/nodepool/core/create.gocmd/nodepool/powervs/create.gocmd/oadp/backup.gocmd/oadp/restore.gocmd/oadp/schedule.gocmd/oadp/schedule_test.gocmd/util/azure_test.gocmd/util/log.gocontrol-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.gocontrol-plane-operator/controllers/azureprivatelinkservice/controller.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/observer.gocontrol-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.gocontrol-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/cmca/configmap_observer.gocontrol-plane-operator/hostedclusterconfigoperator/controllers/cmca/setup.gocontrol-plane-operator/main.gohypershift-operator/controllers/auditlogpersistence/configmap_webhook.gohypershift-operator/controllers/auditlogpersistence/configmap_webhook_test.gohypershift-operator/controllers/auditlogpersistence/pod_webhook.gohypershift-operator/controllers/auditlogpersistence/pod_webhook_test.gohypershift-operator/controllers/auditlogpersistence/snapshot_controller.gohypershift-operator/controllers/auditlogpersistence/snapshot_controller_test.gohypershift-operator/controllers/hostedcluster/hostedcluster_controller.gohypershift-operator/controllers/nodepool/nodepool_controller.gohypershift-operator/controllers/platform/aws/controller.gohypershift-operator/controllers/platform/azure/controller.gohypershift-operator/controllers/platform/gcp/privateserviceconnect_controller.gohypershift-operator/controllers/platform/gcp/privateserviceconnect_controller_test.gohypershift-operator/controllers/sharedingress/sharedingress_controller.gohypershift-operator/controllers/supportedversion/reconciler.gohypershift-operator/controllers/uwmtelemetry/uwm_telemetry.gohypershift-operator/main.goproduct-cli/cmd/cluster/agent/destroy.goproduct-cli/cmd/cluster/aws/destroy.goproduct-cli/cmd/cluster/cluster.goproduct-cli/cmd/cluster/kubevirt/destroy.goproduct-cli/cmd/cluster/openstack/destroy.goproduct-cli/cmd/iam/azure/create.goproduct-cli/cmd/iam/azure/destroy.goproduct-cli/cmd/infra/azure/create.goproduct-cli/cmd/infra/azure/destroy.goproduct-cli/cmd/nodepool/destroy.go
💤 Files with no reviewable changes (3)
- cmd/log/log.go
- hypershift-operator/controllers/auditlogpersistence/snapshot_controller_test.go
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller_test.go
✅ Files skipped from review due to trivial changes (15)
- product-cli/cmd/cluster/openstack/destroy.go
- cmd/util/log.go
- cmd/cluster/none/destroy.go
- cmd/infra/gcp/create_iam.go
- cmd/infra/gcp/destroy_iam.go
- cmd/cluster/kubevirt/destroy.go
- control-plane-operator/main.go
- hypershift-operator/controllers/sharedingress/sharedingress_controller.go
- cmd/cluster/agent/destroy.go
- cmd/cluster/core/destroy_test.go
- hypershift-operator/controllers/auditlogpersistence/snapshot_controller.go
- cmd/infra/powervs/create.go
- cmd/oadp/schedule_test.go
- product-cli/cmd/nodepool/destroy.go
- cmd/cluster/azure/destroy.go
🚧 Files skipped from review as they are similar to previous changes (56)
- product-cli/cmd/infra/azure/create.go
- product-cli/cmd/cluster/kubevirt/destroy.go
- hypershift-operator/controllers/platform/aws/controller.go
- product-cli/cmd/iam/azure/destroy.go
- product-cli/cmd/infra/azure/destroy.go
- product-cli/cmd/cluster/aws/destroy.go
- cmd/bastion/aws/create.go
- cmd/cluster/openstack/destroy.go
- cmd/cluster/gcp/destroy.go
- control-plane-operator/controllers/gcpprivateserviceconnect/psc_endpoint_controller.go
- hypershift-operator/controllers/auditlogpersistence/configmap_webhook_test.go
- cmd/cluster/cluster.go
- cmd/nodepool/powervs/create.go
- cmd/infra/gcp/destroy_infra.go
- cmd/infra/gcp/create_infra.go
- hypershift-operator/controllers/supportedversion/reconciler.go
- product-cli/cmd/iam/azure/create.go
- cmd/oadp/schedule.go
- control-plane-operator/controllers/azureprivatelinkservice/controller.go
- hypershift-operator/controllers/uwmtelemetry/uwm_telemetry.go
- cmd/infra/aws/destroy_iam.go
- hypershift-operator/controllers/platform/azure/controller.go
- hypershift-operator/controllers/nodepool/nodepool_controller.go
- control-plane-operator/hostedclusterconfigoperator/controllers/cmca/setup.go
- hypershift-operator/controllers/auditlogpersistence/pod_webhook_test.go
- cmd/infra/aws/create_cli_role.go
- cmd/oadp/backup.go
- hypershift-operator/controllers/platform/gcp/privateserviceconnect_controller_test.go
- cmd/infra/azure/create_iam.go
- cmd/util/azure_test.go
- cmd/cluster/core/dump.go
- cmd/bastion/aws/destroy.go
- cmd/oadp/restore.go
- hypershift-operator/main.go
- cmd/cluster/azure/destroy_test.go
- cmd/cluster/powervs/destroy.go
- product-cli/cmd/cluster/cluster.go
- cmd/infra/azure/destroy_iam.go
- cmd/cluster/core/create.go
- cmd/infra/azure/destroy.go
- cmd/consolelogs/aws/getlogs.go
- cmd/infra/aws/create_iam.go
- cmd/infra/aws/destroy.go
- control-plane-operator/controllers/gcpprivateserviceconnect/observer.go
- hypershift-operator/controllers/auditlogpersistence/configmap_webhook.go
- cmd/cluster/aws/destroy.go
- cmd/infra/powervs/destroy.go
- cmd/nodepool/core/create.go
- control-plane-operator/hostedclusterconfigoperator/controllers/cmca/configmap_observer.go
- hypershift-operator/controllers/auditlogpersistence/pod_webhook.go
- cmd/infra/aws/create.go
- cmd/infra/azure/create.go
- hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
- cmd/infra/azure/rbac.go
- control-plane-operator/controllers/awsprivatelink/awsprivatelink_controller.go
- control-plane-operator/controllers/hostedcontrolplane/hostedcontrolplane_controller.go
We want to ensure that contextual, structural logging is used everywhere. When it's not, crucial information like what reconciler is spitting out the log or what key is being reconciled is missing, leaving users with no easy way to write structured queries over our outputs. Removed the `pkg/log` export as it's always the wrong thing to use. Signed-off-by: Steve Kuznetsov <stekuznetsov@microsoft.com>
stevekuznetsov
force-pushed
the
skuznets/controller-logs
branch
from
3992c0f to
9c3df5c
Compare
Signed-off-by: Steve Kuznetsov <stekuznetsov@microsoft.com>
csrwng
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@stevekuznetsov: This pull request references CNTRLPLANE-3701 which is a valid jira issue. DetailsIn response to this:
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. |
|
/override codecov/patch This is simply changing how logging is invoked, no new logic is introduced. |
|
@csrwng: Overrode contexts on behalf of csrwng: codecov/patch DetailsIn response to this:
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. |
csrwng
left a comment
csrwng
left a comment
There was a problem hiding this comment.
LGTM. Clean and consistent migration to contextual structured logging.
Key wins:
- Fixes the
r.Logconcurrency bug (shared mutable state across reconcile calls) - Deterministic
ControllerNameconstants for stable log identifiers - Removes
cmd/log.Logsingleton that controllers shouldn't have been using - Incidentally fixes incorrect
%wformat verbs inlogr.Error()calls
Mechanical changes are all consistent — verified a representative sample across controllers, webhooks, and CLI layer.
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: csrwng, stevekuznetsov The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Test Resultse2e-aks
e2e-aws
|
|
@stevekuznetsov: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
All those are TestCreateClusterHABreakGlassCredentials and TestCreateClusterProxy (different tests). No output from TestCreateCluster (the one without suffix) after line 2417. The test was stuck waiting for EnsureMetricsForwarderWorking for the remaining ~65 minutes until timeout. Now I have all the evidence I need. Let me produce the final report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryThe Root CauseThe root cause is a test timing flake — Detailed chain of events:
Why the metrics-forwarder target was not found: The metrics forwarding data path ( Why this is unrelated to PR #8802: The PR modifies 76 files, all related to logging changes ( Recommendations
Evidence
|
4 participants
We want to ensure that contextual, structural logging is used everywhere. When it's not, crucial information like what reconciler is spitting out the log or what key is being reconciled is missing, leaving users with no easy way to write structured queries over our outputs. Removed the
pkg/logexport as it's always the wrong thing to use.What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes
Special notes for your reviewer:
Checklist:
Summary by CodeRabbit
NewLogger()instances (including consistent timestamp formatting).