Add a deprecated cmd flag for the time interval between flushing pods from unschedualbeQ to activeQ or backoffQ. #108017

denkensk · 2022-02-09T03:23:26Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

#87850 (comment)

Which issue(s) this PR fixes:

The first part of #87850

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Add a deprecated cmd flag for the time interval between flushing pods from unschedualbeQ to activeQ or backoffQ.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

k8s-ci-robot · 2022-02-09T03:23:33Z

@denkensk: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

kerthcet · 2022-02-09T08:43:56Z

cmd/kube-scheduler/app/config/config.go

+	// unschedulableQ. If a pod stays in unschedulableQ for longer than this
+	// value, the pod will be moved from unschedulableQ to backoffQ or activeQ.
+	// If this value is empty, the default value (60s) will be used.
+	PodMaxUnscheduleQDuration metav1.Duration


why not move this to ComponentConfig? There is a similar field there

kubernetes/pkg/scheduler/apis/config/types.go

Lines 85 to 88 in 2047936

// PodMaxBackoffSeconds is the max backoff for unschedulable pods.

// If specified, it must be greater than or equal to podInitialBackoffSeconds. If this value is null,

// the default value (10s) will be used.

PodMaxBackoffSeconds int64

It is a command line flag with a deprecated notice from the beginning. It will be deleted in the future. So I want to keep it independent of CC.

kerthcet · 2022-02-09T09:00:29Z

pkg/scheduler/internal/queue/scheduling_queue.go

@@ -170,6 +175,7 @@ type priorityQueueOptions struct {
 	clock                     util.Clock
 	podInitialBackoffDuration time.Duration
 	podMaxBackoffDuration     time.Duration
+	podMaxUnscheduleQDuration time.Duration


I think we should rename this to podMaxUnschedulableDuration, to keep the consistent style as above.

Done podMaxUnscheduleQDuration --> podMaxUnschedulableQDuration

kerthcet · 2022-02-09T09:16:56Z

cmd/kube-scheduler/app/options/options.go

@@ -297,6 +297,9 @@ func (o *Options) Config() (*schedulerappconfig.Config, error) {
 	dynClient := dynamic.NewForConfigOrDie(kubeConfig)
 	c.DynInformerFactory = dynamicinformer.NewFilteredDynamicSharedInformerFactory(dynClient, 0, corev1.NamespaceAll, nil)
 	c.LeaderElection = leaderElectionConfig
+	if o.Deprecated != nil {


Shouldn't we move this to here, as --config not specified.

kubernetes/cmd/kube-scheduler/app/options/options.go

Lines 110 to 141 in 2047936

// ApplyDeprecated obtains the deprecated CLI args and set them to `o.ComponentConfig` if specified.

func (o *Options) ApplyDeprecated() {

if o.Flags == nil {

return

}

// Obtain deprecated CLI args. Set them to cfg if specified in command line.

deprecated := o.Flags.FlagSet("deprecated")

if deprecated.Changed("profiling") {

o.ComponentConfig.EnableProfiling = o.Deprecated.EnableProfiling

}

if deprecated.Changed("contention-profiling") {

o.ComponentConfig.EnableContentionProfiling = o.Deprecated.EnableContentionProfiling

}

if deprecated.Changed("kubeconfig") {

o.ComponentConfig.ClientConnection.Kubeconfig = o.Deprecated.Kubeconfig

}

if deprecated.Changed("kube-api-content-type") {

o.ComponentConfig.ClientConnection.ContentType = o.Deprecated.ContentType

}

if deprecated.Changed("kube-api-qps") {

o.ComponentConfig.ClientConnection.QPS = o.Deprecated.QPS

}

if deprecated.Changed("kube-api-burst") {

o.ComponentConfig.ClientConnection.Burst = o.Deprecated.Burst

}

if deprecated.Changed("lock-object-namespace") {

o.ComponentConfig.LeaderElection.ResourceNamespace = o.Deprecated.ResourceNamespace

}

if deprecated.Changed("lock-object-name") {

o.ComponentConfig.LeaderElection.ResourceName = o.Deprecated.ResourceName

}

}

It is just used to set to CC。

kerthcet · 2022-02-09T09:17:15Z

Hi @denkensk , I commented for several places, but I'm not sure whether I understood your thoughts correctly, so if not, please correct me. Thanks a lot.

denkensk · 2022-02-09T14:33:19Z

/retest-required

denkensk · 2022-02-10T03:07:27Z

/assign @alculquicondor

alculquicondor · 2022-02-10T21:32:07Z

cmd/kube-scheduler/app/options/deprecated.go

+	// unschedulableQ. If a pod stays in unschedulableQ for longer than this
+	// value, the pod will be moved from unschedulableQ to backoffQ or activeQ.
+	// If this value is empty, the default value (60s) will be used.
+	PodMaxUnschedulableQDuration metav1.Duration


you can just use time.Duration, because this is not an API.

alculquicondor · 2022-02-10T21:34:34Z

cmd/kube-scheduler/app/options/deprecated.go

@@ -44,4 +52,5 @@ func (o *DeprecatedOptions) AddFlags(fs *pflag.FlagSet) {
 	fs.Int32Var(&o.Burst, "kube-api-burst", 100, "DEPRECATED: burst to use while talking with kubernetes apiserver. This parameter is ignored if a config file is specified in --config.")
 	fs.StringVar(&o.ResourceNamespace, "lock-object-namespace", "kube-system", "DEPRECATED: define the namespace of the lock object. Will be removed in favor of leader-elect-resource-namespace. This parameter is ignored if a config file is specified in --config.")
 	fs.StringVar(&o.ResourceName, "lock-object-name", "kube-scheduler", "DEPRECATED: define the name of the lock object. Will be removed in favor of leader-elect-resource-name. This parameter is ignored if a config file is specified in --config.")
+	fs.DurationVar(&o.PodMaxUnschedulableQDuration.Duration, "pod-max-unschedulableq-duration", 60*time.Second, "DEPRECATED: the maximum time a pod can stay in unschedulableQ. If a pod stays in unschedulableQ for longer than this value, the pod will be moved from unschedulableQ to backoffQ or activeQ. will be removed once the mechanism for flushing pods from unschedulableQ to backoffQ or activeQ has been removed.")


Let's put an estimate: Will be removed in 1.26.

Avoid talking about internal fields in the user documentation.

alculquicondor · 2022-02-10T21:36:26Z

cmd/kube-scheduler/app/options/options.go

@@ -231,6 +233,11 @@ func (o *Options) ApplyTo(c *schedulerappconfig.Config) error {
 		}
 	}
 	o.Metrics.Apply()
+
+	if o.Deprecated != nil {


Add a comment about why this is not part of ApplyDeprecated.

Presumably because this can't be configured via ComponentConfig.

pkg/scheduler/factory.go

alculquicondor · 2022-02-10T21:39:03Z

pkg/scheduler/internal/queue/scheduling_queue.go

+	// for longer than this value, the pod will be moved from unschedulableQ to
+	// backoffQ or activeQ. If this value is empty, the default value (60s)
+	// will be used.
+	DefaultPodMaxUnschedulableQDuration time.Duration = 60 * time.Second


We agreed to increase the default. Are you doing that in a separate PR?

Yes. It is planed to be updated in a separate PR.

pkg/scheduler/internal/queue/scheduling_queue_test.go

pkg/scheduler/scheduler.go

denkensk · 2022-02-11T11:06:16Z

/test pull-kubernetes-e2e-kind

alculquicondor

You can squash

cmd/kube-scheduler/app/options/deprecated.go

denkensk · 2022-02-16T03:07:38Z

You can squash

Thanks. Squashed. @alculquicondor

alculquicondor · 2022-02-16T14:53:28Z

/approve
/lgtm

k8s-ci-robot · 2022-02-16T14:54:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, denkensk

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~cmd/kube-scheduler/OWNERS~~ [alculquicondor]
~~pkg/scheduler/OWNERS~~ [alculquicondor]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Feb 9, 2022

k8s-ci-robot requested review from alculquicondor and damemi February 9, 2022 03:24

k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Feb 9, 2022

kerthcet reviewed Feb 9, 2022

View reviewed changes

denkensk force-pushed the add-flush-flag branch from 2b67d81 to ab7d3dc Compare February 9, 2022 10:56

k8s-ci-robot assigned alculquicondor Feb 10, 2022

alculquicondor reviewed Feb 10, 2022

View reviewed changes

kerthcet reviewed Feb 11, 2022

View reviewed changes

pkg/scheduler/scheduler.go Outdated Show resolved Hide resolved

denkensk requested a review from alculquicondor February 15, 2022 03:49

alculquicondor reviewed Feb 15, 2022

View reviewed changes

cmd/kube-scheduler/app/options/deprecated.go Outdated Show resolved Hide resolved

add deprecated flag for flush pods to activeq interval

8754920

denkensk force-pushed the add-flush-flag branch from 572a3d7 to 8754920 Compare February 16, 2022 03:06

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 16, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 16, 2022

k8s-ci-robot merged commit f538b0b into kubernetes:master Feb 16, 2022

k8s-ci-robot added this to the v1.24 milestone Feb 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a deprecated cmd flag for the time interval between flushing pods from unschedualbeQ to activeQ or backoffQ. #108017

Add a deprecated cmd flag for the time interval between flushing pods from unschedualbeQ to activeQ or backoffQ. #108017

denkensk commented Feb 9, 2022

k8s-ci-robot commented Feb 9, 2022

kerthcet Feb 9, 2022

denkensk Feb 9, 2022

kerthcet Feb 11, 2022

kerthcet Feb 9, 2022

denkensk Feb 9, 2022 •

edited

kerthcet Feb 9, 2022

denkensk Feb 9, 2022

kerthcet commented Feb 9, 2022

denkensk commented Feb 9, 2022

denkensk commented Feb 10, 2022

alculquicondor Feb 10, 2022

denkensk Feb 11, 2022

alculquicondor Feb 10, 2022

denkensk Feb 11, 2022

alculquicondor Feb 10, 2022

alculquicondor Feb 10, 2022

denkensk Feb 11, 2022

denkensk commented Feb 11, 2022

alculquicondor left a comment

denkensk commented Feb 16, 2022

alculquicondor commented Feb 16, 2022

k8s-ci-robot commented Feb 16, 2022

	// PodMaxBackoffSeconds is the max backoff for unschedulable pods.
	// If specified, it must be greater than or equal to podInitialBackoffSeconds. If this value is null,
	// the default value (10s) will be used.
	PodMaxBackoffSeconds int64

	// ApplyDeprecated obtains the deprecated CLI args and set them to `o.ComponentConfig` if specified.
	func (o *Options) ApplyDeprecated() {
	if o.Flags == nil {
	return
	}
	// Obtain deprecated CLI args. Set them to cfg if specified in command line.
	deprecated := o.Flags.FlagSet("deprecated")
	if deprecated.Changed("profiling") {
	o.ComponentConfig.EnableProfiling = o.Deprecated.EnableProfiling
	}
	if deprecated.Changed("contention-profiling") {
	o.ComponentConfig.EnableContentionProfiling = o.Deprecated.EnableContentionProfiling
	}
	if deprecated.Changed("kubeconfig") {
	o.ComponentConfig.ClientConnection.Kubeconfig = o.Deprecated.Kubeconfig
	}
	if deprecated.Changed("kube-api-content-type") {
	o.ComponentConfig.ClientConnection.ContentType = o.Deprecated.ContentType
	}
	if deprecated.Changed("kube-api-qps") {
	o.ComponentConfig.ClientConnection.QPS = o.Deprecated.QPS
	}
	if deprecated.Changed("kube-api-burst") {
	o.ComponentConfig.ClientConnection.Burst = o.Deprecated.Burst
	}
	if deprecated.Changed("lock-object-namespace") {
	o.ComponentConfig.LeaderElection.ResourceNamespace = o.Deprecated.ResourceNamespace
	}
	if deprecated.Changed("lock-object-name") {
	o.ComponentConfig.LeaderElection.ResourceName = o.Deprecated.ResourceName
	}
	}

Add a deprecated cmd flag for the time interval between flushing pods from unschedualbeQ to activeQ or backoffQ. #108017

Add a deprecated cmd flag for the time interval between flushing pods from unschedualbeQ to activeQ or backoffQ. #108017

Conversation

denkensk commented Feb 9, 2022

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Feb 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denkensk Feb 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerthcet commented Feb 9, 2022

denkensk commented Feb 9, 2022

denkensk commented Feb 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denkensk commented Feb 11, 2022

alculquicondor left a comment

Choose a reason for hiding this comment

denkensk commented Feb 16, 2022

alculquicondor commented Feb 16, 2022

k8s-ci-robot commented Feb 16, 2022

denkensk Feb 9, 2022 •

edited