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

Don't CommitUpdate twice in a single reconcile loop #1684

Merged
merged 2 commits into from
Aug 4, 2021

Conversation

matthchr
Copy link
Member

@matthchr matthchr commented Aug 3, 2021

Multiple updates in a single loop means that there are multiple changes with new resourceVersions floating around. This then leads to races where you might end up reconciling on the resource from after update1 when you really want the change from after update2.

Since the two-updates was just an optimization, removed it.

@matthchr matthchr added this to In progress in CRD Code Generation via automation Aug 3, 2021
Multiple updates in a single loop means that there are multiple changes
with new resourceVersions floating around. This then leads to races
where you might end up reconciling on the resource from after update1
when you really want the change from after update2.

Since the two-updates was just an optimization, removed it.
@codecov-commenter
Copy link

codecov-commenter commented Aug 4, 2021

Codecov Report

Merging #1684 (e0dd5b6) into master (e0b7929) will decrease coverage by 0.02%.
The diff coverage is 77.55%.

❗ Current head e0dd5b6 differs from pull request most recent head 2abefce. Consider uploading reports for the commit 2abefce to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1684      +/-   ##
==========================================
- Coverage   68.37%   68.34%   -0.03%     
==========================================
  Files         214      214              
  Lines       15353    15342      -11     
==========================================
- Hits        10498    10486      -12     
- Misses       4090     4093       +3     
+ Partials      765      763       -2     
Impacted Files Coverage Δ
...ted/pkg/reconcilers/azure_deployment_reconciler.go 68.80% <73.80%> (-1.57%) ⬇️
hack/generated/controllers/generic_controller.go 75.75% <100.00%> (+2.28%) ⬆️
...erated/pkg/testcommon/kube_test_context_envtest.go 83.54% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0b7929...2abefce. Read the comment docs.

Copy link
Member

@theunrepentantgeek theunrepentantgeek left a comment

Choose a reason for hiding this comment

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

Concerns resolved.

gif

@matthchr matthchr merged commit 1db5905 into Azure:master Aug 4, 2021
CRD Code Generation automation moved this from In progress to Done Aug 4, 2021
@matthchr matthchr deleted the feature/fix-nondeterminism branch August 4, 2021 21:11
theunrepentantgeek pushed a commit that referenced this pull request Aug 6, 2021
Multiple updates in a single loop means that there are multiple changes
with new resourceVersions floating around. This then leads to races
where you might end up reconciling on the resource from after update1
when you really want the change from after update2.

Since the two-updates was just an optimization, removed it.
theunrepentantgeek pushed a commit that referenced this pull request Aug 11, 2021
Multiple updates in a single loop means that there are multiple changes
with new resourceVersions floating around. This then leads to races
where you might end up reconciling on the resource from after update1
when you really want the change from after update2.

Since the two-updates was just an optimization, removed it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants