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

bugfix: filter repeated resources and optimize memory usage. #4454

Merged
merged 1 commit into from
Oct 22, 2024

Conversation

qicz
Copy link
Member

@qicz qicz commented Oct 16, 2024

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #4395 and optimize for #4181

before:
image
image
image

after
image
image
image

@qicz qicz requested a review from a team as a code owner October 16, 2024 10:35
@arkodg
Copy link
Contributor

arkodg commented Oct 16, 2024

@qicz thanks for picking this one up, its worth investigating why this happening before working around it by using a set

@zirain
Copy link
Member

zirain commented Oct 17, 2024

we need some test to guratee this

@qicz
Copy link
Member Author

qicz commented Oct 17, 2024

@qicz thanks for picking this one up, its worth investigating why this happening before working around it by using a set

for my analysis, the endpointslice with more repeated value for the service

if endpointSliceLabelKey != "" {
endpointSliceList := new(discoveryv1.EndpointSliceList)
opts := []client.ListOption{
client.MatchingLabels(map[string]string{
endpointSliceLabelKey: string(backendRef.Name),
}),
client.InNamespace(string(*backendRef.Namespace)),
}
if err := r.client.List(ctx, endpointSliceList, opts...); err != nil {
r.log.Error(err, "failed to get EndpointSlices", "namespace", string(*backendRef.Namespace),
backendRefKind, string(backendRef.Name))
} else {
for _, endpointSlice := range endpointSliceList.Items {
endpointSlice := endpointSlice //nolint:copyloopvar
r.log.Info("added EndpointSlice to resource tree", "namespace", endpointSlice.Namespace,
"name", endpointSlice.Name)
gwcResource.EndpointSlices = append(gwcResource.EndpointSlices, &endpointSlice)
}
}
}
}
}

logging:

image

Copy link

codecov bot commented Oct 17, 2024

Codecov Report

Attention: Patch coverage is 19.65318% with 139 lines in your changes missing coverage. Please review.

Project coverage is 65.88%. Comparing base (04fc944) to head (79d7928).
Report is 8 commits behind head on main.

Files with missing lines Patch % Lines
internal/provider/kubernetes/controller.go 8.41% 93 Missing and 5 partials ⚠️
internal/provider/kubernetes/routes.go 0.00% 41 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4454      +/-   ##
==========================================
- Coverage   65.97%   65.88%   -0.10%     
==========================================
  Files         203      204       +1     
  Lines       31154    31234      +80     
==========================================
+ Hits        20554    20578      +24     
- Misses       9415     9475      +60     
+ Partials     1185     1181       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@qicz qicz force-pushed the fix-endpoints-repeated branch from abf1930 to e73cff7 Compare October 17, 2024 09:54
@arkodg
Copy link
Contributor

arkodg commented Oct 18, 2024

ah thanks for pointing to the controller/provider code, it looks like since the service was referenced twice, we added the corresponding endpoint slice twice. The fix in this PR is not going to work since it has visibility into a single EndpointSlice

We can either fix this in the provider or in the gateway api layer

@qicz
Copy link
Member Author

qicz commented Oct 18, 2024

ah thanks for pointing to the controller/provider code, it looks like since the service was referenced twice, we added the corresponding endpoint slice twice. The fix in this PR is not going to work since it has visibility into a single EndpointSlice

We can either fix this in the provider or in the gateway api layer

I 'll fix this in gateway API layer

@qicz qicz force-pushed the fix-endpoints-repeated branch 3 times, most recently from 12032a1 to 414c3b0 Compare October 18, 2024 10:05
@qicz qicz changed the title bugfix: filter repeated endpoints. bugfix: filter repeated resources. Oct 18, 2024
@qicz qicz force-pushed the fix-endpoints-repeated branch 16 times, most recently from 22c9bed to 55d5a5a Compare October 19, 2024 14:47
@qicz qicz marked this pull request as draft October 20, 2024 03:04
@qicz qicz force-pushed the fix-endpoints-repeated branch from c15548d to b1da953 Compare October 20, 2024 03:22
@qicz qicz force-pushed the fix-endpoints-repeated branch 3 times, most recently from c262a85 to 7a59d5e Compare October 20, 2024 14:05
@qicz qicz marked this pull request as ready for review October 20, 2024 14:05
Copy link
Contributor

@shawnh2 shawnh2 left a comment

Choose a reason for hiding this comment

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

thanks for the fix @qicz, now the mem usage decrease dramatically !

image

internal/gatewayapi/resource/resource_cache.go Outdated Show resolved Hide resolved
@qicz
Copy link
Member Author

qicz commented Oct 20, 2024

thanks for the fix @qicz, now the mem usage decrease dramatically !

image

Yep, the duplicate resources have already been filtered, which will optimize the memory for many big objects used, such as ConfigMap, Secret, EndpointSlice, etc.
cc @arkodg

xref #3980 #3698 #4181 #4263

@qicz qicz force-pushed the fix-endpoints-repeated branch from ebd3517 to 295a44b Compare October 20, 2024 15:51
@qicz qicz changed the title bugfix: filter repeated resources. bugfix: filter repeated resources and optimize memory usage. Oct 20, 2024
@qicz qicz requested review from shawnh2 and arkodg October 21, 2024 02:08
@arkodg
Copy link
Contributor

arkodg commented Oct 21, 2024

looks like we are already doing something similar in the provider layer using this structure to ensure deduplication, can we implement this there instead ?

@qicz
Copy link
Member Author

qicz commented Oct 22, 2024

looks like we are already doing something similar in the provider layer using this structure to ensure deduplication, can we implement this there instead ?

They are similar,but cache used in controller and translate, IMO should in gateway layer resource pkg. resourcemap just for controller @arkodg

@arkodg
Copy link
Contributor

arkodg commented Oct 22, 2024

looks like we are already doing something similar in the provider layer using this structure to ensure deduplication, can we implement this there instead ?

They are similar,but cache used in controller and translate, IMO should in gateway layer resource pkg. resourcemap just for controller @arkodg

since we already have an existing way to do this, prefer the provider approach to reduce code complexity

@qicz
Copy link
Member Author

qicz commented Oct 22, 2024

looks like we are already doing something similar in the provider layer using this structure to ensure deduplication, can we implement this there instead ?

They are similar,but cache used in controller and translate, IMO should in gateway layer resource pkg. resourcemap just for controller @arkodg

since we already have an existing way to do this, prefer the provider approach to reduce code complexity

you mean reuse resourcemap struct? or reuse resourcemap set properties? @arkodg

@arkodg
Copy link
Contributor

arkodg commented Oct 22, 2024

looks like we are already doing something similar in the provider layer using this structure to ensure deduplication, can we implement this there instead ?

They are similar,but cache used in controller and translate, IMO should in gateway layer resource pkg. resourcemap just for controller @arkodg

since we already have an existing way to do this, prefer the provider approach to reduce code complexity

you mean reuse resourcemap struct? or reuse resourcemap set properties? @arkodg

create an addition field within resourceMappings for service to ensure we add one unique item into the list

@arkodg
Copy link
Contributor

arkodg commented Oct 22, 2024

@qicz my suggestion is to solve the problem in the provider package similar to what we are doing for other resources

@qicz qicz force-pushed the fix-endpoints-repeated branch 2 times, most recently from 1302a4c to 93bc44b Compare October 22, 2024 07:13
@qicz qicz force-pushed the fix-endpoints-repeated branch from cd6a818 to 79d7928 Compare October 22, 2024 09:24
@qicz
Copy link
Member Author

qicz commented Oct 22, 2024

@qicz my suggestion is to solve the problem in the provider package similar to what we are doing for other resources

done, ptal @arkodg

@arkodg arkodg added this to the v1.2.0-rc1 milestone Oct 22, 2024
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

LGTM thanks !
great to see the massive drop in mem usage

@arkodg arkodg requested review from a team October 22, 2024 20:21
@zirain zirain merged commit d9dd4e6 into envoyproxy:main Oct 22, 2024
23 of 24 checks passed
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.

endpoints repeated
4 participants