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

Use Update (PUT) not Patch when modifying spec and status #1674

Merged
merged 4 commits into from
Aug 3, 2021

Conversation

matthchr
Copy link
Member

@matthchr matthchr commented Jul 30, 2021

What this PR does / why we need it:
Patch has a number of issues:

  1. You aren't guaranteed that you're operating on the latest version of the resource, so you may be patching in a way that is actually nonsensical.
  2. It's difficult to remove things

Rather than fight with Patch, moved to use Update (full replace) everywhere. As part of this work also refactored a little to make the controller more deterministic.

How does this PR make you feel:
gif

If applicable:

  • this PR contains tests

@matthchr
Copy link
Member Author

matthchr commented Jul 30, 2021

Note: There are lots more refactors that we need to do to azure_deployment_reconciler.go but I'm putting them off given the work I have in that space coming up with conditions and also the reality that the crufty-ness of the controller isn't going to be impacting customers whereas some of the resource shape stuff we haven't finished yet does.

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.

A couple of questions, but mostly looks good.

gif

PATCHing means that we don't know for sure the state
of the resource after the PATCH (because it might have been
a different version than we thought). PATCHing also makes
removal of fields difficult, which is especially important in
places like Status where there's no telling what can come or go
from reconcile loop iteration N to iteration N+1.
It's significantly more complex than we need for our use case
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.

Looks good.

gif

@codecov-commenter
Copy link

Codecov Report

Merging #1674 (b8d33fa) into master (d8fd4d2) will decrease coverage by 0.00%.
The diff coverage is 67.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1674      +/-   ##
==========================================
- Coverage   67.98%   67.98%   -0.01%     
==========================================
  Files         212      210       -2     
  Lines       15151    15076      -75     
==========================================
- Hits        10300    10249      -51     
+ Misses       4082     4069      -13     
+ Partials      769      758      -11     
Impacted Files Coverage Δ
hack/generated/pkg/armclient/template_client.go 52.06% <42.85%> (+1.62%) ⬆️
...ted/pkg/reconcilers/azure_deployment_reconciler.go 69.90% <68.26%> (+0.65%) ⬆️
hack/generated/pkg/armclient/types.go 77.77% <80.00%> (-2.43%) ⬇️
.../generated/pkg/testcommon/kube_per_test_context.go 83.76% <100.00%> (-0.42%) ⬇️
hack/generated/pkg/testcommon/test_context.go 64.40% <100.00%> (+1.80%) ⬆️
hack/generator/pkg/astmodel/package_reference.go 88.28% <0.00%> (-2.71%) ⬇️

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 d8fd4d2...b8d33fa. Read the comment docs.

@matthchr matthchr merged commit dfff16c into Azure:master Aug 3, 2021
@matthchr matthchr deleted the feature/put-not-patch branch August 3, 2021 15:40
theunrepentantgeek pushed a commit that referenced this pull request Aug 6, 2021
* Use PUT not PATCH for all k8s resource updates
PATCHing means that we don't know for sure the state
of the resource after the PATCH (because it might have been
a different version than we thought). PATCHing also makes
removal of fields difficult, which is especially important in
places like Status where there's no telling what can come or go
from reconcile loop iteration N to iteration N+1.

* Remove util/patch
It's significantly more complex than we need for our use case
theunrepentantgeek pushed a commit that referenced this pull request Aug 11, 2021
* Use PUT not PATCH for all k8s resource updates
PATCHing means that we don't know for sure the state
of the resource after the PATCH (because it might have been
a different version than we thought). PATCHing also makes
removal of fields difficult, which is especially important in
places like Status where there's no telling what can come or go
from reconcile loop iteration N to iteration N+1.

* Remove util/patch
It's significantly more complex than we need for our use case
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