Skip to content

Add retry logic for App CR updates to handle optimistic concurrency conflicts#1766

Merged
praveenrewar merged 3 commits intocarvel-dev:developfrom
mdzhigarov:fix/app-update-retry-logic
Oct 14, 2025
Merged

Add retry logic for App CR updates to handle optimistic concurrency conflicts#1766
praveenrewar merged 3 commits intocarvel-dev:developfrom
mdzhigarov:fix/app-update-retry-logic

Conversation

@mdzhigarov
Copy link
Contributor

Summary

This PR implements retry logic for App CR updates in the PackageInstall reconciler to handle optimistic concurrency control conflicts that can occur when multiple operations attempt to update the same App CR simultaneously.

Changes

Core Implementation

  • Added updateAppWithRetry() function in pkg/packageinstall/packageinstall.go
    • Implements retry logic with up to 5 attempts for App CR updates
    • Re-fetches App CR after each failed update to get the latest resourceVersion
    • Handles NotFound errors immediately without retries to avoid unnecessary delays
    • Uses flexible updateFunc signature that returns an App, allowing direct integration with NewApp()

Integration Points

  • Updated two App update locations in PackageInstall reconciler:
    • Line 233: App creation/update during normal reconciliation (uses NewApp() for transformation)
    • Line 408: App update during deletion reconciliation (applies specific field changes)

Testing

  • Added comprehensive unit tests in pkg/packageinstall/packageinstall_test.go:
    • Test_UpdateAppWithRetry_HandlesConflictErrors: Tests retry logic with simulated conflict errors
    • Test_UpdateAppWithRetry_HandlesNotFoundError: Verifies immediate return for NotFound errors
    • Test_UpdateAppWithRetry_HandlesUpdateFunctionError: Tests error propagation from update function

Problem Solved

Fixes the "object has been modified; please apply your changes to the latest version and try again" error that occurs when:

  • Multiple PackageInstall reconcilers attempt to update the same App CR
  • App CR is updated by other controllers or operations between fetch and update
  • High-frequency reconciliation triggers concurrent updates

Testing

All existing tests pass, and new tests specifically validate:

  • ✅ Retry logic correctly handles optimistic concurrency conflicts
  • ✅ NotFound errors are handled specially (no retries)
  • ✅ Update function errors are properly propagated
  • ✅ Retry mechanism works as designed (5 attempts max, re-fetch between attempts)
go test ./pkg/packageinstall -v
# All tests pass including new retry logic tests

Backward Compatibility

This change is fully backward compatible:

  • No API changes
  • No breaking changes to existing behavior
  • Only adds resilience to existing update operations

@mdzhigarov mdzhigarov force-pushed the fix/app-update-retry-logic branch from 673129c to 444d1fe Compare October 12, 2025 12:33
…onflicts

- Implement updateAppWithRetry() function in PackageInstall reconciler
- Add retry logic with up to 5 attempts for App CR updates
- Re-fetch App CR after each failed update to get latest resourceVersion
- Handle NotFound errors immediately without retries
- Add comprehensive unit tests for retry logic covering:
  - Conflict error handling with retries
  - NotFound error immediate return
  - Update function error propagation
- Use NewApp function for App transformations in retry logic

Fixes optimistic concurrency control conflicts that occur when
multiple operations attempt to update the same App CR simultaneously.

Signed-off-by: Marin Dzhigarov <m.dzhigarov@gmail.com>
- Rename unused 'action' parameters to '_' in test reactors
- Rename unused 'app' parameter to '_' in failing update function test
- Addresses revive linter warnings for unused parameters

Signed-off-by: Marin Dzhigarov <m.dzhigarov@gmail.com>
@mdzhigarov mdzhigarov force-pushed the fix/app-update-retry-logic branch from 9105dba to b75623d Compare October 14, 2025 07:34
praveenrewar
praveenrewar previously approved these changes Oct 14, 2025
Copy link
Member

@praveenrewar praveenrewar left a comment

Choose a reason for hiding this comment

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

LGTM!

@mdzhigarov mdzhigarov force-pushed the fix/app-update-retry-logic branch from de33b80 to b1eef69 Compare October 14, 2025 10:52
- Update updateAppWithRetry to only retry on conflict errors, not on NotFound errors
- Change retry condition from !errors.IsNotFound(err) to !errors.IsConflict(err)
- Update unit tests to reflect new retry behavior:
  - Fix Test_UpdateAppWithRetry_HandlesNotFoundError to expect no retries for NotFound
  - Add Test_UpdateAppWithRetry_HandlesNonConflictError for non-conflict error handling
  - Update Test_UpdateAppWithRetry_HandlesConflictErrors to use proper errors.NewConflict()
  - Add k8s.io/apimachinery/pkg/api/errors import for proper error types

This ensures that only conflict errors trigger retries, while all other errors
(including NotFound) are returned immediately without retries.

Signed-off-by: Marin Dzhigarov <m.dzhigarov@gmail.com>
@mdzhigarov mdzhigarov force-pushed the fix/app-update-retry-logic branch from b1eef69 to f4033d5 Compare October 14, 2025 11:07
@praveenrewar praveenrewar merged commit 0c1aa96 into carvel-dev:develop Oct 14, 2025
12 checks passed
@github-project-automation github-project-automation bot moved this to Closed in Carvel Oct 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants