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

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

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
8 changes: 8 additions & 0 deletions pkg/cache/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -930,6 +930,8 @@ func (ctx *Context) notifyTaskComplete(appID, taskID string) {
// 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) {
namespaceObj := ctx.getNamespaceObject(namespace)
if namespaceObj == nil {
Expand All @@ -951,6 +953,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
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
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
21 changes: 21 additions & 0 deletions pkg/common/utils/utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,27 @@ 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 != "" {
numMaxApp, err := strconv.Atoi(maxApps)
if 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 ""
}
if numMaxApp < 0 {
log.Log(log.ShimUtils).Warn("Invalid value for 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
47 changes: 47 additions & 0 deletions pkg/common/utils/utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,53 @@ 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: "-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