Adding sandbox schema #13669

kaushalmahi12 · 2024-05-14T19:45:30Z

Description

This change introduces the fundamental construct which we will use to enforce node level resiliency as part of this RFC: #12342.

Related Issues

RFC

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
API changes companion pull request created.
Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)
Public documentation issue/PR created

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.

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

…odes Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

github-actions · 2024-05-14T20:45:14Z

❌ Gradle check result for c7e56f4: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

jainankitk · 2024-05-15T01:58:39Z

server/src/main/java/org/opensearch/cluster/metadata/ResourceLimitGroup.java

+ * {
+ *     "analytics":{
+ *              "jvm": 0.4,
+ *              "mpde": "enforced",


nit: typo, spacing. Although, I like this much better than the previous one. Wondering if the key should be name or id?

I think either is fine but I agree _id makes more sense

jainankitk · 2024-05-15T02:00:27Z

server/src/main/java/org/opensearch/cluster/metadata/ResourceLimitGroup.java

+ * Class to define the ResourceLimitGroup schema
+ * {
+ *     "analytics":{
+ *              "jvm": 0.4,


This looks inconsistent with the class attributes below. jvm should be within resourceLimits?

It is not inconsistent, the resourceLimits in the code are modeled using map but the toXContent method will just add the attributes of the resource limits plain and simple.

deshsidd · 2024-05-15T19:05:19Z

server/src/main/java/org/opensearch/cluster/metadata/ResourceLimitGroup.java

+        Objects.requireNonNull(_id, "ResourceLimitGroup._id can't be null");
+
+        if (name.length() > MAX_CHARS_ALLOWED_IN_NAME) {
+            throw new IllegalArgumentException("ResourceLimitGroup.name shouldn't be more than 50 chars long");


nit: can use variable here rather than static constant (50) in case we change this in the future

deshsidd · 2024-05-15T19:06:12Z

server/src/main/java/org/opensearch/cluster/metadata/ResourceLimitGroup.java

+            Objects.requireNonNull(resourceName, "resourceName can't be null");
+            Objects.requireNonNull(threshold, "resource value can't be null");
+
+            if (Double.compare(threshold, 1.0) > 0) {


nit: do we need a constant for this?

This is standard comparator return values in cases where first arg in the comparator is greater than second one

deshsidd · 2024-05-15T19:09:30Z

server/src/main/java/org/opensearch/cluster/metadata/ResourceLimitGroup.java

+
+        String fieldName = "";
+        // Map to hold resources
+        final Map<String, Object> resourceLimitGroup_ = new HashMap<>();


Curious why this naming with "_" suffix here, is this some sort of convention?

No since this is local to the method, I went ahead with this.

deshsidd · 2024-05-15T19:10:13Z

server/src/main/java/org/opensearch/cluster/metadata/ResourceLimitGroup.java

+                } else if (ALLOWED_RESOURCES.contains(fieldName)) {
+                    resourceLimitGroup_.put(fieldName, parser.doubleValue());
+                } else {
+                    throw new IllegalArgumentException("unrecognised [field=" + fieldName + " in ResourceLimitGroup");


Need to add a closed bracked ]

Good Catch! thanks

deshsidd · 2024-05-15T19:12:42Z

server/src/main/java/org/opensearch/cluster/metadata/ResourceLimitGroupMetadata.java

+@ExperimentalApi
+public class ResourceLimitGroupMetadata implements Metadata.Custom {
+    public static final String TYPE = "resourceLimitGroup";
+    private static final ParseField RESOURCE_LIMIT_GROUP_FIELD = new ParseField("resourceLimitGroups");


"resourceLimitGroup" ?

The metadata will contain all the ResourceLimitGroups in the cluster.

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

github-actions · 2024-05-15T22:07:41Z

❌ Gradle check result for bd1f864: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

github-actions · 2024-05-20T21:22:15Z

❌ Gradle check result for 9119813: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

sgup432 · 2024-05-28T07:25:59Z

server/src/main/java/org/opensearch/cluster/metadata/ResourceLimitGroup.java

+ * }
+ */
+@ExperimentalApi
+public class ResourceLimitGroup extends AbstractDiffable<ResourceLimitGroup> implements ToXContentObject {


Considering name below signifies a tenant, putting this inside ResourceLimitGroup looks confusing. I think it will help by decoupling tenant and resource logic like below?

class Tenant { private String tenantName; private String tenantId; private List<ResourceLimitGroup> resourceLimitGroups; // Can be extended with more details later if needed like tenant attributes etc. }

class ResourceLimitGroup { String resourceName; ResourceLimitGroupMode mode; Double threshold; // etc etc }

Let me know what you think.

kaushalmahi12 added 14 commits May 14, 2024 11:22

rebase with opensearch/main

b09fa73

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

add resourceLimitGroupId propagation logic from coordinator to data n…

70ff1e3

…odes Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

add sandbox schema

40feb6f

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

add resourceLimitGroupTests

511c0a5

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

add resourceLimitGroupMetadata tests

b54b91f

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

run spotlessApply

bf5645a

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

add mode field in ResourceLimitGroup schema

a6a14f1

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

fix breaking testcases

95b4b87

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

add task cancellation skeleton

a1355d1

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

add multitenant labels in searchSource builder

13f79f7

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

write custom xcontent parser for ResourceLimitGroup

80bd755

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

remove unrelated changes

89f2694

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

remove non-existing import fro cluster settings

ff4629c

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

remove non releated changes

c7e56f4

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

kaushalmahi12 requested review from anasalkouz, andrross, Bukhtawar, CEHENKLE, dblock, dbwiddis, dreamer-89, gbbafna, kotwanikunal, mch2, msfroh, nknize, owaiskazi19, reta, Rishikesh1159 and sachinpkale as code owners May 14, 2024 19:45

kaushalmahi12 requested review from saratvemulapalli, shwetathareja, sohami, tlfeng and VachaShah as code owners May 14, 2024 19:45

jainankitk reviewed May 15, 2024

View reviewed changes

deshsidd reviewed May 15, 2024

View reviewed changes

add _id as the resourceLimitGroup key

bd1f864

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

add change to register resource limit group metadata

9119813

Signed-off-by: Kaushal Kumar <ravi.kaushal97@gmail.com>

sgup432 reviewed May 28, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding sandbox schema #13669

Adding sandbox schema #13669

kaushalmahi12 commented May 14, 2024

github-actions bot commented May 14, 2024

jainankitk May 15, 2024

kaushalmahi12 May 15, 2024

jainankitk May 15, 2024

kaushalmahi12 May 15, 2024 •

edited

deshsidd May 15, 2024

deshsidd May 15, 2024

kaushalmahi12 May 15, 2024

deshsidd May 15, 2024

kaushalmahi12 May 15, 2024

deshsidd May 15, 2024

kaushalmahi12 May 15, 2024

deshsidd May 15, 2024

kaushalmahi12 May 15, 2024

github-actions bot commented May 15, 2024

github-actions bot commented May 20, 2024

sgup432 May 28, 2024 •

edited

Adding sandbox schema #13669

Are you sure you want to change the base?

Adding sandbox schema #13669

Conversation

kaushalmahi12 commented May 14, 2024

Description

Related Issues

Check List

github-actions bot commented May 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaushalmahi12 May 15, 2024 • edited

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

Choose a reason for hiding this comment

github-actions bot commented May 15, 2024

github-actions bot commented May 20, 2024

sgup432 May 28, 2024 • edited

Choose a reason for hiding this comment

kaushalmahi12 May 15, 2024 •

edited

sgup432 May 28, 2024 •

edited