Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Improve log k/v pairs and a improve/drop a few log lines #9813

Merged
merged 1 commit into from
Dec 14, 2023
Merged

🐛 Improve log k/v pairs and a improve/drop a few log lines #9813

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 12 additions & 11 deletions bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,14 +147,6 @@ func (r *KubeadmConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques
if apierrors.IsNotFound(err) {
return ctrl.Result{}, nil
}
log.Error(err, "Failed to get config")
return ctrl.Result{}, err
}

// AddOwners adds the owners of KubeadmConfig as k/v pairs to the logger.
// Specifically, it will add KubeadmControlPlane, MachineSet and MachineDeployment.
ctx, log, err := clog.AddOwners(ctx, r.Client, config)
if err != nil {
return ctrl.Result{}, err
}

Expand All @@ -165,13 +157,22 @@ func (r *KubeadmConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques
return ctrl.Result{}, nil
}
if err != nil {
log.Error(err, "Failed to get owner")
return ctrl.Result{}, err
return ctrl.Result{}, errors.Wrapf(err, "failed to get owner")
}
if configOwner == nil {
return ctrl.Result{}, nil
}
log = log.WithValues(configOwner.GetKind(), klog.KRef(configOwner.GetNamespace(), configOwner.GetName()), "resourceVersion", configOwner.GetResourceVersion())
ctx = ctrl.LoggerInto(ctx, log)

if configOwner.GetKind() == "Machine" {
// AddOwners adds the owners of Machine as k/v pairs to the logger.
// Specifically, it will add KubeadmControlPlane, MachineSet and MachineDeployment.
ctx, log, err = clog.AddOwners(ctx, r.Client, configOwner)
Copy link
Member Author

Choose a reason for hiding this comment

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

We have to call clog.AddOwner with the Machine and not with the KubeadmConfig. Only the Machine has the KCP, MS, MD owner hierarchy

if err != nil {
return ctrl.Result{}, err
}
}

log = log.WithValues("Cluster", klog.KRef(configOwner.GetNamespace(), configOwner.ClusterName()))
ctx = ctrl.LoggerInto(ctx, log)
Expand Down Expand Up @@ -1043,7 +1044,7 @@ func (r *KubeadmConfigReconciler) storeBootstrapData(ctx context.Context, scope
if !apierrors.IsAlreadyExists(err) {
return errors.Wrapf(err, "failed to create bootstrap data secret for KubeadmConfig %s/%s", scope.Config.Namespace, scope.Config.Name)
}
log.Info("bootstrap data secret for KubeadmConfig already exists, updating", "Secret", klog.KObj(secret))
log.Info("Bootstrap data secret for KubeadmConfig already exists, updating", "Secret", klog.KObj(secret))
if err := r.Client.Update(ctx, secret); err != nil {
return errors.Wrapf(err, "failed to update bootstrap data secret for KubeadmConfig %s/%s", scope.Config.Namespace, scope.Config.Name)
}
Expand Down
3 changes: 1 addition & 2 deletions bootstrap/kubeadm/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -294,13 +294,12 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) {

// Set up a ClusterCacheTracker and ClusterCacheReconciler to provide to controllers
// requiring a connection to a remote cluster
log := ctrl.Log.WithName("remote").WithName("ClusterCacheTracker")
tracker, err := remote.NewClusterCacheTracker(
mgr,
remote.ClusterCacheTrackerOptions{
SecretCachingClient: secretCachingClient,
ControllerName: controllerName,
Log: &log,
Log: &ctrl.Log,
},
)
if err != nil {
Expand Down
3 changes: 3 additions & 0 deletions controllers/remote/cluster_cache_tracker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,9 @@ func setDefaultOptions(opts *ClusterCacheTrackerOptions) {
opts.Log = &l
}

l := opts.Log.WithValues("component", "remote/clustercachetracker")
opts.Log = &l

if len(opts.ClientUncachedObjects) == 0 {
opts.ClientUncachedObjects = []client.Object{
&corev1.ConfigMap{},
Expand Down
3 changes: 1 addition & 2 deletions controlplane/kubeadm/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,11 +302,10 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) {

// Set up a ClusterCacheTracker to provide to controllers
// requiring a connection to a remote cluster
log := ctrl.Log.WithName("remote").WithName("ClusterCacheTracker")
tracker, err := remote.NewClusterCacheTracker(mgr, remote.ClusterCacheTrackerOptions{
SecretCachingClient: secretCachingClient,
ControllerName: controllerName,
Log: &log,
Log: &ctrl.Log,
ClientUncachedObjects: []client.Object{
&corev1.ConfigMap{},
&corev1.Secret{},
Expand Down
2 changes: 1 addition & 1 deletion docs/book/src/tasks/experimental-features/runtime-sdk/implement-extensions.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ func InitFlags(fs *pflag.FlagSet) {

func main() {
// Creates a logger to be used during the main func.
setupLog := ctrl.Log.WithName("main")
setupLog := ctrl.Log.WithName("setup")

// Initialize and parse command line flags.
InitFlags(pflag.CommandLine)
Expand Down
3 changes: 1 addition & 2 deletions internal/controllers/cluster/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,11 +64,10 @@ func TestMain(m *testing.M) {
setupReconcilers := func(ctx context.Context, mgr ctrl.Manager) {
// Set up a ClusterCacheTracker and ClusterCacheReconciler to provide to controllers
// requiring a connection to a remote cluster
log := ctrl.Log.WithName("remote").WithName("ClusterCacheTracker")
tracker, err := remote.NewClusterCacheTracker(
mgr,
remote.ClusterCacheTrackerOptions{
Log: &log,
Log: &ctrl.Log,
Indexes: []remote.Index{remote.NodeProviderIDIndex},
},
)
Expand Down
3 changes: 1 addition & 2 deletions internal/controllers/machine/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,11 +69,10 @@ func TestMain(m *testing.M) {
setupReconcilers := func(ctx context.Context, mgr ctrl.Manager) {
// Set up a ClusterCacheTracker and ClusterCacheReconciler to provide to controllers
// requiring a connection to a remote cluster
log := ctrl.Log.WithName("remote").WithName("ClusterCacheTracker")
tracker, err := remote.NewClusterCacheTracker(
mgr,
remote.ClusterCacheTrackerOptions{
Log: &log,
Log: &ctrl.Log,
Indexes: []remote.Index{remote.NodeProviderIDIndex},
},
)
Expand Down
1 change: 0 additions & 1 deletion internal/controllers/machinedeployment/machinedeployment_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re

err = r.reconcile(ctx, cluster, deployment)
if err != nil {
log.Error(err, "Failed to reconcile MachineDeployment")
r.recorder.Eventf(deployment, corev1.EventTypeWarning, "ReconcileError", "%v", err)
}
return ctrl.Result{}, err
Expand Down
3 changes: 1 addition & 2 deletions internal/controllers/machinedeployment/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,10 @@ func TestMain(m *testing.M) {
setupReconcilers := func(ctx context.Context, mgr ctrl.Manager) {
// Set up a ClusterCacheTracker and ClusterCacheReconciler to provide to controllers
// requiring a connection to a remote cluster
log := ctrl.Log.WithName("remote").WithName("ClusterCacheTracker")
tracker, err := remote.NewClusterCacheTracker(
mgr,
remote.ClusterCacheTrackerOptions{
Log: &log,
Log: &ctrl.Log,
Indexes: []remote.Index{remote.NodeProviderIDIndex},
},
)
Expand Down
3 changes: 1 addition & 2 deletions internal/controllers/machinehealthcheck/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,11 +67,10 @@ func TestMain(m *testing.M) {
setupReconcilers := func(ctx context.Context, mgr ctrl.Manager) {
// Set up a ClusterCacheTracker and ClusterCacheReconciler to provide to controllers
// requiring a connection to a remote cluster
log := ctrl.Log.WithName("remote").WithName("ClusterCacheTracker")
tracker, err := remote.NewClusterCacheTracker(
mgr,
remote.ClusterCacheTrackerOptions{
Log: &log,
Log: &ctrl.Log,
Indexes: []remote.Index{remote.NodeProviderIDIndex},
},
)
Expand Down
2 changes: 1 addition & 1 deletion internal/controllers/machineset/machineset_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -890,7 +890,7 @@ func (r *Reconciler) updateStatus(ctx context.Context, cluster *clusterv1.Cluste
availableReplicasCount++
}
} else if machine.GetDeletionTimestamp().IsZero() {
log.Info("Waiting for the Kubernetes node on the machine to report ready state")
log.V(4).Info("Waiting for the Kubernetes node on the machine to report ready state")
Copy link
Member Author

Choose a reason for hiding this comment

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

This is way to verbose on log level 0 and seems totally out-of-place (it's confusing if the MS controller is logging on log level 0 that it's waiting for a node to become ready. It is only doing that for status calculation but otherwise doesn't influence MS reconciliation). Similar logs around here use log level 4

Copy link
Contributor

Choose a reason for hiding this comment

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

Ever so slight nit to break this out into its own PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

I moved the event recorder stuff to a separate PR and changed the scope of this PR to "fix k/v paris and improve/drop a few log lines". I think that should be okay

}
}

Expand Down
3 changes: 1 addition & 2 deletions internal/controllers/machineset/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,11 +70,10 @@ func TestMain(m *testing.M) {
setupReconcilers := func(ctx context.Context, mgr ctrl.Manager) {
// Set up a ClusterCacheTracker and ClusterCacheReconciler to provide to controllers
// requiring a connection to a remote cluster
log := ctrl.Log.WithName("remote").WithName("ClusterCacheTracker")
tracker, err := remote.NewClusterCacheTracker(
mgr,
remote.ClusterCacheTrackerOptions{
Log: &log,
Log: &ctrl.Log,
Indexes: []remote.Index{remote.NodeProviderIDIndex},
},
)
Expand Down
3 changes: 1 addition & 2 deletions internal/controllers/topology/cluster/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ func TestMain(m *testing.M) {
}
// Set up a ClusterCacheTracker and ClusterCacheReconciler to provide to controllers
// requiring a connection to a remote cluster
log := ctrl.Log.WithName("remote").WithName("ClusterCacheTracker")
secretCachingClient, err := client.New(mgr.GetConfig(), client.Options{
HTTPClient: mgr.GetHTTPClient(),
Cache: &client.CacheOptions{
Expand All @@ -89,7 +88,7 @@ func TestMain(m *testing.M) {
tracker, err := remote.NewClusterCacheTracker(
mgr,
remote.ClusterCacheTrackerOptions{
Log: &log,
Log: &ctrl.Log,
SecretCachingClient: secretCachingClient,
},
)
Expand Down
3 changes: 1 addition & 2 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,13 +380,12 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) {

// Set up a ClusterCacheTracker and ClusterCacheReconciler to provide to controllers
// requiring a connection to a remote cluster
log := ctrl.Log.WithName("remote").WithName("ClusterCacheTracker")
tracker, err := remote.NewClusterCacheTracker(
mgr,
remote.ClusterCacheTrackerOptions{
SecretCachingClient: secretCachingClient,
ControllerName: controllerName,
Log: &log,
Log: &ctrl.Log,
Indexes: []remote.Index{remote.NodeProviderIDIndex},
},
)
Expand Down
3 changes: 1 addition & 2 deletions test/infrastructure/docker/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,13 +303,12 @@ func setupReconcilers(ctx context.Context, mgr ctrl.Manager) {
os.Exit(1)
}

log := ctrl.Log.WithName("remote").WithName("ClusterCacheTracker")
tracker, err := remote.NewClusterCacheTracker(
mgr,
remote.ClusterCacheTrackerOptions{
SecretCachingClient: secretCachingClient,
ControllerName: controllerName,
Log: &log,
Log: &ctrl.Log,
},
)
if err != nil {
Expand Down