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

Incremental conditions patching leads to reconciliation errors #3260

Closed
gab-satchi opened this issue Jun 25, 2020 · 2 comments · Fixed by #3261
Closed

Incremental conditions patching leads to reconciliation errors #3260

gab-satchi opened this issue Jun 25, 2020 · 2 comments · Fixed by #3261
Assignees
Labels
kind/design Categorizes issue or PR as related to design. lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor.

Comments

@gab-satchi
Copy link
Member

While adding conditions to AWSMachine and other CAPA resources, we wanted to show incremental updates using conditions for long running processes that do a busy wait. For ex: InstanceProvisionStarted

In the same reconciliation loop, the same condition can be updated further down the line for other reasons. When the reconcile finishes, patch is called which attempts to patch conditions but sees a conflict as the condition has already been updated to show the incremental progress. A subsequent reconcile will be successful.

Some half baked ideas:

  • Allow bypassing the comparison with latest altogether. Will lead to the newest 'Apply' becoming the source of truth but since subsequent reconciles will succeed anyway this might already be the case.
  • Convert each of the incremental updates to be Conditions themselves and not Reasons. With dedicated conditions, they won't conflict with others but may lead to a lot of noise in the Status.
  • Have conditions patching consider the severity. An Error can override a Warning which can override an Info even if it differs from the before object’s conditions

/kind design
cc @fabriziopandini

@k8s-ci-robot k8s-ci-robot added the kind/design Categorizes issue or PR as related to design. label Jun 25, 2020
@fabriziopandini
Copy link
Member

Condition patching was originally designed to handle concurrency between two controllers, and in that case each controller was expected to control different conditions. As a consequence, the conflict resolution logic was implemented "at best effort" mimicking the three-way merge.

This is actually a new use case because the same controller is patching the same conditions both incrementally and in the deferred patch, with the latest expected to have priority in conflict resolution.

My first reaction is that we should declare when we are in this case, eventually specifying the list of conditions owned by the controller. But let me dig a little bit more

@fabriziopandini
Copy link
Member

/assign
/lifecycle active

@k8s-ci-robot k8s-ci-robot added the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/design Categorizes issue or PR as related to design. lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants