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

cleanup manipulation of CQ/Cohort resource accounting in tests #2502

Open
gabesaba opened this issue Jun 28, 2024 · 9 comments · May be fixed by #2749
Open

cleanup manipulation of CQ/Cohort resource accounting in tests #2502

gabesaba opened this issue Jun 28, 2024 · 9 comments · May be fixed by #2749
Assignees
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.

Comments

@gabesaba
Copy link
Contributor

gabesaba commented Jun 28, 2024

What would you like to be cleaned:
We sometimes manipulate the internal accounting of ClusterQueue and Cohort snapshots. This is brittle. Quota/Lending Limit/Borrowing Limit/Usage should be set by calling public api methods, or by defining the CQs/Cohorts via API objects, adding to cache, and creating snapshot (see #2486 for an example).

See this thread #2486 (comment)

Why is this needed:
Tests are brittle, and make cache/snapshot hard to update

@gabesaba gabesaba added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Jun 28, 2024
@alculquicondor
Copy link
Contributor

Also #2486 (comment)

@gabesaba
Copy link
Contributor Author

gabesaba commented Jul 8, 2024

We also test against internal state of usage/quotas/limits. I will link those cleanup PRs to this issue as well.

@gabesaba
Copy link
Contributor Author

These are the (hopefully) last two remaining. See #2583 for a complete example of refactoring test, changing ClusterQueueSnapshot to API type, and representing cohort capacity/usage via use of a 2nd cluster queue.

@highpon
Copy link
Member

highpon commented Jul 20, 2024

/assign

@gabesaba
Copy link
Contributor Author

Thanks for helping with the cleanup, @highpon! Will you also be fixing flavorassigner_test.go?

@highpon
Copy link
Member

highpon commented Jul 22, 2024

Yes! I would love to work with you on flavorassigner_test.go as well!

@gabesaba
Copy link
Contributor Author

gabesaba commented Aug 1, 2024

Hi @highpon, will you still cleanup flavorassigner_test.go? If not, I will see if @vladikkuzn can work on it

@highpon highpon linked a pull request Aug 1, 2024 that will close this issue
@highpon
Copy link
Member

highpon commented Aug 1, 2024

@gabesaba
Sorry for the delay in responding!
I have now refactored the file up to the halfway point.
Three tests are failing. After fixing them, I will change the PR from Draft to Open.

@gabesaba
Copy link
Contributor Author

Discussed over Slack with @highpon. @vladikkuzn, could you complete the remaining cleanup please?

/assign @vladikkuzn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants