Skip to content
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

Merged
merged 4 commits into from
Jul 24, 2024

Conversation

igor-karpukhin
Copy link
Collaborator

@igor-karpukhin igor-karpukhin commented Jul 18, 2024

All Submissions:

Copy link
Contributor

github-actions bot commented Jul 18, 2024

Copy link
Collaborator

@s-urbaniak s-urbaniak left a 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:

  1. Please add a resource version predicate on all Secret watches. Otherwise we will have double reconciliations in the face of resyncs.
  2. Please make the resync interval configurable in the builder and add an e2e test.

@s-urbaniak
Copy link
Collaborator

re 2.: for now, we shouldn't expose the knob to main just yet but just for the e2e test.

@igor-karpukhin
Copy link
Collaborator Author

@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

@igor-karpukhin
Copy link
Collaborator Author

igor-karpukhin commented Jul 18, 2024

Otherwise we will have double reconciliations in the face of resyncs.

I just checked that locally, and it doesn't double the reconciliations. It doubles them if we actually remove the predicated

Copy link
Collaborator

@s-urbaniak s-urbaniak left a 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
Copy link
Collaborator

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?

Copy link
Collaborator

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.

igor-karpukhin and others added 2 commits July 24, 2024 13:45
Co-authored-by: Sergiusz Urbaniak <sergiusz.urbaniak@gmail.com>
@igor-karpukhin igor-karpukhin force-pushed the CLOUDP-261715/fix-sync-interval branch from 69d965c to 7f97c67 Compare July 24, 2024 11:46
@igor-karpukhin igor-karpukhin merged commit 83375fe into main Jul 24, 2024
57 checks passed
@roothorp roothorp deleted the CLOUDP-261715/fix-sync-interval branch September 26, 2024 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AtlasDatabaseUser is not reconciled periodically - manual changes are not overridden until pod restart
3 participants