You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2022/09/03 13:03:09 UTC

[GitHub] [yunikorn-k8shim] manirajv06 opened a new pull request, #460: [YUNIKORN-1281] Add e2e tests

manirajv06 opened a new pull request, #460:
URL: https://github.com/apache/yunikorn-k8shim/pull/460

   ### What is this PR for?
   Added e2e tests to cover arbitrary resources support. 
   
   
   ### What type of PR is it?
   * [ ] - Improvement
   
   ### Todos
   * [ ] - Task
   
   ### What is the Jira issue?
   https://issues.apache.org/jira/browse/YUNIKORN-1281
   
   ### How should this be tested?
   
   ### Screenshots (if appropriate)
   
   ### Questions:
   * [ ] - The licenses files need update.
   * [ ] - There is breaking changes for older versions.
   * [ ] - It needs documentation.
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [yunikorn-k8shim] pbacsko commented on a diff in pull request #460: [YUNIKORN-1281] Add e2e tests

Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #460:
URL: https://github.com/apache/yunikorn-k8shim/pull/460#discussion_r968370372


##########
test/e2e/queue_quota_mgmt/queue_quota_mgmt_test.go:
##########
@@ -108,38 +145,39 @@ var _ = Describe("", func() {
 			usedPerctResource.ParseResourceUsage(queueInfo.AbsUsedCapacity)
 
 			By(fmt.Sprintf("App-%d: Verify max capacity on the queue is accurate", iter))
-			Ω(maxResource.GetVCPU()).Should(Equal(maxCPU))
-			Ω(maxResource.GetMemory()).Should(Equal(maxMem * 1000 * 1000))
+			Ω(maxResource.GetResourceValue(siCommon.CPU)).Should(Equal(maxCPU))
+			Ω(maxResource.GetResourceValue(siCommon.Memory)).Should(Equal(maxMem * 1000 * 1000))
+			Ω(maxResource.GetResourceValue("nvidia.com/gpu")).Should(Equal(maxGPU))
 
 			By(fmt.Sprintf("App-%d: Verify used capacity on the queue is accurate after 1st pod deployment", iter))
-			Ω(usedResource.GetVCPU()).Should(Equal(reqCPU * iter))
-			Ω(usedResource.GetMemory()).Should(Equal(reqMem * iter * 1000 * 1000))
+			Ω(usedResource.GetResourceValue(siCommon.CPU)).Should(Equal(reqCPU * iter))
+			Ω(usedResource.GetResourceValue(siCommon.Memory)).Should(Equal(reqMem * iter * 1000 * 1000))
 
 			var perctCPU = int64(math.Floor((float64(reqCPU*iter) / float64(maxCPU)) * 100))
 			var perctMem = int64(math.Floor((float64(reqMem*iter) / float64(maxMem)) * 100))
 			By(fmt.Sprintf("App-%d: Verify used percentage capacity on the queue is accurate after 1st pod deployment", iter))
-			Ω(usedPerctResource.GetVCPU()).Should(Equal(perctCPU))
-			Ω(usedPerctResource.GetMemory()).Should(Equal(perctMem))
+			Ω(usedPerctResource.GetResourceValue(siCommon.CPU)).Should(Equal(perctCPU))
+			Ω(usedPerctResource.GetResourceValue(siCommon.Memory)).Should(Equal(perctMem))
 		}
 
 		By("App-4: Submit another app which exceeds the queue quota limitation")

Review Comment:
   This is still `App-4`, is this OK?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [yunikorn-k8shim] manirajv06 commented on a diff in pull request #460: [YUNIKORN-1281] Add e2e tests

Posted by GitBox <gi...@apache.org>.
manirajv06 commented on code in PR #460:
URL: https://github.com/apache/yunikorn-k8shim/pull/460#discussion_r968422735


##########
test/e2e/queue_quota_mgmt/queue_quota_mgmt_test.go:
##########
@@ -108,38 +145,39 @@ var _ = Describe("", func() {
 			usedPerctResource.ParseResourceUsage(queueInfo.AbsUsedCapacity)
 
 			By(fmt.Sprintf("App-%d: Verify max capacity on the queue is accurate", iter))
-			Ω(maxResource.GetVCPU()).Should(Equal(maxCPU))
-			Ω(maxResource.GetMemory()).Should(Equal(maxMem * 1000 * 1000))
+			Ω(maxResource.GetResourceValue(siCommon.CPU)).Should(Equal(maxCPU))
+			Ω(maxResource.GetResourceValue(siCommon.Memory)).Should(Equal(maxMem * 1000 * 1000))
+			Ω(maxResource.GetResourceValue("nvidia.com/gpu")).Should(Equal(maxGPU))
 
 			By(fmt.Sprintf("App-%d: Verify used capacity on the queue is accurate after 1st pod deployment", iter))
-			Ω(usedResource.GetVCPU()).Should(Equal(reqCPU * iter))
-			Ω(usedResource.GetMemory()).Should(Equal(reqMem * iter * 1000 * 1000))
+			Ω(usedResource.GetResourceValue(siCommon.CPU)).Should(Equal(reqCPU * iter))
+			Ω(usedResource.GetResourceValue(siCommon.Memory)).Should(Equal(reqMem * iter * 1000 * 1000))
 
 			var perctCPU = int64(math.Floor((float64(reqCPU*iter) / float64(maxCPU)) * 100))
 			var perctMem = int64(math.Floor((float64(reqMem*iter) / float64(maxMem)) * 100))
 			By(fmt.Sprintf("App-%d: Verify used percentage capacity on the queue is accurate after 1st pod deployment", iter))
-			Ω(usedPerctResource.GetVCPU()).Should(Equal(perctCPU))
-			Ω(usedPerctResource.GetMemory()).Should(Equal(perctMem))
+			Ω(usedPerctResource.GetResourceValue(siCommon.CPU)).Should(Equal(perctCPU))
+			Ω(usedPerctResource.GetResourceValue(siCommon.Memory)).Should(Equal(perctMem))
 		}
 
 		By("App-4: Submit another app which exceeds the queue quota limitation")
 		sleepPodConfigs := k8s.SleepPodConfig{NS: ns, Time: 60, CPU: reqCPU, Mem: reqMem}
 		sleepObj, app4PodErr := k8s.InitSleepPod(sleepPodConfigs)
 		Ω(app4PodErr).NotTo(HaveOccurred())
 
-		By(fmt.Sprintf("App-4: Deploy the sleep app:%s to %s namespace", sleepObj.Name, ns))
+		By(fmt.Sprintf("App-N: Deploy the sleep app:%s to %s namespace", sleepObj.Name, ns))

Review Comment:
   Ideally it should be iter+1, named it as "N" as it can be read as nth app/pod. Now, named it with the correct count to avoid confusion.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [yunikorn-k8shim] pbacsko commented on a diff in pull request #460: [YUNIKORN-1281] Add e2e tests

Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #460:
URL: https://github.com/apache/yunikorn-k8shim/pull/460#discussion_r968372188


##########
test/e2e/queue_quota_mgmt/queue_quota_mgmt_test.go:
##########
@@ -108,38 +145,39 @@ var _ = Describe("", func() {
 			usedPerctResource.ParseResourceUsage(queueInfo.AbsUsedCapacity)
 
 			By(fmt.Sprintf("App-%d: Verify max capacity on the queue is accurate", iter))
-			Ω(maxResource.GetVCPU()).Should(Equal(maxCPU))
-			Ω(maxResource.GetMemory()).Should(Equal(maxMem * 1000 * 1000))
+			Ω(maxResource.GetResourceValue(siCommon.CPU)).Should(Equal(maxCPU))
+			Ω(maxResource.GetResourceValue(siCommon.Memory)).Should(Equal(maxMem * 1000 * 1000))
+			Ω(maxResource.GetResourceValue("nvidia.com/gpu")).Should(Equal(maxGPU))
 
 			By(fmt.Sprintf("App-%d: Verify used capacity on the queue is accurate after 1st pod deployment", iter))
-			Ω(usedResource.GetVCPU()).Should(Equal(reqCPU * iter))
-			Ω(usedResource.GetMemory()).Should(Equal(reqMem * iter * 1000 * 1000))
+			Ω(usedResource.GetResourceValue(siCommon.CPU)).Should(Equal(reqCPU * iter))
+			Ω(usedResource.GetResourceValue(siCommon.Memory)).Should(Equal(reqMem * iter * 1000 * 1000))
 
 			var perctCPU = int64(math.Floor((float64(reqCPU*iter) / float64(maxCPU)) * 100))
 			var perctMem = int64(math.Floor((float64(reqMem*iter) / float64(maxMem)) * 100))
 			By(fmt.Sprintf("App-%d: Verify used percentage capacity on the queue is accurate after 1st pod deployment", iter))
-			Ω(usedPerctResource.GetVCPU()).Should(Equal(perctCPU))
-			Ω(usedPerctResource.GetMemory()).Should(Equal(perctMem))
+			Ω(usedPerctResource.GetResourceValue(siCommon.CPU)).Should(Equal(perctCPU))
+			Ω(usedPerctResource.GetResourceValue(siCommon.Memory)).Should(Equal(perctMem))
 		}
 
 		By("App-4: Submit another app which exceeds the queue quota limitation")
 		sleepPodConfigs := k8s.SleepPodConfig{NS: ns, Time: 60, CPU: reqCPU, Mem: reqMem}
 		sleepObj, app4PodErr := k8s.InitSleepPod(sleepPodConfigs)
 		Ω(app4PodErr).NotTo(HaveOccurred())
 
-		By(fmt.Sprintf("App-4: Deploy the sleep app:%s to %s namespace", sleepObj.Name, ns))
+		By(fmt.Sprintf("App-N: Deploy the sleep app:%s to %s namespace", sleepObj.Name, ns))

Review Comment:
   The same question applies to line 173, 180, 189.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [yunikorn-k8shim] codecov[bot] commented on pull request #460: [YUNIKORN-1281] Add e2e tests

Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #460:
URL: https://github.com/apache/yunikorn-k8shim/pull/460#issuecomment-1236118137

   # [Codecov](https://codecov.io/gh/apache/yunikorn-k8shim/pull/460?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
   > Merging [#460](https://codecov.io/gh/apache/yunikorn-k8shim/pull/460?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (54564dd) into [master](https://codecov.io/gh/apache/yunikorn-k8shim/commit/1f050c40396513b0b60eec87c6b10b0647fcfbc7?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (1f050c4) will **decrease** coverage by `0.02%`.
   > The diff coverage is `n/a`.
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #460      +/-   ##
   ==========================================
   - Coverage   67.00%   66.97%   -0.03%     
   ==========================================
     Files          41       41              
     Lines        6767     6767              
   ==========================================
   - Hits         4534     4532       -2     
   - Misses       2062     2064       +2     
     Partials      171      171              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/yunikorn-k8shim/pull/460?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
   |---|---|---|
   | [pkg/dispatcher/dispatcher.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/460/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2Rpc3BhdGNoZXIvZGlzcGF0Y2hlci5nbw==) | `74.82% <0.00%> (-1.40%)` | :arrow_down: |
   
   :mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [yunikorn-k8shim] pbacsko closed pull request #460: [YUNIKORN-1281] Add e2e tests

Posted by GitBox <gi...@apache.org>.
pbacsko closed pull request #460: [YUNIKORN-1281] Add e2e tests
URL: https://github.com/apache/yunikorn-k8shim/pull/460


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [yunikorn-k8shim] pbacsko commented on a diff in pull request #460: [YUNIKORN-1281] Add e2e tests

Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #460:
URL: https://github.com/apache/yunikorn-k8shim/pull/460#discussion_r968371385


##########
test/e2e/queue_quota_mgmt/queue_quota_mgmt_test.go:
##########
@@ -108,38 +145,39 @@ var _ = Describe("", func() {
 			usedPerctResource.ParseResourceUsage(queueInfo.AbsUsedCapacity)
 
 			By(fmt.Sprintf("App-%d: Verify max capacity on the queue is accurate", iter))
-			Ω(maxResource.GetVCPU()).Should(Equal(maxCPU))
-			Ω(maxResource.GetMemory()).Should(Equal(maxMem * 1000 * 1000))
+			Ω(maxResource.GetResourceValue(siCommon.CPU)).Should(Equal(maxCPU))
+			Ω(maxResource.GetResourceValue(siCommon.Memory)).Should(Equal(maxMem * 1000 * 1000))
+			Ω(maxResource.GetResourceValue("nvidia.com/gpu")).Should(Equal(maxGPU))
 
 			By(fmt.Sprintf("App-%d: Verify used capacity on the queue is accurate after 1st pod deployment", iter))
-			Ω(usedResource.GetVCPU()).Should(Equal(reqCPU * iter))
-			Ω(usedResource.GetMemory()).Should(Equal(reqMem * iter * 1000 * 1000))
+			Ω(usedResource.GetResourceValue(siCommon.CPU)).Should(Equal(reqCPU * iter))
+			Ω(usedResource.GetResourceValue(siCommon.Memory)).Should(Equal(reqMem * iter * 1000 * 1000))
 
 			var perctCPU = int64(math.Floor((float64(reqCPU*iter) / float64(maxCPU)) * 100))
 			var perctMem = int64(math.Floor((float64(reqMem*iter) / float64(maxMem)) * 100))
 			By(fmt.Sprintf("App-%d: Verify used percentage capacity on the queue is accurate after 1st pod deployment", iter))
-			Ω(usedPerctResource.GetVCPU()).Should(Equal(perctCPU))
-			Ω(usedPerctResource.GetMemory()).Should(Equal(perctMem))
+			Ω(usedPerctResource.GetResourceValue(siCommon.CPU)).Should(Equal(perctCPU))
+			Ω(usedPerctResource.GetResourceValue(siCommon.Memory)).Should(Equal(perctMem))
 		}
 
 		By("App-4: Submit another app which exceeds the queue quota limitation")
 		sleepPodConfigs := k8s.SleepPodConfig{NS: ns, Time: 60, CPU: reqCPU, Mem: reqMem}
 		sleepObj, app4PodErr := k8s.InitSleepPod(sleepPodConfigs)
 		Ω(app4PodErr).NotTo(HaveOccurred())
 
-		By(fmt.Sprintf("App-4: Deploy the sleep app:%s to %s namespace", sleepObj.Name, ns))
+		By(fmt.Sprintf("App-N: Deploy the sleep app:%s to %s namespace", sleepObj.Name, ns))

Review Comment:
   Bit confused about this, up there is a code which uses `App-%d` where`%d` = `iter`, isn't that what we need here?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [yunikorn-k8shim] manirajv06 commented on pull request #460: [YUNIKORN-1281] Add e2e tests

Posted by GitBox <gi...@apache.org>.
manirajv06 commented on PR #460:
URL: https://github.com/apache/yunikorn-k8shim/pull/460#issuecomment-1243854277

   test failures are not related.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org