update comment with EnqueueExtensions #103686

kerthcet · 2021-07-14T11:42:38Z

Signed-off-by: kerthcet kerthcet@gmail.com

What type of PR is this?

/sig scheduling
/kind documentation

What this PR does / why we need it:

add comment for EnqueueExtensions

Which issue(s) this PR refs:

Refs #103633

Special notes for your reviewer:

New to community, hope to hear as much advices as better.

Does this PR introduce a user-facing change?

NONE

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

k8s-ci-robot · 2021-07-14T11:42:45Z

@kerthcet: 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.

k8s-ci-robot · 2021-07-14T11:42:46Z

Hi @kerthcet. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

pacoxu · 2021-07-14T11:51:56Z

/ok-to-test

kerthcet · 2021-07-14T14:00:04Z

/retest

Huang-Wei · 2021-07-14T16:57:14Z

pkg/scheduler/framework/interface.go

+// move unschedulable Pods in internal scheduling queues. It's recommended for
+// plugins may impacting scheduling, other implemented plugins are useless and
+// harmless.


The statement "It's recommended..." doesn't seem to provide extra information. Correct me if you intend to emphasize something here. If no, I'd suggest removing it.

Huang-Wei · 2021-07-14T16:59:41Z

pkg/scheduler/framework/interface.go

+// When setting up a new scheduler, it will load events registered by
+// EventsToRegister(), and convert them to gvks to build dynamic event handlers
+// with.


It's redundant with the comments of EventsToRegister(). Prefer to remove it.

Huang-Wei · 2021-07-14T17:00:34Z

pkg/scheduler/framework/interface.go

-	// EventsToRegister returns a series of interested events that
-	// will be registered when instantiating the internal scheduling queue.
+	// EventsToRegister returns a series of possible events that may cause a Pod
+	// scheduling failed by this plugin.


Suggested change

// scheduling failed by this plugin.

// failed by this plugin schedulable.

Huang-Wei · 2021-07-14T17:02:05Z

pkg/scheduler/framework/interface.go

+	// EventsToRegister() is called to fill ClusterEvents to frameworkOptions's
+	// clusterEventMap, which will be used to build event handlers.


// The events will be registered when instantiating the internal scheduling queue, // and leveraged to build event handlers dynamically.

Huang-Wei · 2021-07-14T17:02:35Z

/milestone v1.23

kerthcet · 2021-07-15T03:24:12Z

hey @Huang-Wei , thanks for your advices. would you mind to review the updated comments again.

chendave · 2021-07-15T06:05:43Z

pkg/scheduler/framework/interface.go

+// impacting scheduling will take effect, other implemented plugins are useless and
+// harmless.
+// When setting up a new scheduler, it will load events and convert them to gvks
+// to build dynamic event handlers with.


Suggested change

// to build dynamic event handlers with.

// to dynamically build event handlers.

or simply remove this line, as it is a redundant information.

chendave · 2021-07-15T06:05:51Z

pkg/scheduler/framework/interface.go

+// impacting scheduling will take effect, other implemented plugins are useless and
+// harmless.


not quite clear to me, do we have any plugin doesn't impact scheduling at all? also, I cannot understand this statement

other implemented plugins are useless and harmless.

sorry about the ambiguous words，I updated my comments right now. 🥲

kerthcet · 2021-07-15T15:06:49Z

/retest

Huang-Wei

Some nits. LGTM otherwise.

Huang-Wei · 2021-07-15T18:17:55Z

pkg/scheduler/framework/interface.go

@@ -307,10 +307,14 @@ type QueueSortPlugin interface {
 }

 // EnqueueExtensions is an optional interface that plugins can implement to efficiently
-// move unschedulable Pods in internal scheduling queues.
+// move unschedulable Pods in internal scheduling queues. Plugins
+// who will fail pod scheduling should implement this interface.


Suggested change

// who will fail pod scheduling should implement this interface.

// that fail pod scheduling (e.g., Filter plugins) are expected to implement this interface.

Huang-Wei · 2021-07-15T18:18:25Z

pkg/scheduler/framework/interface.go

-// move unschedulable Pods in internal scheduling queues.
+// move unschedulable Pods in internal scheduling queues. Plugins
+// who will fail pod scheduling should implement this interface.
+// With registered events, scheduler can build dynamically event handlers.


It's described in L316~L317, so let's remove it.

yep, it's duplicated👍

Signed-off-by: kerthcet <kerthcet@gmail.com> update comment with EnqueueExtensions Signed-off-by: kerthcet <kerthcet@gmail.com> update comment with EnqueueExtensions Signed-off-by: kerthcet <kerthcet@gmail.com> update comment with EnqueueExtensions Signed-off-by: kerthcet <kerthcet@gmail.com>

kerthcet · 2021-07-16T02:08:08Z

Updated the comments as advised. Thanks for all your patience @Huang-Wei @chendave , it's my first pr, twist but happy. Looking forward to contributing more codes. 💪

Huang-Wei · 2021-07-16T21:18:23Z

/lgtm
/approve

k8s-ci-robot · 2021-07-16T21:18:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Huang-Wei, kerthcet

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:

~~pkg/scheduler/OWNERS~~ [Huang-Wei]

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

kerthcet · 2021-07-20T01:23:09Z

/retest

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jul 14, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 14, 2021

k8s-ci-robot requested review from ahg-g and chendave July 14, 2021 11:43

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 14, 2021

kerthcet marked this pull request as ready for review July 14, 2021 15:17

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 14, 2021

Huang-Wei reviewed Jul 14, 2021

View reviewed changes

k8s-ci-robot added this to the v1.23 milestone Jul 14, 2021

kerthcet force-pushed the document/add_comment_for_enqueueExtensions branch from f420751 to 22f31b1 Compare July 15, 2021 02:06

chendave reviewed Jul 15, 2021

View reviewed changes

kerthcet force-pushed the document/add_comment_for_enqueueExtensions branch from 22f31b1 to 6f7594e Compare July 15, 2021 14:19

Huang-Wei reviewed Jul 15, 2021

View reviewed changes

kerthcet force-pushed the document/add_comment_for_enqueueExtensions branch from 6f7594e to 1f87118 Compare July 16, 2021 01:44

k8s-ci-robot removed the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jul 16, 2021

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 16, 2021

kerthcet force-pushed the document/add_comment_for_enqueueExtensions branch from 1f87118 to d1e9da9 Compare July 16, 2021 01:56

k8s-ci-robot assigned Huang-Wei Jul 16, 2021

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

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

k8s-ci-robot merged commit b16f7e8 into kubernetes:master Aug 5, 2021

kerthcet deleted the document/add_comment_for_enqueueExtensions branch August 5, 2021 06:17

kerthcet mentioned this pull request Aug 6, 2021

REQUEST: New membership for kerthcet kubernetes/org#2877

Closed

7 tasks

Huang-Wei mentioned this pull request Aug 18, 2021

scheduler: Make it clear that EnqueueExtensions has the role of dynamic event handlers registration #103633

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update comment with EnqueueExtensions #103686

update comment with EnqueueExtensions #103686

kerthcet commented Jul 14, 2021

k8s-ci-robot commented Jul 14, 2021

k8s-ci-robot commented Jul 14, 2021

pacoxu commented Jul 14, 2021

kerthcet commented Jul 14, 2021

Huang-Wei Jul 14, 2021

Huang-Wei Jul 14, 2021

Huang-Wei Jul 14, 2021

Huang-Wei Jul 14, 2021

Huang-Wei commented Jul 14, 2021

kerthcet commented Jul 15, 2021

chendave Jul 15, 2021

chendave Jul 15, 2021

kerthcet Jul 15, 2021

kerthcet commented Jul 15, 2021

Huang-Wei left a comment

Huang-Wei Jul 15, 2021

Huang-Wei Jul 15, 2021

kerthcet Jul 16, 2021

kerthcet commented Jul 16, 2021

Huang-Wei commented Jul 16, 2021

k8s-ci-robot commented Jul 16, 2021

kerthcet commented Jul 20, 2021

	// scheduling failed by this plugin.
	// failed by this plugin schedulable.

		// EventsToRegister() is called to fill ClusterEvents to frameworkOptions's
		// clusterEventMap, which will be used to build event handlers.

	// to build dynamic event handlers with.
	// to dynamically build event handlers.

		// impacting scheduling will take effect, other implemented plugins are useless and
		// harmless.

	// who will fail pod scheduling should implement this interface.
	// that fail pod scheduling (e.g., Filter plugins) are expected to implement this interface.

update comment with EnqueueExtensions #103686

update comment with EnqueueExtensions #103686

Conversation

kerthcet commented Jul 14, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR refs:

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 Jul 14, 2021

k8s-ci-robot commented Jul 14, 2021

pacoxu commented Jul 14, 2021

kerthcet commented Jul 14, 2021

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

Huang-Wei commented Jul 14, 2021

kerthcet commented Jul 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kerthcet commented Jul 15, 2021

Huang-Wei left a 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

kerthcet commented Jul 16, 2021

Huang-Wei commented Jul 16, 2021

k8s-ci-robot commented Jul 16, 2021

kerthcet commented Jul 20, 2021