Skip to content

provider: only store resource if spec has changed#480

Merged
arkodg merged 8 commits intoenvoyproxy:mainfrom
arkodg:reduce-status-churn
Oct 11, 2022
Merged

provider: only store resource if spec has changed#480
arkodg merged 8 commits intoenvoyproxy:mainfrom
arkodg:reduce-status-churn

Conversation

@arkodg
Copy link
Contributor

@arkodg arkodg commented Sep 30, 2022

Leverage the metadata.Generation field to consider whether to update the newly reconciled resource into the watchable map which will trigger translations in the backend.

Fixes: #407

Signed-off-by: Arko Dasgupta arko@tetrate.io

@arkodg arkodg requested a review from a team as a code owner September 30, 2022 20:34
@arkodg arkodg added the provider/kubernetes Issues related to the Kubernetes provider label Sep 30, 2022
@arkodg arkodg added this to the 0.2.0 milestone Sep 30, 2022
@codecov-commenter
Copy link

codecov-commenter commented Sep 30, 2022

Codecov Report

Merging #480 (d1ded31) into main (c4fbdc0) will decrease coverage by 0.01%.
The diff coverage is 91.30%.

@@            Coverage Diff             @@
##             main     #480      +/-   ##
==========================================
- Coverage   62.51%   62.49%   -0.02%     
==========================================
  Files          42       42              
  Lines        4498     4509      +11     
==========================================
+ Hits         2812     2818       +6     
- Misses       1540     1547       +7     
+ Partials      146      144       -2     
Impacted Files Coverage Δ
internal/provider/kubernetes/gatewayclass.go 70.28% <ø> (+1.44%) ⬆️
internal/provider/kubernetes/gateway.go 68.92% <89.47%> (-1.25%) ⬇️
internal/provider/kubernetes/httproute.go 62.12% <100.00%> (ø)
internal/gatewayapi/translator.go 91.16% <0.00%> (-0.02%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@arkodg arkodg marked this pull request as draft October 3, 2022 16:26
@arkodg arkodg marked this pull request as ready for review October 3, 2022 16:28
@arkodg arkodg marked this pull request as draft October 3, 2022 19:14
@arkodg
Copy link
Contributor Author

arkodg commented Oct 6, 2022

blocked on #508

@arkodg arkodg force-pushed the reduce-status-churn branch 2 times, most recently from 5a5f877 to eafda12 Compare October 7, 2022 20:30
@chauhanshubham
Copy link
Member

chauhanshubham commented Oct 8, 2022

I remember that if we Patch the subresource the resource version/generation doesn't change (as compared to using Update).
So I think if we consider using a Patch then that too can prevent the endless loop of reconcile -> status update -> reconcile.

I tried by replacing the Status update with

u.client.Status().Patch(context.Background(), newObj, client.MergeFrom(newObj))

and it seems to be working fine for me.

@chauhanshubham
Copy link
Member

chauhanshubham commented Oct 8, 2022

Also, just checking if the spec has changed, sometimes doesn't go well if users (or other systems) are changing certain metadata fields like annotations or labels, or if a finalizer was added/removed (also deletionTimestamp) let's say.

@danehans
Copy link
Contributor

danehans commented Oct 8, 2022

@arkodg now that #508 and #516 have merged, are you able to update this PR?

@arkodg
Copy link
Contributor Author

arkodg commented Oct 10, 2022

I remember that if we Patch the subresource the resource version/generation doesn't change (as compared to using Update). So I think if we consider using a Patch then that too can prevent the endless loop of reconcile -> status update -> reconcile.

I tried by replacing the Status update with

u.client.Status().Patch(context.Background(), newObj, client.MergeFrom(newObj))

and it seems to be working fine for me.

does Patch not trigger a reconcile ?

@arkodg
Copy link
Contributor Author

arkodg commented Oct 10, 2022

@arkodg now that #508 and #516 have merged, are you able to update this PR?

now its blocked on #535 due to Changs introduced in #518

@arkodg
Copy link
Contributor Author

arkodg commented Oct 10, 2022

Also, just checking if the spec has changed, sometimes doesn't go well if users (or other systems) are changing certain metadata fields like annotations or labels, or if a finalizer was added/removed (also deletionTimestamp) let's say.

for translation, the spec affects a translation change (for now) so imho this approach should be safe for now

Arko Dasgupta added 8 commits October 10, 2022 11:05
Leverage the metadata.Generation field to consider whether
to update the newly reconciled resource into the watchable map
which will trigger translations in the backend.

Fixes: envoyproxy#407

Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
@arkodg arkodg force-pushed the reduce-status-churn branch from b126877 to 75243c6 Compare October 10, 2022 18:06
@arkodg arkodg marked this pull request as ready for review October 10, 2022 18:15
@danehans
Copy link
Contributor

@arkodg should the same be done for HTTPRoutes?

@arkodg
Copy link
Contributor Author

arkodg commented Oct 11, 2022

@danehans the diffs in this commit apply to HTTPRoutes as well

@arkodg arkodg merged commit a370277 into envoyproxy:main Oct 11, 2022
@arkodg arkodg deleted the reduce-status-churn branch October 11, 2022 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

provider/kubernetes Issues related to the Kubernetes provider

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve Provider resource reconciliation

4 participants