From 414b92032f628b2df471f2d2ea2048307291984c Mon Sep 17 00:00:00 2001 From: Johnu George Date: Thu, 8 Aug 2019 00:06:01 +0530 Subject: [PATCH 1/2] Moving labels to common types --- dashboard/backend/handler/api_handler.go | 4 ++-- pkg/common/jobcontroller/jobcontroller.go | 23 +++++++++++-------- pkg/common/util/v1/testutil/util.go | 2 +- pkg/common/util/v1beta2/testutil/util.go | 2 +- pkg/controller.v1/tensorflow/controller.go | 9 +++----- .../tensorflow/controller_test.go | 2 +- pkg/controller.v1/tensorflow/pod.go | 2 +- .../tensorflow/controller.go | 9 +++----- .../tensorflow/controller_test.go | 2 +- pkg/controller.v1beta2/tensorflow/pod.go | 2 +- py/kubeflow/tf_operator/tf_job_client.py | 2 +- 11 files changed, 29 insertions(+), 30 deletions(-) diff --git a/dashboard/backend/handler/api_handler.go b/dashboard/backend/handler/api_handler.go index 42547de070..2bd05484d5 100644 --- a/dashboard/backend/handler/api_handler.go +++ b/dashboard/backend/handler/api_handler.go @@ -25,7 +25,7 @@ type APIHandler struct { // if any and related pods type TFJobDetail struct { TFJob *tfv1.TFJob `json:"tfJob"` - Pods []v1.Pod `json:"pods"` + Pods []v1.Pod `json:"pods"` } // TFJobList is a list of TFJobs @@ -160,7 +160,7 @@ func (apiHandler *APIHandler) handleGetTFJobDetail(request *restful.Request, res // Get associated pods pods, err := apiHandler.cManager.ClientSet.CoreV1().Pods(namespace).List(metav1.ListOptions{ - LabelSelector: fmt.Sprintf("group-name=kubeflow.org,tf-job-name=%s", name), + LabelSelector: fmt.Sprintf("group-name=kubeflow.org,job-name=%s", name), }) if err != nil { log.Warningf("failed to list pods for TFJob %v under namespace %v: %v", name, namespace, err) diff --git a/pkg/common/jobcontroller/jobcontroller.go b/pkg/common/jobcontroller/jobcontroller.go index 543ea5aff8..5fa59d97e9 100644 --- a/pkg/common/jobcontroller/jobcontroller.go +++ b/pkg/common/jobcontroller/jobcontroller.go @@ -55,9 +55,6 @@ type ControllerInterface interface { // Returns the Replica Index(value) in the labels of the job GetReplicaIndexLabelKey() string - // Returns the Job Role(key) in the labels of the job - GetJobRoleKey() string - // Returns the Job from Informer Cache GetJobFromInformerCache(namespace, name string) (metav1.Object, error) @@ -139,6 +136,14 @@ type JobController struct { Recorder record.EventRecorder } +const ( + // JobNameLabel represents the label key for the job name, the value is job name + JobNameLabel = "job-name" + + // JobRoleLabel represents the label key for the job role, e.g. the value is master + JobRoleLabel = "job-role" +) + func NewJobController( controllerImpl ControllerInterface, reconcilerSyncPeriod metav1.Duration, @@ -200,11 +205,13 @@ func (jc *JobController) GenOwnerReference(obj metav1.Object) *metav1.OwnerRefer func (jc *JobController) GenLabels(jobName string) map[string]string { labelGroupName := jc.Controller.GetGroupNameLabelKey() - labelJobName := jc.Controller.GetJobNameLabelKey() + // deprecatedLabel is kept for backward compatibility. Has to be removed later + deprecatedLabelJobName := jc.Controller.GetJobNameLabelKey() groupName := jc.Controller.GetGroupNameLabelValue() return map[string]string{ - labelGroupName: groupName, - labelJobName: strings.Replace(jobName, "/", "-", -1), + labelGroupName: groupName, + JobNameLabel: strings.Replace(jobName, "/", "-", -1), + deprecatedLabelJobName: strings.Replace(jobName, "/", "-", -1), } } @@ -236,8 +243,6 @@ func (jc *JobController) SyncPodGroup(job metav1.Object, minAvailableReplicas in // SyncPdb will create a PDB for gang scheduling by kube-batch. func (jc *JobController) SyncPdb(job metav1.Object, minAvailableReplicas int32) (*v1beta1.PodDisruptionBudget, error) { - labelJobName := jc.Controller.GetJobNameLabelKey() - // Check the pdb exist or not pdb, err := jc.KubeClientSet.PolicyV1beta1().PodDisruptionBudgets(job.GetNamespace()).Get(job.GetName(), metav1.GetOptions{}) if err == nil || !k8serrors.IsNotFound(err) { @@ -260,7 +265,7 @@ func (jc *JobController) SyncPdb(job metav1.Object, minAvailableReplicas int32) MinAvailable: &minAvailable, Selector: &metav1.LabelSelector{ MatchLabels: map[string]string{ - labelJobName: job.GetName(), + JobNameLabel: job.GetName(), }, }, }, diff --git a/pkg/common/util/v1/testutil/util.go b/pkg/common/util/v1/testutil/util.go index 1f68dd9e73..29f9c94bad 100644 --- a/pkg/common/util/v1/testutil/util.go +++ b/pkg/common/util/v1/testutil/util.go @@ -29,7 +29,7 @@ import ( const ( LabelGroupName = "group-name" - LabelTFJobName = "tf-job-name" + LabelTFJobName = "job-name" ) var ( diff --git a/pkg/common/util/v1beta2/testutil/util.go b/pkg/common/util/v1beta2/testutil/util.go index c1fd9c6814..d1bde8cb2d 100644 --- a/pkg/common/util/v1beta2/testutil/util.go +++ b/pkg/common/util/v1beta2/testutil/util.go @@ -29,7 +29,7 @@ import ( const ( LabelGroupName = "group-name" - LabelTFJobName = "tf-job-name" + LabelTFJobName = "job-name" ) var ( diff --git a/pkg/controller.v1/tensorflow/controller.go b/pkg/controller.v1/tensorflow/controller.go index 4cd9b6e6a7..a10a49ee3b 100644 --- a/pkg/controller.v1/tensorflow/controller.go +++ b/pkg/controller.v1/tensorflow/controller.go @@ -55,8 +55,8 @@ const ( tfReplicaTypeLabel = "tf-replica-type" tfReplicaIndexLabel = "tf-replica-index" labelGroupName = "group-name" - labelTFJobName = "tf-job-name" - labelTFJobRole = "tf-job-role" + // Deprecated label for backwards compatibility. Has to be removed + labelTFJobName = "tf-job-name" ) var ( @@ -570,6 +570,7 @@ func (tc *TFController) GetGroupNameLabelKey() string { return labelGroupName } +// Deprecated function for backwards compatibility. Has to be removed later func (tc *TFController) GetJobNameLabelKey() string { return labelTFJobName } @@ -586,10 +587,6 @@ func (tc *TFController) GetReplicaIndexLabelKey() string { return tfReplicaIndexLabel } -func (tc *TFController) GetJobRoleKey() string { - return labelTFJobRole -} - func (tc *TFController) ControllerName() string { return controllerName } diff --git a/pkg/controller.v1/tensorflow/controller_test.go b/pkg/controller.v1/tensorflow/controller_test.go index b8a47c5f5d..d39bfc23e2 100644 --- a/pkg/controller.v1/tensorflow/controller_test.go +++ b/pkg/controller.v1/tensorflow/controller_test.go @@ -455,7 +455,7 @@ func TestSyncPdb(t *testing.T) { MinAvailable: &minAvailable, Selector: &metav1.LabelSelector{ MatchLabels: map[string]string{ - "tf-job-name": "test-sync-pdb", + "job-name": "test-sync-pdb", }, }, }, diff --git a/pkg/controller.v1/tensorflow/pod.go b/pkg/controller.v1/tensorflow/pod.go index 7297358ac2..98d1b511e6 100644 --- a/pkg/controller.v1/tensorflow/pod.go +++ b/pkg/controller.v1/tensorflow/pod.go @@ -153,7 +153,7 @@ func (tc *TFController) createNewPod(tfjob *tfv1.TFJob, rt, index string, spec * labels[tfReplicaIndexLabel] = index if masterRole { - labels[labelTFJobRole] = "master" + labels[jobcontroller.JobRoleLabel] = "master" } podTemplate := spec.Template.DeepCopy() diff --git a/pkg/controller.v1beta2/tensorflow/controller.go b/pkg/controller.v1beta2/tensorflow/controller.go index ed53d2a266..db1830fb25 100644 --- a/pkg/controller.v1beta2/tensorflow/controller.go +++ b/pkg/controller.v1beta2/tensorflow/controller.go @@ -53,8 +53,8 @@ const ( tfReplicaTypeLabel = "tf-replica-type" tfReplicaIndexLabel = "tf-replica-index" labelGroupName = "group-name" - labelTFJobName = "tf-job-name" - labelTFJobRole = "tf-job-role" + // Deprecated label for backwards compatibility. Has to be removed + labelTFJobName = "tf-job-name" ) var ( @@ -559,6 +559,7 @@ func (tc *TFController) GetGroupNameLabelKey() string { return labelGroupName } +// Deprecated function for backwards compatibility. Has to be removed later func (tc *TFController) GetJobNameLabelKey() string { return labelTFJobName } @@ -575,10 +576,6 @@ func (tc *TFController) GetReplicaIndexLabelKey() string { return tfReplicaIndexLabel } -func (tc *TFController) GetJobRoleKey() string { - return labelTFJobRole -} - func (tc *TFController) ControllerName() string { return controllerName } diff --git a/pkg/controller.v1beta2/tensorflow/controller_test.go b/pkg/controller.v1beta2/tensorflow/controller_test.go index 07ba3ae5dc..4ed1d23055 100644 --- a/pkg/controller.v1beta2/tensorflow/controller_test.go +++ b/pkg/controller.v1beta2/tensorflow/controller_test.go @@ -455,7 +455,7 @@ func TestSyncPdb(t *testing.T) { MinAvailable: &minAvailable, Selector: &metav1.LabelSelector{ MatchLabels: map[string]string{ - "tf-job-name": "test-sync-pdb", + "job-name": "test-sync-pdb", }, }, }, diff --git a/pkg/controller.v1beta2/tensorflow/pod.go b/pkg/controller.v1beta2/tensorflow/pod.go index b8fccf8d2b..49b6717f00 100644 --- a/pkg/controller.v1beta2/tensorflow/pod.go +++ b/pkg/controller.v1beta2/tensorflow/pod.go @@ -153,7 +153,7 @@ func (tc *TFController) createNewPod(tfjob *tfv1beta2.TFJob, rt, index string, s labels[tfReplicaIndexLabel] = index if masterRole { - labels[labelTFJobRole] = "master" + labels[jobcontroller.JobRoleLabel] = "master" } podTemplate := spec.Template.DeepCopy() diff --git a/py/kubeflow/tf_operator/tf_job_client.py b/py/kubeflow/tf_operator/tf_job_client.py index 238fbac3e5..f0d3377a86 100644 --- a/py/kubeflow/tf_operator/tf_job_client.py +++ b/py/kubeflow/tf_operator/tf_job_client.py @@ -15,7 +15,7 @@ TF_JOB_GROUP = "kubeflow.org" TF_JOB_PLURAL = "tfjobs" TF_JOB_KIND = "TFJob" -TF_JOB_NAME_LABEL = "tf-job-name" +TF_JOB_NAME_LABEL = "job-name" # How long to wait in seconds for requests to the ApiServer TIMEOUT = 120 From 29dae7255e807b717ad91e8e78c84de0c208a4ac Mon Sep 17 00:00:00 2001 From: Johnu George Date: Thu, 8 Aug 2019 00:52:59 +0530 Subject: [PATCH 2/2] Fixes for test --- pkg/common/util/v1/testutil/util.go | 9 ++++++--- pkg/common/util/v1beta2/testutil/util.go | 10 ++++++---- pkg/controller.v1/tensorflow/util_test.go | 6 ++++-- pkg/controller.v1beta2/tensorflow/util_test.go | 7 +++++-- 4 files changed, 21 insertions(+), 11 deletions(-) diff --git a/pkg/common/util/v1/testutil/util.go b/pkg/common/util/v1/testutil/util.go index 29f9c94bad..c3c232a24d 100644 --- a/pkg/common/util/v1/testutil/util.go +++ b/pkg/common/util/v1/testutil/util.go @@ -29,7 +29,9 @@ import ( const ( LabelGroupName = "group-name" - LabelTFJobName = "job-name" + JobNameLabel = "job-name" + // Deprecated label. Has to be removed later + DeprecatedLabelTFJobName = "tf-job-name" ) var ( @@ -42,8 +44,9 @@ var ( func GenLabels(jobName string) map[string]string { return map[string]string{ - LabelGroupName: GroupName, - LabelTFJobName: strings.Replace(jobName, "/", "-", -1), + LabelGroupName: GroupName, + JobNameLabel: strings.Replace(jobName, "/", "-", -1), + DeprecatedLabelTFJobName: strings.Replace(jobName, "/", "-", -1), } } diff --git a/pkg/common/util/v1beta2/testutil/util.go b/pkg/common/util/v1beta2/testutil/util.go index d1bde8cb2d..6bc59d44eb 100644 --- a/pkg/common/util/v1beta2/testutil/util.go +++ b/pkg/common/util/v1beta2/testutil/util.go @@ -28,8 +28,9 @@ import ( ) const ( - LabelGroupName = "group-name" - LabelTFJobName = "job-name" + LabelGroupName = "group-name" + JobNameLabel = "job-name" + DeprecatedLabelTFJobName = "tf-job-name" ) var ( @@ -42,8 +43,9 @@ var ( func GenLabels(jobName string) map[string]string { return map[string]string{ - LabelGroupName: GroupName, - LabelTFJobName: strings.Replace(jobName, "/", "-", -1), + LabelGroupName: GroupName, + JobNameLabel: strings.Replace(jobName, "/", "-", -1), + DeprecatedLabelTFJobName: strings.Replace(jobName, "/", "-", -1), } } diff --git a/pkg/controller.v1/tensorflow/util_test.go b/pkg/controller.v1/tensorflow/util_test.go index 4cf950f512..2beeea11c1 100644 --- a/pkg/controller.v1/tensorflow/util_test.go +++ b/pkg/controller.v1/tensorflow/util_test.go @@ -21,6 +21,7 @@ import ( "k8s.io/apimachinery/pkg/types" tfv1 "github.com/kubeflow/tf-operator/pkg/apis/tensorflow/v1" + "github.com/kubeflow/tf-operator/pkg/common/jobcontroller" "github.com/kubeflow/tf-operator/pkg/common/util/v1/testutil" ) @@ -51,9 +52,10 @@ func TestGenLabels(t *testing.T) { expctedKey := "test-key" labels := testutil.GenLabels(testKey) + jobNamelabel := jobcontroller.JobNameLabel - if labels[labelTFJobName] != expctedKey { - t.Errorf("Expected %s %s, got %s", labelTFJobName, expctedKey, labels[labelTFJobName]) + if labels[jobNamelabel] != expctedKey { + t.Errorf("Expected %s %s, got %s", jobNamelabel, expctedKey, jobNamelabel) } if labels[labelGroupName] != tfv1.GroupName { t.Errorf("Expected %s %s, got %s", labelGroupName, tfv1.GroupName, labels[labelGroupName]) diff --git a/pkg/controller.v1beta2/tensorflow/util_test.go b/pkg/controller.v1beta2/tensorflow/util_test.go index 77144843a2..85daf5c6c3 100644 --- a/pkg/controller.v1beta2/tensorflow/util_test.go +++ b/pkg/controller.v1beta2/tensorflow/util_test.go @@ -21,6 +21,7 @@ import ( "k8s.io/apimachinery/pkg/types" tfv1beta2 "github.com/kubeflow/tf-operator/pkg/apis/tensorflow/v1beta2" + "github.com/kubeflow/tf-operator/pkg/common/jobcontroller" "github.com/kubeflow/tf-operator/pkg/common/util/v1beta2/testutil" ) @@ -51,10 +52,12 @@ func TestGenLabels(t *testing.T) { expctedKey := "test-key" labels := testutil.GenLabels(testKey) + jobNamelabel := jobcontroller.JobNameLabel - if labels[labelTFJobName] != expctedKey { - t.Errorf("Expected %s %s, got %s", labelTFJobName, expctedKey, labels[labelTFJobName]) + if labels[jobNamelabel] != expctedKey { + t.Errorf("Expected %s %s, got %s", jobNamelabel, expctedKey, jobNamelabel) } + if labels[labelGroupName] != tfv1beta2.GroupName { t.Errorf("Expected %s %s, got %s", labelGroupName, tfv1beta2.GroupName, labels[labelGroupName]) }