From 2952376ea400d72f6a52978af69904e607d50cda Mon Sep 17 00:00:00 2001 From: Daniel Widdis Date: Sat, 27 Jan 2024 12:37:26 -0800 Subject: [PATCH] Debug Signed-off-by: Daniel Widdis --- .github/workflows/CI.yml | 73 +------------------ build.gradle | 4 +- .../flowframework/FlowFrameworkPlugin.java | 3 +- .../rest/RestDeleteWorkflowAction.java | 6 +- .../rest/RestDeprovisionWorkflowAction.java | 6 +- .../rest/RestGetWorkflowAction.java | 6 +- .../rest/RestGetWorkflowStateAction.java | 6 +- .../rest/RestProvisionWorkflowAction.java | 6 +- .../FlowFrameworkRestTestCase.java | 4 +- .../rest/FlowFrameworkRestApiIT.java | 32 ++++---- 10 files changed, 45 insertions(+), 101 deletions(-) diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index bb0a42585..b7df68882 100644 --- a/.github/workflows/CI.yml +++ b/.github/workflows/CI.yml @@ -9,83 +9,12 @@ on: types: [opened, synchronize, reopened] jobs: - spotless: - if: github.repository == 'opensearch-project/flow-framework' - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - # Spotless requires JDK 17+ - - uses: actions/setup-java@v4 - with: - java-version: 17 - distribution: temurin - - name: Spotless Check - run: ./gradlew spotlessCheck - javadoc: - if: github.repository == 'opensearch-project/flow-framework' - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - name: Javadoc CheckStyle - run: ./gradlew checkstyleMain - - name: Javadoc Check - run: ./gradlew javadoc - build: - needs: [spotless, javadoc] - strategy: - matrix: - os: [ubuntu-latest, macos-latest, windows-latest] - java: [11, 21] - include: - - os: ubuntu-latest - java: 17 - codecov: yes - name: Test JDK${{ matrix.java }}, ${{ matrix.os }} - runs-on: ${{ matrix.os }} - steps: - - uses: actions/checkout@v4 - - name: Set up JDK ${{ matrix.java }} - uses: actions/setup-java@v4 - with: - java-version: ${{ matrix.java }} - distribution: temurin - - name: Build and Run Tests - run: | - ./gradlew check -x integTest -x yamlRestTest -x spotlessJava - - name: Upload Coverage Report - if: ${{ matrix.codecov }} - uses: codecov/codecov-action@v3 - with: - file: ./build/reports/jacoco/test/jacocoTestReport.xml - integTest: - needs: [spotless, javadoc] - strategy: - fail-fast: false - matrix: - os: [ubuntu-latest, macos-latest, windows-latest] - java: [11, 21] - include: - - os: ubuntu-latest - java: 17 - name: Integ Test JDK${{ matrix.java }}, ${{ matrix.os }} - runs-on: ${{ matrix.os }} - steps: - - uses: actions/checkout@v4 - - name: Set up JDK ${{ matrix.java }} - uses: actions/setup-java@v4 - with: - java-version: ${{ matrix.java }} - distribution: temurin - - name: Build and Run Tests - run: | - ./gradlew integTest yamlRestTest integMultiNodeTest: - needs: [spotless, javadoc] strategy: fail-fast: false matrix: os: [ubuntu-latest, macos-latest, windows-latest] - java: [21] + java: [11, 17, 18, 19, 20, 21] name: Multi-Node Integ Test JDK${{ matrix.java }}, ${{ matrix.os }} runs-on: ${{ matrix.os }} steps: diff --git a/build.gradle b/build.gradle index fd5f23e94..92530bb86 100644 --- a/build.gradle +++ b/build.gradle @@ -255,6 +255,8 @@ integTest { // not being written, the waitForAllConditions ensures it's written getClusters().forEach { cluster -> cluster.waitForAllConditions() + // we can't change the timeout and it's flaky if we don't wait long enough, so call it again + cluster.waitForAllConditions() } } @@ -415,7 +417,7 @@ allprojects { if (System.getenv().containsKey("CI")) { maxRetries = 1 maxFailures = 3 - failOnPassedAfterRetry = false + failOnPassedAfterRetry = true } } } diff --git a/src/main/java/org/opensearch/flowframework/FlowFrameworkPlugin.java b/src/main/java/org/opensearch/flowframework/FlowFrameworkPlugin.java index ea1327c1d..a6a8921ce 100644 --- a/src/main/java/org/opensearch/flowframework/FlowFrameworkPlugin.java +++ b/src/main/java/org/opensearch/flowframework/FlowFrameworkPlugin.java @@ -185,7 +185,8 @@ public List> getExecutorBuilders(Settings settings) { new ScalingExecutorBuilder( WORKFLOW_THREAD_POOL, 1, - OpenSearchExecutors.allocatedProcessors(settings), + // use no more than half the processors + Math.max(1, OpenSearchExecutors.allocatedProcessors(settings) / 2), TimeValue.timeValueMinutes(5), FLOW_FRAMEWORK_THREAD_POOL_PREFIX + WORKFLOW_THREAD_POOL ) diff --git a/src/main/java/org/opensearch/flowframework/rest/RestDeleteWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestDeleteWorkflowAction.java index d4fcb824b..fea16e354 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestDeleteWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestDeleteWorkflowAction.java @@ -71,8 +71,10 @@ protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request } // Validate content if (request.hasContent()) { - // BaseRestHandler will give appropriate error message - return channel -> channel.sendResponse(null); + throw new FlowFrameworkException( + "request [" + request.method() + " " + request.path() + "] does not support having a body", + RestStatus.BAD_REQUEST + ); } // Validate params if (workflowId == null) { diff --git a/src/main/java/org/opensearch/flowframework/rest/RestDeprovisionWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestDeprovisionWorkflowAction.java index b3dc5e713..b85cbd832 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestDeprovisionWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestDeprovisionWorkflowAction.java @@ -66,8 +66,10 @@ protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request } // Validate content if (request.hasContent()) { - // BaseRestHandler will give appropriate error message - return channel -> channel.sendResponse(null); + throw new FlowFrameworkException( + "request [" + request.method() + " " + request.path() + "] does not support having a body", + RestStatus.BAD_REQUEST + ); } // Validate params if (workflowId == null) { diff --git a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java index d1097fb68..45bd8a33c 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowAction.java @@ -71,8 +71,10 @@ protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request } // Validate content if (request.hasContent()) { - // BaseRestHandler will give appropriate error message - return channel -> channel.sendResponse(null); + throw new FlowFrameworkException( + "request [" + request.method() + " " + request.path() + "] does not support having a body", + RestStatus.BAD_REQUEST + ); } // Validate params if (workflowId == null) { diff --git a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStateAction.java b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStateAction.java index 9db927f34..6f473873d 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStateAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestGetWorkflowStateAction.java @@ -67,8 +67,10 @@ protected BaseRestHandler.RestChannelConsumer prepareRequest(RestRequest request // Validate content if (request.hasContent()) { - // BaseRestHandler will give appropriate error message - return channel -> channel.sendResponse(null); + throw new FlowFrameworkException( + "request [" + request.method() + " " + request.path() + "] does not support having a body", + RestStatus.BAD_REQUEST + ); } // Validate params if (workflowId == null) { diff --git a/src/main/java/org/opensearch/flowframework/rest/RestProvisionWorkflowAction.java b/src/main/java/org/opensearch/flowframework/rest/RestProvisionWorkflowAction.java index 124b6bf49..215717526 100644 --- a/src/main/java/org/opensearch/flowframework/rest/RestProvisionWorkflowAction.java +++ b/src/main/java/org/opensearch/flowframework/rest/RestProvisionWorkflowAction.java @@ -77,8 +77,10 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli } // Validate content if (request.hasContent()) { - // BaseRestHandler will give appropriate error message - return channel -> channel.sendResponse(null); + throw new FlowFrameworkException( + "request [" + request.method() + " " + request.path() + "] does not support having a body", + RestStatus.BAD_REQUEST + ); } // Validate params if (workflowId == null) { diff --git a/src/test/java/org/opensearch/flowframework/FlowFrameworkRestTestCase.java b/src/test/java/org/opensearch/flowframework/FlowFrameworkRestTestCase.java index 666a1541d..7e796ee90 100644 --- a/src/test/java/org/opensearch/flowframework/FlowFrameworkRestTestCase.java +++ b/src/test/java/org/opensearch/flowframework/FlowFrameworkRestTestCase.java @@ -572,8 +572,8 @@ protected void getAndAssertWorkflowStatus( assertEquals(RestStatus.OK, TestHelpers.restStatus(response)); Map responseMap = entityAsMap(response); - assertEquals(stateStatus.name(), (String) responseMap.get(CommonValue.STATE_FIELD)); - assertEquals(provisioningStatus.name(), (String) responseMap.get(CommonValue.PROVISIONING_PROGRESS_FIELD)); + assertEquals(stateStatus.name(), responseMap.get(CommonValue.STATE_FIELD)); + assertEquals(provisioningStatus.name(), responseMap.get(CommonValue.PROVISIONING_PROGRESS_FIELD)); } diff --git a/src/test/java/org/opensearch/flowframework/rest/FlowFrameworkRestApiIT.java b/src/test/java/org/opensearch/flowframework/rest/FlowFrameworkRestApiIT.java index 408e8f811..f6b133f42 100644 --- a/src/test/java/org/opensearch/flowframework/rest/FlowFrameworkRestApiIT.java +++ b/src/test/java/org/opensearch/flowframework/rest/FlowFrameworkRestApiIT.java @@ -22,8 +22,11 @@ import org.opensearch.flowframework.model.WorkflowEdge; import org.opensearch.flowframework.model.WorkflowNode; import org.opensearch.flowframework.model.WorkflowState; +import org.junit.Before; import org.junit.ComparisonFailure; +import java.io.IOException; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.HashSet; @@ -39,6 +42,13 @@ public class FlowFrameworkRestApiIT extends FlowFrameworkRestTestCase { + @Before + public void waitForMlConfigIndes() throws Exception { + if (!indexExistsWithAdminClient(".plugins-ml-config")) { + assertBusy(() -> assertTrue(indexExistsWithAdminClient(".plugins-ml-config")), 120, TimeUnit.SECONDS); + } + } + public void testSearchWorkflows() throws Exception { // Create a Workflow that has a credential 12345 @@ -69,7 +79,7 @@ public void testSearchWorkflows() throws Exception { } } - public void testFailedUpdateWorkflow() throws Exception { + public void XXtestFailedUpdateWorkflow() throws Exception { Template templateCreation = TestHelpers.createTemplateFromFile("createconnector-registerremotemodel-deploymodel.json"); Response responseCreate = createWorkflow(client(), templateCreation); assertEquals(RestStatus.CREATED, TestHelpers.restStatus(responseCreate)); @@ -88,7 +98,7 @@ public void testFailedUpdateWorkflow() throws Exception { // Ensure Ml config index is initialized as creating a connector requires this, then hit Provision API and assert status Response provisionResponse; if (!indexExistsWithAdminClient(".plugins-ml-config")) { - assertBusy(() -> assertTrue(indexExistsWithAdminClient(".plugins-ml-config")), 40, TimeUnit.SECONDS); + // FIXME assertBusy(() -> assertTrue(indexExistsWithAdminClient(".plugins-ml-config")), 40, TimeUnit.SECONDS); provisionResponse = provisionWorkflow(client(), workflowId); } else { provisionResponse = provisionWorkflow(client(), workflowId); @@ -105,7 +115,7 @@ public void testFailedUpdateWorkflow() throws Exception { } - public void testCreateAndProvisionLocalModelWorkflow() throws Exception { + public void XXtestCreateAndProvisionLocalModelWorkflow() throws Exception { // Using a 1 step template to register a local model and deploy model Template template = TestHelpers.createTemplateFromFile("register-deploylocalsparseencodingmodel.json"); @@ -216,13 +226,7 @@ public void testCreateAndProvisionRemoteModelWorkflow() throws Exception { String workflowId = (String) responseMap.get(WORKFLOW_ID); getAndAssertWorkflowStatus(client(), workflowId, State.NOT_STARTED, ProvisioningProgress.NOT_STARTED); - // Ensure Ml config index is initialized as creating a connector requires this, then hit Provision API and assert status - if (!indexExistsWithAdminClient(".plugins-ml-config")) { - assertBusy(() -> assertTrue(indexExistsWithAdminClient(".plugins-ml-config")), 40, TimeUnit.SECONDS); - response = provisionWorkflow(client(), workflowId); - } else { - response = provisionWorkflow(client(), workflowId); - } + response = provisionWorkflow(client(), workflowId); assertEquals(RestStatus.OK, TestHelpers.restStatus(response)); getAndAssertWorkflowStatus(client(), workflowId, State.PROVISIONING, ProvisioningProgress.IN_PROGRESS); @@ -240,17 +244,15 @@ public void testCreateAndProvisionRemoteModelWorkflow() throws Exception { assertNotNull(resourcesCreated.get(2).resourceId()); } - public void testCreateAndProvisionAgentFrameworkWorkflow() throws Exception { + public void xtestCreateAndProvisionAgentFrameworkWorkflow() throws Exception { Template template = TestHelpers.createTemplateFromFile("agent-framework.json"); // Hit Create Workflow API to create agent-framework template, with template validation check and provision parameter Response response; if (!indexExistsWithAdminClient(".plugins-ml-config")) { - assertBusy(() -> assertTrue(indexExistsWithAdminClient(".plugins-ml-config")), 40, TimeUnit.SECONDS); - response = createWorkflowWithProvision(client(), template); - } else { - response = createWorkflowWithProvision(client(), template); + assertBusy(() -> assertTrue(indexExistsWithAdminClient(".plugins-ml-config")), 60, TimeUnit.SECONDS); } + response = createWorkflowWithProvision(client(), template); assertEquals(RestStatus.CREATED, TestHelpers.restStatus(response)); Map responseMap = entityAsMap(response); String workflowId = (String) responseMap.get(WORKFLOW_ID);