MESOS: scheduler: implement role awareness #15775
Conversation
|
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. |
|
Labelling this PR as size/XL |
k8s-github-robot
added
the
size/XL
| var roleResources []*mesos.Resource | ||
| role := starred(res.GetRole()) | ||
|
|
||
| if _, ok := rolesMap[role]; !ok { |
There was a problem hiding this comment.
Compare https://play.golang.org/p/O11HWFc4pa. Append know how to append to a nil slice.
s-urbaniak
force-pushed
the
sur-k8sm-482-resource-roles
branch
from
646c122 to
87601ea
Compare
s-urbaniak
changed the title
| roles[i] = strings.TrimSpace(r) | ||
| } | ||
|
|
||
| roles = filterRoles(roles, not(emptyRole), not(duplicateRole())) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
s-urbaniak
force-pushed
the
sur-k8sm-482-resource-roles
branch
from
2224dde to
416a4c6
Compare
|
It looks like this is still undergoing refactoring. Holding off on review until it settles. Ping me when ready. |
k8s-github-robot
added
size/XXL
|
Labelling this PR as size/XXL |
|
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
|
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. |
|
The author of this PR is not in the whitelist for merge, can one of the admins add the 'ok-to-merge' label? |
|
@bgrant0607 @brendandburns can one of you review the change outside of contrib/mesos? |
s-urbaniak
changed the title
|
@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} |
|
Sure On Nov 25, 2015, at 10:25 AM, James DeFelice notifications@github.com @smarterclayton https://github.com/smarterclayton mind giving the 2nd — |
|
@smarterclayton could you take a look at the mesos part? |
|
Second commit looks fine, probably don't have time right now to review the first one. |
|
@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
added
the
lgtm
|
For reference, @jdef gave the lgtm above for the mesos code: #15775 (comment) |
|
@k8s-bot test this Tests are more than 48 hours old. Re-running tests. |
|
GCE e2e test build/test passed for commit 9eae47c. |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
GCE e2e test build/test passed for commit 9eae47c. |
|
Automatic merge from submit-queue |
Auto commit by PR queue bot
…urce-roles Auto commit by PR queue bot
…urce-roles Auto commit by PR queue bot
| 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) |
There was a problem hiding this comment.
xref d2iq-archive/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.
This commit implements role awareness in the Mesos scheduler.
TODO:
executorinfo.Registry, write unit testsFor follow-up PRs: