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.
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: added may resync check for EKS #2846
fix: added may resync check for EKS #2846
Conversation
@richardcase: This issue is currently awaiting triage. If CAPA/CAPI contributors determines this is a relevant issue, they will accept it by applying the The 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. |
main.go
Outdated
@@ -289,6 +291,11 @@ func enableGates(ctx context.Context, mgr ctrl.Manager, awsServiceEndpoints []sc | |||
if feature.Gates.Enabled(feature.EKS) { | |||
setupLog.Info("enabling EKS controllers") | |||
|
|||
if syncPeriod > maxEKSSyncPeriod { | |||
setupLog.Error(errMaxSyncPeriodExceeded, "sync period exceeded maximum allowed when using EKS", "max-sync-period", maxEKSSyncPeriod) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: I think this will show the same message twice?
""sync period exceeded maximum allowed when using EKS: sync period greater than maximum allowed"
setupLog.Error(errMaxSyncPeriodExceeded, "sync period exceeded maximum allowed when using EKS", "max-sync-period", maxEKSSyncPeriod) | |
setupLog.Error(errMaxSyncPeriodExceeded, "failed to enable EKS controllers") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed
@@ -105,7 +105,9 @@ var ( | |||
healthAddr string | |||
serviceEndpoints string | |||
|
|||
errEKSInvalidFlags = errors.New("invalid EKS flag combination") | |||
maxEKSSyncPeriod = time.Minute * 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please explain why 10m is the period in a comment, or reference to doc 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment added
I hope you can address the comments, but overall looks good. Thanks for fixing this! /lgtm |
When EKS was graduated the check to make sure that the max sync time wasn't greater than 10 mins was missed. Signed-off-by: Richard Case <richard@weave.works>
/priority critical/urgent |
@randomvariable: The label(s) In response to this:
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. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: randomvariable 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 |
/cherrypick release-0.7 |
/cherrypick release-0.7 |
@randomvariable: new pull request created: #2856 In response to this:
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. |
What type of PR is this?
/kind regression
What this PR does / why we need it:
When EKS was graduated the check to make sure that the
max sync time wasn't greater than 10 mins was missed.
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 #2845
Special notes for your reviewer:
Checklist:
Release note: