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

🌱 Fix error handling when the resource is not found #10907

Merged
merged 10 commits into from
Aug 27, 2024
+9 −8
Merged

🌱 Fix error handling when the resource is not found #10907

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3d3199b
fix: error handling when the resource is not found
sivchari Jul 19, 2024
34989ac
fix: test
sivchari Jul 19, 2024
0b2867b
fix: owner cluster handling
sivchari Jul 19, 2024
38390e4
remove duplicated error
sivchari Jul 19, 2024
ca19b57
remove log variable
sivchari Jul 19, 2024
11bb972
fix error handling when the controller reads the cluster
sivchari Aug 3, 2024
ad13de1
revert test modification
sivchari Aug 3, 2024
a827815
delete log
sivchari Aug 3, 2024
32083e4
remove unnecessary deletion
sivchari Aug 3, 2024
9eb3d90
add detail of error
sivchari Aug 21, 2024
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
11 changes: 7 additions & 4 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 @@ -152,16 +152,19 @@ func (r *KubeadmConfigReconciler) Reconcile(ctx context.Context, req ctrl.Reques
if apierrors.IsNotFound(err) {
return ctrl.Result{}, nil
}
// Error reading the object - requeue the request.
log.Error(err, "Failed to fetch KubeadmConfig")
Copy link
Member

Choose a reason for hiding this comment

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

Just a quick first comment. We intentionally try to not log & return errors. The problem is that doing this leads to duplicate error messages in the logs (especially if this is done across multiple levels of a call stack)

(there are some exceptions, but only if we think there is some information that we should immediately surface (e.g. additional key value pairs from the logger)

Copy link
Member Author

@sivchari sivchari Jul 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

OK, I'd remove the log later.

The problem is that doing this leads to duplicate error messages in the logs

I've met the this problem many times and I was about to repeat same thing. Thank you for your good review.

return ctrl.Result{}, err
}

// Look up the owner of this kubeadm config if there is one
configOwner, err := bsutil.GetTypedConfigOwner(ctx, r.Client, config)
if apierrors.IsNotFound(err) {
// Could not find the owner yet, this is not an error and will rereconcile when the owner gets set.
return ctrl.Result{}, nil
}
if err != nil {
if apierrors.IsNotFound(err) {
// Could not find the owner yet, this is not an error and will rereconcile when the owner gets set.
return ctrl.Result{}, nil
}
log.Error(err, "Failed to retrieve owner Cluster from the API Server")
return ctrl.Result{}, errors.Wrapf(err, "failed to get owner")
}
if configOwner == nil {
Expand Down
14 changes: 10 additions & 4 deletions controlplane/kubeadm/internal/controllers/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,19 +145,25 @@ func (r *KubeadmControlPlaneReconciler) Reconcile(ctx context.Context, req ctrl.
if apierrors.IsNotFound(err) {
return ctrl.Result{}, nil
}
return ctrl.Result{Requeue: true}, nil

// Error reading the object - requeue the request.
log.Error(err, "Failed to fetch KubeadmControlPlane")
return ctrl.Result{}, err
}

// Fetch the Cluster.
cluster, err := util.GetOwnerCluster(ctx, r.Client, kcp.ObjectMeta)
if err != nil {
log.Error(err, "Failed to retrieve owner Cluster from the API Server")
return ctrl.Result{}, err
if !apierrors.IsNotFound(err) {
log.Error(err, "Failed to retrieve owner Cluster from the API Server")
return ctrl.Result{}, err
}
}
if cluster == nil {
if apierrors.IsNotFound(err) || cluster == nil {
log.Info("Cluster Controller has not yet set OwnerRef")
return ctrl.Result{}, nil
}

log = log.WithValues("Cluster", klog.KObj(cluster))
ctx = ctrl.LoggerInto(ctx, log)

Expand Down
6 changes: 3 additions & 3 deletions controlplane/kubeadm/internal/controllers/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ func TestClusterToKubeadmControlPlaneOtherControlPlane(t *testing.T) {
g.Expect(got).To(BeNil())
}

func TestReconcileReturnErrorWhenOwnerClusterIsMissing(t *testing.T) {
func TestReconcileDoesNothingWhenOwnerClusterIsMissing(t *testing.T) {
g := NewWithT(t)

ns, err := env.CreateNamespace(ctx, "test-reconcile-return-error")
Expand Down Expand Up @@ -165,7 +165,7 @@ func TestReconcileReturnErrorWhenOwnerClusterIsMissing(t *testing.T) {
g.Eventually(func() error {
_, err := r.Reconcile(ctx, ctrl.Request{NamespacedName: util.ObjectKey(kcp)})
return err
}, 10*time.Second).Should(HaveOccurred())
}, 10*time.Second).Should(BeNil())
}

func TestReconcileUpdateObservedGeneration(t *testing.T) {
Expand Down Expand Up @@ -343,7 +343,7 @@ func TestReconcileNoCluster(t *testing.T) {
}

_, err = r.Reconcile(ctx, ctrl.Request{NamespacedName: util.ObjectKey(kcp)})
g.Expect(err).To(HaveOccurred())
g.Expect(err).ToNot(HaveOccurred())

machineList := &clusterv1.MachineList{}
g.Expect(fakeClient.List(ctx, machineList, client.InNamespace(metav1.NamespaceDefault))).To(Succeed())
Expand Down
1 change: 1 addition & 0 deletions internal/controllers/cluster/cluster_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
}

// Error reading the object - requeue the request.
log.Error(err, "Failed to fetch Cluster")
return ctrl.Result{}, err
}

Expand Down
1 change: 1 addition & 0 deletions internal/controllers/clusterclass/clusterclass_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
return ctrl.Result{}, nil
}
// Error reading the object - requeue the request.
log.Error(err, "Failed to fetch ClusterClass")
return ctrl.Result{}, err
}

Expand Down
3 changes: 3 additions & 0 deletions internal/controllers/machine/machine_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,8 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt
}

func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Result, reterr error) {
log := ctrl.LoggerFrom(ctx)

// Fetch the Machine instance
m := &clusterv1.Machine{}
if err := r.Client.Get(ctx, req.NamespacedName, m); err != nil {
Expand All @@ -162,6 +164,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
}

// Error reading the object - requeue the request.
log.Error(err, "failed to fetch Machine")
return ctrl.Result{}, err
}

Expand Down
1 change: 1 addition & 0 deletions internal/controllers/machinedeployment/machinedeployment_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
return ctrl.Result{}, nil
}
// Error reading the object - requeue the request.
log.Error(err, "Failed to fetch MachineDeployment")
return ctrl.Result{}, err
}

Expand Down
3 changes: 3 additions & 0 deletions internal/controllers/machineset/machineset_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,8 @@ func (r *Reconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager, opt
}

func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Result, reterr error) {
log := ctrl.LoggerFrom(ctx)

machineSet := &clusterv1.MachineSet{}
if err := r.Client.Get(ctx, req.NamespacedName, machineSet); err != nil {
if apierrors.IsNotFound(err) {
Expand All @@ -141,6 +143,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (_ ctrl.Re
return ctrl.Result{}, nil
}
// Error reading the object - requeue the request.
log.Error(err, "Failed to fetch MachineSet")
return ctrl.Result{}, err
}

Expand Down
Loading