search

AmadeusITGroup / workflow-controller

Kubernetes workflow controller
Apache License 2.0
24 stars 15 forks source link

Job needs template.metadata.labels for monitoring metrics (currently only put on Pod) #62

Closed davidc-donorschoose closed 6 years ago

davidc-donorschoose commented 6 years ago

The jobTemplate.spec.template.metadata.labels in the steps of a Workflow manifest only become metadata.labels for the Pods that are created, not for the Jobs. When the same jobTemplate is used for a CronJob, they appear for both Pods and the Jobs.

The CronJob behavior seems correct to me. Having the labels appear on both Pods and Jobs is very useful for monitoring tools. I would like to use an app label on my Workflow steps to filter monitoring metrics in tools such as Prometheus (or DataDog, etc). It is possible to drill down to Pod metrics by the app label, since kube-state-metrics creates a kube_pod_labels that can be joined to metrics such as kube_pod_status_ready (matching by the pod and/or namespace name). It is not currently possible to drill down to Job metrics with my app label: although there is a kube_job_labels that can be joined to metrics such as kube_job_status_active, there is no label_app field for Workflow because there is no app label on the Job. (On the other hand, this does work for CronJob).

It's possible that the controller_util.go source file intended to do what I need in its func getJobLabelsSet. There is a loop to copy range template.Labels into the desiredLabels for the Job, but the code seems to be a no-op (a silent failure). I don't think that template.Labels is parsed from the Workflow manifest by low-level code. (1) I don't see it in the spec. (2) I've tried jobTemplate.labels, jobTemplate.spec.labels, jobTemplate.spec.template.labels, and jobTemplate.spec.template.metadata.labels in my Workflow manifests. None appear on the Jobs.

The latter (jobTemplate.spec.template.metadata.labels) do appear as Pod labels with both Workflow and CronJob, but they only appear as Job labels with CronJob. If func getJobLabelsSet were to use a different variable reference (perhaps range template.template.metadata.labels), I believe it would do what I need - apply the labels to Jobs as well as Pods. If you are interested in this enhancement, I can send a pull request when I get it to work.

clamoriniere1A commented 6 years ago

Hi @davidc-donorschoose

Thanks for opening this issue. Having the same behavior than CronJob seems legit for me too. We will be more than happy to have your contribution for this enhancement.

Let me know or @sdminonne if you have any questions.

Thanks

sdminonne commented 6 years ago

SGTM too, happy to review a PR or to fix it. Thanks!

davidc-donorschoose commented 6 years ago

Pull request #68 contains my implementation.

clamoriniere1A commented 6 years ago

Close this issue since pull request #68 has been merged.