Patches based implementation for DRA snapshot.

[mtrqq]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This PR enhances the performance of DRA snapshot which directly impacts scheduling simulations speed and cluster-autoscaler decision making process overall. In this PR we are changing the approach for its state management from deep copies based into patches based one.

Which issue(s) this PR fixes:

Fixes #7681

Special notes for your reviewer:

This PR removes the original implementation for dynamicresources.Snapshot and replaces it with the patches based approach, while we can keep the original implementation for the sake of safety and ability to switch store implementation in the running cluster autoscaler, but it would require maintaining two implementations, I've attempted to use clone-based Snapshot as a baseline for the new changes, but it only resulted in complex code while yielding minimal benefits.

In the change you may find a benchmark test which uses exagerrated scheduling scenario to test the performance of two implementations, what I've found that patches based option is roughly 50x times faster in terms of overall runtime while allocating 40x less memory on the heap primarily because Fork/Commit/Revert operations are used A LOT in the suite

Here's a few profiling insights in differences:

CPU Profile / Forking

CPU Profile / GC

Memory Profile / Allocated Space

Memory Profile / Allocated Objects

Grab a copy of profiling samples -> Profiles.zip

Does this PR introduce a user-facing change?

NONE

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

[k8s-ci-robot]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[mtrqq]

/assign towca

[jackfrancis]

Is this meant to address map access race conditions between:

1. enumerate through the set of patches and begin copying into a new, merged map
2. one or more patches in the set begin a delete operation

func (p *patch[K, V]) Delete(key K) {
	p.Deleted[key] = true
	delete(p.Modified, key)
}

3. ensure that our source patch wasn't copied from a state in between the 1st of the above 2 statements by double-checking the same key against any corresponding existence in the Deleted map

?

(If so have we considered the tradeoffs of inverting the order of operations in the Delete() method?)

[mtrqq]

I would say that Snapshot is not thread-safe in general, but that's a fair call, I'll move the order of operations in Delete() method so that it'll become consistent with other data manipulation functions. In case we need it to be truly suitable for concurrent usage (and as I see based on simulations code - we don't) - we'll probably need to use sync.Map to actually store the data, in current implementation we are using bare maps just the sake of performance gain

The reason why we account for Deleted keys here - is to handle following situations:

Prerequisite: Type in the example -> PatchSet[int, int]

Patch#1: Modified: {1: 1, 2: 2, 3: 3}, Deleted: {}
Patch#2: Modified: {}, Deleted: {1, 2}
Patch#3: Modified: {1: 5}, Deleted: {}

The result of the AsMap() call on the PatchSet holding these 3 patches should be: {1: 5, 3: 3}, because keys 1 and 2 are getting deleted in the second patch, but key 1 is getting reintroduced in Patch#3

If I misunderstood your comment - LMK

[jackfrancis]

Yeah, we're aligned, do we have a UT case for that example scenario?

[towca]

I'm not sure I get the difference between the order of operations in Delete(), we're not planning for any concurrency here for now, right?

(I do get the need for iterating over Deleted and deleting from merged here, but not sure how it relates to Jack's comment 😅)

[jackfrancis]

The reason to invert the order in Delete() (actually delete first, take an accounting of that deletion second) is so that we don't have to double-check that the data is really deleted before composing the merge from the data.

I agree with @towca's other comment that this is a great addition, and was really fun to review. I did have to try really hard to find something to criticize! 😀😂

[mtrqq]

At least at this point of time I don't see Snapshot being used concurrently anywhere - and if it will be, I think DRA Snapshot would be the only blocker there, so let's keep it as is at least for now, WDYT?

I agree with @towca's other comment that this is a great addition, and was really fun to review. I did have to try really hard to find something to criticize! 😀😂

I forgot the last time I had to implement something that technical and fun so it's coming from both sides :P

[towca]

At least at this point of time I don't see Snapshot being used concurrently anywhere - and if it will be, I think DRA Snapshot would be the only blocker there, so let's keep it as is at least for now, WDYT?

Yup, the current version looks good to me! I was just curious about the comment.

[towca]

Reviewed everything but the tests (I'll do that in a second pass). In general the PR looks very good, the only bigger comments are about the caching strategy.

Two general comments:

1. The patchSet is a very elegant solution, I'm a big fan! (also yay generics, this would be a nightmare otherwise..)
2. (for future reference) Breaking the PR down into smaller, meaningful commits would have seriously helped the review here. For example a structure like: change Snapshot to be a pointer (this just pollutes the diff) -> introduce patch&patchset -> add caching to patchset -> use patchset in Snapshot -> use the new dra.Snapshot in ClusterSnapshot implementations -> add benchmark.

[towca]

Why change this (here and for the other subtypes)? IMO the previous pattern was more readable - no need for redundant methods on Snapshot.

[mtrqq]

I don't have a specific preference tbh, but first iteration implementation was way harder in terms of interaction with patchSet and I wanted to keep all the interaction with it inside the Snapshot itself, while this introduces some duplication - it makes me thinking about these wrapper objects as just wrappers without any logic apart of snapshot function calls.

If you want - I may change it back, right now it doesn't make a lot of difference from my point of view

[towca]

I don't have a specific preference tbh, but first iteration implementation was way harder in terms of interaction with patchSet

Interesting, I'm really curious why?

If you want - I may change it back, right now it doesn't make a lot of difference from my point of view

I think right now it only introduces some code duplication for no clear reason. I'd prefer to revert back to the previous approach unless it's less extensible for the future or something (I still don't understand the limitations here).

[mtrqq]

Interesting, I'm really curious why?

As I've mentioned previously - first iteration was based on the ability for each operation to have an operation that would cancel the effect of the first one, essentially if you have Add1 function on the API surface - you should have Remove1 available at least internally, we dropped this idea because of the unoptimized behavior of Revert operation which was truly O(N) and way worse compared to what's implemented right now.

The rationale behind bringing all the logic into the Snapshot - hiding the complexity behind human readable functions as each operation essentially looked like this

func DoSomething() {
  return applyPatch({
       Apply: func() { doSomethingImpl() }
       Revert: func() {undoSomethingImpl()}
 })
}

And I wanted to hide this complexity within the snapshot itself and not bring it into wrapper objects so that they will simply call internal handlers specifically made available for them, so that was the idea

[towca]

I see, my mental model for these types was less "wrapper" and more "view", or "same type just with methods separated into multiple subtypes because we need multiple List() with different return types".

Thinking about it again yeah you're right, the wrapper model is way more intuitive despite having a bit more duplication. Let's leave the current version!

[k8s-ci-robot]

@mtrqq: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-e2e-autoscaling-vpa-full	74e0b71	link	false	/test pull-kubernetes-e2e-autoscaling-vpa-full

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

[towca]

Thanks for addressing all the comments @mtrqq! I think all mine are resolved, apart from the benchmark changes that will be done as a follow-up.

Holding for @jackfrancis to LGTM as well before merging.

/lgtm
/approve
/hold

[mtrqq]

New changes only affected the header within patchset_test.go file to match the expectation (previously indentation was broken)

[jackfrancis]

/lgtm
/approve

thank you @mtrqq for all the iteration here and @towca for doing the heavy lifting during review process

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, mtrqq, towca

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:

• cluster-autoscaler/OWNERS [jackfrancis,towca]

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

[jackfrancis]

/hold cancel