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

eviction_manager _test.go should have unit tests that cover localStorageCapacityIsolation #120062

Closed
kannon92 opened this issue Aug 18, 2023 · 16 comments · Fixed by #120185
Closed
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. sig/node Categorizes an issue or PR as relevant to SIG Node. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@kannon92
Copy link
Contributor

As I was reading the test code, I noticed that we are missing a pretty large chunk of logic around localStorageCapacityIsolation.

This feature was GA in 1.25 so it would be ideal to have unit test coverage around this code.

	// evict pods if there is a resource usage violation from local volume temporary storage
	// If eviction happens in localStorageEviction function, skip the rest of eviction action
	if m.localStorageCapacityIsolation {
		if evictedPods := m.localStorageEviction(activePods, statsFunc); len(evictedPods) > 0 {
			return evictedPods
		}
	}

This code monitors whether or not there is a violation in temporary storage and if there is it would evict the pod.

One should be able to intiailize the manager with localStorageCapacityIsolation of true and then write some test cases to verify this behavior.

/help
/sig node
/good-first-issue
/kind bug

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/bug Categorizes issue or PR as related to a bug. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 18, 2023
@kannon92
Copy link
Contributor Author

/cc @jingxu97
do you have any recollection why there weren't tests added for this path?

@charles-chenzz
Copy link
Member

I can help with this. can you assign me after jingxu replies to you?(for more context here) @kannon92

@jingxu97
Copy link
Contributor

that would be great @charles-chenzz !

@charles-chenzz
Copy link
Member

that would be great @charles-chenzz !

you're welcome. do you have any context here that why didn't cover localStorageCapacityIsolation?

@jingxu97
Copy link
Contributor

I don't recall a specific reason. The feature was implemented a while back.

@whitebeard10
Copy link

/assign

@whitebeard10 whitebeard10 removed their assignment Aug 19, 2023
@Sajiyah-Salat
Copy link

Hey @charles-chenzz Are you still working. If not can I take this up?

@charles-chenzz
Copy link
Member

Hey @charles-chenzz Are you still working. If not can I take this up?

@Sajiyah-Salat hi, yes I'm still working on it

@shindodkar
Copy link

shindodkar commented Aug 20, 2023

@charles-chenzz hii I am new to k8s , so i wanna know can I use windows os for it? if yes would you show me how to get nodes...

@charles-chenzz
Copy link
Member

@charles-chenzz hii I am new to k8s , so i wanna know can I use windows os for it? if yes would you show me how to get nodes...

hi, the answer is yes you can use windows and I think if you need support you better open another issue and @ me there, as this issue is not for discuss how to run k8s on windows @shindodkar

@SergeyKanzhelev SergeyKanzhelev added this to Triage in SIG Node Bugs Aug 22, 2023
@mmiranda96
Copy link
Contributor

/remove-kind bug
/kind feature
/triage accepted

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed kind/bug Categorizes issue or PR as related to a bug. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Aug 23, 2023
@mmiranda96 mmiranda96 moved this from Triage to Triaged in SIG Node Bugs Aug 23, 2023
@kannon92
Copy link
Contributor Author

I see https://github.com/kubernetes/kubernetes/pull/119570/files and I am wondering if we have duplicated efforts here.

@claassen did you end up testing this path in our above PR?

@Pankaj-SinghR
Copy link

Hey, @kannon92 is someone working on this issue ? or let me know if someone need help

@sachasmart
Copy link

sachasmart commented Sep 21, 2023

@kannon92 Happy to help with this. Do you still need help

@ananthmuppidi
Copy link

@kannon92 hey, is this currently still unassigned

@kannon92
Copy link
Contributor Author

kannon92 commented Oct 1, 2023

@charles-chenzz has a #120185 ready for approval.

/remove-good-first-issue
/remove-help

@k8s-ci-robot k8s-ci-robot removed help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. labels Oct 1, 2023
SIG Node Bugs automation moved this from Triaged to Done Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. sig/node Categorizes an issue or PR as relevant to SIG Node. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
Development

Successfully merging a pull request may close this issue.