Skip to content

Adds logic to the reconcile function to handle the optional infinite requeue (Issue #826) #23

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

Merged
merged 2 commits into from
Jun 24, 2021
Merged

Adds logic to the reconcile function to handle the optional infinite requeue (Issue #826) #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74b6578
Initial Commit: Infinite Requeue
mbaijal Jun 18, 2021
2770256
Address Review Comments
mbaijal Jun 18, 2021
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
27 changes: 17 additions & 10 deletions pkg/runtime/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ package runtime

import (
"context"

"time"

"github.com/go-logr/logr"
"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -279,15 +280,21 @@ func (r *resourceReconciler) Sync(
return err
}
for _, condition := range latest.Conditions() {
if condition.Type == ackv1alpha1.ConditionTypeResourceSynced &&
condition.Status != corev1.ConditionTrue {
rlog.Debug(
"requeueing resource after finding resource synced condition false",
)
return requeue.NeededAfter(
ackerr.TemporaryOutOfSync,
requeue.DefaultRequeueAfterDuration,
)
if condition.Type == ackv1alpha1.ConditionTypeResourceSynced {
if condition.Status == corev1.ConditionTrue {
if duration := r.rmf.RequeueOnSuccessSeconds(); duration > 0 {
rlog.Debug(
"requeueing resource after resource synced condition true",
)
return requeue.NeededAfter(nil, time.Duration(duration)*time.Second)
Copy link
Member

Choose a reason for hiding this comment

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

@jaypipes isn't it better to pass the debug message on line #286 to requeue instead of nil?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@surajkota the nil in the NeededAfter is for an error. There is no error here.

}
} else {
rlog.Debug(
"requeueing resource after finding resource synced condition false",
)
return requeue.NeededAfter(
ackerr.TemporaryOutOfSync, requeue.DefaultRequeueAfterDuration)
}
}
}
return nil
Expand Down
4 changes: 4 additions & 0 deletions pkg/types/aws_resource_manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ type AWSResourceManager interface {

// AWSResourceManagerFactory returns an AWSResourceManager that can be used to
// manage AWS resources for a particular AWS account
// TODO(jaypipes): Move AWSResourceManagerFactory into its own file
Copy link
Collaborator

Choose a reason for hiding this comment

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

haha! :) nice.

type AWSResourceManagerFactory interface {
// ResourceDescriptor returns an AWSResourceDescriptor that can be used by
// the upstream controller-runtime to introspect the CRs that the resource
Expand All @@ -89,4 +90,7 @@ type AWSResourceManagerFactory interface {
) (AWSResourceManager, error)
// IsAdoptable returns true if the resource is able to be adopted
IsAdoptable() bool
// RequeueOnSuccessSeconds returns true if the resource should be requeued after specified seconds
Copy link
Contributor

@vijtrip2 vijtrip2 Jun 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

godoc is out of date. Please fix it and everything else looks very good. 💯

Copy link
Collaborator

Choose a reason for hiding this comment

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

@vijtrip2 I'll fix on a followup (handling the TODO above) :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Jay.

// Default is false which means resource will not be requeued after success.
RequeueOnSuccessSeconds() int
}