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-2963] E2E Test for Foreign Pod Tracking #935

Closed

Conversation

rrajesh-cloudera
Copy link
Contributor

@rrajesh-cloudera rrajesh-cloudera commented Nov 5, 2024

What is this PR for?

This pull request introduces several end-to-end tests for verifying the tracking of foreign pods in the YuniKorn scheduler. The tests cover the following scenarios:

  1. Verify foreign pod tracking fails for untracked or incorrectly mapped pods
  2. Verify multiple foreign pods
  3. Verify foreign pods on different nodes

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-2963

How should this be tested?

Screenshots (if appropriate)

Questions:

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

@rrajesh-cloudera
Copy link
Contributor Author

Hi @pbacsko , Could you please take a look ?

@pbacsko
Copy link
Contributor

pbacsko commented Nov 5, 2024

@rrajesh-cloudera I'm sorry to say but we already have e2e tests. Please see tests. Looks like there was a communication issue :/

I'll take a look but I don't think we need more tests.

EDIT: oh, I can see you added the new tests to the same file. Anyway, I'm looking at them.

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.05%. Comparing base (412d337) to head (e962835).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #935   +/-   ##
=======================================
  Coverage   68.05%   68.05%           
=======================================
  Files          70       70           
  Lines        9195     9195           
=======================================
  Hits         6258     6258           
  Misses       2730     2730           
  Partials      207      207           

☔ 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.

Ok. I've checked the tests. I don't see what extra they're performing besides the original Verify foreign pod tracking which I added 2 weeks ago. All tests just retrieve pods from kube-system and the assertions are slightly different, but that's it, it's basically the same in all.

Could you explain?

@rrajesh-cloudera
Copy link
Contributor Author

@rrajesh-cloudera I'm sorry to say but we already have e2e tests. Please see tests. Looks like there was a communication issue :/

I'll take a look but I don't think we need more tests.

EDIT: oh, I can see you added the new tests to the same file. Anyway, I'm looking at them.

The First one is positive test case which you added , the 2nd is a negative test case for the first in which we are simulating an error where a pod from kube-system is either untracked or incorrectly mapped.
The Third case Focuses on ensuring multiple pods from kube-system are all being tracked and 4th case Verifies that foreign pods are accurately tracked across different nodes.
Each case targets different potential issues, ensuring comprehensive coverage of the system’s functionality and robustness

Comment on lines +101 to +105
// Introduce a mismatch to simulate an error
if string(falloc.AllocationKey) == "incorrect-uid" {
foreignAllocs[falloc.AllocationKey] = true
foreignNodes[falloc.AllocationKey] = "wrong-node"
} else {
Copy link
Contributor

@pbacsko pbacsko Nov 5, 2024

Choose a reason for hiding this comment

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

I genuinely don't get this. I don't think this path is ever taken.

In all tests, you work with a fixed set of pods (allocations) from "kube-system". Those are system pods that are already running as they should be. You're not doing anything with those pods or with Yunikorn. The contents of n.ForeignAllocations will always be the same.

Unless there are some uncommitted/already-committed changes somewhere else which is not visible in the PR, this test (and the remaining ones) does not make any sense.

@pbacsko
Copy link
Contributor

pbacsko commented Nov 5, 2024

The First one is positive test case which you added , the 2nd is a negative test case for the first in which we are simulating an error where a pod from kube-system is either untracked or incorrectly mapped.
The Third case Focuses on ensuring multiple pods from kube-system are all being tracked and 4th case Verifies that foreign pods are accurately tracked across different nodes.
Each case targets different potential issues, ensuring comprehensive coverage of the system’s functionality and robustness

This is just simply not the case. Let's discuss this offline.

EDIT: sry, I closed the PR by accident.

@pbacsko pbacsko closed this Nov 5, 2024
@pbacsko pbacsko reopened this Nov 5, 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.

2 participants