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

[YUNIKORN-2908] Remove associated metrics when queue is removed #987

Closed
wants to merge 1 commit into from

Conversation

hguo25
Copy link

@hguo25 hguo25 commented Oct 18, 2024

What is this PR for?

Delete all metrics so a deleted queue will not continue to report 'guaranteed resource' 'max resource'

What type of PR is it?

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

Todos

  • - Task

What is the Jira issue?

https://issues.apache.org/jira/browse/YUNIKORN-2908

How should this be tested?

Screenshots (if appropriate)

Questions:

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

@pbacsko
Copy link
Contributor

pbacsko commented Oct 21, 2024

@hguo25 is this change ready for review? I can only see two "wip" commits. If it's not ready, please make it draft by clicking on "Convert to Draft".

@pbacsko
Copy link
Contributor

pbacsko commented Oct 21, 2024

Also, when ready, squash the commits into a single one and change the commit message to "[YUNIKORN-nnnn] JIRA title" format. Thanks.

@hguo25 hguo25 force-pushed the hguoossremovequeuemetrics branch from 88ccbd9 to e544145 Compare October 21, 2024 20:13
@hguo25
Copy link
Author

hguo25 commented Oct 21, 2024

@pbacsko yes it's ready. Squashed commits as you asked. Please review, thanks

@pbacsko pbacsko requested review from wilfred-s, craigcondit, pbacsko and manirajv06 and removed request for wilfred-s and craigcondit October 22, 2024 12:41
Copy link

codecov bot commented Oct 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (master@5d3eb36). Learn more about missing BASE report.
Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #987   +/-   ##
=========================================
  Coverage          ?   81.32%           
=========================================
  Files             ?       97           
  Lines             ?    15491           
  Branches          ?        0           
=========================================
  Hits              ?    12598           
  Misses            ?     2610           
  Partials          ?      283           

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

Copy link
Contributor

@pbacsko pbacsko left a comment

Choose a reason for hiding this comment

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

There's no coverage for the new functions in the "metrics" package. Please extend the coverage.

Copy link
Contributor

@craigcondit craigcondit left a comment

Choose a reason for hiding this comment

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

Approach is good, add tests (as Peter indicated).

@hguo25 hguo25 force-pushed the hguoossremovequeuemetrics branch 2 times, most recently from 35c7c74 to e175210 Compare October 24, 2024 00:43
@hguo25
Copy link
Author

hguo25 commented Oct 24, 2024

thanks @pbacsko @craigcondit I have added test

pkg/metrics/queue_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@pbacsko pbacsko left a comment

Choose a reason for hiding this comment

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

Address linter error.

Run locally with 'make lint".

@hguo25 hguo25 force-pushed the hguoossremovequeuemetrics branch from e175210 to 4eee4d0 Compare October 25, 2024 21:34
@hguo25 hguo25 force-pushed the hguoossremovequeuemetrics branch from 4eee4d0 to 6e1def7 Compare October 26, 2024 00:30
Copy link
Contributor

@pbacsko pbacsko left a comment

Choose a reason for hiding this comment

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

+1 LGTM (pending e2e)

@hguo25
Copy link
Author

hguo25 commented Oct 29, 2024

@pbacsko thanks for the review. The e2e failed in gang scheduling, which doesn't seem to be related to my change?

@wilfred-s
Copy link
Contributor

We need a follow up on YUNIKORN-2926 to fix the gang scheduling e2e test. The changes I made there are not enough to fix the whole issue. Logged new jira for that.

Copy link
Contributor

@craigcondit craigcondit left a comment

Choose a reason for hiding this comment

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

+1 LGTM.

@craigcondit craigcondit changed the title [YUNIKORN-2908] unregister its metrics when a queue is removed [YUNIKORN-2908] Remove associated metrics when queue is removed Oct 29, 2024
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.

4 participants