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

Jump to bottom

Support for wrapped RequeueAfterError/interface #1020

Merged
merged 1 commit into from
Jun 18, 2019
Merged

Support for wrapped RequeueAfterError/interface #1020

merged 1 commit into from
Jun 18, 2019

Conversation

akutz
Copy link
Contributor

@akutz akutz commented Jun 12, 2019

What this PR does / why we need it:
This patch fixes #1018 and supports wrapped "RequeueAfterError" errors as well as a new interface for supporting custom "RequeueAfterError" errors.

Which issue(s) this PR fixes:
Fixes #1018

Special notes for your reviewer:
Hi there. I hope all is well in your neck of the woods.

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 12, 2019
@k8s-ci-robot k8s-ci-robot requested review from detiber and justinsb June 12, 2019 20:25
@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 12, 2019
@akutz
Copy link
Contributor Author

akutz commented Jun 12, 2019 •
edited
Loading

Hmm, I didn't mean to remove the - in the message requeue after. I am going to re-add that. Sorry.

Fixed.

@akutz akutz force-pushed the feature/requeue-error-ex branch from c22bd47 to a8d128d Compare June 12, 2019 20:28
vincepri
vincepri approved these changes Jun 12, 2019
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

LGTM, nice job @akutz!

/ok-to-test
/approve
/assign @detiber @chuckha

pkg/controller/cluster/cluster_controller.go
if err != nil {
if requeueErr, ok := err.(*controllerError.RequeueAfterError); ok {
if err := r.actuator.Reconcile(cluster); err != nil {
if requeueErr, ok := errors.Cause(err).(controllerError.HasRequeueAfterError); ok {
Copy link
Member

Choose a reason for hiding this comment

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

For others that might not know: from the docs of errors.Cause

If the error does not implement Cause, the original error will be returned. If the error is nil, nil will be returned without further investigation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @vincepri,

Thank you. I actually had to go look up Cause to see what it did and found the same. The fact that it handles nil and even returns the original error made this very efficient to implement.

Copy link
Contributor Author

@akutz akutz Jun 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Technically lines 141 and 142 (and the others like them) could be combined, but I think the way it is is more readable.

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 12, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akutz, vincepri

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 12, 2019
@akutz
Support for wrapped RequeueAfterError/interface
94a8feb 
This patch adds support for wrapped "RequeueAfterError" errors
as well as a new interface for supporting custom "RequeueAfterError"
errors.
@akutz akutz force-pushed the feature/requeue-error-ex branch from a8d128d to 94a8feb Compare June 12, 2019 20:35
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jun 12, 2019
@vincepri
Copy link
Member

/test pull-cluster-api-test

@chuckha
Copy link
Contributor

chuckha commented Jun 18, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 18, 2019
@k8s-ci-robot k8s-ci-robot merged commit bd0628f into kubernetes-sigs:master Jun 18, 2019
ncdc pushed a commit to ncdc/cluster-api that referenced this pull request Jun 25, 2019
@akutz @ncdc
Support for wrapped RequeueAfterError/interface (kubernetes-sigs#1020)
3eb8955 
This patch adds support for wrapped "RequeueAfterError" errors
as well as a new interface for supporting custom "RequeueAfterError"
errors.

(cherry picked from commit bd0628f)
@ncdc ncdc mentioned this pull request Jun 25, 2019
Merged
k8s-ci-robot pushed a commit that referenced this pull request Jun 25, 2019
@ncdc @k8s-ci-robot
0.1 backports for noderef (#1069)
d1b07f4 
* Remove duplicate checks for deletionTimestamp (#987)

Signed-off-by: Vince Prignano <[email protected]>
(cherry picked from commit aacb0c6)

* Rename pkg/controller/controller (#998)

Signed-off-by: Vince Prignano <[email protected]>
(cherry picked from commit eafafe8)

* Add remote/util.go helpers to work with KubeConfig Secrets (#1004)

Signed-off-by: Vince Prignano <[email protected]>
(cherry picked from commit d48025d)

* Add remote.ClusterClient to access remote workload clusters (#1006)

Signed-off-by: Vince Prignano <[email protected]>
(cherry picked from commit 0b25546)

* Add events to MachineSet operations (#1012)

Add events to the following MachineSet operations
- Adopt Machine
- Create Machine
- Delete Machine

(cherry picked from commit b0170a0)

* Add patch to events rbac (#1021)

Signed-off-by: Vince Prignano <[email protected]>
(cherry picked from commit 7365154)

* Update some bazel dependencies (#1019)


(cherry picked from commit 77836d8)

* Update boilerplate check and generated files (#1010)

Signed-off-by: Vince Prignano <[email protected]>
(cherry picked from commit 0b215f4)

* Add CLUSTER_API_KUBECONFIG_READY_TIMEOUT to clusterctl (#1026)

Signed-off-by: Vince Prignano <[email protected]>
(cherry picked from commit 9cd85f7)

* Add events to MachineDeployment operations (#1014)


(cherry picked from commit a5e5fdb)

* Update controller-runtime and controller-tools (#1032)


(cherry picked from commit da61280)

* Support for wrapped RequeueAfterError/interface (#1020)

This patch adds support for wrapped "RequeueAfterError" errors
as well as a new interface for supporting custom "RequeueAfterError"
errors.

(cherry picked from commit bd0628f)

* Use klog in manager and setup controller-runtime logger (#1022)

Signed-off-by: Vince Prignano <[email protected]>
(cherry picked from commit 0ebf076)

* Bazel updates (#1043)

- Remove use of deprecated bazel attribute label single_file
- Add bazel version checking
- Include go_rules fix for cross-compiling darwin binaries

(cherry picked from commit 2123eed)

* clean up shell scripts in cluster-api (#1045)

(cherry picked from commit d63d4be)

* Update k8s/code-generator to latest release-1.13 (#1048)

Signed-off-by: Vince Prignano <[email protected]>
(cherry picked from commit c2faf86)

* downgrade log of util.ExecuteCommand (#975)

As this error E0531 is not that ueful and no need to be mark
this as 'E', use Info level should be fine as this won't affect
return value.

(cherry picked from commit 97f25c9)

* Make ClusterNetwork ClusterNetworkingConfig field optional within (#919)

Cluster resources. This is useful for Managed Control Planes and
Bare Metal where networking configuration options may be limited.

(cherry picked from commit 9e1eb56)

* NodeRef controller (#1011)

Signed-off-by: Vince Prignano <[email protected]>
(cherry picked from commit 9e89546)

* Update generated files, add workaround for controller-tools (#1050)

Signed-off-by: Vince Prignano <[email protected]>
(cherry picked from commit 1c10a3f)

* Update controller-tools to 0.1.11 (#1053)

Signed-off-by: Vince Prignano <[email protected]>
(cherry picked from commit 1cb3deb)

* Update dep check (#1056)

Signed-off-by: Vince Prignano <[email protected]>
(cherry picked from commit aecec29)

* Fix register bootstrap options in alpha phases (#1064)

Fixes issue: #1063

(cherry picked from commit 8141304)

* Use remote NodeRef in Machine and MachineSet controllers (#1052)

Signed-off-by: Vince Prignano <[email protected]>
(cherry picked from commit 1eaf864)

* Bazel fixes

Signed-off-by: Andy Goldstein <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincepri vincepri vincepri approved these changes

@detiber detiber Awaiting requested review from detiber

@justinsb justinsb Awaiting requested review from justinsb

Assignees

@chuckha chuckha

@detiber detiber

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhance the RequeueError to support wrapping/interfaces
5 participants
@akutz @k8s-ci-robot @vincepri @chuckha @detiber