[YUNIKORN-2855] Rethink the call for update metrics with nil resource

[zhuqi-lucas]

What is this PR for?

We will not update the nil resource to resource metrics, we need to fix it.

What type of PR is it?

• - Bug Fix
• - Improvement
• - Feature
• - Documentation
• - Hot Fix
• - Refactoring

Todos

• - Task

What is the Jira issue?

• Open an issue on Jira https://issues.apache.org/jira/browse/YUNIKORN-2855
• Put link here, and add [YUNIKORN-Jira number] in PR title, eg. [YUNIKORN-2] Gang scheduling interface parameters

How should this be tested?

Screenshots (if appropriate)

Questions:

• - The licenses files need update.
• - There is breaking changes for older versions.
• - It needs documentation.

[codecov]

Codecov Report

Attention: Patch coverage is 85.71429% with 3 lines in your changes missing coverage. Please review.

Project coverage is 80.96%. Comparing base (9e10746) to head (d520339).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
pkg/metrics/queue.go	76.92%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #960      +/-   ##
==========================================
+ Coverage   80.95%   80.96%   +0.01%     
==========================================
  Files          97       97              
  Lines       12514    12527      +13     
==========================================
+ Hits        10131    10143      +12     
- Misses       2113     2115       +2     
+ Partials      270      269       -1     

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

[craigcondit]

I'm -1 on this approach. This adds significant complexity to the code and requires all callers to do the calculations... make the metrics functions handle the nil case properly themselves.

[zhuqi-lucas]

Thanks @craigcondit for review, i addressed the code to a more reasonable way and setting zero resource for nil update case.

[craigcondit]

Maybe I'm missing something entirely... Why are we setting arbitrary resources to zero here? What is it we're trying to accomplish?

[zhuqi-lucas]

@craigcondit , we support setting maxResource/ guaranteed resource to nil, but the metrics update is wrong because we don't update it from previous value update to zero/nil, for example:

yunikorn-core/pkg/scheduler/objects/queue.go

Line 415 in 7c51e82

sq.maxResource = nil

yunikorn-core/pkg/scheduler/objects/queue.go

Line 441 in 7c51e82

sq.guaranteedResource = nil

[craigcondit]

Then it seems the proper solution is to retrieve the previous value of the metric and update the values there (an "unprune" so to speak). Missing values in the new map should be set to zero. Setting arbitrary resources is just wrong. Doing it this way also has the advantage of ensuring that any resource types that were emitted by the metric previously are still emitted, but with zero instead of missing.

[zhuqi-lucas]

Thanks, @craigcondit i like the idea unprune, i will address this way.

[craigcondit]

Just... no. You already have the metric in the collector itself. Read it out to get the list of values that are previously set and build a new metric using a combination of the old and new values. There should be no code changes outside the collector at all.

[zhuqi-lucas]

I see... @craigcondit let me change my code.

[zhuqi-lucas]

Addressed in latest PR:

1. Getting the existed metrics.
2. Setting those field to zero.

[craigcondit]

The code is still too complex. See review comments.

[craigcondit]

This should be made an internal function once logic is moved here from scheduler/objects/queue.go.

[craigcondit]

Add locking per comments.

[craigcondit]

Change lock type.

[craigcondit]

This doesn't need to be an RWMutex as we never lock for reads.

[zhuqi-lucas]

Good catch, addressed in latest PR, thanks!

[craigcondit]

+1 LGTM.