-
Notifications
You must be signed in to change notification settings - Fork 580
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
Set license to be at the last part of reconciliation #7110
Conversation
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.
Left some questions
} | ||
|
||
// setting license should be at the last part as it requires AdminAPI to be running | ||
if err := r.setLicense(ctx, &redpandaCluster, log); err != nil { |
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.
is the call idempotent? I mean what happens if you re-set the same license? Is redpanda fine with it or is it trying to store the same license over and over?
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.
I looked into this and we can't check I think if same license is present as we can't decode the one referenced in secret
|
||
// setting license should be at the last part as it requires AdminAPI to be running | ||
if err := r.setLicense(ctx, &redpandaCluster, log); err != nil { | ||
log.Info(fmt.Sprintf("setting license: %s", err.Error())) |
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.
why is this not error? also can you say something like "error setting license, will retry in 1 minute.."?
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.
because it's expected to happen at some point (i.e. on first reconciliation while cluster is not yet ready) but I've updated it to check for that 🙏
// setting license should be at the last part as it requires AdminAPI to be running | ||
if err := r.setLicense(ctx, &redpandaCluster, log); err != nil { | ||
log.Info(fmt.Sprintf("setting license: %s", err.Error())) | ||
return ctrl.Result{RequeueAfter: time.Minute * 1}, nil |
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.
why one minute and not rely on the default backoff scheme?
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.
this is bound to happen at first reconciliation and wanted to avoid a lot of requeues
78cb57a
to
0d877bd
Compare
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.
LGTM but let me know what the repeated call to setLicense does :)
Cover letter
Setting license must be set at the end of reconciliation because it requires AdminAPI to be running.
SetLicense
don't return specific errors and we don't currently enforce license in the Redpanda cluster.This PR ignores set license errors and just requeues them after one minute.
Backport Required
UX changes
None
Release notes
Bug Fixes