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

Reconciler: improvements for processing of async operations #3311

Merged
merged 3 commits into from
Jul 4, 2023

Conversation

milan-zededa
Copy link
Contributor

@milan-zededa milan-zededa commented Jul 3, 2023

3 commits in this PR related to reconciliation of async operations:

  • added unit test to cover handling of failures of async operations
  • added option to reconciler API to select which async operations to cancel (previously was possible to only cancel all running async ops without exceptions)
  • avoid race conditions by not processing async ops that finalize during reconciliation - they are instead left to be processed
    in the next reconciliation run (some race conditions were found during zedrouter testing and this way they can be easily avoided)

This updates libs/reconciler package. Changes made here will be propagated to pillar in the next PR together with some zedrouter patches.

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Stylistic nits otherwise fine.

@milan-zededa
Copy link
Contributor Author

milan-zededa commented Jul 4, 2023

Note that since this is only in libs, it has no effect on EVE until I bring these changes to pillar in the next PR. So it does not make much sense to wait for eden.

Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

Kick off regression tests in parallel with review.

Signed-off-by: Milan Lenco <milan@zededa.com>
Signed-off-by: Milan Lenco <milan@zededa.com>
Async ops that finalize *during* reconciliation are left to be processed
in the next reconciliation run.
This prevents race-conditions and simplifies the reconciliation algorithm.

Signed-off-by: Milan Lenco <milan@zededa.com>
Copy link
Member

@uncleDecart uncleDecart left a comment

Choose a reason for hiding this comment

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

Do you think it makes sense to split reconciler_test.go into multiple files grouped by functionality?

WaitForAsyncOps func()
}

// CancelFunc is used to cancel all or only some asynchronously running operations.
type CancelFunc func(cancelForItem func(ref dg.ItemRef) bool)
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we want to use semantics of Cancel function defined by context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously I used that (see the diff) but I needed to add cancelForItem argument.

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.

3 participants