[YUNIKORN-1978] Add e2e test for node sorting with the fairness policy #673
Conversation
Codecov Report
@@ Coverage Diff @@
## master #673 +/- ##
==========================================
+ Coverage 71.90% 71.92% +0.02%
==========================================
Files 51 51
Lines 8082 8082
==========================================
+ Hits 5811 5813 +2
+ Misses 2075 2073 -2
Partials 196 196 see 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| workerID1, | ||
| workerID2, | ||
| } | ||
| increaseUtiliz := []map[string]float64{ |
There was a problem hiding this comment.
Nit: use the full word in the variable name, eg. increaseUtilization. Or perhaps incUtilization.
| {"vcore": 0.05, "memory": 0.10}, | ||
| {"vcore": 0.10, "memory": 0.05}, | ||
| {"vcore": 0.15, "memory": 0.15}, | ||
| {"vcore": 0.15, "memory": 0.15}, |
There was a problem hiding this comment.
Please extract constants, not just here, but everywhere in this file. These literals are repeated way too often.
| }) | ||
| }) | ||
|
|
||
| func fillNodeUtil(nodeId string, resourceType string, percent float64) int64 { |
There was a problem hiding this comment.
Rename the function, it calulates something, but what? I suppose it's something based on a percentage. It could be sth like getFillResourceValue().
Add a comment to make this clear.
| // Delete all sleep pods | ||
| ginkgo.By("Delete all sleep pods") | ||
| err := kClient.DeletePods(ns) | ||
| if err != nil { | ||
| fmt.Fprintf(ginkgo.GinkgoWriter, "Failed to delete pods in namespace %s - reason is %s\n", ns, err.Error()) | ||
| } |
There was a problem hiding this comment.
Drop this and run all tests in separate namespace. Running multiple tests in the same namespace is asking for trouble. See #753 as an example.
| ginkgo.By("create development namespace") | ||
| namespace, err = kClient.CreateNamespace(ns, nil) | ||
| gomega.Ω(err).NotTo(gomega.HaveOccurred()) | ||
| gomega.Ω(namespace.Status.Phase).To(gomega.Equal(v1.NamespaceActive)) |
There was a problem hiding this comment.
Drop this and run all tests in a separate namespace. Running multiple tests in the same namespace is asking for trouble. See #753 as an example.
| var namespace *v1.Namespace | ||
| var err error | ||
| var queuePath string | ||
| var basicUtiliz float64 |
There was a problem hiding this comment.
Nit: use the full word in the variable name, eg. basicUtilization. This sounds a bit strange to me.
| memUtiliz := 1.0 - float64(node.Available["memory"])/float64(node.Capacity["memory"]) | ||
| cpuUtiliz := 1.0 - float64(node.Available["vcore"])/float64(node.Capacity["vcore"]) |
There was a problem hiding this comment.
Nit: "memUsage", "cpuUsage", etc.
| By(fmt.Sprintf("Creating test namespace %s", ns)) | ||
| namespace, err = kClient.UpdateNamespace(ns, map[string]string{"vcore": "500m", "memory": "500M"}) | ||
| Ω(err).ShouldNot(HaveOccurred()) | ||
| Ω(namespace.Status.Phase).Should(Equal(v1.NamespaceActive)) |
There was a problem hiding this comment.
As explained, create this in BeforeEach() and clean up in AfterEach().
|
I added comments to the JIRA. Testing this with e2e might not be the best place. Messing around with taints to check node ordering is a hassle. I'm not giving -1 right now, but we need to make sure that we validate things on the right abstraction level. Consider moving this to the scheduler-core using It's probably even better to start with |
|
@pbacsko Thank you for your insightful review. I understand your concerns. I'll close the current PR and create a new JIRA issue to focus on unit testing. |
What is this PR for?
Create test cases for both the base case and the case with resource weights added.
What type of PR is it?
What is the Jira issue?
How should this be tested?
Screenshots (if appropriate)
Questions: