Skip to content

Conversation

@ywskycn
Copy link
Member

@ywskycn ywskycn commented Jan 21, 2020

Expose main container name as a configurable field. This is used for cases where launcher/worker pods have sidecars. Existing kubectl exec command in mpi_job_controller.go doesn't include container name, causing the command fails.


This change is Reviewable

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: terrytangyuan

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:

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

@terrytangyuan
Copy link
Member

@ywskycn Could you fix this?

ERRO Running error: buildssa: analysis skipped: errors in package: [/home/travis/gopath/src/github.com/kubeflow/mpi-operator/pkg/controllers/v1alpha2/mpi_job_controller_test.go:180:2: too few arguments in call to NewMPIJobController] 

@ywskycn
Copy link
Member Author

ywskycn commented Jan 22, 2020

@terrytangyuan , may need to change this PR. In the v1alpha2 spec, users can specify container name, thus a global config main_container may be not reasonable.

One approach would be, we read the worker's container name on-the-fly for each job, ant put the name in the kubectl script. But this is still have risk if users specify more than one container in the worker spec, in which we don't which is the main container, which is the sidecar...

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jan 23, 2020
@k8s-ci-robot
Copy link

New changes are detected. LGTM label has been removed.

@ywskycn
Copy link
Member Author

ywskycn commented Jan 23, 2020

Add a new PR which exposes the mainContainer as a field in job level, instead of the previous operator-level.

If users don't specify mainContainer, the exec command would be kubectl exec {POD_NAME} -- /bin/sh -c "$*", and it will run against the default container. If specified, the command would be kubectl exec ${POD_NAME} --container ${mainContainer} -- /bin/sh -c "$*".

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thanks!

@terrytangyuan terrytangyuan merged commit 5253329 into kubeflow:master Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants