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

fix(sysadvisor): fix create duplicated regions for distinct container… #37

Merged
merged 1 commit into from
Apr 25, 2023

Conversation

cheney-lin
Copy link
Member

…s owned to identical pod

What type of PR is this?

Bug fixes

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

@cheney-lin cheney-lin added bug Something isn't working workflow/need-review review: test succeeded, need to review labels Apr 20, 2023
@codecov
Copy link

codecov bot commented Apr 20, 2023

Codecov Report

Patch coverage: 68.88% and project coverage change: -0.07 ⚠️

Comparison is base (81ce219) 50.99% compared to head (ea621e6) 50.93%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #37      +/-   ##
==========================================
- Coverage   50.99%   50.93%   -0.07%     
==========================================
  Files         311      311              
  Lines       31796    31822      +26     
==========================================
- Hits        16215    16209       -6     
- Misses      13666    13694      +28     
- Partials     1915     1919       +4     
Flag Coverage Δ
unittest 50.93% <68.88%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...r/plugin/qosaware/reporter/manager/resource/cpu.go 100.00% <ø> (ø)
...lugin/qosaware/reporter/manager/resource/memory.go 100.00% <ø> (ø)
.../sysadvisor/qosaware/reporter/headroom_reporter.go 100.00% <ø> (ø)
...sysadvisor/plugin/qosaware/resource/cpu/advisor.go 74.60% <62.85%> (-4.46%) ⬇️
.../sysadvisor/qosaware/reporter/headroom_reporter.go 70.83% <66.66%> (ø)
pkg/agent/sysadvisor/metacache/metacache.go 82.22% <100.00%> (-1.78%) ⬇️
...ugin/qosaware/reporter/manager/resource/generic.go 100.00% <100.00%> (ø)
pkg/agent/sysadvisor/types/helper.go 71.20% <100.00%> (+0.46%) ⬆️

... and 6 files with indirect coverage changes

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

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@cheney-lin cheney-lin force-pushed the fix/duplicate_regions branch 3 times, most recently from b6fce35 to 0c5c04e Compare April 21, 2023 09:29
@cheney-lin cheney-lin added this to the v0.2 milestone Apr 21, 2023
waynepeking348
waynepeking348 previously approved these changes Apr 23, 2023
@waynepeking348
Copy link
Collaborator

why don't we just store container -> region info in metacache's container-info? for instance, other metrics pipeline will also need to get this mapping relationships too cc @sun-yuliang

@waynepeking348 waynepeking348 self-requested a review April 23, 2023 08:36
@waynepeking348 waynepeking348 dismissed their stale review April 23, 2023 08:36

as commented

@cheney-lin cheney-lin force-pushed the fix/duplicate_regions branch 2 times, most recently from 6e2242e to e9d0d71 Compare April 24, 2023 10:12
waynepeking348
waynepeking348 previously approved these changes Apr 24, 2023
@cheney-lin cheney-lin added workflow/merge-ready merge-ready: code is ready and can be merged and removed workflow/need-review review: test succeeded, need to review labels Apr 24, 2023
}
return nil, false
return regions, nil
}

func (cra *cpuResourceAdvisor) setContainerRegions(ci *types.ContainerInfo, regions []region.QoSRegion) {
Copy link
Collaborator

@sun-yuliang sun-yuliang Apr 24, 2023

Choose a reason for hiding this comment

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

setContainerRegions will not change container info in metacache because it is called by metacache.RangeContainer, which is a reader function. Use RangeAndUpdateContainer instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed

containerRegionMap map[string]map[string][]region.QoSRegion // map[podUID][containerName]regions
poolRegionMap map[string][]region.QoSRegion // map[poolName]regions
regionMap map[string]region.QoSRegion // map[regionName]region
poolRegionMap map[string][]region.QoSRegion // map[poolName]regions
Copy link
Collaborator

Choose a reason for hiding this comment

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

Move pool region names to pool info in metacache as region names in container info.

@cheney-lin cheney-lin force-pushed the fix/duplicate_regions branch 4 times, most recently from 1e2db42 to dc0a0a2 Compare April 24, 2023 13:07
sun-yuliang
sun-yuliang previously approved these changes Apr 24, 2023
…ers owned to identical pod

Signed-off-by: linzhecheng <linzhecheng@bytedance.com>
@waynepeking348 waynepeking348 self-requested a review April 25, 2023 02:09
@waynepeking348 waynepeking348 merged commit 2978188 into kubewharf:main Apr 25, 2023
@cheney-lin cheney-lin deleted the fix/duplicate_regions branch September 13, 2023 06:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working workflow/merge-ready merge-ready: code is ready and can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants