Add queue controller #128
Add queue controller #128
Conversation
hzxuzhonghu
force-pushed
the
queue-controller
branch
from
8f8b157
to
d3f2fac
Compare
pkg/apis/helpers/helpers.go
Outdated
| @@ -63,7 +63,7 @@ func ControlledBy(obj interface{}, gvk schema.GroupVersionKind) bool { | |||
| return false | |||
| } | |||
|
|
|||
| func CreateConfigMapIfNotExist(job *vkv1.Job, kubeClients *kubernetes.Clientset, data map[string]string, cmName string) error { | |||
| func CreateConfigMapIfNotExist(job *vkv1.Job, kubeClients kubernetes.Interface, data map[string]string, cmName string) error { | |||
There was a problem hiding this comment.
prefer to have seperate PR for such kind of cleanup.
| glog.V(2).Infoln("queue %s has not been seen or deleted") | ||
| return nil | ||
| } | ||
| for pgKey := range c.podGroups[key] { |
There was a problem hiding this comment.
prefer to handle this in Cache: Queue -> Job -> PodGroup.
There was a problem hiding this comment.
I donot want to overlap with job controller. For queue controller, it just care about the podgroups, not care about Job or anything else.
podGroups is keyed by queue name and value is the podgroups that are using this queue.
There was a problem hiding this comment.
pkg/cache is a common component in controller, and we need to clean up jobs when queue was deleted.
There was a problem hiding this comment.
IC, then we need to record jobs too.
There was a problem hiding this comment.
But have a doubt: why would user delete their queues before jobs? Is there a use case? If there is a malicious user who does this, we should not delete the running jobs by any reason.
There was a problem hiding this comment.
For this, I can think of a more simple way to do this: I remember there will be an admission controller to validate queue existence before create jobs. Then we can also set the job's owner reference. Then when the queue is deleted, k8s garbage collector can reclaim all children resources automatically.
So we have two choice:
-
do it in controller, need to record all dependents relationship.
-
setting ownerreference, do nothing else, leave it to GC.
There was a problem hiding this comment.
Personally prefer not adding this into cache even we need considering deleting jobs when queue deleted.
There was a problem hiding this comment.
setting ownerreference, do nothing else, leave it to GC.
owerReference should be reserved for JobGroup :)
There was a problem hiding this comment.
OwnerReferences is an array, can be more than one.
owerReference should be reserved for JobGroup :)
Sorry, there is no JobGroup now.
There was a problem hiding this comment.
not sure whether GC will check controlled owner reference.
there is no JobGroup now.
so "reserved" it.
hzxuzhonghu
force-pushed
the
queue-controller
branch
from
08b043d
to
de3bcf8
Compare
| @@ -72,10 +76,17 @@ func Run(opt *options.ServerOption) error { | |||
| return err | |||
| } | |||
|
|
|||
| jobController := job.NewJobController(config) | |||
| // TODO: add user agent for different controllers | |||
There was a problem hiding this comment.
It is just for http server to know the client identity.
| glog.V(2).Infoln("queue %s has not been seen or deleted") | ||
| return nil | ||
| } | ||
| for pgKey := range c.podGroups[key] { |
There was a problem hiding this comment.
Personally prefer not adding this into cache even we need considering deleting jobs when queue deleted.
|
I am adding e2e |
|
@hzxuzhonghu please update the cluster role to get appropriate rights to handle queue resource. |
|
Thanks |
hzxuzhonghu
force-pushed
the
queue-controller
branch
from
d141e07
to
f5113d1
Compare
hzxuzhonghu
force-pushed
the
queue-controller
branch
from
f5113d1
to
942f198
Compare
|
/lgtm |
|
/approve |
volcano-sh-bot
added
the
approved
Add queue controller
fixes: #16