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

run scheduler by wiring up to a command #16015

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 0 additions & 47 deletions pkg/cmd/server/kubernetes/master/controller/config.go
Original file line number Diff line number Diff line change
@@ -1,19 +1,9 @@
package controller

import (
"fmt"
"io/ioutil"
"os"

"k8s.io/apimachinery/pkg/runtime"
kerrors "k8s.io/apimachinery/pkg/util/errors"
kubecontroller "k8s.io/kubernetes/cmd/kube-controller-manager/app"
scheduleroptions "k8s.io/kubernetes/plugin/cmd/kube-scheduler/app/options"
schedulerapi "k8s.io/kubernetes/plugin/pkg/scheduler/api"
latestschedulerapi "k8s.io/kubernetes/plugin/pkg/scheduler/api/latest"

configapi "github.com/openshift/origin/pkg/cmd/server/api"
cmdflags "github.com/openshift/origin/pkg/cmd/util/flags"
"github.com/openshift/origin/pkg/cmd/util/variable"
"k8s.io/kubernetes/pkg/volume"
)
Expand All @@ -22,9 +12,6 @@ import (
// launch the set of kube (not openshift) controllers.
type KubeControllerConfig struct {
HorizontalPodAutoscalerControllerConfig HorizontalPodAutoscalerControllerConfig

// TODO the scheduler should move out into its own logical component
SchedulerControllerConfig SchedulerControllerConfig
}

// GetControllerInitializers return the controller initializer functions for kube controllers
Expand All @@ -36,48 +23,14 @@ func (c KubeControllerConfig) GetControllerInitializers() (map[string]kubecontro
// in openshift-infra, and pass it a scale client that knows how to scale DCs
ret["horizontalpodautoscaling"] = c.HorizontalPodAutoscalerControllerConfig.RunController

// FIXME: Move this under openshift controller intialization once we figure out
// deployment (options).
ret["openshift.io/scheduler"] = c.SchedulerControllerConfig.RunController

return ret, nil
}

// BuildKubeControllerConfig builds the struct to create the controller initializers. Eventually we want this to be fully
// stock kube with no modification.
func BuildKubeControllerConfig(options configapi.MasterConfig) (*KubeControllerConfig, error) {
var err error
ret := &KubeControllerConfig{}

kubeExternal, _, err := configapi.GetExternalKubeClient(options.MasterClients.OpenShiftLoopbackKubeConfig, options.MasterClients.OpenShiftLoopbackClientConnectionOverrides)
Copy link
Contributor

@aveshagarwal aveshagarwal Aug 30, 2017

Choose a reason for hiding this comment

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

I dont see we are making sure now that connection overrides still apply, since now the client will be created in scheduler itself, are we ok with not having this?

// applyClientConnectionOverrides updates a kubeConfig with the overrides from the config.
func applyClientConnectionOverrides(overrides *ClientConnectionOverrides, kubeConfig *restclient.Config) {
        if overrides == nil {
                return
        }
        kubeConfig.QPS = overrides.QPS
        kubeConfig.Burst = int(overrides.Burst)
        kubeConfig.ContentConfig.AcceptContentTypes = overrides.AcceptContentTypes
        kubeConfig.ContentConfig.ContentType = overrides.ContentType
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dont see we are making sure now that connection overrides still apply, since now the client will be created in scheduler itself, are we ok with this?

Yes. The path forward will be to configure the differences we need in the scheduler CLI. If the scheduler doesn't support it feature we need, we'll be pursuing them upstream and perhaps picking them back.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thats ok I agree with this. I am concerned if someone supplied QPS/Burst/ContentType in openshift, and doesn't realize that these values wont be passed to scheduler any more? wouldn't it create regressions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats ok I agree with this. I am concerned if someone supplied QPS/Burst/ContentType in openshift, and doesn't realize that these values wont be passed to scheduler any more? wouldn't it create regressions?

Are the defaults bad? If the defaults upstream are good, I'm probably willing to crack some eggs. If they're bad, we do some shenanigans to override. The overrides in openshift are pretty coarse grained and targeted at loopback (which we'll probably be disabling) and controllers (which we have to keep).

Just remember that everything we plumb through special is another thing you'll be describing to ansible to special-case during the (hopefully) 3.8 migration.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not really sure upstream defaults are good or bad, lets say they are good. But I agree with your ansible thing.

But It is getting a bit confusing now as there are 4 ways to set QPS/Burst etc

  1. Via kubeconfig
  2. Via overrides
  3. Via scheduler's defaults (https://github.com/openshift/origin/blob/master/vendor/k8s.io/kubernetes/pkg/apis/componentconfig/v1alpha1/defaults.go#L139)
  4. Via scheduler server's flags

Lets say overrides are not in picture any more, I think I still see issue in createClient function: https://github.com/openshift/origin/blob/master/vendor/k8s.io/kubernetes/plugin/cmd/kube-scheduler/app/configurator.go#L57

Even if a user does not set any scheduler flags for QPS/burst etc, QPS/Burst kubeconfig values are being overwritten by defaults, as there is no way to differentiate if these were set by user or are defaults.

I think we will have to decide if we are ok with kubeconfig supplied values being overwritten by defaults. I think this issue is something I will have to look into kube upstream too.

Though, one thing I am wondering, I am not sure, in practice what is preferred way of configuration for these QPS/Burst, via kubeconfig, or via scheduler's flags etc.

if err != nil {
return nil, err
}

var schedulerPolicy *schedulerapi.Policy
if _, err := os.Stat(options.KubernetesMasterConfig.SchedulerConfigFile); err == nil {
schedulerPolicy = &schedulerapi.Policy{}
configData, err := ioutil.ReadFile(options.KubernetesMasterConfig.SchedulerConfigFile)
if err != nil {
return nil, fmt.Errorf("unable to read scheduler config: %v", err)
}
if err := runtime.DecodeInto(latestschedulerapi.Codec, configData, schedulerPolicy); err != nil {
return nil, fmt.Errorf("invalid scheduler configuration: %v", err)
}
}
// resolve extended arguments
// TODO: this should be done in config validation (along with the above) so we can provide
// proper errors
schedulerserver := scheduleroptions.NewSchedulerServer()
schedulerserver.PolicyConfigFile = options.KubernetesMasterConfig.SchedulerConfigFile
if err := cmdflags.Resolve(options.KubernetesMasterConfig.SchedulerArguments, schedulerserver.AddFlags); len(err) > 0 {
return nil, kerrors.NewAggregate(err)
}
ret.SchedulerControllerConfig = SchedulerControllerConfig{
PrivilegedClient: kubeExternal,
SchedulerServer: schedulerserver,
}

imageTemplate := variable.NewDefaultImageTemplate()
imageTemplate.Format = options.ImageConfig.Format
imageTemplate.Latest = options.ImageConfig.Latest
Expand Down
47 changes: 0 additions & 47 deletions pkg/cmd/server/kubernetes/master/controller/core.go

This file was deleted.

3 changes: 3 additions & 0 deletions pkg/cmd/server/start/start_master.go
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,9 @@ func (m *Master) Start() error {
go func() {
controllerPlug.WaitForStart()

// continuously run the scheduler while we have the primary lease
go runEmbeddedScheduler(m.config.MasterClients.OpenShiftLoopbackKubeConfig, m.config.KubernetesMasterConfig.SchedulerConfigFile, m.config.KubernetesMasterConfig.SchedulerArguments)

controllerContext, err := getControllerContext(*m.config, kubeControllerManagerConfig, cloudProvider, informers, utilwait.NeverStop)
if err != nil {
glog.Fatal(err)
Expand Down
53 changes: 53 additions & 0 deletions pkg/cmd/server/start/start_scheduler.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
package start

import (
"github.com/golang/glog"

kerrors "k8s.io/apimachinery/pkg/util/errors"
schedulerapp "k8s.io/kubernetes/plugin/cmd/kube-scheduler/app"
scheduleroptions "k8s.io/kubernetes/plugin/cmd/kube-scheduler/app/options"
_ "k8s.io/kubernetes/plugin/pkg/scheduler/algorithmprovider"

cmdflags "github.com/openshift/origin/pkg/cmd/util/flags"
)

func newScheduler(kubeconfigFile, schedulerConfigFile string, cmdLineArgs map[string][]string) (*scheduleroptions.SchedulerServer, error) {
if cmdLineArgs == nil {
cmdLineArgs = map[string][]string{}
}
if len(cmdLineArgs["kubeconfig"]) == 0 {
cmdLineArgs["kubeconfig"] = []string{kubeconfigFile}
}
if len(cmdLineArgs["policy-config-file"]) == 0 {
cmdLineArgs["policy-config-file"] = []string{schedulerConfigFile}
}
// disable serving http since we didn't used to expose it
if len(cmdLineArgs["port"]) == 0 {
cmdLineArgs["port"] = []string{"-1"}
}

// resolve arguments
schedulerServer := scheduleroptions.NewSchedulerServer()
if err := cmdflags.Resolve(cmdLineArgs, schedulerServer.AddFlags); len(err) > 0 {
return nil, kerrors.NewAggregate(err)
}

return schedulerServer, nil
}

func runEmbeddedScheduler(kubeconfigFile, schedulerConfigFile string, cmdLineArgs map[string][]string) {
for {
// TODO we need a real identity for this. Right now it's just using the loopback connection like it used to.
scheduler, err := newScheduler(kubeconfigFile, schedulerConfigFile, cmdLineArgs)
if err != nil {
glog.Error(err)
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems to me that if there is any parsing error or anything, it will keep filling the logs. How continuing here is helpful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems to me that if there is any parsing error or anything, it will keep filling the logs. How continuing here is helpful?

It keeps the entire process (including the apiserver) from dying.

}
// this does a second leader election, but doing the second leader election will allow us to move out process in
// 3.8 if we so choose.
if err := schedulerapp.Run(scheduler); err != nil {
glog.Error(err)
continue
}
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

45 changes: 27 additions & 18 deletions vendor/k8s.io/kubernetes/plugin/cmd/kube-scheduler/app/server.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.