-
Notifications
You must be signed in to change notification settings - Fork 453
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
Make seed lease/duration configurable #5092
Conversation
@ary1992 Thank you for your contribution. |
Thank you @ary1992 for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below. |
b37ae6d
to
3b8f337
Compare
/invite @shafeeqes @acumino |
/reviewed/ok-to-test |
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.
Generally, I'm wondering: Do we really need these two new fields or would one (used at both occurrences) be enough? Are there use-cases for which I'd like the .spec.leaseDurationSeconds
value in the Lease
object to be different compared to the duration how of the lease is renewed? Or wouldn't this rather be always equal?
Can you add a proper release note to make operators aware that this is now configurable? |
@acumino You have pull request review open invite, please check |
@ary1992 The |
09a0e66 looks good, thank you! Please proceed with the other improvements as discussed yesterday :) |
Earlier two separate fields were defined as constant integer |
I would rather say the opposite: We need to know about use cases where only one configuration value is NOT enough. |
09a0e66
to
3574e10
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.
Thanks @ary1992, apart from the naming suggestion it looks good. I think we should go ahead with merging the two fields LeaseResyncSeconds
and LeaseDurationSeconds
since nobody speaks up with a justifying use-case.
/merge squash |
@ary1992 You are not permitted to touch label merge/squash. |
/merge squash |
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.
It looks good, apart from the failing test I have no further comments.
9e6b864
to
59b367e
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
* make seed lease/duration period configurable * Move the configuration fields to SeedControllerConfiguration * Make LeaseResyncGracePeriod configurable * Merge two fields LeaseResyncSeconds and LeaseDurationSeconds into one field LeaseResyncSeconds. * Make changes to unit test
* make seed lease/duration period configurable * Move the configuration fields to SeedControllerConfiguration * Make LeaseResyncGracePeriod configurable * Merge two fields LeaseResyncSeconds and LeaseDurationSeconds into one field LeaseResyncSeconds. * Make changes to unit test
How to categorize this PR?
/area scalability
/kind enhancement
What this PR does / why we need it:
Make seed lease/duration period configurable.
Which issue(s) this PR fixes:
Fixes #4504
Special notes for your reviewer:
Release note: