Skip to content

Conversation

kleesc
Copy link
Member

@kleesc kleesc commented Sep 17, 2019

Add event filter for ManagedDatabase to ignore updates to subresources unless it's the ManagedDatabase's currentVersion, in which case we want to reconcile the next migration.

@kleesc kleesc requested a review from jakedt September 17, 2019 19:15
Copy link
Contributor

@jakedt jakedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should invert the model and just filter out events for which the only thing that changed were things in the status subresource. As it stands right now you're going to filter out things that the user will expect to be able to change, and you're going to filter out reconciliations on downstream resources, e.g. jobs.

@jzelinskie jzelinskie mentioned this pull request Sep 30, 2019
@kleesc kleesc force-pushed the subresource-event-filter-predicate branch from ffacfa1 to e0b8cfa Compare October 1, 2019 14:51
@kleesc
Copy link
Member Author

kleesc commented Oct 1, 2019

I think we should invert the model and just filter out events for which the only thing that changed were things in the status subresource. As it stands right now you're going to filter out things that the user will expect to be able to change, and you're going to filter out reconciliations on downstream resources, e.g. jobs.

Updated to filter only on ManagedDatabases generation change. Other types will update when resourceVersion is changed. Doesn't look like Jobs were using the generation field anyways... So I'm guessing we can't just make the assumption that an update to the status of any object type will increment its generation.

Copy link
Contributor

@jakedt jakedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to think about the event filter some more. There are a lot of cases where we want to run a reconciliation that I think this will filter. For example, we will get awakened when the job or secret changes, but the underlying mdb will not have changed, we want to run reconciliation and I think this will filter those cases. Rather than filter, can we just focus on converging by making the status blocks state dependent and reproduceable?

oneMigration.log.Info("Migration job is complete")

// TODO: should we write the metric here or wait until cleanup?
} else if job.Status.Active == 0 && job.Status.Conditions != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you break just this part out into a separate PR.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#9

@kleesc kleesc force-pushed the subresource-event-filter-predicate branch from e0b8cfa to 7804ad6 Compare October 3, 2019 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants