MESOS: Support a pre-installed km binary at a well known, agent-local path #28447

k82cn · 2016-07-04T03:58:23Z

k8s-bot · 2016-07-04T03:58:34Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-07-04T03:58:53Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-07-04T03:59:11Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-07-04T03:59:29Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-07-04T04:00:18Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-07-04T13:08:37Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-07-04T13:08:56Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-07-04T13:09:17Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-07-04T13:09:36Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-07-04T13:10:21Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

jdef · 2016-07-05T17:05:45Z

@k8s-bot ok to test

jdef · 2016-07-05T17:11:53Z

contrib/mesos/pkg/scheduler/service/service.go

@@ -389,7 +391,10 @@ func (s *SchedulerServer) prepareExecutorInfo(hks hyperkube.Interface) (*mesos.E
 	} else if !hks.FindServer(hyperkube.CommandMinion) {
 		return nil, fmt.Errorf("either run this scheduler via km or else --executor-path is required")
 	} else {
-		if strings.Index(s.kmPath, "://") > 0 {
+		if s.kmLocalPath != "" { // The default value of kmLocalPath is "".
+			// TODO(k82): Check whether `kmLocalPath` start with `file://` or `/`.


What do you think about making this a subset of the :// case?

} else if strings.Index(s.kmPath, "://") > 0 { if strings.startsWith(s.kmPath, "file://") { // ... don't add this as a URI because it's local to the agent; parse the basename of kmPath and use that for ci.Valkue // (new bits) } else { // ... assume this is something to be downloaded; append a URI and assume that `km` will land in the sandbox. // (exiting bits) } } else if ...

The idea being that people wanting to use a preinstalled km on the agent would always have to specify a URI using the file:// form

k8s-bot · 2016-07-05T17:41:38Z

GCE e2e build/test passed for commit 58f3048ba4fe9953acda4cac1423ca7453fba0c3.

k82cn · 2016-07-06T06:55:44Z

Address jdef's comments.

k8s-bot · 2016-07-06T06:56:13Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-07-06T06:56:33Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-07-06T06:56:52Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-07-06T06:57:13Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-07-06T06:57:34Z

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message may repeat a few times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

Otherwise, if this message is too spammy, please complain to ixdy.

k8s-bot · 2016-07-06T07:37:53Z

GCE e2e build/test passed for commit 540de16b19524cbda6085b43bcbaf523c162ef5b.

jdef · 2016-07-06T14:11:10Z

@k8s-bot ok to test

jdef · 2016-07-06T14:13:09Z

contrib/mesos/pkg/scheduler/service/service.go

-			ci.Value = proto.String("./km") // TODO(jdef) extract constant
+			if strings.HasPrefix(s.kmPath, "file://") {
+				// If `kmPath` started with "file://", `km` in agent local path was used.
+				ci.Value = proto.String(s.kmPath)


did you test this? you probably need to strip the file:// prefix in order to make this work properly

"file://" is handled by Mesos Fetcher at https://github.com/apache/mesos/blob/master/src/slave/containerizer/fetcher.cpp#L199

So it is -- perfect

On Wed, Jul 6, 2016 at 11:19 AM, Klaus Ma notifications@github.com wrote:

In contrib/mesos/pkg/scheduler/service/service.go
#28447 (comment)
:

@@ -390,11 +390,16 @@ func (s _SchedulerServer) prepareExecutorInfo(hks hyperkube.Interface) (_mesos.E
return nil, fmt.Errorf("either run this scheduler via km or else --executor-path is required")
} else {
if strings.Index(s.kmPath, "://") > 0 {

// URI could point directly to executable, e.g. hdfs:///km

// or else indirectly, e.g. http://acmestorage/tarball.tgz

// so we assume that for this case the command will always "km"

ci.Uris = append(ci.Uris, &mesos.CommandInfo_URI{Value: proto.String(s.kmPath), Executable: proto.Bool(true)})

ci.Value = proto.String("./km") // TODO(jdef) extract constant

if strings.HasPrefix(s.kmPath, "file://") {

// If `kmPath` started with "file://", `km` in agent local path was used.

ci.Value = proto.String(s.kmPath)

"file://" is handled by Mesos Fetcher at
https://github.com/apache/mesos/blob/master/src/slave/containerizer/fetcher.cpp#L199

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes/kubernetes/pull/28447/files/0d62260bb2fca8d34b92c647d67087951a1c21b3#r69751134,
or mute the thread
https://github.com/notifications/unsubscribe/ACPVLCqkomu4FCw0L6ZoKL_C1lFNaQZgks5qS8eZgaJpZM4JECIE
.

actually I think I spoke too soon. The mesos code you're pointing at appears to be for handling URIs. Which is fine if you were copying kmPath into a URI. but here you're using it as the arg0 of a command line. so you'll want to strip the file:// prefix upon assigning it to ci.Value.

again, have you tested this?

@jdef , I think you're right, it handles the URI, not the command line. Update the code accordingly.

Re test, yes, but seems tested with old binaries :(. Sorry for the confuse.

jdef · 2016-07-06T14:17:35Z

@k82 looking better, left some comments

k8s-bot · 2016-07-06T14:23:32Z

GCE e2e build/test passed for commit 0d62260bb2fca8d34b92c647d67087951a1c21b3.

k8s-bot · 2016-07-06T14:45:09Z

GCE e2e build/test passed for commit 0d62260bb2fca8d34b92c647d67087951a1c21b3.

k8s-bot · 2016-07-06T16:01:55Z

GCE e2e build/test passed for commit 13dc5a0df58e86444285f511d54d5461605832a4.

jdef · 2016-07-07T15:57:03Z

lgtm

k8s-bot · 2016-07-07T16:16:24Z

GCE e2e build/test passed for commit b361f40.

k8s-github-robot · 2016-07-07T16:53:30Z

Automatic merge from submit-queue

googlebot added the cla: yes label Jul 4, 2016

k8s-github-robot assigned jdef Jul 4, 2016

k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Jul 4, 2016

k82cn closed this Jul 4, 2016

k82cn force-pushed the k8s-26930-km-local-path branch from 392b427 to 40b8fb4 Compare July 4, 2016 13:06

k82cn reopened this Jul 4, 2016

jdef reviewed Jul 5, 2016
View reviewed changes

k82cn closed this Jul 6, 2016

k82cn force-pushed the k8s-26930-km-local-path branch from 58f3048 to 203e1e9 Compare July 6, 2016 02:26

jdef added area/platform/mesos team/mesosphere labels Jul 6, 2016

k82cn reopened this Jul 6, 2016

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 6, 2016

k82cn force-pushed the k8s-26930-km-local-path branch from 540de16 to 0d62260 Compare July 6, 2016 13:43

jdef reviewed Jul 6, 2016
View reviewed changes

jdef added e2e-not-required [deprecated use retest-not-required] and removed e2e-not-required [deprecated use retest-not-required] labels Jul 6, 2016

k82cn force-pushed the k8s-26930-km-local-path branch from 0d62260 to 13dc5a0 Compare July 6, 2016 15:23

Enable km to use local km in agent.

b361f40

k82cn force-pushed the k8s-26930-km-local-path branch from 13dc5a0 to b361f40 Compare July 7, 2016 15:39

jdef changed the title ~~Enable k8sm agent load km locally.~~ MESOS: Support a pre-installed km binary at a well known, agent-local path Jul 7, 2016

jdef added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Jul 7, 2016

jdef added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2016

k8s-github-robot merged commit 32e5996 into kubernetes:master Jul 7, 2016

k82cn deleted the k8s-26930-km-local-path branch July 8, 2016 02:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MESOS: Support a pre-installed km binary at a well known, agent-local path #28447

MESOS: Support a pre-installed km binary at a well known, agent-local path #28447

k82cn commented Jul 4, 2016 •

edited

Loading

k8s-bot commented Jul 4, 2016

k8s-bot commented Jul 4, 2016

k8s-bot commented Jul 4, 2016

k8s-bot commented Jul 4, 2016

k8s-bot commented Jul 4, 2016

k8s-bot commented Jul 4, 2016

k8s-bot commented Jul 4, 2016

k8s-bot commented Jul 4, 2016

k8s-bot commented Jul 4, 2016

k8s-bot commented Jul 4, 2016

jdef commented Jul 5, 2016

jdef Jul 5, 2016

jdef Jul 5, 2016

k8s-bot commented Jul 5, 2016

k82cn commented Jul 6, 2016

k8s-bot commented Jul 6, 2016

k8s-bot commented Jul 6, 2016

k8s-bot commented Jul 6, 2016

k8s-bot commented Jul 6, 2016

k8s-bot commented Jul 6, 2016

k8s-bot commented Jul 6, 2016

jdef commented Jul 6, 2016

jdef Jul 6, 2016

k82cn Jul 6, 2016

jdef Jul 6, 2016

jdef Jul 7, 2016

k82cn Jul 7, 2016

jdef commented Jul 6, 2016

k8s-bot commented Jul 6, 2016

k8s-bot commented Jul 6, 2016

k8s-bot commented Jul 6, 2016

jdef commented Jul 7, 2016

k8s-bot commented Jul 7, 2016

k8s-github-robot commented Jul 7, 2016

MESOS: Support a pre-installed km binary at a well known, agent-local path #28447

MESOS: Support a pre-installed km binary at a well known, agent-local path #28447

Conversation

k82cn commented Jul 4, 2016 • edited Loading

k8s-bot commented Jul 4, 2016

k8s-bot commented Jul 4, 2016

k8s-bot commented Jul 4, 2016

k8s-bot commented Jul 4, 2016

k8s-bot commented Jul 4, 2016

k8s-bot commented Jul 4, 2016

k8s-bot commented Jul 4, 2016

k8s-bot commented Jul 4, 2016

k8s-bot commented Jul 4, 2016

k8s-bot commented Jul 4, 2016

jdef commented Jul 5, 2016

jdef Jul 5, 2016

Choose a reason for hiding this comment

jdef Jul 5, 2016

Choose a reason for hiding this comment

k8s-bot commented Jul 5, 2016

k82cn commented Jul 6, 2016

k8s-bot commented Jul 6, 2016

k8s-bot commented Jul 6, 2016

k8s-bot commented Jul 6, 2016

k8s-bot commented Jul 6, 2016

k8s-bot commented Jul 6, 2016

k8s-bot commented Jul 6, 2016

jdef commented Jul 6, 2016

jdef Jul 6, 2016

Choose a reason for hiding this comment

k82cn Jul 6, 2016

Choose a reason for hiding this comment

jdef Jul 6, 2016

Choose a reason for hiding this comment

jdef Jul 7, 2016

Choose a reason for hiding this comment

k82cn Jul 7, 2016

Choose a reason for hiding this comment

jdef commented Jul 6, 2016

k8s-bot commented Jul 6, 2016

k8s-bot commented Jul 6, 2016

k8s-bot commented Jul 6, 2016

jdef commented Jul 7, 2016

k8s-bot commented Jul 7, 2016

k8s-github-robot commented Jul 7, 2016

k82cn commented Jul 4, 2016 •

edited

Loading