[Feature/multi_tenancy] Tenant-aware integration tests for Connector, Model, Agent, Model Groups #2818

dbwiddis · 2024-08-09T05:14:14Z

Description

Tests the entire Create-Get-Update-Search-Delete cycle with multi-tenancy (both enabled and not).

Validates expected results when tenant aware and when not (current status quo).

Looking for review on the approach before replicating to models and agents and other tenant-aware updates, although I won't wait before starting that work. If you want a simpler review, get started now. :)

To execute just these tests:

./gradlew ':opensearch-ml-plugin:integTest' -Dtests.rest.tenantaware=true

or

./gradlew ':opensearch-ml-plugin:integTest' -Dtests.rest.tenantaware=false

Because the classes end in IT, the "false" version executes with normal integ tests as well, a separate test needs to be done for the true case.

Check List

New functionality includes testing.
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.

Signed-off-by: Daniel Widdis <widdis@gmail.com>

dhrubo-os · 2024-09-24T19:28:18Z

plugin/build.gradle

@@ -184,10 +187,23 @@ integTest {
    systemProperty "user", System.getProperty("user")
    systemProperty "password", System.getProperty("password")

+    // Only tenant aware test if set
+    if (System.getProperty("tests.rest.tenantaware") != null) {


don't we need to check true too?

Yay, you're looking at this PR! :)

Yes, we do. Most of the build.gradle right now is a work in progress; we need to figure out a way to test on all the different remote stores but they also require a lot more setup (docker, ddb, etc.). Still need to do some cleanup at the end of all the tests as well (since the "wipe all indices" doesn't work remotely).

dhrubo-os · 2024-09-25T16:59:18Z

common/src/main/java/org/opensearch/sdk/SdkClient.java

@@ -19,18 +19,14 @@

 import static org.opensearch.sdk.SdkClientUtils.unwrapAndConvertToException;

-public class SdkClient implements SettingsChangeListener {
+public class SdkClient {


Do we really need to change this?

I understand if we make the settings static, there's no need for the listener. But even if we keep the settings static this piece of code shouldn't break the functionality.

IMO, it's even a two way door, like if we want to make the settings dynamic, corresponding listener solution is already in place. On top of that we haven't finalized the nature of this settings yet.

dhrubo-os · 2024-09-25T17:01:48Z

common/src/test/java/org/opensearch/sdk/client/LocalClusterIndicesClientTests.java

@@ -677,4 +674,26 @@ public void testSearchDataObject_NullTenantId() throws IOException {
        assertEquals(OpenSearchStatusException.class, cause.getClass());
        assertEquals("Tenant ID is required when multitenancy is enabled.", cause.getMessage());
    }
+
+    public void testSearchDataObject_NullTenantNoMultitenancy() throws IOException {


add @Test

dhrubo-os · 2024-09-25T17:06:55Z

plugin/src/main/java/org/opensearch/ml/settings/MLCommonsSettings.java

    public static final Setting<Boolean> ML_COMMONS_MULTI_TENANCY_ENABLED = Setting
-        .boolSetting("plugins.ml_commons.multi_tenancy_enabled", false, Setting.Property.NodeScope, Setting.Property.Dynamic);
+        .boolSetting("plugins.ml_commons.multi_tenancy_enabled", false, Setting.Property.NodeScope);


Even if we need to make this settings static, do we need to do this now?
Usually dynamic was helpful for manual testing :D

dhrubo-os · 2024-09-25T17:08:09Z

plugin/src/test/java/org/opensearch/ml/rest/RestMLConnectorTenantAwareIT.java

+
+public class RestMLConnectorTenantAwareIT extends MLCommonsTenantAwareRestTestCase {
+
+    public void testConnectorCRUD() throws Exception {


add @Test

dbwiddis requested review from b4sjoo, dhrubo-os, jngz-es, model-collapse, rbhavna, ylwu-amzn, zane-neo, Zhangxunmt, austintlee, HenryL27 and samuel-oci as code owners August 9, 2024 05:14

dbwiddis had a problem deploying to ml-commons-cicd-env August 9, 2024 05:14 — with GitHub Actions Failure

dbwiddis had a problem deploying to ml-commons-cicd-env August 9, 2024 05:14 — with GitHub Actions Error

dbwiddis had a problem deploying to ml-commons-cicd-env August 9, 2024 05:14 — with GitHub Actions Failure

dbwiddis force-pushed the integ-test branch from 198931c to 8e4e88a Compare August 9, 2024 05:24

dbwiddis had a problem deploying to ml-commons-cicd-env August 9, 2024 05:24 — with GitHub Actions Failure

dbwiddis had a problem deploying to ml-commons-cicd-env August 9, 2024 05:24 — with GitHub Actions Error

dbwiddis had a problem deploying to ml-commons-cicd-env August 9, 2024 05:24 — with GitHub Actions Failure

dbwiddis marked this pull request as draft August 9, 2024 15:29

dbwiddis had a problem deploying to ml-commons-cicd-env August 9, 2024 22:17 — with GitHub Actions Failure

dbwiddis had a problem deploying to ml-commons-cicd-env August 9, 2024 22:17 — with GitHub Actions Error

dbwiddis added 9 commits September 23, 2024 23:31

Fix SdkClient credential handling for AWS endpoints

a2f4c99

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Use assertBusy for searching

f8a457d

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Implement retryOnConflict on UpdateDataObject

9a7f61f

Signed-off-by: Daniel Widdis <widdis@gmail.com>

All integ tests working

5623e84

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Add more missing tenant ids in requests

f685a7d

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Fetch existing object and only update changed fields

fa8ecff

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Revert inadvertent migration of deleteController to SdkClient

7a4750b

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Simplify restoring context with runBefore

2ae1dfe

Signed-off-by: Daniel Widdis <widdis@gmail.com>

Refactor to move some objects out of the retry loop

63fe7c9

Signed-off-by: Daniel Widdis <widdis@gmail.com>

dbwiddis force-pushed the integ-test branch from 31995ee to b48efcc Compare September 24, 2024 06:37

dbwiddis had a problem deploying to ml-commons-cicd-env September 24, 2024 06:37 — with GitHub Actions Error

dbwiddis had a problem deploying to ml-commons-cicd-env September 24, 2024 06:37 — with GitHub Actions Failure

dbwiddis had a problem deploying to ml-commons-cicd-env September 24, 2024 06:37 — with GitHub Actions Error

dbwiddis had a problem deploying to ml-commons-cicd-env September 24, 2024 06:37 — with GitHub Actions Failure

Make tenant awareness setting static

cca0c6c

Signed-off-by: Daniel Widdis <widdis@gmail.com>

dbwiddis force-pushed the integ-test branch from b48efcc to cca0c6c Compare September 24, 2024 07:09

dbwiddis had a problem deploying to ml-commons-cicd-env September 24, 2024 07:09 — with GitHub Actions Failure

dbwiddis had a problem deploying to ml-commons-cicd-env September 24, 2024 07:09 — with GitHub Actions Error

dbwiddis had a problem deploying to ml-commons-cicd-env September 24, 2024 07:09 — with GitHub Actions Failure

dbwiddis temporarily deployed to ml-commons-cicd-env September 24, 2024 19:23 — with GitHub Actions Inactive

dhrubo-os reviewed Sep 24, 2024

View reviewed changes

dhrubo-os reviewed Sep 25, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature/multi_tenancy] Tenant-aware integration tests for Connector, Model, Agent, Model Groups #2818

[Feature/multi_tenancy] Tenant-aware integration tests for Connector, Model, Agent, Model Groups #2818

dbwiddis commented Aug 9, 2024 •

edited

Loading

dhrubo-os Sep 24, 2024

dbwiddis Sep 25, 2024

dhrubo-os Sep 25, 2024

dhrubo-os Sep 25, 2024

dhrubo-os Sep 25, 2024

dhrubo-os Sep 25, 2024


		public class RestMLConnectorTenantAwareIT extends MLCommonsTenantAwareRestTestCase {

		public void testConnectorCRUD() throws Exception {

[Feature/multi_tenancy] Tenant-aware integration tests for Connector, Model, Agent, Model Groups #2818

Are you sure you want to change the base?

[Feature/multi_tenancy] Tenant-aware integration tests for Connector, Model, Agent, Model Groups #2818

Conversation

dbwiddis commented Aug 9, 2024 • edited Loading

Description

Check List

dhrubo-os Sep 24, 2024

Choose a reason for hiding this comment

dbwiddis Sep 25, 2024

Choose a reason for hiding this comment

dhrubo-os Sep 25, 2024

Choose a reason for hiding this comment

dhrubo-os Sep 25, 2024

Choose a reason for hiding this comment

dhrubo-os Sep 25, 2024

Choose a reason for hiding this comment

dhrubo-os Sep 25, 2024

Choose a reason for hiding this comment

dbwiddis commented Aug 9, 2024 •

edited

Loading