🐛 Remove generated names from error messages to reduce reconciliation #5971

killianmuldoon · 2022-01-21T19:33:22Z

Signed-off-by: killianmuldoon [email protected]

Sanitize error messages in the topology controller to prevent reported errors from causing reconciliation.

This is a Work In Progress and should not be merged.

internal/controllers/topology/cluster/reconcile_state.go

fabriziopandini · 2022-01-24T11:37:23Z

internal/controllers/topology/cluster/reconcile_state.go

@@ -269,7 +269,7 @@ func (r *Reconciler) reconcileMachineHealthCheck(ctx context.Context, current, d

 		log.Infof("Creating %s", tlog.KObj{Obj: desired})
 		if err := r.Client.Create(ctx, desired); err != nil {
-			return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: desired})
+			return createError(err, desired)


for sake of consistency, should this be

Suggested change

return createError(err, desired)

errors.Wrapf(err, "failed to create %s", desired.Kind)

same below/in other places where we are returning createError without wrapping

I think we have to decide if we want to add the "failed to create" here or in the func.

I think it could be better to do it here (as we can provide additional context which is needed in some cases). But then the createError/apiErrorWithoutObjectName should focus on sanitizing the StatusError and not also wrap it with failed to create ....

P.S. for additional context I made some proposals here: #5945 (comment)

For now I'd prefer to keep the create specific information here - I don't think this is more than a single-use function. The additional info can be added by wrapping errors I think - e.g. where the MachineDeployment object kind is added.

Sure, give it a try.

internal/controllers/topology/cluster/reconcile_state.go

sbueringer · 2022-01-24T14:57:47Z

internal/controllers/topology/cluster/reconcile_state.go

@@ -269,7 +269,7 @@ func (r *Reconciler) reconcileMachineHealthCheck(ctx context.Context, current, d

 		log.Infof("Creating %s", tlog.KObj{Obj: desired})
 		if err := r.Client.Create(ctx, desired); err != nil {
-			return errors.Wrapf(err, "failed to create %s", tlog.KObj{Obj: desired})
+			return createError(err, desired)


I think we have to decide if we want to add the "failed to create" here or in the func.

I think it could be better to do it here (as we can provide additional context which is needed in some cases). But then the createError/apiErrorWithoutObjectName should focus on sanitizing the StatusError and not also wrap it with failed to create ....

P.S. for additional context I made some proposals here: #5945 (comment)

internal/controllers/topology/cluster/reconcile_state.go

sbueringer · 2022-01-25T13:13:38Z

internal/controllers/topology/cluster/reconcile_state.go

+		}
+
+		// Replace the statusError message with the constructed message.
+		statusError.ErrStatus.Message = fmt.Sprintf("failed to create %s", out)


Suggested change

statusError.ErrStatus.Message = fmt.Sprintf("failed to create %s", out)

statusError.ErrStatus.Message = fmt.Sprintf("failed to create: %s", out)

nit, usually errors are concatenated with a colon as separator

Should we only add out if it's not empty? Currently the error would have a trailing space in that case

If out is empty at this point the error isn't going to be make any sense regardless I think - should we instead return the error on L750 if there's nothing in out?

internal/controllers/topology/cluster/reconcile_state.go

Signed-off-by: killianmuldoon <[email protected]>

fabriziopandini · 2022-01-25T18:05:24Z

Thanks @killianmuldoon for sorting out this problem
/lgtm

sbueringer · 2022-01-25T18:09:41Z

@killianmuldoon
Thank you!
also for fixing related error propagation issues

/lgtm

sbueringer · 2022-01-25T18:10:39Z

/cherry-pick release-1.1

As it's a problematic bug we should include it in the v1.1 release

k8s-infra-cherrypick-robot · 2022-01-25T18:10:40Z

@sbueringer: once the present PR merges, I will cherry-pick it on top of release-1.1 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.1

As it's a problematic bug we should include it in the v1.1 release

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/test-infra repository.

fabriziopandini · 2022-01-25T21:05:38Z

/approve

k8s-ci-robot · 2022-01-25T21:06:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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~~ [fabriziopandini]

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

k8s-infra-cherrypick-robot · 2022-01-25T21:08:34Z

@sbueringer: new pull request created: #5992

In response to this:

/cherry-pick release-1.1

As it's a problematic bug we should include it in the v1.1 release

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/test-infra repository.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 21, 2022

k8s-ci-robot requested review from sbueringer and ykakarap January 21, 2022 19:33

fabriziopandini reviewed Jan 24, 2022

View reviewed changes

sbueringer reviewed Jan 24, 2022

View reviewed changes

killianmuldoon force-pushed the fix/error-reporting branch 5 times, most recently from 93c74f0 to 4a72596 Compare January 24, 2022 18:41

killianmuldoon changed the title ~~🐛 [WIP] Remove generated names from error messages to reduce reconciliation~~ 🐛 Remove generated names from error messages to reduce reconciliation Jan 24, 2022

killianmuldoon mentioned this pull request Jan 24, 2022

🐛 WIP: Stop cluster/topology reconciliation on Status changes #5956

Closed

fabriziopandini reviewed Jan 25, 2022

View reviewed changes

internal/controllers/topology/cluster/reconcile_state.go Outdated Show resolved Hide resolved

internal/controllers/topology/cluster/reconcile_state.go Show resolved Hide resolved

sbueringer reviewed Jan 25, 2022

View reviewed changes

killianmuldoon force-pushed the fix/error-reporting branch from 4a72596 to ed4d92a Compare January 25, 2022 16:01

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 25, 2022

killianmuldoon force-pushed the fix/error-reporting branch 2 times, most recently from 9423b3b to bb599a0 Compare January 25, 2022 16:25

sbueringer reviewed Jan 25, 2022

View reviewed changes

killianmuldoon force-pushed the fix/error-reporting branch from bb599a0 to 9be7337 Compare January 25, 2022 18:03

Remove generated names from error messages to reduce reconciliation

78eec8e

Signed-off-by: killianmuldoon <[email protected]>

killianmuldoon force-pushed the fix/error-reporting branch from 9be7337 to 78eec8e Compare January 25, 2022 18:05

k8s-ci-robot assigned fabriziopandini Jan 25, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 25, 2022

k8s-ci-robot assigned sbueringer Jan 25, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 25, 2022

k8s-ci-robot merged commit 5d126b1 into kubernetes-sigs:main Jan 25, 2022

k8s-ci-robot added this to the v1.2 milestone Jan 25, 2022

k8s-infra-cherrypick-robot mentioned this pull request Jan 25, 2022

🐛 Remove generated names from error messages to reduce reconciliation #5992

Merged

killianmuldoon mentioned this pull request Aug 10, 2022

🐛 Strip control plane creation error of unique name #7051

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Remove generated names from error messages to reduce reconciliation #5971

🐛 Remove generated names from error messages to reduce reconciliation #5971

killianmuldoon commented Jan 21, 2022

fabriziopandini Jan 24, 2022

sbueringer Jan 24, 2022 •

edited

Loading

killianmuldoon Jan 24, 2022

sbueringer Jan 24, 2022 •

edited

Loading

sbueringer Jan 24, 2022 •

edited

Loading

sbueringer Jan 25, 2022

sbueringer Jan 25, 2022

killianmuldoon Jan 25, 2022

sbueringer Jan 25, 2022

fabriziopandini commented Jan 25, 2022

sbueringer commented Jan 25, 2022

sbueringer commented Jan 25, 2022

k8s-infra-cherrypick-robot commented Jan 25, 2022

fabriziopandini commented Jan 25, 2022

k8s-ci-robot commented Jan 25, 2022

k8s-infra-cherrypick-robot commented Jan 25, 2022

	return createError(err, desired)
	errors.Wrapf(err, "failed to create %s", desired.Kind)

	statusError.ErrStatus.Message = fmt.Sprintf("failed to create %s", out)
	statusError.ErrStatus.Message = fmt.Sprintf("failed to create: %s", out)

🐛 Remove generated names from error messages to reduce reconciliation #5971

🐛 Remove generated names from error messages to reduce reconciliation #5971

Conversation

killianmuldoon commented Jan 21, 2022

fabriziopandini Jan 24, 2022

Choose a reason for hiding this comment

sbueringer Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

killianmuldoon Jan 24, 2022

Choose a reason for hiding this comment

sbueringer Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

sbueringer Jan 24, 2022 • edited Loading

Choose a reason for hiding this comment

sbueringer Jan 25, 2022

Choose a reason for hiding this comment

sbueringer Jan 25, 2022

Choose a reason for hiding this comment

killianmuldoon Jan 25, 2022

Choose a reason for hiding this comment

sbueringer Jan 25, 2022

Choose a reason for hiding this comment

fabriziopandini commented Jan 25, 2022

sbueringer commented Jan 25, 2022

sbueringer commented Jan 25, 2022

k8s-infra-cherrypick-robot commented Jan 25, 2022

fabriziopandini commented Jan 25, 2022

k8s-ci-robot commented Jan 25, 2022

k8s-infra-cherrypick-robot commented Jan 25, 2022

sbueringer Jan 24, 2022 •

edited

Loading

sbueringer Jan 24, 2022 •

edited

Loading

sbueringer Jan 24, 2022 •

edited

Loading