Add AdmissionController interface #3716

mitalawachat · 2022-06-27T18:57:14Z

Signed-off-by: Mital Awachat [email protected]

Description

This pull request serves as a starting point for Admission Control. In this pull request we are adding AdmissionController interface which will be fulfilled by concrete implementation in subsequent pull requests as mentioned in design proposal given below.

Issues Resolved

Feature Proposal: #1144
Design Proposal: #3400

Check List

[-] New functionality includes testing.
- [-] All tests pass
New functionality has been documented.
- New functionality has javadoc added
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

opensearch-ci-bot · 2022-06-27T19:29:50Z

✅ Gradle Check success 213ce6b51d023710f8732979a4cebcdce45c02fa
Log 6377

Reports 6377

nknize

This interface needs to be included in a PR that implements it and includes some kind of tests. We don't merge PRs for unused interfaces. If this is a merge of a fairly large downstream implementation make sure it's split into smaller incremental PRs.

Also, we'll want to feature flag this so we can let it bake before releasing GA.

nknize · 2022-06-28T15:00:50Z

server/src/main/java/org/opensearch/common/throttle/AdmissionController.java

+ * Provides methods for any tracking-object that can be incremented (such as memory size),
+ * Admission control can be applied if configured limit has been reached.
+ */
+public interface AdmissionController {


Do we intend for plugins to be able to implement this interface? Can we make this package private?

If not please mark this with an appropriate opensearch annotation (@opensearch.internal, @opensearch.experimental, @opensearch.api) to document extensibility restrictions.

This interface needs to be included in a PR that implements it and includes some kind of tests. We don't merge PRs for unused interfaces. If this is a merge of a fairly large downstream implementation make sure it's split into smaller incremental PRs.

Also, we'll want to feature flag this so we can let it bake before releasing GA.

Thanks for the feedback @nknize

I planned to raise implementations as part of next pull-requests to keep scope of each pull-request smaller.

I will update this pull request with other suggested changes.

Do we intend for plugins to be able to implement this interface? Can we make this package private?

Made it package private as it is not intended for plugins to implement this interface.

Also, we'll want to feature flag this so we can let it bake before releasing GA.

Yes, this feature will be behind dynamic setting.

nknize

Thanks for including the requested changes. But we do need to include this interface in a PR that implements it and includes some kind of tests. We don't merge PRs with unused interfaces. If this is a merge of a fairly large downstream implementation make sure it's split into smaller incremental PRs. Can you at least include this with some part of a mechanism that uses it?

getsaurabh02 · 2022-07-15T23:27:38Z

+1 on @nknize on having one of the implementations out before we merge just the interface. However, the only difficulty I see is that if we do as a separate followup PR, it will fail checks since interface is not yet available in the core.

@mitalawachat Can we have one of the smaller implementations added to this same PR. That will help validate the consistency and completeness of the Interface being added.

However, if it consists of bigger chunk of changes and we wish to break them up into multiple followup PRs, we should discuss the possibility of a feature-branch as well.

github-actions · 2022-07-18T18:11:11Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/740/
CommitID: 76ec5e27bcc756542149e5a7f22f7676897c8160

github-actions · 2022-07-18T18:15:39Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/742/
CommitID: 03844becf66b917657b80000be5e10a4c7913dee

github-actions · 2022-07-18T20:23:26Z

Gradle Check (Jenkins) Run Completed with:

RESULT: FAILURE ❌
URL: https://build.ci.opensearch.org/job/gradle-check/746/
CommitID: 9420385d2dba7572c7501278f17c8deeddef2354

github-actions · 2022-07-18T23:24:41Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/758/
CommitID: f1727cf0bfe65b0f9e7ddffc28b64cc9eed20470

Signed-off-by: Mital Awachat <[email protected]>

github-actions · 2022-07-19T16:29:15Z

Gradle Check (Jenkins) Run Completed with:

RESULT: SUCCESS ✅
URL: https://build.ci.opensearch.org/job/gradle-check/798/
CommitID: 7e181b5

mitalawachat · 2022-07-19T16:38:41Z

server/src/main/java/org/opensearch/common/throttle/AdmissionControlSetting.java

+
+    @Override
+    public String toString() {
+        return "AdmissionControlSetting{" + "name='" + name + '\'' + ", enabled=" + enabled + ", limit=" + limits + '}';


Will fix this along with review comments.

Bukhtawar · 2022-07-31T11:45:18Z

server/src/main/java/org/opensearch/common/throttle/AdmissionController.java

+    /**
+     * @return current value of the rejection count metric tracked by the admission-controller.
+     */
+    long getRejectionCount();
+
+    /**
+     * Adds the rejection count for the controller. Primarily used when copying controller states.
+     * @param count To add the value of the tracking resource object as the provided count
+     * @return count/value by which the resource tracking object is updated with.
+     */
+    long addRejectionCount(long count);
+


Can metric tracking be modelled as a listener

Bukhtawar · 2022-07-31T11:46:09Z

server/src/main/java/org/opensearch/common/throttle/RequestSizeAdmissionController.java

+    @Override
+    public long setInitialQuota(long count) {
+        usedBytes.set(count);
+        return usedBytes.get();
+    }


do we need a reset? or can it called as a part of the constructor

dreamer-89 · 2023-03-27T16:35:38Z

@mitalawachat : Are you still working on this ?

psychbot · 2023-04-03T06:45:31Z

We can mark this PR as closed as it requires additional discussion if Admission Control needs to be part of core or can it be a separate plugin all together.

ketanv3 · 2023-05-06T08:46:47Z

server/src/main/java/org/opensearch/common/throttle/AdmissionControlSetting.java

+ *
+ * @opensearch.internal
+ */
+class AdmissionControlSetting {


We should allow each admission controller to bring their own settings, rather than enforcing this fixed format.

ketanv3 · 2023-05-06T08:54:01Z

server/src/main/java/org/opensearch/common/throttle/AdmissionController.java

+    /**
+     * Increment the tracking-object with provided value.
+     * Apply the admission control if threshold is breached.
+     * Mostly applicable while acquiring the quota.
+     * Later Releasable is used to decrement the tracking-object with previously acquired value.
+     *
+     * @param count value to incrementation the resource racking-object with.
+     * @return Releasable for tokens acquired from the resource tracking object.
+     */
+    Releasable addBytesAndMaybeBreak(long count);
+
+    /**
+     * Sets the initial used quota for the controller. Primarily used when copying controller states.
+     * @param count To set the value of the tracking resource object as the provided count
+     * @return count/value by which the resource tracking object is updated with.
+     */
+    long setInitialQuota(long count);
+
+    /**
+     * @return currently acquired value of the tracking-object being tracked by the admission-controller.
+     */
+    long getUsedQuota();
+
+    /**
+     * @return current value of the rejection count metric tracked by the admission-controller.
+     */
+    long getRejectionCount();
+
+    /**
+     * Adds the rejection count for the controller. Primarily used when copying controller states.
+     * @param count To add the value of the tracking resource object as the provided count
+     * @return count/value by which the resource tracking object is updated with.
+     */
+    long addRejectionCount(long count);


I feel that these interface methods model each admission controller as a token-bucket, which is not always true. For example, an admission controller which only looks at the node's CPU/memory usage doesn't need to acquire/release any tokens.

ketanv3 · 2023-05-06T08:55:15Z

server/src/main/java/org/opensearch/common/throttle/AdmissionController.java

+     * Admission Controllers
+     */
+    public enum Controllers {
+        REQUEST_SIZE("RequestSize");


I'm not sure why this belongs here. We should make admission controllers pluggable.

mitalawachat requested review from a team and reta as code owners June 27, 2022 18:57

nknize requested changes Jun 28, 2022

View reviewed changes

mitalawachat force-pushed the main branch from 213ce6b to 580f30d Compare June 29, 2022 09:11

mitalawachat requested a review from nknize June 29, 2022 11:06

nknize requested changes Jul 1, 2022

View reviewed changes

mitalawachat force-pushed the main branch 2 times, most recently from 76ec5e2 to 03844be Compare July 18, 2022 17:55

mitalawachat force-pushed the main branch from 03844be to 9420385 Compare July 18, 2022 20:04

mitalawachat force-pushed the main branch from 9420385 to f1727cf Compare July 18, 2022 23:01

mitalawachat requested a review from nknize July 18, 2022 23:43

mitalawachat added 2 commits July 19, 2022 16:32

Add AdmissionController interface

fe21950

Signed-off-by: Mital Awachat <[email protected]>

Add RequestSizeAdmissionController

7e181b5

Signed-off-by: Mital Awachat <[email protected]>

mitalawachat force-pushed the main branch from f1727cf to 7e181b5 Compare July 19, 2022 16:06

mitalawachat commented Jul 19, 2022

View reviewed changes

Bukhtawar reviewed Jul 31, 2022

View reviewed changes

reta mentioned this pull request Sep 23, 2022

Update GeoGrid base class access modifier to support extensibility #4572

Merged

6 tasks

tlfeng closed this Apr 5, 2023

ketanv3 reviewed May 6, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AdmissionController interface #3716

Add AdmissionController interface #3716

mitalawachat commented Jun 27, 2022

opensearch-ci-bot commented Jun 27, 2022

nknize left a comment

nknize Jun 28, 2022 •

edited

Loading

mitalawachat Jun 29, 2022

mitalawachat Jun 29, 2022

nknize left a comment

getsaurabh02 commented Jul 15, 2022

github-actions bot commented Jul 18, 2022

github-actions bot commented Jul 18, 2022

github-actions bot commented Jul 18, 2022

github-actions bot commented Jul 18, 2022

github-actions bot commented Jul 19, 2022

mitalawachat Jul 19, 2022

Bukhtawar Jul 31, 2022

Bukhtawar Jul 31, 2022

dreamer-89 commented Mar 27, 2023

psychbot commented Apr 3, 2023

ketanv3 May 6, 2023

ketanv3 May 6, 2023

ketanv3 May 6, 2023

Add AdmissionController interface #3716

Add AdmissionController interface #3716

Conversation

mitalawachat commented Jun 27, 2022

Description

Issues Resolved

Check List

opensearch-ci-bot commented Jun 27, 2022

nknize left a comment

Choose a reason for hiding this comment

nknize Jun 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nknize left a comment

Choose a reason for hiding this comment

getsaurabh02 commented Jul 15, 2022

github-actions bot commented Jul 18, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 18, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 18, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 18, 2022

Gradle Check (Jenkins) Run Completed with:

github-actions bot commented Jul 19, 2022

Gradle Check (Jenkins) Run Completed with:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dreamer-89 commented Mar 27, 2023

psychbot commented Apr 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nknize Jun 28, 2022 •

edited

Loading