feat: refactor to maintain policy state in-memory by cmaclaughlin · Pull Request #49 · Azure/placement-policy-scheduler-plugins

cmaclaughlin · 2022-01-19T19:54:38Z

The capacity scheduling plugin was used for frame of reference. A collection of policy information is kept in-memory on the PlacementPolicyManager. Instead of annotations, the policy info in the collection includes 2 sets:

all pods that qualify for / fall under the policy
all pods assigned to nodes in accordance with the policy ("managed by")

An event handler was attached to the pod informer so pod deletion can trigger updating the in-memory version of the policy accordingly. Additionally, the custom info object is included in the state object added to cycle state as a part of PreFilter and PreScore stages so that Filter and Score stages have access to it as well.

…notations. unit tests for key functionality.

helayoty

The way you implemented it is great. Thank you for your efforts!

aramase

Thanks for the PR!

I've reviewed core.go and policy.go mostly and added comments. I'll review the other files next and add comments if any.

aramase · 2022-02-08T07:42:31Z

+	p.qualifiedPods = p.qualifiedPods.Delete(key)
+
+	if p.PodIsManagedByPolicy(key) {
+		p.managedPods = p.managedPods.Delete(key)


Do you know if Delete from the set is thread safe?

I don't think it is since it is implemented via map[string]struct{} and delete(map, key) is not thread safe.

Would you like a thread safe implementation or leave it as-is?

Co-authored-by: Anish Ramasekar <anish.ramasekar@gmail.com>

aramase

Added few more comments!

One thing I noticed is that PreScore now depends on cycle state set by the PreFilter phase. If a user wants to to use the scheduler plugin only for the preScore and score extension points and not configure it for preFilter and filter the plugin will now not work. IMO we should not depend on state being set by a different extension point.

Co-authored-by: Anish Ramasekar <anish.ramasekar@gmail.com>

cmaclaughlin · 2022-02-25T21:42:51Z

One thing I noticed is that PreScore now depends on cycle state set by the PreFilter phase. If a user wants to to use the scheduler plugin only for the preScore and score extension points and not configure it for preFilter and filter the plugin will now not work. IMO we should not depend on state being set by a different extension point.

I've removed that dependency now. For context, I had been looking at the Volume Binding plugin which sets state data in PreFilter and then reads/consumes it in all other extension points (Filter, Score, Reserve and PreBind). Out of curiosity, though, if the user only wants to use PreScore and Score and disables PreFilter and Filter, any policy with a Strict enforcement mode is essentially ignored. Is this the intent?

helayoty · 2023-03-20T17:12:58Z

@aramase what is the status of the PR? Can we merge it soon?

cmaclaughlin added 2 commits January 19, 2022 13:50

maintain state of a placement policy in-memory rather than use pod an…

75b3f7d

…notations. unit tests for key functionality.

attempt to resolve issues

67eb90e

cmaclaughlin marked this pull request as ready for review January 21, 2022 20:48

helayoty requested review from aramase, helayoty and khenidak January 21, 2022 22:30

helayoty added the enhancement New feature or request label Jan 21, 2022

helayoty reviewed Jan 24, 2022

View reviewed changes

Comment thread pkg/plugins/placementpolicy/core/core.go Outdated

Comment thread pkg/plugins/placementpolicy/placementpolicy.go Outdated

cmaclaughlin added 2 commits January 24, 2022 15:26

initial PR changes

f457f74

fix compilation error

6d0aea4

cmaclaughlin requested a review from helayoty January 24, 2022 21:41

helayoty linked an issue Feb 7, 2022 that may be closed by this pull request

Updates to scheduler plugin logic #25

Open

aramase reviewed Feb 8, 2022

View reviewed changes

cmaclaughlin and others added 4 commits February 8, 2022 10:21

Update pkg/plugins/placementpolicy/core/core.go

cba7a0c

Co-authored-by: Anish Ramasekar <anish.ramasekar@gmail.com>

Update pkg/plugins/placementpolicy/core/policy.go

8059236

Co-authored-by: Anish Ramasekar <anish.ramasekar@gmail.com>

Merge branch 'Azure:main' into inmemory-policy-state

138af22

changes per initial PR comments

1f17889

cmaclaughlin requested a review from aramase February 8, 2022 20:28

cmaclaughlin and others added 2 commits February 22, 2022 13:24

Merge branch 'Azure:main' into inmemory-policy-state

744c441

adding comments for clarity

f104ce5

aramase reviewed Feb 24, 2022

View reviewed changes

aramase changed the title ~~enhancement: refactor to maintain policy state in-memory~~ feat: refactor to maintain policy state in-memory Feb 24, 2022

cmaclaughlin and others added 8 commits February 24, 2022 18:07

Update pkg/plugins/placementpolicy/placementpolicy.go

da2c4e6

Co-authored-by: Anish Ramasekar <anish.ramasekar@gmail.com>

Update pkg/plugins/placementpolicy/placementpolicy.go

8f418f9

Co-authored-by: Anish Ramasekar <anish.ramasekar@gmail.com>

Update pkg/plugins/placementpolicy/core/policy.go

db96728

Co-authored-by: Anish Ramasekar <anish.ramasekar@gmail.com>

Update pkg/plugins/placementpolicy/core/policy.go

cedd819

Co-authored-by: Anish Ramasekar <anish.ramasekar@gmail.com>

Update pkg/plugins/placementpolicy/core/policy.go

3adaed7

Co-authored-by: Anish Ramasekar <anish.ramasekar@gmail.com>

Update pkg/plugins/placementpolicy/core/policy.go

cd95269

Co-authored-by: Anish Ramasekar <anish.ramasekar@gmail.com>

Update pkg/plugins/placementpolicy/core/policy.go

0d482b2

Co-authored-by: Anish Ramasekar <anish.ramasekar@gmail.com>

Update pkg/plugins/placementpolicy/core/policy.go

34b0d77

Co-authored-by: Anish Ramasekar <anish.ramasekar@gmail.com>

cmaclaughlin and others added 6 commits February 24, 2022 18:11

Update pkg/plugins/placementpolicy/core/policy.go

c2fb51e

Co-authored-by: Anish Ramasekar <anish.ramasekar@gmail.com>

Update pkg/plugins/placementpolicy/placementpolicy.go

5f93723

Co-authored-by: Anish Ramasekar <anish.ramasekar@gmail.com>

initial changes for next round of feedback

e346776

Additional changes per PR feedback

ef85acc

remove stage dependency

934623e

final cleanup/fixes

d2fc5ef

cmaclaughlin requested a review from aramase February 25, 2022 21:43

Merge branch 'Azure:main' into inmemory-policy-state

0d7a9f3

Conversation

cmaclaughlin commented Jan 19, 2022

Uh oh!

helayoty left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aramase left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aramase Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

cmaclaughlin Feb 8, 2022

Choose a reason for hiding this comment

Uh oh!

cmaclaughlin Feb 25, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aramase left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cmaclaughlin commented Feb 25, 2022

Uh oh!

helayoty commented Mar 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants