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 2020/06/02 22:10:27 UTC

[GitHub] [incubator-yunikorn-k8shim] yangwwei opened a new pull request #132: [YUNIKORN-200] K8shim reports resource quota based on namespace annotations

yangwwei opened a new pull request #132:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/132


   


----------------------------------------------------------------
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.

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

[GitHub] [incubator-yunikorn-k8shim] wilfred-s closed pull request #132: [YUNIKORN-200] K8shim reports resource quota based on namespace annotations

Posted by GitBox <gi...@apache.org>.
wilfred-s closed pull request #132:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/132


   


----------------------------------------------------------------
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.

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

[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #132: [YUNIKORN-200] K8shim reports resource quota based on namespace annotations

Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #132:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/132#discussion_r434977759



##########
File path: pkg/cache/context.go
##########
@@ -426,6 +428,28 @@ func (ctx *Context) NotifyTaskComplete(appID, taskID string) {
 	}
 }
 
+// get namespace resource quota from annotation
+// if the namespace is unable to be listed from api-server, a nil is returned
+// if the annotation doesn't have the quota defined, a nil is returned
+// if cpu or memory quota is defined in the annotation, a corresponding si.Resource is returned
+func (ctx *Context) getNamespaceResourceQuota(namespace string) *si.Resource {
+	if namespace == "" {
+		return nil
+	}
+
+	nsLister := ctx.apiProvider.GetAPIs().NamespaceInformer.Lister()
+	namespaceObj, err := nsLister.Get(namespace)
+	if err != nil {
+		// every app should belong to a namespace,
+		// if we cannot list the namespace here, probably something is wrong
+		// log an error here and do not add the app to cache
+		log.Logger.Error("failed to get app namespace", zap.Error(err))
+		return nil

Review comment:
       > I do not see the error being logged when the namespace is empty and we also return a nil there. Does that mean we need a message there too?
   
   Yes, I have added a DEBUG level message when the namespace is empty. 
   We are not expecting this to happen, as each pod is always added in a namespace, `default` if not explicitly set. And I do not think an empty string is a valid namespace name. So when this happens, we should prompt a warning. 
   
   > I also do not see how returning a nil at this point distinguishes from no tags being found and returning a nil. You cannot stop adding the app based on a nil being returned
   
   For both cases, a nil means the resource quota is not set in the annotation. We only skip adding this info the app tag:
   ```
   request.Metadata.Tags[common.AppTagNamespaceResourceQuota] = string(quotaStr)
   ```
   but this won't stop us adding the app for scheduling. 




----------------------------------------------------------------
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.

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

[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #132: [YUNIKORN-200] K8shim reports resource quota based on namespace annotations

Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #132:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/132#discussion_r434977881



##########
File path: pkg/common/constants.go
##########
@@ -29,6 +29,9 @@ const LabelApplicationID = "applicationId"
 const LabelQueueName = "queue"
 const ApplicationDefaultQueue = "root.sandbox"
 const DefaultPartition = "default"
+const AppTagNamespace = "namespace"
+const AppTagNamespaceResourceQuota = "namespace.resourcequota"

Review comment:
       Yes, I am adding this in another PR.




----------------------------------------------------------------
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.

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

[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #132: [YUNIKORN-200] K8shim reports resource quota based on namespace annotations

Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #132:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/132#discussion_r434981910



##########
File path: pkg/common/utils/utils.go
##########
@@ -102,6 +105,22 @@ func GetApplicationIDFromPod(pod *v1.Pod) (string, error) {
 		pod.Spec.String())
 }
 
+func GetNamespaceQuotaFromAnnotation(namespaceObj *v1.Namespace) *si.Resource {
+	log.Logger.Info("### GetNamespaceQuotaFromAnnotation")
+	// retrieve resource quota info from annotations
+	cpuQuota := namespaceObj.Annotations["yunikorn.apache.org/queue.max.cpu"]
+	memQuota := namespaceObj.Annotations["yunikorn.apache.org/queue.max.memory"]
+
+	log.Logger.Info("### GetNamespaceQuotaFromAnnotation", zap.String("cpu", cpuQuota), zap.String("mem", memQuota))
+
+	// no quota found
+	if cpuQuota == "" && memQuota == "" {
+		return nil
+	}
+
+	return common.ParseResource(cpuQuota, memQuota)

Review comment:
       After offline discussion, we agree to have such sanity checks in core alone. We do not duplicate them in shim because currently there is no way to send feedback back to the client  event it fails.




----------------------------------------------------------------
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.

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

[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #132: [YUNIKORN-200] K8shim reports resource quota based on namespace annotations

Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #132:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/132#discussion_r434977759



##########
File path: pkg/cache/context.go
##########
@@ -426,6 +428,28 @@ func (ctx *Context) NotifyTaskComplete(appID, taskID string) {
 	}
 }
 
+// get namespace resource quota from annotation
+// if the namespace is unable to be listed from api-server, a nil is returned
+// if the annotation doesn't have the quota defined, a nil is returned
+// if cpu or memory quota is defined in the annotation, a corresponding si.Resource is returned
+func (ctx *Context) getNamespaceResourceQuota(namespace string) *si.Resource {
+	if namespace == "" {
+		return nil
+	}
+
+	nsLister := ctx.apiProvider.GetAPIs().NamespaceInformer.Lister()
+	namespaceObj, err := nsLister.Get(namespace)
+	if err != nil {
+		// every app should belong to a namespace,
+		// if we cannot list the namespace here, probably something is wrong
+		// log an error here and do not add the app to cache
+		log.Logger.Error("failed to get app namespace", zap.Error(err))
+		return nil

Review comment:
       > I do not see the error being logged when the namespace is empty and we also return a nil there. Does that mean we need a message there too?
   Yes, I have added a DEBUG level message when the namespace is empty. 
   We are not expecting this to happen, as each pod is always added in a namespace, `default` if not explicitly set. And I do not think an empty string is a valid namespace name. So when this happens, we should prompt a warning. 
   
   > I also do not see how returning a nil at this point distinguishes from no tags being found and returning a nil. You cannot stop adding the app based on a nil being returned
   For both cases, a nil means the resource quota is not set in the annotation. We only skip adding this info the app tag:
   ```
   request.Metadata.Tags[common.AppTagNamespaceResourceQuota] = string(quotaStr)
   ```
   but this won't stop us adding the app for scheduling. 




----------------------------------------------------------------
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.

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

[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull request #132: [YUNIKORN-200] K8shim reports resource quota based on namespace annotations

Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #132:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/132#discussion_r435008066



##########
File path: pkg/cache/context.go
##########
@@ -426,6 +428,28 @@ func (ctx *Context) NotifyTaskComplete(appID, taskID string) {
 	}
 }
 
+// get namespace resource quota from annotation
+// if the namespace is unable to be listed from api-server, a nil is returned
+// if the annotation doesn't have the quota defined, a nil is returned
+// if cpu or memory quota is defined in the annotation, a corresponding si.Resource is returned
+func (ctx *Context) getNamespaceResourceQuota(namespace string) *si.Resource {
+	if namespace == "" {
+		return nil
+	}
+
+	nsLister := ctx.apiProvider.GetAPIs().NamespaceInformer.Lister()
+	namespaceObj, err := nsLister.Get(namespace)
+	if err != nil {
+		// every app should belong to a namespace,
+		// if we cannot list the namespace here, probably something is wrong
+		// log an error here and do not add the app to cache
+		log.Logger.Error("failed to get app namespace", zap.Error(err))
+		return nil

Review comment:
       > but this won't stop us adding the app for scheduling.
   Which means that the comment is wrong:
   ```
   		// if we cannot list the namespace here, probably something is wrong
   		// log an error here and do not add the app to cache
   ```
   It says that you do not add the app to the cache, which is contradictory to what the behaviour is.
   
   Beside that comment this part looks good




----------------------------------------------------------------
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.

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

[GitHub] [incubator-yunikorn-k8shim] codecov-commenter edited a comment on pull request #132: [YUNIKORN-200] K8shim reports resource quota based on namespace annotations

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #132:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/132#issuecomment-637839018


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=h1) Report
   > Merging [#132](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c4730e3996d6196e8719f284686977ce7f0fcf08&el=desc) will **increase** coverage by `0.10%`.
   > The diff coverage is `59.57%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #132      +/-   ##
   ==========================================
   + Coverage   52.78%   52.89%   +0.10%     
   ==========================================
     Files          32       32              
     Lines        2980     3025      +45     
   ==========================================
   + Hits         1573     1600      +27     
   - Misses       1347     1364      +17     
   - Partials       60       61       +1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `21.14% <0.00%> (-1.15%)` | :arrow_down: |
   | [pkg/appmgmt/general/general.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132/diff?src=pr&el=tree#diff-cGtnL2FwcG1nbXQvZ2VuZXJhbC9nZW5lcmFsLmdv) | `50.32% <50.00%> (ø)` | |
   | [pkg/common/resource.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZS5nbw==) | `100.00% <100.00%> (ø)` | |
   | [pkg/common/utils/utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi91dGlscy91dGlscy5nbw==) | `13.54% <100.00%> (+5.76%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=footer). Last update [c4730e3...d783411](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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.

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

[GitHub] [incubator-yunikorn-k8shim] codecov-commenter edited a comment on pull request #132: [YUNIKORN-200] K8shim reports resource quota based on namespace annotations

Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #132:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/132#issuecomment-637839018


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=h1) Report
   > Merging [#132](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c4730e3996d6196e8719f284686977ce7f0fcf08&el=desc) will **increase** coverage by `0.10%`.
   > The diff coverage is `59.57%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #132      +/-   ##
   ==========================================
   + Coverage   52.78%   52.89%   +0.10%     
   ==========================================
     Files          32       32              
     Lines        2980     3025      +45     
   ==========================================
   + Hits         1573     1600      +27     
   - Misses       1347     1364      +17     
   - Partials       60       61       +1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `21.14% <0.00%> (-1.15%)` | :arrow_down: |
   | [pkg/appmgmt/general/general.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132/diff?src=pr&el=tree#diff-cGtnL2FwcG1nbXQvZ2VuZXJhbC9nZW5lcmFsLmdv) | `50.32% <50.00%> (ø)` | |
   | [pkg/common/resource.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZS5nbw==) | `100.00% <100.00%> (ø)` | |
   | [pkg/common/utils/utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi91dGlscy91dGlscy5nbw==) | `13.54% <100.00%> (+5.76%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=footer). Last update [c4730e3...e1c36dd](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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.

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

[GitHub] [incubator-yunikorn-k8shim] codecov-commenter commented on pull request #132: [YUNIKORN-200] K8shim reports resource quota based on namespace annotations

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #132:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/132#issuecomment-637839018


   # [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=h1) Report
   > Merging [#132](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c4730e3996d6196e8719f284686977ce7f0fcf08&el=desc) will **increase** coverage by `0.17%`.
   > The diff coverage is `64.44%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132/graphs/tree.svg?width=650&height=150&src=pr&token=LZImIuvleR)](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #132      +/-   ##
   ==========================================
   + Coverage   52.78%   52.96%   +0.17%     
   ==========================================
     Files          32       32              
     Lines        2980     3023      +43     
   ==========================================
   + Hits         1573     1601      +28     
   - Misses       1347     1361      +14     
   - Partials       60       61       +1     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `21.55% <6.25%> (-0.74%)` | :arrow_down: |
   | [pkg/appmgmt/general/general.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132/diff?src=pr&el=tree#diff-cGtnL2FwcG1nbXQvZ2VuZXJhbC9nZW5lcmFsLmdv) | `50.32% <50.00%> (ø)` | |
   | [pkg/common/resource.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZS5nbw==) | `100.00% <100.00%> (ø)` | |
   | [pkg/common/utils/utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi91dGlscy91dGlscy5nbw==) | `15.30% <100.00%> (+7.52%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=footer). Last update [c4730e3...0cb24a0](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/132?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
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.

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

[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #132: [YUNIKORN-200] K8shim reports resource quota based on namespace annotations

Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #132:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/132#discussion_r435668833



##########
File path: pkg/cache/context.go
##########
@@ -426,6 +428,28 @@ func (ctx *Context) NotifyTaskComplete(appID, taskID string) {
 	}
 }
 
+// get namespace resource quota from annotation
+// if the namespace is unable to be listed from api-server, a nil is returned
+// if the annotation doesn't have the quota defined, a nil is returned
+// if cpu or memory quota is defined in the annotation, a corresponding si.Resource is returned
+func (ctx *Context) getNamespaceResourceQuota(namespace string) *si.Resource {
+	if namespace == "" {
+		return nil
+	}
+
+	nsLister := ctx.apiProvider.GetAPIs().NamespaceInformer.Lister()
+	namespaceObj, err := nsLister.Get(namespace)
+	if err != nil {
+		// every app should belong to a namespace,
+		// if we cannot list the namespace here, probably something is wrong
+		// log an error here and do not add the app to cache
+		log.Logger.Error("failed to get app namespace", zap.Error(err))
+		return nil

Review comment:
       Fixed that comment.




----------------------------------------------------------------
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.

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

[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #132: [YUNIKORN-200] K8shim reports resource quota based on namespace annotations

Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #132:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/132#discussion_r434984708



##########
File path: pkg/common/resource.go
##########
@@ -81,6 +83,37 @@ func GetNodeResource(nodeStatus *v1.NodeStatus) *si.Resource {
 	return getResource(nodeStatus.Allocatable)
 }
 
+// parse cpu and memory from string to si.Resource, both of them are optional
+// if parse failed with some errors, log the error and ignore the resource
+func ParseResource(cpuStr, memStr string) *si.Resource {
+	if cpuStr == "" && memStr == "" {
+		return nil
+	}
+
+	result := NewResourceBuilder()
+	if cpuStr != "" {
+		if vcore, err := resource.ParseQuantity(cpuStr); err == nil {
+			result.AddResource(CPU, vcore.MilliValue())
+		} else {
+			log.Logger.Error("failed to parse cpu resource",
+				zap.String("cpuStr", cpuStr),
+				zap.Error(err))

Review comment:
       To make this be more consistent with the yunikorn-core, I think we can do that. Just updated the code for this. Thanks.




----------------------------------------------------------------
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.

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

[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull request #132: [YUNIKORN-200] K8shim reports resource quota based on namespace annotations

Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #132:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/132#discussion_r434299611



##########
File path: pkg/common/resource.go
##########
@@ -81,6 +83,37 @@ func GetNodeResource(nodeStatus *v1.NodeStatus) *si.Resource {
 	return getResource(nodeStatus.Allocatable)
 }
 
+// parse cpu and memory from string to si.Resource, both of them are optional
+// if parse failed with some errors, log the error and ignore the resource
+func ParseResource(cpuStr, memStr string) *si.Resource {
+	if cpuStr == "" && memStr == "" {
+		return nil
+	}
+
+	result := NewResourceBuilder()
+	if cpuStr != "" {
+		if vcore, err := resource.ParseQuantity(cpuStr); err == nil {
+			result.AddResource(CPU, vcore.MilliValue())
+		} else {
+			log.Logger.Error("failed to parse cpu resource",
+				zap.String("cpuStr", cpuStr),
+				zap.Error(err))

Review comment:
       If we fail to parse we should not return a resource object but a nil to clearly show that we're not enforcing the quota. Returning half the quota set is going to give inconsistent behaviour.
   
   Negative values and zeros do not have to stop parsing at this point.

##########
File path: pkg/common/constants.go
##########
@@ -29,6 +29,9 @@ const LabelApplicationID = "applicationId"
 const LabelQueueName = "queue"
 const ApplicationDefaultQueue = "root.sandbox"
 const DefaultPartition = "default"
+const AppTagNamespace = "namespace"
+const AppTagNamespaceResourceQuota = "namespace.resourcequota"

Review comment:
       These two should be moved to the SI as we use them in both the core and shim

##########
File path: pkg/cache/context.go
##########
@@ -426,6 +428,28 @@ func (ctx *Context) NotifyTaskComplete(appID, taskID string) {
 	}
 }
 
+// get namespace resource quota from annotation
+// if the namespace is unable to be listed from api-server, a nil is returned
+// if the annotation doesn't have the quota defined, a nil is returned
+// if cpu or memory quota is defined in the annotation, a corresponding si.Resource is returned
+func (ctx *Context) getNamespaceResourceQuota(namespace string) *si.Resource {
+	if namespace == "" {
+		return nil
+	}
+
+	nsLister := ctx.apiProvider.GetAPIs().NamespaceInformer.Lister()
+	namespaceObj, err := nsLister.Get(namespace)
+	if err != nil {
+		// every app should belong to a namespace,
+		// if we cannot list the namespace here, probably something is wrong
+		// log an error here and do not add the app to cache
+		log.Logger.Error("failed to get app namespace", zap.Error(err))
+		return nil

Review comment:
       1. I do not see the error being logged when the namespace is empty and we also return a nil there. Does that mean we need a message there too?
   2. I also do not see how returning a nil at this point distinguishes from no tags being found and returning a nil. You cannot stop adding the app based on a nil being returned

##########
File path: pkg/common/utils/utils.go
##########
@@ -22,12 +22,15 @@ import (
 	"strings"
 	"time"
 
+	"github.com/apache/incubator-yunikorn-k8shim/pkg/log"

Review comment:
       ordering of imports incorrect

##########
File path: pkg/common/utils/utils.go
##########
@@ -102,6 +105,22 @@ func GetApplicationIDFromPod(pod *v1.Pod) (string, error) {
 		pod.Spec.String())
 }
 
+func GetNamespaceQuotaFromAnnotation(namespaceObj *v1.Namespace) *si.Resource {
+	log.Logger.Info("### GetNamespaceQuotaFromAnnotation")
+	// retrieve resource quota info from annotations
+	cpuQuota := namespaceObj.Annotations["yunikorn.apache.org/queue.max.cpu"]
+	memQuota := namespaceObj.Annotations["yunikorn.apache.org/queue.max.memory"]

Review comment:
       These should be referring to namespace not to queue. The fact that we map that in the core to a queue is beside the point. On the k8s side it is applying to a namespace.

##########
File path: pkg/common/utils/utils.go
##########
@@ -102,6 +105,22 @@ func GetApplicationIDFromPod(pod *v1.Pod) (string, error) {
 		pod.Spec.String())
 }
 
+func GetNamespaceQuotaFromAnnotation(namespaceObj *v1.Namespace) *si.Resource {
+	log.Logger.Info("### GetNamespaceQuotaFromAnnotation")
+	// retrieve resource quota info from annotations
+	cpuQuota := namespaceObj.Annotations["yunikorn.apache.org/queue.max.cpu"]
+	memQuota := namespaceObj.Annotations["yunikorn.apache.org/queue.max.memory"]
+
+	log.Logger.Info("### GetNamespaceQuotaFromAnnotation", zap.String("cpu", cpuQuota), zap.String("mem", memQuota))
+
+	// no quota found
+	if cpuQuota == "" && memQuota == "" {
+		return nil
+	}
+
+	return common.ParseResource(cpuQuota, memQuota)

Review comment:
       This is the point we need to build in sanity checks with clear logging as to what is checked and failed.:
   - zero values for either setting
   - negative values for either setting
   
   Only return a resource that has both values set to a value larger than 0 or if only one is specified that one needs to be larger than 0. A nil should be returned in all other cases.

##########
File path: pkg/cache/context.go
##########
@@ -434,6 +458,19 @@ func (ctx *Context) AddApplication(request *interfaces.AddApplicationRequest) in
 
 	ctx.lock.Lock()
 	defer ctx.lock.Unlock()
+
+	// add resource quota info as a app tag
+	log.Logger.Info("### retrieve namespace info 1")
+	if ns, ok := request.Metadata.Tags[common.AppTagNamespace]; ok {
+		log.Logger.Info("### retrieve namespace info 2", zap.String("namespace", ns))
+		resourceQuota := ctx.getNamespaceResourceQuota(ns)
+		if resourceQuota != nil && !common.IsZero(resourceQuota) {

Review comment:
       This does not capture the case that we set `mem=0` and `cpu=10` but we cannot apply that on the core side as it would stop scheduling for that namespace.
   I think we need to catch this earlier than the core side.




----------------------------------------------------------------
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.

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

[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on pull request #132: [YUNIKORN-200] K8shim reports resource quota based on namespace annotations

Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #132:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/132#issuecomment-639234758


   Logged https://issues.apache.org/jira/browse/YUNIKORN-206 for the constants refactoring follow up


----------------------------------------------------------------
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.

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