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

MESOS: scheduler: implement role awareness #15775

Merged
merged 2 commits into from Dec 1, 2015
Merged

MESOS: scheduler: implement role awareness #15775

merged 2 commits into from Dec 1, 2015

Conversation

s-urbaniak
Copy link
Contributor

This commit implements role awareness in the Mesos scheduler.

TODO:

For follow-up PRs:

@k8s-bot
Copy link

k8s-bot commented Oct 16, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@k8s-github-robot
Copy link

Labelling this PR as size/XL

@k8s-github-robot k8s-github-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 16, 2015
sttts
sttts reviewed Oct 16, 2015
View reviewed changes
contrib/mesos/pkg/scheduler/podtask/resources.go
var roleResources []*mesos.Resource
role := starred(res.GetRole())

if _, ok := rolesMap[role]; !ok {
Copy link
Contributor

Choose a reason for hiding this comment

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

Compare https://play.golang.org/p/O11HWFc4pa. Append know how to append to a nil slice.

@s-urbaniak s-urbaniak changed the title EXPERIMENTAL/MESOS: scheduler: implement role awareness WIP/MESOS: scheduler: implement role awareness Oct 19, 2015
@s-urbaniak
Copy link
Contributor Author

@jdef @sttts PTAL

sttts
sttts reviewed Oct 20, 2015
View reviewed changes
contrib/mesos/pkg/scheduler/podtask/pod_task.go
roles[i] = strings.TrimSpace(r)
}

roles = filterRoles(roles, not(emptyRole), not(duplicateRole()))
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't like the duplicateRole predicate. Building up the map is O(n*log(n)). Evaluating the filter is O(n^2 * log(n)). Of course we only have a handful of roles. But still this waste is bad style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, not following, the map is built on the fly during filter evaluation, there is no separate build step. duplicateRole returns a func closing over an empty map.

@jdef
Copy link
Contributor

jdef commented Oct 21, 2015

It looks like this is still undergoing refactoring. Holding off on review until it settles. Ping me when ready.

@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 21, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/XXL

@k8s-bot
Copy link

k8s-bot commented Oct 21, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

1 similar comment
@k8s-bot
Copy link

k8s-bot commented Oct 26, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@k8s-github-robot
Copy link

The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label?

@sttts
Copy link
Contributor

sttts commented Nov 23, 2015

@bgrant0607 @brendandburns can one of you review the change outside of contrib/mesos?

@s-urbaniak s-urbaniak changed the title WIP/MESOS: scheduler: implement role awareness MESOS: scheduler: implement role awareness Nov 25, 2015
@jdef
Copy link
Contributor

jdef commented Nov 25, 2015

@smarterclayton mind giving the 2nd commit a quick look? it's the only non-mesos part of this PR, and it looks like you're listed in blunderbuss for both pkg/{api,kubectl}

@smarterclayton
Copy link
Contributor

Sure

On Nov 25, 2015, at 10:25 AM, James DeFelice notifications@github.com
wrote:

@smarterclayton https://github.com/smarterclayton mind giving the 2nd
commit a quick look? it's the only non-mesos part of this PR, and it looks
like you're listed in blunderbuss for both pkg/{api,kubectl}

—
Reply to this email directly or view it on GitHub
#15775 (comment)
.

@s-urbaniak s-urbaniak mentioned this pull request Nov 30, 2015
Open
@sttts
Copy link
Contributor

sttts commented Dec 1, 2015

@smarterclayton could you take a look at the mesos part?

@smarterclayton
Copy link
Contributor

Second commit looks fine, probably don't have time right now to review the first one.

@sttts
Copy link
Contributor

sttts commented Dec 1, 2015

@smarterclayton thanks for the review. my fault in my comment above: of course I only meant the non-mesos part. I count that as lgtm for the second commit.

@sttts sttts added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 1, 2015
@sttts
Copy link
Contributor

sttts commented Dec 1, 2015

For reference, @jdef gave the lgtm above for the mesos code: #15775 (comment)

@k8s-github-robot
Copy link

@k8s-bot test this

Tests are more than 48 hours old. Re-running tests.

@k8s-bot
Copy link

k8s-bot commented Dec 1, 2015

GCE e2e test build/test passed for commit 9eae47c.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Dec 1, 2015

GCE e2e test build/test passed for commit 9eae47c.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

k8s-github-robot pushed a commit that referenced this pull request Dec 1, 2015
Merge pull request #15775 from mesosphere/sur-k8sm-482-resource-roles
7644d34 
Auto commit by PR queue bot
@k8s-github-robot k8s-github-robot merged commit 7644d34 into kubernetes:master Dec 1, 2015
@s-urbaniak s-urbaniak deleted the sur-k8sm-482-resource-roles branch December 9, 2015 15:13
s-urbaniak pushed a commit to mesosphere-backup/kubernetes that referenced this pull request Dec 10, 2015
Merge pull request kubernetes#15775 from mesosphere/sur-k8sm-482-reso…
291b0bf 
…urce-roles

Auto commit by PR queue bot
jdef pushed a commit to mesosphere-backup/kubernetes that referenced this pull request Jan 25, 2016
Merge pull request kubernetes#15775 from mesosphere/sur-k8sm-482-reso…
74691b8 
…urce-roles

Auto commit by PR queue bot
@ravilr ravilr mentioned this pull request Feb 15, 2016
Closed
jdef
jdef reviewed Feb 23, 2016
View reviewed changes
contrib/mesos/pkg/scheduler/service/service.go
log.Infof("registering for election at %v with id %v", path, sid)
go election.Notify(election.NewEtcdMasterElector(etcdClient), path, sid, srv, nil)
log.Infof("registering for election at %v with id %v", path, eid.GetValue())
go election.Notify(election.NewEtcdMasterElector(etcdClient), path, eid.GetValue(), srv, nil)
Copy link
Contributor

Choose a reason for hiding this comment

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

xref mesosphere/kubernetes-mesos#787: sid used to contain a UUID so that each scheduler registered a unique name. this PR breaks multi-scheduler HA because identical executor ID's eid are reused as the scheduler UUID across multiple schedulers.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jdef jdef

Labels
area/platform/mesos lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@s-urbaniak @k8s-bot @k8s-github-robot @jdef @sttts @karlkfi @googlebot @smarterclayton @k8s-teamcity-mesosphere