Add skeleton code for reconcile service #25

merlintang · 2019-04-24T07:23:12Z

This PR adds skeleton code for reconcile service.

This change is

googlebot · 2019-04-24T07:23:15Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

merlintang · 2019-04-24T07:26:17Z

I signed it!

gaocegege

/lgtm

k8s-ci-robot · 2019-04-24T07:38:04Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: gaocegege

If they are not already assigned, you can assign the PR to them by writing /assign @gaocegege in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

gaocegege · 2019-04-24T07:40:03Z

job_controller/job_controller.go

@@ -87,7 +87,10 @@ type ControllerInterface interface {
 // DeletePod deletes the pod
 DeletePod(job interface{}, pod *v1.Pod) error

- // SetClusterSpec sets the cluster spec for the pod
+ // Get the deafult container port number
+ GetDefaultContainerPortName() string


Could we place the function after GetDefaultContainerName() string?

good point, let me update it.

googlebot · 2019-04-24T07:58:37Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

gaocegege

/lgtm

terrytangyuan · 2019-04-24T12:20:58Z

job_controller/service.go

+ return nil
+}
+
+// GetPortFromJob gets the port of tensorflow container.


Remove "tensorflow"

terrytangyuan · 2019-04-24T12:21:22Z

job_controller/service.go

+// createNewService creates a new service for the given index and type.
+func (jc *JobController) createNewService(job metav1.Object, rtype commonv1.ReplicaType,
+ spec *commonv1.ReplicaSpec, index string) error {
+ tfjobKey, err := KeyFunc(job)


Remove "tf"

terrytangyuan · 2019-04-24T12:24:06Z

job_controller/job_controller.go

@@ -93,6 +93,9 @@ type ControllerInterface interface {
 // Returns the default container name in pod
 GetDefaultContainerName() string

+ // Get the deafult container port number


port number -> port name to be consistent with the function name

Typo - default

terrytangyuan · 2019-04-24T12:47:49Z

@merlintang Travis CI build failed on golangci-lint run. I believe you can fix it by changing reconcileServices to ReconcileServices for now.

richardsliu · 2019-04-26T17:54:20Z

job_controller/service.go

+ for index, serviceSlice := range serviceSlices {
+ if len(serviceSlice) > 1 {
+ util.LoggerForReplica(job, rt).Warningf("We have too many services for %s %d", rt, index)
+ // TODO(gaocegege): Kill some services.


Please change this unless Gao Ce is still planning to fix this :)

richardsliu · 2019-04-26T17:54:36Z

job_controller/job_controller.go

@@ -93,6 +93,9 @@ type ControllerInterface interface {
 // Returns the default container name in pod
 GetDefaultContainerName() string

+ // Get the deafult container port number


Typo - default

richardsliu · 2019-04-26T17:57:50Z

job_controller/service.go

+ }
+ }
+ }
+ return -1, fmt.Errorf("failed to found the port")


failed to find

thanks! Liu, I have updated them.

richardsliu · 2019-04-26T19:25:54Z

job_controller/service.go

+}
+
+// GetPortFromJob gets the port of job container.
+func (jc *JobController) GetPortFromJob(spec *commonv1.ReplicaSpec) (int32, error) {


GetPortFromReplicaSpec?

the input is the replicaSpec, then the underline logic is the replicaSpec related container. thus, I prefer the container name for further understand. if you think replicaSpec is better for understanding, I can change it.

richardsliu · 2019-04-29T18:54:00Z

/lgtm

johnugeorge · 2019-04-30T03:13:40Z

job_controller/job_controller.go

@@ -48,6 +48,12 @@ type ControllerInterface interface {
 // Returns the Group Name(value) in the labels of the job
 GetGroupNameLabelValue() string

+ // Returns the Replica Type(key) in the labels of the job
+ GetReplicaTypeLabelKey() string


Is this because of merge conflict? #29 removes these labels

I think so. @merlintang Could you rebase?

it is fixed. thanks for your suggestion.

These interfaces are supposed to be removed and use this instead

gaocegege

/lgtm

merlintang · 2019-05-05T05:30:22Z

@jian-he can you review this as well?

gaocegege · 2019-05-05T05:30:43Z

/cc @jian-he

jian-he · 2019-05-06T18:42:47Z

job_controller/job_controller.go

@@ -48,6 +48,12 @@ type ControllerInterface interface {
 // Returns the Group Name(value) in the labels of the job
 GetGroupNameLabelValue() string

+ // Returns the Replica Type(key) in the labels of the job
+ GetReplicaTypeLabelKey() string


These interfaces are supposed to be removed and use this instead

jian-he · 2019-05-06T18:43:00Z

job_controller/job_controller.go

+
+ // Returns the Replica Index(value) in the labels of the job
+ GetReplicaIndexLabelKey() string
+


same for this

These two also need to be fixed.

common/job_controller/job.go

Line 206 in b615b1c

// TODO(terrytangyuan): Uncomment this once service reconciliation logic is in place

common/job_controller/job.go

Line 103 in b615b1c

// TODO(terrytangyuan): Uncomment this once service reconciliation logic is in place

jian-he · 2019-05-06T18:46:41Z

job_controller/test_job_controller.go

+func (TestJobController) GetReplicaIndexLabelKey() string {
+ return util.ReplicaIndexLabel
+}
+


same remove these two

jian-he · 2019-05-06T20:43:27Z

job_controller/job.go

@@ -204,12 +203,12 @@ func (jc *JobController) ReconcileJobs(
 }

 // TODO(terrytangyuan): Uncomment this once service reconciliation logic is in place


could you remove this TODO ?

jian-he · 2019-05-06T21:20:38Z

/lgtm

jian-he · 2019-05-06T21:21:13Z

@richardsliu could you take a look ?

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

洁懿 added 2 commits April 24, 2019 14:57

reconcile service

e9e10f3

Merge remote-tracking branch 'upstream/master' into reconcile-service

fc5670a

k8s-ci-robot requested review from johnugeorge and richardsliu April 24, 2019 07:23

k8s-ci-robot added the size/L label Apr 24, 2019

gaocegege approved these changes Apr 24, 2019

View reviewed changes

k8s-ci-robot assigned gaocegege Apr 24, 2019

k8s-ci-robot added the lgtm label Apr 24, 2019

remove the duplicate code

f58e0df

k8s-ci-robot removed the lgtm label Apr 24, 2019

gaocegege reviewed Apr 24, 2019

View reviewed changes

update with comments

6115438

gaocegege reviewed Apr 24, 2019

View reviewed changes

k8s-ci-robot added the lgtm label Apr 24, 2019

terrytangyuan reviewed Apr 24, 2019

View reviewed changes

update with terry's comments

1a3caa9

k8s-ci-robot removed the lgtm label Apr 25, 2019

richardsliu reviewed Apr 26, 2019

View reviewed changes

洁懿 added 2 commits April 27, 2019 09:58

update with Liu's comment

15d97d5

Merge remote-tracking branch 'upstream/master' into reconcile-service

d1aa95b

k8s-ci-robot assigned richardsliu Apr 29, 2019

k8s-ci-robot added the lgtm label Apr 29, 2019

johnugeorge reviewed Apr 30, 2019

View reviewed changes

Merge branch 'master' into reconcile-service

fc6edca

k8s-ci-robot removed the lgtm label May 5, 2019

gaocegege reviewed May 5, 2019

View reviewed changes

k8s-ci-robot added the lgtm label May 5, 2019

k8s-ci-robot requested a review from jian-he May 5, 2019 05:30

jian-he reviewed May 6, 2019

View reviewed changes

update with jian's comments

62b6c04

k8s-ci-robot removed the lgtm label May 6, 2019

jian-he reviewed May 6, 2019

View reviewed changes

remove the todo comment

5cc88a3

k8s-ci-robot assigned jian-he May 6, 2019

k8s-ci-robot added the lgtm label May 6, 2019

richardsliu merged commit 38d6b71 into kubeflow:master May 9, 2019

georgkaleido pushed a commit to georgkaleido/common that referenced this pull request Jun 9, 2022

Update kaleido-models version (kubeflow#25)

9a26cc8

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add skeleton code for reconcile service #25

Add skeleton code for reconcile service #25

merlintang commented Apr 24, 2019 •

edited by jlewi

Loading

googlebot commented Apr 24, 2019

merlintang commented Apr 24, 2019

gaocegege left a comment

k8s-ci-robot commented Apr 24, 2019

gaocegege Apr 24, 2019

merlintang Apr 24, 2019

googlebot commented Apr 24, 2019

gaocegege left a comment

terrytangyuan Apr 24, 2019

terrytangyuan Apr 24, 2019

terrytangyuan Apr 24, 2019

richardsliu Apr 26, 2019

terrytangyuan commented Apr 24, 2019

richardsliu Apr 26, 2019

richardsliu Apr 26, 2019

richardsliu Apr 26, 2019

merlintang Apr 27, 2019

richardsliu Apr 26, 2019

merlintang Apr 27, 2019

richardsliu commented Apr 29, 2019

johnugeorge Apr 30, 2019

terrytangyuan Apr 30, 2019

merlintang May 5, 2019

jian-he May 6, 2019

gaocegege left a comment

merlintang commented May 5, 2019

gaocegege commented May 5, 2019

jian-he May 6, 2019

jian-he May 6, 2019

jian-he May 6, 2019

jian-he May 6, 2019

jian-he May 6, 2019

jian-he commented May 6, 2019

jian-he commented May 6, 2019 •

edited

Loading


		// Returns the Replica Index(value) in the labels of the job
		GetReplicaIndexLabelKey() string

		@@ -204,12 +203,12 @@ func (jc *JobController) ReconcileJobs(
		}

		// TODO(terrytangyuan): Uncomment this once service reconciliation logic is in place

Add skeleton code for reconcile service #25

Add skeleton code for reconcile service #25

Conversation

merlintang commented Apr 24, 2019 • edited by jlewi Loading

googlebot commented Apr 24, 2019

merlintang commented Apr 24, 2019

gaocegege left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Apr 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Apr 24, 2019

gaocegege left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terrytangyuan commented Apr 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richardsliu commented Apr 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaocegege left a comment

Choose a reason for hiding this comment

merlintang commented May 5, 2019

gaocegege commented May 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jian-he commented May 6, 2019

jian-he commented May 6, 2019 • edited Loading

merlintang commented Apr 24, 2019 •

edited by jlewi

Loading

jian-he commented May 6, 2019 •

edited

Loading