-
Notifications
You must be signed in to change notification settings - Fork 78
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
CLOUDP-261715: Enabled resync for Atlas custom resources #1707
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.
This is not enough:
- Please add a resource version predicate on all Secret watches. Otherwise we will have double reconciliations in the face of resyncs.
- Please make the resync interval configurable in the builder and add an e2e test.
re 2.: for now, we shouldn't expose the knob to main just yet but just for the e2e test. |
@s-urbaniak the sync interval is already configurable. The main just doesn't set it and it's set up to the default value of 3 hours |
I just checked that locally, and it doesn't double the reconciliations. It doubles them if we actually remove the predicated |
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.
one nit, else lgtm 👏
|
||
var _ = Describe("Sync Period test", Label("int", "sync-period"), func() { | ||
const interval = time.Second * 2 | ||
const syncInterval = 40 * time.Second |
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 pick
would 10s be enough to test the same?
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.
As a side note: i am working on a pure unit-test approach for this. I don't think we need a full blown e2e test to assert resyncs. I will submit once ready.
Co-authored-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com>
69d965c
to
7f97c67
Compare
All Submissions:
closes #XXXX
in your comment to auto-close the issue that your PR fixes (if there is one). Closes AtlasDatabaseUser is not reconciled periodically - manual changes are not overridden until pod restart #1689