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

[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