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.

Sign up for GitHub

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-1697] [shim] Make namespace annotation to support max appli… #885

Closed
wants to merge 4 commits into from
Closed

[YUNIKORN-1697] [shim] Make namespace annotation to support max appli… #885

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7dd26a3
[YUNIKORN-1697] [shim] Make namespace annotation to support max appli…
zhuqi-lucas Jul 26, 2024
df3002a
fix lint
zhuqi-lucas Jul 26, 2024
f434158
Address comments
zhuqi-lucas Jul 31, 2024
5e02645
Fix golint
zhuqi-lucas Jul 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pkg/cache/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -951,6 +951,12 @@ func (ctx *Context) updateApplicationTags(request *AddApplicationRequest, namesp
}
}

// add maxApps resource info as an app tag
maxApps := utils.GetNamespaceMaxAppsFromAnnotation(namespaceObj)
if maxApps != "" {
request.Metadata.Tags[siCommon.AppTagNamespaceResourceMaxApps] = maxApps
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment of this function should include "namespace.resourcemaxapps". Besides, the "namespace.resourceguaranteed" looks like missing in the previous change.

// update application tags in the AddApplicationRequest based on the namespace annotation
// adds the following tags to the request based on annotations (if exist):
//   - namespace.resourcequota
//   - namespace.parentqueue
//   - namespace.resourceguaranteed
//   - namespace.resourcemaxapps
func (ctx *Context) updateApplicationTags(request *AddApplicationRequest, namespace string) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this, addressed in latest PR.

}

// add parent queue info as an app tag
parentQueue := utils.GetNameSpaceAnnotationValue(namespaceObj, constants.AnnotationParentQueue)
if parentQueue != "" {
Expand Down
7 changes: 7 additions & 0 deletions pkg/cache/context_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1519,6 +1519,7 @@ func TestAddApplicationsWithTags(t *testing.T) {
constants.NamespaceQuota: "{\"cpu\": \"1\", \"memory\": \"256M\", \"nvidia.com/gpu\": \"1\"}",
constants.DomainYuniKorn + "parentqueue": "root.test",
constants.NamespaceGuaranteed: "{\"cpu\": \"1\", \"memory\": \"256M\", \"nvidia.com/gpu\": \"1\"}",
constants.NamespaceMaxApps: "1000",
},
},
}
Expand Down Expand Up @@ -1607,6 +1608,12 @@ func TestAddApplicationsWithTags(t *testing.T) {
t.Fatalf("resource parsing failed")
}

maxApps, ok := request.Metadata.Tags[siCommon.AppTagNamespaceResourceMaxApps]
if !ok {
t.Fatalf("max apps tag is not updated from the namespace")
}
assert.Equal(t, maxApps, "1000")

parentQueue, ok := request.Metadata.Tags[constants.AppTagNamespaceParentQueue]
if !ok {
t.Fatalf("parent queue tag is not updated from the namespace")
Expand Down
3 changes: 3 additions & 0 deletions pkg/common/constants/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,9 @@ const NamespaceQuota = DomainYuniKorn + "namespace.quota"
// NamespaceGuaranteed Namespace Guaranteed
const NamespaceGuaranteed = DomainYuniKorn + "namespace.guaranteed"

// NamespaceMaxApps Namespace Max Apps
const NamespaceMaxApps = DomainYuniKorn + "namespace.maxApps"

// AnnotationAllowPreemption set on PriorityClass, opt out of preemption for pods with this priority class
const AnnotationAllowPreemption = DomainYuniKorn + "allow-preemption"

Expand Down
14 changes: 14 additions & 0 deletions pkg/common/utils/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,20 @@ func GetNamespaceGuaranteedFromAnnotation(namespaceObj *v1.Namespace) *si.Resour
return nil
}

// get namespace max apps from namespace annotation
func GetNamespaceMaxAppsFromAnnotation(namespaceObj *v1.Namespace) string {
if maxApps := GetNameSpaceAnnotationValue(namespaceObj, constants.NamespaceMaxApps); maxApps != "" {
if _, err := strconv.Atoi(maxApps); err != nil {
log.Log(log.ShimUtils).Warn("Unable to process namespace.maxApps annotation",
zap.String("namespace", namespaceObj.Name),
zap.String("namespace.maxApps is", maxApps))
return ""
}
return maxApps
}
return ""
}

func GetNamespaceQuotaFromAnnotation(namespaceObj *v1.Namespace) *si.Resource {
// retrieve resource quota info from annotations
cpuQuota := GetNameSpaceAnnotationValue(namespaceObj, constants.CPUQuota)
Expand Down
38 changes: 38 additions & 0 deletions pkg/common/utils/utils_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,44 @@ func TestGetNamespaceGuaranteedFromAnnotation(t *testing.T) {
}
}

func TestGetNamespaceMaxAppsFromAnnotation(t *testing.T) {
testCases := []struct {
namespace *v1.Namespace
expectedMaxApp string
}{
{&v1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Namespace: "test",
},
}, ""},
{&v1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Namespace: "test",
Annotations: map[string]string{
constants.NamespaceMaxApps: "5",
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add a negative test case as well? eg. -5.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, addressed in latest PR!

},
},
}, "5"},
{&v1.Namespace{
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Namespace: "test",
Annotations: map[string]string{
constants.NamespaceMaxApps: "error",
},
},
}, ""},
}
for _, tc := range testCases {
t.Run(fmt.Sprintf("namespace: %v", tc.namespace), func(t *testing.T) {
maxApp := GetNamespaceMaxAppsFromAnnotation(tc.namespace)
assert.Equal(t, maxApp, tc.expectedMaxApp)
})
}
}

func TestGetNamespaceQuotaFromAnnotationUsingNewAndOldAnnotations(t *testing.T) {
testCases := []struct {
namespace *v1.Namespace
Expand Down