Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added hosts into environment. #610

Merged
merged 1 commit into from
Dec 17, 2019
Merged

Added hosts into environment. #610

merged 1 commit into from
Dec 17, 2019

Conversation

k82cn
Copy link
Member

@k82cn k82cn commented Dec 15, 2019

Signed-off-by: Klaus Ma klaus1982.cn@gmail.com

fixed #260

Added hosts into pod's environment.

@volcano-sh-bot volcano-sh-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 15, 2019
@TravisBuddy
Copy link

Travis tests have failed

Hey @k82cn,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 4bf2e3d0-1f52-11ea-ba47-7f442aed9c1e

@zrss
Copy link
Contributor

zrss commented Dec 16, 2019

generally, AI workload also need the current_host info, it would be better if vj provides the current_host env too

@zrss
Copy link
Contributor

zrss commented Dec 16, 2019

by the way, from this PR, in my point of view, the env plugin should change to a more suitable name ...

@k82cn
Copy link
Member Author

k82cn commented Dec 16, 2019

by the way, from this PR, in my point of view, the env plugin should change to a more suitable name ...

svc plugin will handle hosts related issue, including environment. env plugin will help to handle others :)

@k82cn
Copy link
Member Author

k82cn commented Dec 16, 2019

generally, AI workload also need the current_host info, it would be better if vj provides the current_host env too

what does urrent_host` info mean? you can get hostname in container by POSIX api, e.g. hostname.

@TravisBuddy
Copy link

Travis tests have failed

Hey @k82cn,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 1d303e60-1fa4-11ea-ba47-7f442aed9c1e

@zrss
Copy link
Contributor

zrss commented Dec 16, 2019

generally, AI workload also need the current_host info, it would be better if vj provides the current_host env too

what does urrent_host` info mean? you can get hostname in container by POSIX api, e.g. hostname.

hostname is the podName, VC_CURRENT_HOST = the headless svc of current pod, and may be in this format [VC-Job-Name]-[VC-Task-Name]-[VC-Task-Index].[VC-Job-Name]

@hzxuzhonghu
Copy link
Collaborator

VC_CURRENT_HOST = the headless svc of current pod, and may be in this format [VC-Job-Name]-[VC-Task-Name]-[VC-Task-Index].[VC-Job-Name]

Is this a common requirement, I am worried this is just for TF job

@TravisBuddy
Copy link

Hey @k82cn,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: a5660a80-1fae-11ea-ba47-7f442aed9c1e

@zrss
Copy link
Contributor

zrss commented Dec 16, 2019

VC_CURRENT_HOST = the headless svc of current pod, and may be in this format [VC-Job-Name]-[VC-Task-Name]-[VC-Task-Index].[VC-Job-Name]

Is this a common requirement, I am worried this is just for TF job

hostname or podname can work well in the container network, but it doesn't work in the host network mode, but the headless svc work

@k82cn
Copy link
Member Author

k82cn commented Dec 16, 2019

hm.. maybe we can support podName [VC-Job-Name]-[VC-Task-Name]-[VC-Task-Index].[VC-Job-Name] style in hostfile.

@k82cn
Copy link
Member Author

k82cn commented Dec 16, 2019

just checked code here: https://github.com/volcano-sh/volcano/blob/master/pkg/controllers/job/plugins/svc/svc.go#L274 . It seems we already support that; so I'd like to support host network in other PR :)

@hzxuzhonghu
Copy link
Collaborator

I am still not sure what do you mean by hostnetwork. IIRC, the hostnetwork mode has nothing special for headless service.

@k82cn
Copy link
Member Author

k82cn commented Dec 16, 2019

I am still not sure what do you mean by hostnetwork. IIRC, the hostnetwork mode has nothing special for headless service.

If so, please work with @zrss on this.

@hzxuzhonghu
Copy link
Collaborator

@zrss please clarify, we need a little bit more. Not just an issue without any context

@k82cn
Copy link
Member Author

k82cn commented Dec 16, 2019

@zrss please clarify, we need a little bit more. Not just an issue without any context

@hzxuzhonghu , please start another thread for this discussion.

@k82cn k82cn force-pushed the vc_260 branch 2 times, most recently from 90ecd64 to 184553c Compare December 16, 2019 07:53
@volcano-sh-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: k82cn

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

@TravisBuddy
Copy link

Hey @k82cn,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: d23cc130-1fda-11ea-ba47-7f442aed9c1e

@TravisBuddy
Copy link

Travis tests have failed

Hey @k82cn,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: 359641c0-1fdb-11ea-ba47-7f442aed9c1e

@TravisBuddy
Copy link

Travis tests have failed

Hey @k82cn,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: d1f894d0-1ff1-11ea-ba47-7f442aed9c1e

@TravisBuddy
Copy link

Hey @k82cn,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 8103be60-1ffb-11ea-ba47-7f442aed9c1e

@TravisBuddy
Copy link

Travis tests have failed

Hey @k82cn,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: fbe64510-200c-11ea-8d2e-279a4df7ebeb

@k82cn k82cn mentioned this pull request Dec 16, 2019
Closed
@TravisBuddy
Copy link

Travis tests have failed

Hey @k82cn,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

TravisBuddy Request Identifier: aea92a00-2017-11ea-8d2e-279a4df7ebeb

hzxuzhonghu
hzxuzhonghu reviewed Dec 17, 2019
View reviewed changes
pkg/controllers/job/plugins/svc/svc.go
for _, name := range envNames {
hostEnv = append(hostEnv, v1.EnvVar{
Name: name,
ValueFrom: &v1.EnvVarSource{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Downward api is used when we can not get the value directly, and is it not that efficient. Here can set the value directly.

hzxuzhonghu
hzxuzhonghu reviewed Dec 17, 2019
View reviewed changes
pkg/controllers/job/plugins/svc/const.go
// EnvTaskHostFmt is the key for host list in environment
EnvTaskHostFmt = "VC_%s_HOSTS"
// EnvHostNumFmt is the key for host number in environment
EnvHostNumFmt = "VC_%s_NUM"
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's the use case for the num.

@k82cn
Added hosts into environment.
9b41dc0 
Signed-off-by: Klaus Ma <klaus1982.cn@gmail.com>
@TravisBuddy
Copy link

Hey @k82cn,
Something went wrong with the build.

TravisCI finished with status errored, which means the build failed because of something unrelated to the tests, such as a problem with a dependency or the build process itself.

View build log

TravisBuddy Request Identifier: 41f3e410-20ba-11ea-830b-038034041c48

@TravisBuddy
Copy link

Hey @k82cn,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: de687d10-20c4-11ea-830b-038034041c48

@k82cn k82cn added the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2019
@volcano-sh-bot volcano-sh-bot merged commit 8b701de into volcano-sh:master Dec 17, 2019
@k82cn k82cn deleted the vc_260 branch December 25, 2019 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hzxuzhonghu hzxuzhonghu hzxuzhonghu left review comments

@hex108 hex108 Awaiting requested review from hex108

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add hosts list into environment
5 participants
@k82cn @TravisBuddy @zrss @hzxuzhonghu @volcano-sh-bot