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-1957] Add user/group limit config change e2e test #735

Closed
wants to merge 1 commit into from
Closed

[YUNIKORN-1957] Add user/group limit config change e2e test #735

wants to merge 1 commit into from

Conversation

pegasas
Copy link
Contributor

@pegasas pegasas commented Nov 23, 2023

What is this PR for?

use the unit test cases in yunikorn-core for add e2e test

What type of PR is it?

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

Todos

  • - Task

What is the Jira issue?

[YUNIKORN-1957] Add user/group limit config change e2e test

How should this be tested?

Specific user/group limits limits changes to ensure order of precedence:

  1. set user limit & wild card user limit only and ensure it has been honoured.
    5a) When the user limit is specified, it should be considered for that specific user
    5b) When the user limit is changed, make sure it meets new limit of for that specific group
  2. set group limit & wild card group limit only and ensure it has been honoured.
    6a) When the group limit is specified, it should be considered for that specific group
    6b) When the group limit is changed, make sure it meets new limit of for that specific group
  3. Repeat the above for max resources and max applications individually and together as well

Screenshots (if appropriate)

Questions:

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

@pegasas pegasas marked this pull request as ready for review November 23, 2023 11:29
@pbacsko
Copy link
Contributor

pbacsko commented Nov 23, 2023

@pegasas please resolve the conflict to proceed

@pegasas
Copy link
Contributor Author

pegasas commented Nov 24, 2023

@pegasas please resolve the conflict to proceed

Hi, @pbacsko , thank you for your quick reply. conflicts are resolved.
Also, I am referencing the sample involved in apache/yunikorn-core@0a05068, it seems not to work as expected.

It is my case not in right way or there is bug with wildcard?

@doupache
Copy link
Contributor

Hi @pegasas, thanks for the effort! I am working on https://issues.apache.org/jira/browse/YUNIKORN-1950
But i found a bug and try to fix it.

add to following test to the end TestUserGroupLimitChange test.

//remove Limits , so we can run TestApp2 again
conf.Queues[0].Queues[0].Limits = nil
assert.NilError(t, manager.UpdateConfig(conf.Queues[0], "root"))

increased = manager.IncreaseTrackedResource(queuePathParent, TestApp2, usage, tc.user)
assert.Equal(t, increased, true, "unable to increase tracked resource: queuepath "+queuePathParent+", app "+TestApp2+", res "+usage.String())

decreased = manager.DecreaseTrackedResource(queuePathParent, TestApp2, usage, tc.user, true)
assert.Equal(t, decreased, true, "unable to decreased tracked resource: queuepath "+queuePathParent+", app "+TestApp2+", res "+usage.String())

//set newLimits again, we should not run TestApp2 now
conf.Queues[0].Queues[0].Limits = tc.newLimits
assert.NilError(t, manager.UpdateConfig(conf.Queues[0], "root"))

increased = manager.IncreaseTrackedResource(queuePathParent, TestApp2, usage, tc.user)
assert.Equal(t, increased, false, "should not increase tracked resource: queuepath "+queuePathParent+", app "+TestApp2+", res "+usage.String())

Copy link

codecov bot commented Nov 25, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2738571) 69.43% compared to head (dec525b) 69.46%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #735      +/-   ##
==========================================
+ Coverage   69.43%   69.46%   +0.02%     
==========================================
  Files          50       50              
  Lines        7993     7993              
==========================================
+ Hits         5550     5552       +2     
+ Misses       2254     2252       -2     
  Partials      189      189              

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

@pbacsko
Copy link
Contributor

pbacsko commented Nov 26, 2023

@manirajv06, you have more context regarding the feature. Please check if the comments above.

@pbacsko pbacsko requested a review from manirajv06 November 26, 2023 13:46
@manirajv06
Copy link
Contributor

@pegasas please resolve the conflict to proceed

Hi, @pbacsko , thank you for your quick reply. conflicts are resolved. Also, I am referencing the sample involved in apache/yunikorn-core@0a05068, it seems not to work as expected.

It is my case not in right way or there is bug with wildcard?

I don't think above mentioned core commit should affect this test behaviour. Can you resolve the conflict and try running again?

@pegasas
Copy link
Contributor Author

pegasas commented Dec 4, 2023

I don't think above mentioned core commit should affect this test behaviour. Can you resolve the conflict and try running again?

Thanks @manirajv06

It seems conflicts was introduced by dd02f2e which was merged yesterday

I have fixed this and re-commit.

Copy link
Contributor

@manirajv06 manirajv06 left a comment

Choose a reason for hiding this comment

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

overall looks good.

Can you organise the changes?

  1. user max resources
  2. user max applications
  3. group max resources
  4. group max applications

})

ginkgo.It("Verify_maxresources_with_a_wildcard_group_limit", func() {
ginkgo.It("Verify_maxapplications_with_users_limits_changed", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

groups ?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this block is for users as I can see 2 blocks below for groups. If it is true, then we need to check the config in this block as it doesn't have any limits for users

})

ginkgo.It("Verify_maxapplications_with_a_wildcard_group_limit", func() {
ginkgo.It("Verify_resources_with_groups_limits_changed", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

maxresources?

})

ginkgo.It("Verify_maxresources_with_a_wildcard_group_limit", func() {
ginkgo.It("Verify_maxapplications_with_users_limits_changed", func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this block is for users as I can see 2 blocks below for groups. If it is true, then we need to check the config in this block as it doesn't have any limits for users

@pbacsko pbacsko self-requested a review January 16, 2024 20:21
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.

@pegasas please investigate&fix the test failures:

Summarizing 4 Failures:
  [FAIL] UserGroupLimit [It] Verify_maxresources_with_users_limits_changed
  /home/runner/work/yunikorn-k8shim/yunikorn-k8shim/test/e2e/user_group_limit/user_group_limit_test.go:841
  [FAIL] UserGroupLimit [It] Verify_maxapplications_with_users_limits_changed
  /home/runner/work/yunikorn-k8shim/yunikorn-k8shim/test/e2e/user_group_limit/user_group_limit_test.go:885
  [FAIL] UserGroupLimit [It] Verify_maxresources_with_groups_limits_changed
  /home/runner/work/yunikorn-k8shim/yunikorn-k8shim/test/e2e/user_group_limit/user_group_limit_test.go:841
  [FAIL] UserGroupLimit [It] Verify_maxapplications_with_groups_limits_changed
  /home/runner/work/yunikorn-k8shim/yunikorn-k8shim/test/e2e/user_group_limit/user_group_limit_test.go:841

@pbacsko
Copy link
Contributor

pbacsko commented Mar 27, 2024

ping @pegasas have you had time to check the test failures?

@pegasas pegasas closed this by deleting the head repository May 14, 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