[YUNIKORN-1901] A basic example for the user tracing and the group tracing #654
Conversation
Codecov Report
@@ Coverage Diff @@
## master #654 +/- ##
==========================================
+ Coverage 71.87% 71.89% +0.02%
==========================================
Files 49 49
Lines 7956 7956
==========================================
+ Hits 5718 5720 +2
+ Misses 2041 2039 -2
Partials 197 197 see 1 file with indirect coverage changes 📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
|
|
Now that #634 has gone in, you will need to rebase and resolve conflicts. |
| Ω(ns1.Status.Phase).To(Equal(v1.NamespaceActive)) | ||
| }) | ||
|
|
||
| It("User qauta trace with 3 users and 2 groups", func() { |
| {"teacher-app-01", queuePath[1], users[0], []string{groups[0]}}, | ||
| {"students-app-01", queuePath[2], users[1], []string{groups[1]}}, | ||
| {"assistant-app-01", queuePath[2], users[2], []string{groups[1]}}, | ||
| {"assistant-app-02", queuePath[1], users[2], []string{groups[0]}}, |
There was a problem hiding this comment.
If these are strings which do not change inside the slice, I'd prefer having them them referenced as literals rather than slice elements. This is difficult to interpret. Extract them to const if that helps.
| for group, quota := range expectedGroups.Groups { | ||
| By(fmt.Sprintf("GroupTracker: Check group resource usage of %s in each queue", group)) | ||
| groupUsage, getGroupErr := restClient.GetGroupResourceUsage(DEFAULT_PARTITION, group) | ||
| Ω(getGroupErr).NotTo(HaveOccurred()) | ||
| Ω(groupUsage.Applications).To(Equal(quota.RunningApplications), "running application IDs are not expected") | ||
| for queuePath, queueQuota := range quota.Queues { | ||
| queue, getQueueErr := yunikorn.GetQueueResourceUsage(groupUsage.Queues, queuePath) | ||
| Ω(getQueueErr).NotTo(HaveOccurred()) | ||
| Ω(queue.RunningApplications).To(Equal(queueQuota.RunningApplications)) | ||
| usedResource.ParseResourceUsage(yunikorn.ParseResource(queue.ResourceUsage)) | ||
| Ω(usedResource).To(Equal(queueQuota.Resources)) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm wondering if using functions is better... because you're trying to avoid code duplication, which is good.
But perhaps having all these checks in a separate func is more readable, like:
type PerGroup struct {
queuePath string
runningApps []string
resources map[string]int64
}
func checkGroup(group string, expected []PerGroup) {
... do things with "group" ...
groupUsage := restClient.GetGroupResourceUsage(DEFAULT_PARTITION, group)
for _, exp := range expected {
checkTrackedDataInGroup(groupUsage, exp.queuePath, exp.runningApps, exp.resources)
}
}
func checkTrackedDataInGroup(groupUsage *dao.GroupResourceUsageDAOInfo, queuePath string, expectedRunningApps []string,
expectedResource map[string]int64) {
queue, getQueueErr := yunikorn.GetQueueResourceUsage(groupUsage.Queues, queuePath)
Ω(getQueueErr).NotTo(HaveOccurred())
Ω(queue.RunningApplications).To(Equal(expectedRunningApps))
usedResource.ParseResourceUsage(yunikorn.ParseResource(queue.ResourceUsage))
Ω(usedResource).To(Equal(expectedResource))
}
Then you call checkGroup() separately for each group, and you create []PerGroup inline with string literals:
checkGroup("group1", []PerGroup{
{
queuePath: "root.path1",
runningApps: "app1", "app2", "app3",
resources: map[string]int64{
"cpu": 1,
"memory": 100,
},
{
queuePath: "root.path2",
runningApps: "app4", "app5",
resources: map[string]int64{
"cpu": 2,
"memory": 200,
},
}
})
I'm not saying it has to look like this (it might not even compile), but it's a start. It's much more obvious to see what we're trying to achieve.
The main idea here is to reduce nesting and the number of structs. I'm really having a hard time understanding what the code does with all these types.
Excuse me if it sounds like nitpicking, it's not my goal. But let's make these things easy to comprehend if possible :-).
There was a problem hiding this comment.
Hi @pbacsko , thanks for review.
Your suggestion is cool in order to reduce the code and make code readable.
I will try to adopt this suggestion to make this code better.
I am going to start the progress in this weekend.
| var _ = BeforeSuite(func() { | ||
| Ω(kClient.SetClient()).To(BeNil()) | ||
| yunikorn.EnsureYuniKornConfigsPresent() | ||
| }) | ||
|
|
||
| var _ = AfterSuite(func() { | ||
| yunikorn.RestoreConfigMapWrapper(oldConfigMap, annotation) | ||
| }) |
There was a problem hiding this comment.
Maybe I'm missing something... Don't you need a TestXXX(t *testing.T) function here?
There was a problem hiding this comment.
I don't know if the test works properly or not... let's see after the rebase. I spotted a logical error regarding the userinfo annotation. That likely need to be fixed.
I mostly added comments regarding maintainability, the current code is difficult to understand. See tips.
Understood, I will rebase in this weekend. |
0yukali0
force-pushed
the
YUNIKORN-1901
branch
from
6d0cb97 to
4e07a6c
Compare
|
The commit is updated but it still lack of the part of admincontroll.externalUser |
What is this PR for?
Validate the user resource usage and group resource usage from the REST API.
What type of PR is it?
Todos
What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-1901
How should this be tested?
Screenshots (if appropriate)
Questions: