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: Refactor scheduler #16316

Merged
merged 74 commits into from
Nov 12, 2015
Merged

MESOS: Refactor scheduler #16316

merged 74 commits into from
Nov 12, 2015

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Oct 26, 2015

Main goals

  • modularize scheduler such that unit tests are possible (in contrast to the huge integation_test.go we have now
  • reduce complexity by reducing dependencies

Steps:

  • rename things with speaking identifiers
  • move code out of the MesosScheduler into operations or the service
  • turn plugin into a SchedulerLoop
  • move SchedulerLoop instantiation out of the MesosScheduler
  • remove ExecutorInfo from podtask.T and create it with the TaskInfo
  • split task and offer registry from MesosScheduler
  • introduce interfaces for all operations
  • clean up package structure
  • document architecture

                    ┌───────────────────────────────────────────────────────────────────────┐                                                                   
                    │                ┌───────────────────────────────────────┐            ┌─┴──────────────────────┐             ┌───────────────┐              
              ┌─────▼─────┐          │Queuer                                 │  Await()   │       podUpdates       │             │               │              
              │  updates  │          │- Yield() *api.Pod                     ├──pod CRUD ─▶ (queue.HistoricalFIFO) ◀──reflector──▶pods ListWatch ├──apiserver──▶
              └─────▲─────┘          │- Requeue(pod)/Dequeue(id)/Reoffer(pod)│   events   │                        │             │               │              
                    │                └───────────────────▲───────────────────┘            └───────────┬────────────┘             └───────────────┘              
                    │                                    │                                            │                                                         
                    │                                    │                                            │                                                         
                    └───────────────┐┌───────────────────▲────────────────────▲─────────────────────┐ └───────────────────────┐                                 
                                    ││                                        │                     │    ┌────────────────────┼─────────────────┐               
                ┌───────────────────┼┼──────────────────────────────────────┐ │ ┌───────────────────┼────┼───────────┐        │                 │               
    ┌───────────▼──────────┐┌───────┴┴───────┐   ┌───────────────────┐   ┌──┴─┴─┴──────┐   ┌────────┴────┴───┐  ┌────▼────────▼─────────────┐   │               
    │Binder (task launcher)││Deleter         │   │PodReconciler      │   │SchedulerLoop│   │  ErrorHandler   │  │SchedulerAlgorithm         │   │               
    │- Bind(binding)       ││- DeleteOne(pod)│   │- Reconcile(pod)   │   │- Run()      │   │- Error(pod, err)│  │- Schedule(pod) -> NodeName│   │               
    │                      ││                │◀──│                   │   │             │──▶│                 │  │                           │   │               
    │               ┌─────┐││    ┌─────┐     │   │      ┌─────┐      │   │   ┌─────┐   │   │    ┌─────┐      │  │┌─────┐                    │   │               
    └───────────────┤sched├┘└────┤sched├─────┘   └──────┤sched├───▲──┘   └───┤sched├───┘   └────┤sched├──────┘  └┤sched├──────────────┬─────┘   │               
                    ├-│││-┴──────┴--││-┴────────────────┴--│--┴───┼──────────┴--│--┴────────────┴-│---┴──────────┴-│││-┤ ┌────────────▼─────────▼─────────┐     
                    │ │││           ││                     │      │             │                 │                │││ │ │          podScheduler          │     
                    │ ││└───────────▼┼─────────────────────▼──────┼─────────────▼─────────────────▼────────────────┘││ │ │    (e.g. fcfsPodScheduler)     │     
                    │ │└─────────────┼────────────────────────────┼─────────────┼──────────────────▼────────────────┘│ │ │                                │     
                    │ │              │                            │             │                  │                 │ │ │  scheduleOne(pod, offers ...)  │     
                    │ │              │                            │             │                  │                 │ │ │     ┌──────────────────────────┤     
                    │ │              │         ╲   │   │   │   ╱  │             │                  │                 ▼ │ │     │    allocationStrategy    │     
                    │ │              │          ╲  └┐  │  ┌┘  ╱   │             │                  │                   │ │     │      - FitPredicate      │     
                    │ │              │           ╲  │  │  │  ╱    │             │                  │                   │ │     │      - Procurement       │     
                    │ │              │            ╲ └┐ │ ┌┘ ╱     │             │                  │                   │ └─────┴──────────────────────────┘     
                    │┌▼────────────┐┌▼──────────┐┌─▼─▼─▼─▼─▼─┐┌───┴────────┐┌───▼───┐         ┌────▼───┐               │                                        
                    ││LaunchTask(t)││KillTask(t)││sync.Mutex ││reconcile(t)││Tasks()│         │Offers()│               │                                        
                    │└──────┬──────┘└─────┬─────┘└───────────┘└────────▲───┘└───┬───┘         └────┬───┘               │                                        
                    │       │             │                            │        │                  │                   │                                        
                    │       │             └──────────────────┐         │    ┌───▼────────────┐     │                   │                                        
                    │       └──────────────────────────────┐ │         │    │podtask.Registry│     │                   │                                        
                    │                                      │ │         │    └────────────────┘     │                   │           ┌──────────────────────┐     
                    │                                      │ │         │                           │                   │           │                      │     
                    │Scheduler                             │ └──────┐  │                           │                   │           │   A ──────────▶ B    │     
                    └──────────────────────────────────────┼────────┼─┬│----┬──────────────────────┼───────────────────┘           │                      │     
                    ┌──────────────────────────────────────┼────────┼─┤sched├──────────────────────┼─────────────────────────┐     │  A has a reference   │     
                    │Framework                             │        │ └─────┘                 ┌────▼───┐                     │     │   on B and calls B   │     
                    │                               ┌──────▼──────┐┌▼──────────┐              │Offers()│                     │     │                      │     
                    │                               │LaunchTask(t)││KillTask(t)│              └────┬───┘                     │     └──────────────────────┘     
                    │                               └─────────┬───┘└──────┬────┘          ┌────────▼───────┐                 │                                  
                    │implements: mesos-go/scheduler.Scheduler └───────────▼               │offers.Registry │                 │                                  
                    │                                                     │               └────────────────┘                 │                                  
                    │                        ┌─────────────────┐       ┌──▼─────────────┐                                    │                                  
                    └────────────────────────┤                 ├───────┤     Mesos      ├────────────────────────────────────┘                                  
                                             │ TasksReconciler │       │   Scheduler    │                                                                       
                                             │                 ├───────▶     Driver     │                                                                       
                                             └─────────────────┘       └────────┬───────┘                                                                       
                                                                                │                                                                               
                                                                                │                                                                               
                                                                                ▼

@sttts sttts self-assigned this Oct 26, 2015
@k8s-github-robot k8s-github-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 26, 2015
@k8s-github-robot
Copy link

Labelling this PR as size/XXL

@k8s-bot
Copy link

k8s-bot commented Oct 26, 2015

GCE e2e build/test failed for commit 6774881.

@sttts sttts force-pushed the scheduler-refactor branch from 6774881 to a2e34a3 Compare October 27, 2015 05:01
@k8s-bot
Copy link

k8s-bot commented Oct 27, 2015

GCE e2e test build/test passed for commit a2e34a3.

@sttts sttts mentioned this pull request Nov 2, 2015
Merged
6 tasks
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2015
@k8s-bot
Copy link

k8s-bot commented Nov 2, 2015

GCE e2e test build/test passed for commit 629667141a9ca1307add6d9be6e39d1274756332.

@sttts sttts force-pushed the scheduler-refactor branch from 6296671 to 4d3c6ce Compare November 3, 2015 07:42
@k8s-bot
Copy link

k8s-bot commented Nov 3, 2015

GCE e2e test build/test passed for commit 4d3c6cea59e451f506e599a388377ea8efc7722a.

@sttts sttts force-pushed the scheduler-refactor branch 2 times, most recently from 78c4a7f to 3a292f0 Compare November 3, 2015 08:17
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2015
@sttts
Copy link
Contributor Author

sttts commented Nov 3, 2015

/cc @jdef @sur ready for review

@sttts sttts force-pushed the scheduler-refactor branch 2 times, most recently from f3e9938 to cdd94dc Compare November 3, 2015 08:25
@sttts
Copy link
Contributor Author

sttts commented Nov 3, 2015

/cc @spacejam

@k8s-bot
Copy link

k8s-bot commented Nov 3, 2015

GCE e2e test build/test passed for commit 78c4a7fca2b54e0a85aa7a82d8a1b55ea19d0ab3.

@k8s-bot
Copy link

k8s-bot commented Nov 3, 2015

GCE e2e test build/test passed for commit 3a292f0442e52aabfa41d736c1af1b782c58fd25.

@k8s-bot
Copy link

k8s-bot commented Nov 3, 2015

GCE e2e test build/test passed for commit f3e9938bce04d89139d998f02c0cab2e69f5218f.

@k8s-bot
Copy link

k8s-bot commented Nov 3, 2015

GCE e2e test build/test passed for commit cdd94dc8579c7d8be73b602a107369b7e960384c.

@k8s-bot
Copy link

k8s-bot commented Nov 3, 2015

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

s-urbaniak
s-urbaniak reviewed Nov 3, 2015
View reviewed changes
contrib/mesos/pkg/scheduler/integration/integration_test.go
@@ -832,131 +806,3 @@ func TestPlugin_LifeCycle(t *testing.T) {
time.Sleep(time.Second / 2)
failPodFromExecutor(launchedTask.taskInfo)
}

func TestDeleteOne_NonexistentPod(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Has this test became untestable due to visibility or was it moved too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved into deleter_test.go

@k8s-bot
Copy link

k8s-bot commented Nov 12, 2015

GCE e2e test build/test passed for commit e71f43d.

@sttts
Copy link
Contributor Author

sttts commented Nov 12, 2015

Submit queue seems to be blocked with this one for days, showing "Github CI tests are not green." which is false. Also the cla/google test is not triggered. Something is pretty broken here.

@sttts
Copy link
Contributor Author

sttts commented Nov 12, 2015

Merging manually as this is completely contrib/mesos related and submit queue ignores it.

sttts added a commit that referenced this pull request Nov 12, 2015
@sttts
Merge pull request #16316 from mesosphere/scheduler-refactor
1a958b0 
MESOS: Refactor scheduler
@sttts sttts merged commit 1a958b0 into kubernetes:master Nov 12, 2015
@sttts sttts deleted the scheduler-refactor branch November 12, 2015 14:28
@mikedanese
Copy link
Member

This needed a squash.

@sttts
Copy link
Contributor Author

sttts commented Nov 12, 2015

We decided against squashing intentionally. A lot of those commits are renamings and movements of code. After squashing it would be nearly impossible to trace code into the code base before the refactoring. I hope that's ok in this special situation. In other cases, I completely agree.

@sttts
Copy link
Contributor Author

sttts commented Nov 12, 2015

@mikedanese another question, I think you mentioned something some time ago: why was this PR always ignored by the merge bot, even with the e2e-not-required label? Is this always the case with contrib-only PRs? It showed "Github CI tests are not green." although everything was green. Any idea?

@mikedanese
Copy link
Member

If you go to http://submit-queue.k8s.io/ and search your username or PR number in the search box it should tell you what merge bot thinks the current status of the pr is

@sttts
Copy link
Contributor Author

sttts commented Nov 12, 2015

That's what I mean. It was staying in "Github CI tests are not green." although everything was green.

@mikedanese mikedanese mentioned this pull request Nov 12, 2015
Closed
@mikedanese
Copy link
Member

A commit message would have been a good place to trace renaming. This also broke unit tests. Squash commits in the future and allow mergebot to merge PRs.

@sttts sttts mentioned this pull request Nov 12, 2015
Closed
@sttts
Copy link
Contributor Author

sttts commented Nov 12, 2015

@mikedanese sorry for the trouble. To avoid this next time I really would like to understand:
if the submit queue thinks differently than Github's test hooks, how can I understand why? Is there still an engineer on duty I can contact somehow somewhere? Is there anything else than the #k8s-dev channel for those kind of questions?

It's not that we merged here immediately. We were waiting for this PR two days, fulfilling all criteria for a mergeable PR. The submit queue didn't pick it up or changed it's status. In fact we had the same situation before with a contrib-only PR and I think you or gmarek clicked the merge button eventually.

@mikedanese
Copy link
Member

I think it may have been rightfully holding off on merging because of unit test failures since submit-queue.k8s.io was giving a correct reading, but not sure why it wasn't surfaced in the PR status. Right now you should look for jenkins/k8s-e2e, jenkins/k8s-unit, and travis-ci to be green. @ixdy if the unit test status isn't showing up on a PR is there a way to kick jenkins? Same way as e2e tests? If you think that k8s-merge-bot is not working, file an issue in contrib/, ping @eparis or @lavalamp on the PR or in k8s-dev slack.

@lavalamp
Copy link
Member

@sttts Next time, please ask @k8s-oncall to merge things instead of merging them yourself--you don't know what the build cop might be in the middle of doing, an unexpected merge can make their life difficult (imagine they're in the process of fixing the build). Plus if merge queue is ignoring something it shouldn't, we'd like to catch it in the act.

I will also complain about the excessive number of commits here. At a minimum, things like "fix rebase error", "gofmt fixes", "fix comment", etc., should be squashed with the commits that introduced the error(s). I buy keeping separate commits to ease rebasing when doing a major rename, I have done that myself. I buy keeping separate commits when there's a possibility you might want to roll them back independently. I buy commits like in this PR to make review easier, but I think they should be squashed before merge. Please see:

I think this is a sausage and should have been squashed at least partially, maybe if it were more like ~10 commits I wouldn't be writing this. Merging it this way leaves us a lot of extra stuff to wade through when e.g. making release notes. I realize this is a bit un-git-ish of us, but it's what we've gotten consensus on.

@sttts
Copy link
Contributor Author

sttts commented Nov 12, 2015

@lavalamp thanks for the hint about @k8s-oncall. This will help in those situations.

About squashing, I totally get the argument about "wading through" for release notes and the gotten consensus in order to keep the repo manageable. We will keep that in mind next time we do something similar.

@ixdy
Copy link
Member

ixdy commented Nov 12, 2015

@mikedanese saying "@k8s-bot unit test this" will trigger unit tests only.

@ixdy
Copy link
Member

ixdy commented Nov 12, 2015

additionally, the "test this" phase actually triggers both e2e and unit tests now.

@sttts
Copy link
Contributor Author

sttts commented Nov 12, 2015

Still thinking unit tests were running: "Jenkins unit/integration 2381 tests run, 8 skipped, 0 failed.". I guess that's the jenkins/k8s-unit job in Jenkins. Unfortunately, I cannot access the logs from the link above. Can anybody confirm that the contrib unit tests are run in that job?

@ixdy
Copy link
Member

ixdy commented Nov 12, 2015

It looks like unit tests ran and passed, yeah. I think you raced with #16992, which got merged at approximately the same time; both individually passed unit tests, but after merging there was a build break.

@ixdy
Copy link
Member

ixdy commented Nov 12, 2015

If you like reading XML or overly-verbose output, you can see which tests were run at https://console.developers.google.com/storage/browser/kubernetes-jenkins/pr-logs/e71f43de930c15c78c71c454836424ddf2829d42/kubernetes-pull-test-unit-integration/1450/artifacts/. The build log is also supposed to be uploaded, but for some reason it was not (which I'm working on fixing).

The submit queue re-runs all tests before merging, so it likely would have caught this merge race.

@sttts
Copy link
Contributor Author

sttts commented Nov 12, 2015

@ixdy thanks for the investigation and for restoring my world view. The submit queue just got a big new fan. The contrib unit tests were actually running. The theory with the race makes sense.

sttts added a commit to mesosphere-backup/kubernetes that referenced this pull request Nov 16, 2015
@sttts
Merge pull request kubernetes#16316 from mesosphere/scheduler-refactor
d96b188 
MESOS: Refactor scheduler
sttts added a commit to mesosphere-backup/kubernetes that referenced this pull request Nov 30, 2015
@sttts
Merge pull request kubernetes#16316 from mesosphere/scheduler-refactor
8b4351d 
MESOS: Refactor scheduler
sttts added a commit to mesosphere-backup/kubernetes that referenced this pull request Dec 10, 2015
@sttts
Merge pull request kubernetes#16316 from mesosphere/scheduler-refactor
8d6ffc4 
MESOS: Refactor scheduler
jdef pushed a commit to mesosphere-backup/kubernetes that referenced this pull request Jan 25, 2016
@sttts
Merge pull request kubernetes#16316 from mesosphere/scheduler-refactor
b0c9dcd 
MESOS: Refactor scheduler
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@sttts sttts

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.
10 participants
@sttts @k8s-github-robot @k8s-bot @s-urbaniak @jdef @mikedanese @lavalamp @ixdy @k8s-teamcity-mesosphere @googlebot