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

sivchari · 2024-07-19T02:09:22Z

What this PR does / why we need it:

Currently, there is no unity to handle error when the resource is not found. So I added log and fixed error handling using apierrors.IsNotFound.
In additional, the controller of control plane used requeue when the error occurred, but this behavior should return error. So I fixed it.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Signed-off-by: sivchari <shibuuuu5@gmail.com>

sivchari · 2024-07-19T03:18:44Z

/assign

chrischdi · 2024-07-19T06:44:42Z

/retitle 🐛 Fix error handling when the resource is not found

chrischdi · 2024-07-19T06:47:24Z

/test help

k8s-ci-robot · 2024-07-19T06:47:27Z

@chrischdi: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-build-main
/test pull-cluster-api-e2e-blocking-main
/test pull-cluster-api-e2e-conformance-ci-latest-main
/test pull-cluster-api-e2e-conformance-main
/test pull-cluster-api-e2e-main
/test pull-cluster-api-e2e-mink8s-main
/test pull-cluster-api-e2e-upgrade-1-30-1-31-main
/test pull-cluster-api-test-main
/test pull-cluster-api-test-mink8s-main
/test pull-cluster-api-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-apidiff-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-apidiff-main
pull-cluster-api-build-main
pull-cluster-api-e2e-blocking-main
pull-cluster-api-test-main
pull-cluster-api-verify-main

In response to this:

/test help

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.

chrischdi · 2024-07-19T06:47:33Z

/test pull-cluster-api-e2e-main

sbueringer · 2024-07-19T07:01:31Z

bootstrap/kubeadm/internal/controllers/kubeadmconfig_controller.go

@@ -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")


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)

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.

Signed-off-by: sivchari <shibuuuu5@gmail.com>

controlplane/kubeadm/internal/controllers/controller.go

controlplane/kubeadm/internal/controllers/controller_test.go

internal/controllers/cluster/cluster_controller.go

Signed-off-by: sivchari <shibuuuu5@gmail.com>

sbueringer

@sivchari one nit, otherwise happy to merge

controlplane/kubeadm/internal/controllers/controller.go

Signed-off-by: sivchari <shibuuuu5@gmail.com>

sbueringer · 2024-08-21T05:58:11Z

Thx!

/lgtm
/approve

k8s-ci-robot · 2024-08-21T05:58:17Z

LGTM label has been added.

Git tree hash: 70b49df707f05f5788970c3bdb0619686f6a9e3b

k8s-ci-robot · 2024-08-21T05:58:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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:

~~OWNERS~~ [sbueringer]

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

sbueringer · 2024-08-21T05:59:24Z

Not sure if we have a good area label. logging describes at least a subset of the change

sbueringer · 2024-08-21T05:59:38Z

/test pull-cluster-api-e2e-main

sbueringer · 2024-08-27T09:00:09Z

Re-added the labels manually, damn race condition with Prow.. :)

regenerate CRDs 🌱 test: improve autoscale tests for to/from zero and running autoscaler in bootstrap cluster (kubernetes-sigs#11082) * test: allow deploying autoscaler to management cluster * test: make machine pools optional in autoscaler test * test: implement optional scale from/to zero tests for autoscale * test: allow modification of apigroup for infrastructure * test: wait for rollouts to finish in autoscaler tests * test: drop cleaning up autoscaler for machine pools * review fix * add comment about AutoScaleFromZero * remove autoscale from zero test for unsupported MachinePools * review fixes update cert-manager to 1.15.3 Signed-off-by: Troy Connor <troy0820@users.noreply.github.com> Collect additional logs with CAPD log collector Signed-off-by: Alexandr Demicev <alexandr.demicev@suse.com> :seedling: Bump tj-actions/changed-files in the all-github-actions group Bumps the all-github-actions group with 1 update: [tj-actions/changed-files](https://github.com/tj-actions/changed-files). Updates `tj-actions/changed-files` from 44.5.7 to 45.0.0 - [Release notes](https://github.com/tj-actions/changed-files/releases) - [Changelog](https://github.com/tj-actions/changed-files/blob/main/HISTORY.md) - [Commits](tj-actions/changed-files@c65cd88...40853de) --- updated-dependencies: - dependency-name: tj-actions/changed-files dependency-type: direct:production update-type: version-update:semver-major dependency-group: all-github-actions ... Signed-off-by: dependabot[bot] <support@github.com> :seedling: Bump google.golang.org/api Bumps the all-go-mod-patch-and-minor group with 1 update in the /hack/tools directory: [google.golang.org/api](https://github.com/googleapis/google-api-go-client). Updates `google.golang.org/api` from 0.193.0 to 0.194.0 - [Release notes](https://github.com/googleapis/google-api-go-client/releases) - [Changelog](https://github.com/googleapis/google-api-go-client/blob/main/CHANGES.md) - [Commits](googleapis/google-api-go-client@v0.193.0...v0.194.0) --- updated-dependencies: - dependency-name: google.golang.org/api dependency-type: direct:production update-type: version-update:semver-minor dependency-group: all-go-mod-patch-and-minor ... Signed-off-by: dependabot[bot] <support@github.com> 🌱 Fix error handling when the resource is not found (kubernetes-sigs#10907) * fix: error handling when the resource is not found Signed-off-by: sivchari <shibuuuu5@gmail.com> * fix: test * fix: owner cluster handling Signed-off-by: sivchari <shibuuuu5@gmail.com> * remove duplicated error Signed-off-by: sivchari <shibuuuu5@gmail.com> * remove log variable Signed-off-by: sivchari <shibuuuu5@gmail.com> * fix error handling when the controller reads the cluster Signed-off-by: sivchari <shibuuuu5@gmail.com> * revert test modification Signed-off-by: sivchari <shibuuuu5@gmail.com> * delete log Signed-off-by: sivchari <shibuuuu5@gmail.com> * remove unnecessary deletion Signed-off-by: sivchari <shibuuuu5@gmail.com> * add detail of error Signed-off-by: sivchari <shibuuuu5@gmail.com> --------- Signed-off-by: sivchari <shibuuuu5@gmail.com> Add nilIsZero to all KSM metric configs where needed Signed-off-by: Tobias Giese <tgiese@nvidia.com> sorted labels and annotations in alphabatical order Signed-off-by: hackeramitkumar <amit9116260192@gmail.com> 📖 Fix CAPZ redirection links in quick-start page Trigger Build Trigger Build Add nilIsZero to all KSM metric configs where needed Signed-off-by: Tobias Giese <tgiese@nvidia.com> sorted labels and annotations in alphabatical order Signed-off-by: hackeramitkumar <amit9116260192@gmail.com> 📖 Fix CAPZ redirection links in quick-start page Trigger Build 📖 Fix CAPZ redirection links in quick-start page

fix: error handling when the resource is not found

3d3199b

Signed-off-by: sivchari <shibuuuu5@gmail.com>

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-area PR is missing an area label labels Jul 19, 2024

k8s-ci-robot requested review from JoelSpeed and richardcase July 19, 2024 02:09

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 19, 2024

sivchari added 2 commits July 19, 2024 11:28

fix: test

34989ac

fix: owner cluster handling

0b2867b

Signed-off-by: sivchari <shibuuuu5@gmail.com>

sivchari force-pushed the fix-err-handling branch from f0bb8a2 to 0b2867b Compare July 19, 2024 02:55

k8s-ci-robot assigned sivchari Jul 19, 2024

k8s-ci-robot changed the title ~~🐛 Fix error handling when the resource is not fouund~~ 🐛 Fix error handling when the resource is not found Jul 19, 2024

sbueringer reviewed Jul 19, 2024

View reviewed changes

sivchari added 2 commits July 19, 2024 16:53

remove duplicated error

38390e4

Signed-off-by: sivchari <shibuuuu5@gmail.com>

remove log variable

ca19b57

Signed-off-by: sivchari <shibuuuu5@gmail.com>

sivchari changed the title ~~🐛 Fix error handling when the resource is not found~~ :see Fix error handling when the resource is not found Jul 23, 2024

sivchari changed the title ~~:see Fix error handling when the resource is not found~~ 🌱 Fix error handling when the resource is not found Jul 23, 2024

sivchari commented Jul 24, 2024

View reviewed changes

controlplane/kubeadm/internal/controllers/controller.go Show resolved Hide resolved

fabriziopandini reviewed Aug 2, 2024

View reviewed changes

fix error handling when the controller reads the cluster

11bb972

Signed-off-by: sivchari <shibuuuu5@gmail.com>

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 3, 2024

sivchari added 3 commits August 3, 2024 11:44

revert test modification

ad13de1

Signed-off-by: sivchari <shibuuuu5@gmail.com>

delete log

a827815

Signed-off-by: sivchari <shibuuuu5@gmail.com>

remove unnecessary deletion

32083e4

Signed-off-by: sivchari <shibuuuu5@gmail.com>

sivchari force-pushed the fix-err-handling branch from cf38757 to 32083e4 Compare August 3, 2024 02:49

sbueringer added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Aug 20, 2024

sbueringer reviewed Aug 20, 2024

View reviewed changes

controlplane/kubeadm/internal/controllers/controller.go Outdated Show resolved Hide resolved

add detail of error

9eb3d90

Signed-off-by: sivchari <shibuuuu5@gmail.com>

k8s-ci-robot assigned sbueringer Aug 21, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 21, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 21, 2024

sbueringer added area/logging Issues or PRs related to logging and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 21, 2024

k8s-ci-robot removed the do-not-merge/needs-area PR is missing an area label label Aug 21, 2024

sbueringer added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Aug 27, 2024

k8s-ci-robot merged commit 12b75cd into kubernetes-sigs:main Aug 27, 2024
24 checks passed

k8s-ci-robot added this to the v1.9 milestone Aug 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

sivchari commented Jul 19, 2024 •

edited

Loading

sivchari commented Jul 19, 2024

chrischdi commented Jul 19, 2024

chrischdi commented Jul 19, 2024

k8s-ci-robot commented Jul 19, 2024

chrischdi commented Jul 19, 2024

sbueringer Jul 19, 2024

sivchari Jul 19, 2024 •

edited

Loading

sbueringer left a comment

sbueringer commented Aug 21, 2024

k8s-ci-robot commented Aug 21, 2024

k8s-ci-robot commented Aug 21, 2024

sbueringer commented Aug 21, 2024

sbueringer commented Aug 21, 2024

sbueringer commented Aug 27, 2024

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

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

Conversation

sivchari commented Jul 19, 2024 • edited Loading

sivchari commented Jul 19, 2024

chrischdi commented Jul 19, 2024

chrischdi commented Jul 19, 2024

k8s-ci-robot commented Jul 19, 2024

chrischdi commented Jul 19, 2024

sbueringer Jul 19, 2024

Choose a reason for hiding this comment

sivchari Jul 19, 2024 • edited Loading

Choose a reason for hiding this comment

sbueringer left a comment

Choose a reason for hiding this comment

sbueringer commented Aug 21, 2024

k8s-ci-robot commented Aug 21, 2024

k8s-ci-robot commented Aug 21, 2024

sbueringer commented Aug 21, 2024

sbueringer commented Aug 21, 2024

sbueringer commented Aug 27, 2024

sivchari commented Jul 19, 2024 •

edited

Loading

sivchari Jul 19, 2024 •

edited

Loading