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.

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

Handle custom executor Mesos details links #808

Merged

Conversation

janisz
Copy link
Contributor

@janisz janisz commented Jun 7, 2017

@janisz janisz force-pushed the bugfix/handle_custom_executor_in_ui branch 3 times, most recently from 467682c to d48235e Compare June 7, 2017 15:00
@janisz
Copy link
Contributor Author

janisz commented Jun 13, 2017

@Poltergeist Can you tak a look at this?

@Poltergeist
Copy link
Contributor

Thanks @janisz for the contribution. I will have a look at this PR within the next week. Sorry for letting you wait so long.

@Poltergeist
Copy link
Contributor

Hey @janisz, sorry for the delay. I started to get to this Pull request today. The Code looks good so far. And I would like to merge it but unfortunately I don't no how I could test this. It would be great if you could provide a app definition which is using a custom executor.

@janisz
Copy link
Contributor Author

janisz commented Jul 4, 2017

It's not so easy to test. First of all you need to have a custom executor. For eample it could be sample executor from mesos-go repository https://github.com/mesos/mesos-go/blob/master/api/v1/cmd/example-executor/main.go once you build it you must make it availabe for Mesos to download.

Application definiton will looks like this (I assume executor will be named `executor and will be available at localhost:3000/executor)

{
    "id": "/executor/test",
    "cmd": "env && sleep 300",
    "cpus": 0.1,
    "mem": 32.0,
    "ports": 1,
    "instances": 1,
    "executor": "./executor",
    "fetch": [
        { "uri": "https://localhost:3000/executor" }
    ]
}

@orlandohohmeier
Copy link
Contributor

@Poltergeist did you have a chance to look into this one?

@Poltergeist
Copy link
Contributor

Hey @janisz
I finally achieved the skills necessary to build the example. And I am currently checking out the thing, sorry that this took so long.

Unfortunately the go executor does finish directly. And I was not able to test this PR with a running task. But, and that is the learning along the way, for failing tasks there is not always the executor attribute in the task object. Maybe it is more secure to set the condition onto the executor attribute in the application config?

@janisz
Copy link
Contributor Author

janisz commented Aug 21, 2017

for failing tasks there is not always the executor attribute in the task object. Maybe it is more secure to set the condition onto the executor attribute in the application config?

@Poltergeist Can you elaborate? Where executor filed is missing? I'll add test case for it but I'm not sure what is missing.

@orlandohohmeier
Copy link
Contributor

@Poltergeist could you please clarify what is not working/missing?

@janisz janisz force-pushed the bugfix/handle_custom_executor_in_ui branch 2 times, most recently from 8cdf078 to 7769cfe Compare September 27, 2017 14:37
@janisz janisz force-pushed the bugfix/handle_custom_executor_in_ui branch from 7769cfe to 956c120 Compare September 28, 2017 12:52
@janisz
Copy link
Contributor Author

janisz commented Sep 28, 2017

@orlandohohmeier @Poltergeist I think I fixed all issues. Can you check it out?

@janisz
Copy link
Contributor Author

janisz commented Oct 3, 2017

@orlandohohmeier @Poltergeist We deployed this fix in our infrastructure and it's solved our problems with custom executor and Mesos links.

@orlandohohmeier
Copy link
Contributor

@janisz Pardon for not getting back to you any earlier. @Poltergeist and I will look into your fix today. Thanks again for investing the time to provide and test a fix for MARATHON_UI-144.

@Poltergeist
Copy link
Contributor

@janisz I had a last look at the version everything looks fine. And everything seems to be wired good. Thanks for your patience. I will merge this now.

@Poltergeist Poltergeist merged commit 73381fa into mesosphere:master Oct 4, 2017
@janisz janisz deleted the bugfix/handle_custom_executor_in_ui branch October 4, 2017 14:01
@janisz
Copy link
Contributor Author

janisz commented Oct 4, 2017

Cool!

@Poltergeist When will you release new version of Marathon UI. I'd like to update it in Marathon, then release Marathon so we can use official releases instead of custom fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants