Skip to content

Conversation

@jgwest
Copy link
Member

@jgwest jgwest commented Apr 11, 2024

What does this PR do / why we need it:

  • Ensures that we only ever update .status field of RolloutManager from a single location in the code
    • We do this by refactoring reconcile logic to return a 'reconcileStatusResult', which indicates what values to set on the status.
  • This allows us to avoid these 'status errors we were seeing when running E2E tests: 2024-04-10T05:51:36Z ERROR rollouts-controller unable to update status of RolloutManager {"error": "Operation cannot be fulfilled on rolloutmanagers.argoproj.io \"basic-rollouts-manager\": the object has been modified; please apply your changes to the latest version and try again"}
  • This PR also adds a quick Namespace-deletion test, to allow us to prevent modification to resources in a Namespace that is being deleted (which lets us avoid nasty error messages in our E2E tests related to deletion)
    • This allows us to avoid these errors: unable to create new content in namespace argo-rollouts because it is being terminated

Have you updated the necessary documentation?

  • Documentation update is required by this PR, and has been updated.

Which issue(s) this PR fixes:
Fixes #?

How to test changes / Special notes to the reviewer:

@jgwest jgwest changed the title Refactor RolloutManager status update logic Refactor RolloutManager status update logic to eliminate failed status update errors Apr 11, 2024
@jgwest jgwest changed the title Refactor RolloutManager status update logic to eliminate failed status update errors Refactor RolloutManager status update logic to eliminate/reduce failed status update errors Apr 11, 2024
@jgwest jgwest requested a review from jparsai April 11, 2024 09:05
Copy link
Collaborator

@jparsai jparsai left a comment

Choose a reason for hiding this comment

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

All changes LGTM, though at some places error is still thrown while updating resource, but frequency of this is less now.

@jgwest jgwest force-pushed the refactor-status-updates-april-2024 branch from 2f90c9e to a0c30f5 Compare May 8, 2024 12:42
@jgwest
Copy link
Member Author

jgwest commented May 8, 2024

Thanks @jparsai for reviewing!

@jgwest jgwest merged commit 837f307 into argoproj-labs:main May 8, 2024
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.

2 participants