Support SharedLister for NodeInfo #115

sanposhiho · 2024-08-22T05:10:17Z

What type of PR is this?

/kind feature
/kind cleanup

What this PR does / why we need it:

Improve how the guest caches nodes for a better efficiency.
- And, wire SharedLister().NodeInfos() and cached Nodes.
Support SharedLister for NodeInfo.
- Only the basic node's data and ImageStates is supported for now. We can support other fields later.
Add ImageLocality plugin, ported from the in-tree plugin.

Which issue(s) this PR fixes:

Fixes #70
Part of #73

Special notes for your reviewer:

Does this PR introduce a user-facing change?

SharedLister for NodeInfo is supported.

What are the benchmark results of this change?

                                       │  before.txt  │              after.txt               │
                                       │    sec/op    │    sec/op      vs base               │
Example_NodeNumber/Simple/New-12         563.2m ±  2%    584.6m ±  3%   +3.82% (p=0.015 n=6)
Example_NodeNumber/Simple/Run-12         93.41µ ± 26%   108.65µ ± 12%        ~ (p=0.093 n=6)
Example_NodeNumber/Simple_Log/New-12     552.2m ±  2%    614.9m ± 16%  +11.35% (p=0.002 n=6)
Example_NodeNumber/Simple_Log/Run-12     109.4µ ±  8%    108.9µ ± 28%        ~ (p=0.485 n=6)
Example_NodeNumber/Advanced/New-12       633.0m ±  3%    660.1m ±  3%   +4.28% (p=0.002 n=6)
Example_NodeNumber/Advanced/Run-12       37.80µ ± 25%    38.30µ ±  4%        ~ (p=0.589 n=6)
Example_NodeNumber/Advanced_Log/New-12   649.9m ± 12%    648.1m ±  5%        ~ (p=0.937 n=6)
Example_NodeNumber/Advanced_Log/Run-12   42.80µ ±  3%    44.93µ ± 10%   +4.98% (p=0.002 n=6)
geomean                                  6.175m          6.486m         +5.03%

sanposhiho · 2024-09-08T08:26:04Z

/cc @Gekko0114 @utam0k

Gekko0114

Thanks!
Though I haven't finished reading all the code, I left several minor comments and questions :)

examples/imagelocality/image_locality.go

Gekko0114 · 2024-09-14T13:57:50Z

guest/api/framework.go

+package api
+
+// ImageStateSummary provides summarized information about the state of an image.
+type ImageStateSummary struct {


minor comment:
Nodes field is not necessary for now?
How about leaving the comment why this field is not implemented?

Nodes field is not necessary for now?

Yes, we don't use it (actually nothing uses it in upstream too).

How about leaving the comment why this field is not implemented?

I don't want to leave comments on everything unsupported. (We have too many unsupported things.

guest/api/framework.go

Gekko0114 · 2024-09-14T14:15:13Z

guest/handle/sharedlister/api/sharedlister.go

+)
+
+type SharedLister interface {
+ NodeInfos() guestapi.NodeInfoList


minor:
How about adding the comment that StorageInfos are not implemented yet?

Ditto, I don't want to leave comments on everything unsupported.

Gekko0114 · 2024-09-14T14:40:29Z

guest/internal/prefilter/prefilter_exports.go

@@ -49,6 +49,8 @@ func _prefilter() uint32 { //nolint
 // This function begins a new scheduling cycle: zero out any cycle state.
 currentPod = nil
 currentCycleState = map[string]any{}
+ currentNodeInfoList = nil
+ isFullNodeInfoList = false


Why does this isFullNodeInfoList logic work? Is it because the Node used in the scheduler's step always goes through the prefilter process?

Yes, we call preFilter regardless of whether an user implements preFilter or not, so that we can reset all stuff the start of the scheduling cycle.

Gekko0114 · 2024-09-14T14:49:20Z

guest/prescore/prescore.go

-type nodeList struct {
- items []proto.Node
+type filteredNodeInfoList struct {
+ // It only contains the Nodes that passed the filtering phase.


Why do you define filteredNodeInfoList even though sharedLister has nodeinfos? Is this because wasm's prescore step is sometimes called without calling previous steps like prefilter?

Because, as documented at this line, we have to know which nodes have gone thru the filtering phase.

As you can see in filteredNodeInfoList.lazyItems, actually filteredNodeInfoList utilizes nodes in sharedLister too to use the cached nodes there.
Also, we don't want to put this "listing nodes after filtering" logic into sharedLister because we want to keep sharedLister pure, and "listing nodes that have gone thru the filtering phase" is specific to PreScore.

sanposhiho · 2024-09-15T03:56:36Z

@Gekko0114 Addressed or replied to your comments.

Gekko0114

LGTM, but leaves lgtm label for another reviewer.
Also left several minor comments.

examples/imagelocality/image_locality.go

Gekko0114 · 2024-09-15T05:01:15Z

scheduler/plugin/host.go

+ bufLimit := bufLimit(stack[3])
+
+ var nodeName string
+ if b, ok := mod.Memory().Read(nodename, nodenameLen); !ok {


Sorry, but at first, I couldn't understand this code.
This code retrieves the node name from the wasm module and returns nodeinfo to the wasm module, right?
Additionally, I couldn't understand why this function is necessary even though k8sSchedulerCurrentNodeNameFn exists. (I understand it now)

Could you leave a brief comment explaining this function?

Added the comment though, what's unclear? (Does 2f5a3eb clarify it?)

k8s-ci-robot · 2024-09-15T05:31:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Gekko0114, sanposhiho

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:

~~OWNERS~~ [sanposhiho]

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

sanposhiho · 2024-09-19T06:02:49Z

Merging the PR.

@Gekko0114 (fyi @utam0k )
This repo is under development anyway and doesn't need to be conservative. So, let's not wait for other people's approval like k/k.
I mean not only PRs from me, but also ones from anyone. I know people besides me cannot /approve though, I'll basically just put the approval for PRs that either of you put the stamp.

k8s-ci-robot requested a review from codefromthecrypt August 22, 2024 05:10

sanposhiho marked this pull request as draft August 22, 2024 05:13

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 22, 2024

sanposhiho force-pushed the sharedsnap branch from 9c6fead to 4b8da14 Compare August 23, 2024 02:03

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 23, 2024

sanposhiho force-pushed the sharedsnap branch from 4b8da14 to f892873 Compare September 8, 2024 07:31

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 8, 2024

sanposhiho force-pushed the sharedsnap branch from f892873 to f64396c Compare September 8, 2024 08:22

feat: Support SharedLister for NodeInfo

29fab6a

sanposhiho force-pushed the sharedsnap branch from f64396c to 29fab6a Compare September 8, 2024 08:23

sanposhiho marked this pull request as ready for review September 8, 2024 08:25

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 Sep 8, 2024

k8s-ci-robot requested review from Gekko0114 and utam0k September 8, 2024 08:26

Gekko0114 reviewed Sep 14, 2024

View reviewed changes

address comment

f65988e

sanposhiho requested a review from Gekko0114 September 15, 2024 03:55

Gekko0114 approved these changes Sep 15, 2024

View reviewed changes

add comment

2f5a3eb

sanposhiho merged commit a5b575b into main Sep 19, 2024
5 of 6 checks passed

sanposhiho deleted the sharedsnap branch September 19, 2024 06:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support SharedLister for NodeInfo #115

Support SharedLister for NodeInfo #115

sanposhiho commented Aug 22, 2024 •

edited

Loading

sanposhiho commented Sep 8, 2024

Gekko0114 left a comment

Gekko0114 Sep 14, 2024

sanposhiho Sep 15, 2024

Gekko0114 Sep 14, 2024

sanposhiho Sep 15, 2024

Gekko0114 Sep 14, 2024

sanposhiho Sep 15, 2024

Gekko0114 Sep 14, 2024

sanposhiho Sep 15, 2024

sanposhiho commented Sep 15, 2024

Gekko0114 left a comment

Gekko0114 Sep 15, 2024

sanposhiho Sep 15, 2024 •

edited

Loading

Gekko0114 Sep 15, 2024

k8s-ci-robot commented Sep 15, 2024

sanposhiho commented Sep 19, 2024

Support SharedLister for NodeInfo #115

Support SharedLister for NodeInfo #115

Conversation

sanposhiho commented Aug 22, 2024 • edited Loading

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?

What are the benchmark results of this change?

sanposhiho commented Sep 8, 2024

Gekko0114 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

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

sanposhiho commented Sep 15, 2024

Gekko0114 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho Sep 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 15, 2024

sanposhiho commented Sep 19, 2024

sanposhiho commented Aug 22, 2024 •

edited

Loading

sanposhiho Sep 15, 2024 •

edited

Loading