Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set up Rest Integration Test framework #251

Merged
merged 19 commits into from
Dec 19, 2023

Conversation

joshpalis
Copy link
Member

@joshpalis joshpalis commented Dec 5, 2023

Description

This PR achieves multiple things :

  • Modifies build.gradle to add integTestRemote task
  • Adds a FlowFrameworkRestTestCase which configures the rest admin client for use in integration tests against a security enabled/disabled cluster
  • Begins each integration test run by enabling the flow framework APIs, ml-common flags necessary to run APIs on non-ml nodes, local model registration via url
  • Adds utility methods for all of our APIs (create workflow, get workflow status, provision workflow, create workflow dry run, search workflows)
  • Adds sample template json files for use in integration tests (we'll just parse these json files instead of trying to recreate them within the integration tests)

Currently the rest integration test cover the following :

  1. Create Workflow API
  2. Create Workflow with dry run validation
  3. Graph and input validation
  4. Update Workflow API
  5. Provision Workflow API
  6. Search Workflow API (covers encryption by asserting that the search response credential fields are encrypted)
  7. Status API (provisioning status of a Workflow during creating/provisioning) and resources created
  8. Create Connector, Register Remote Model, Deploy Model Workflow Steps (encryption is covered via successful indexing and provisioning of a Create Connector Step)
  9. Register Model Group, Register Local Model, and Deploy model Workflow Steps
  10. Resources Created (need to merge with main to have all workflow steps update their resources created correctly), but I've added TODOs to assert the number and values of resources created for each step once we've merged

TODO : After the feature/agent_framework is merged on both Flow Framework main and ML-Commons main, we can add tests for the sample register agent template outlined here

Issues Resolved

#73
Part of #88

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: Joshua Palis <[email protected]>
@joshpalis joshpalis added the backport 2.x backport PRs to 2.x branch label Dec 5, 2023
@owaiskazi19
Copy link
Member

owaiskazi19 commented Dec 5, 2023

@joshpalis we might need to few failure cases which were not tested using unit tests. Overall LGTM!

Signed-off-by: Joshua Palis <[email protected]>
…ntegration test set up to wait for ml config index to become created, fixed settings update to oly occur once

Signed-off-by: Joshua Palis <[email protected]>
@joshpalis joshpalis marked this pull request as ready for review December 5, 2023 22:52
Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. One suggestion, mostly for the future.

@joshpalis
Copy link
Member Author

joshpalis commented Dec 6, 2023

BUILD FAILED in 1m 47s
» ERROR][o.o.f.w.CreateConnectorStep] [integTest-0] Failed to update workflow state with newly created resource: {}
»  org.opensearch.transport.RemoteTransportException: [integTest-0][127.0.0.1:45883][indices:data/write/update[s]]
»  Caused by: org.opensearch.index.engine.VersionConflictEngineException: [zQn9QIwBf_zJI6m9OxAm]: version conflict, required seqNo [4], primary term [1]. current document has seqNo [5] and primary term [1]
»   ↓ last 40 non error or warning messages from /home/runner/work/flow-framework/flow-framework/build/testclusters/integTest-0/logs/opensearch.stdout.log ↓
» [2023-12-06T21:16:27,276][INFO ][o.o.f.w.ProcessNode      ] [integTest-0] Finished workflow_step_2.
» [2023-12-06T21:16:27,276][INFO ][o.o.f.w.ProcessNode      ] [integTest-0] Starting workflow_step_3.
» [2023-12-06T21:16:27,278][INFO ][o.o.m.a.d.TransportDeployModelAction] [integTest-0] Will deploy model on these nodes: qZMlM993QYSJ0vj0u-JCKA
» [2023-12-06T21:16:27,321][INFO ][o.o.f.w.DeployModelStep  ] [integTest-0] Model deployment state COMPLETED
» [2023-12-06T21:16:27,321][INFO ][o.o.m.m.MLModelManager   ] [integTest-0] Set connector zgn9QIwBf_zJI6m9PBCE for the model: 0Qn9QIwBf_zJI6m9PBDP
» [2023-12-06T21:16:27,321][INFO ][o.o.f.w.ProcessNode      ] [integTest-0] Finished workflow_step_3.
» [2023-12-06T21:16:27,321][INFO ][o.o.f.t.ProvisionWorkflowTransportAction] [integTest-0] Provisioning completed successfully for workflow zQn9QIwBf_zJI6m9OxAm

Locally, all the integration tests pass for me.

This error is coming from the resources created update conflict for the state index. This issue has been fixed in this PR that has been merged into the feature/agent_framework branch but not yet to the main branch.

We've mitigated update conflicts by completing workflow step futures only after the update response has been returned, so once everything is merged to main we shouldn't see this error.

@dbwiddis due to this flaky test, I would rather we wait on merging this PR until after everything from feature/agent_framework is merged to main.

CC : @amitgalitz

@joshpalis
Copy link
Member Author

@joshpalis we might need to few failure cases which were not tested using unit tests. Overall LGTM!

@owaiskazi19, I will handle in a separate PR for transport integration tests. This PR is just for the rest integration tests

…istration. Persiting cluster settings between test runs to ensure plugin apis are enabled. Cleaning up resources after all test runs complete, rather than between test runs

Signed-off-by: Joshua Palis <[email protected]>
@dbwiddis
Copy link
Member

dbwiddis commented Dec 7, 2023

due to this flaky test, I would rather we wait on merging this PR until after everything from feature/agent_framework is merged to main.

As long as we're starting that process now I'm fine with this, but if we want to continue building more tests, not having this merged makes it harder. Does this almost completely cover what we've developed so far... and you can keep it up to date as we build more APIs (delete / unprovision)?

@joshpalis
Copy link
Member Author

joshpalis commented Dec 7, 2023

As long as we're starting that process now I'm fine with this, but if we want to continue building more tests, not having this merged makes it harder. Does this almost completely cover what we've developed so far... and you can keep it up to date as we build more APIs (delete / unprovision)?

These tests cover what we have so far in main. The update conflicts started to come up when I added more than 1 test that invoked the provision API, so until we have that fix from feature/agent_framework in the main branch, then we won't be able to add additional tests.

But yes, I'll keep this updated :)

@owaiskazi19
Copy link
Member

owaiskazi19 commented Dec 11, 2023

@joshpalis we might need to few failure cases which were not tested using unit tests. Overall LGTM!

@owaiskazi19, I will handle in a separate PR for transport integration tests. This PR is just for the rest integration tests

The comment was for rest integration tests itself, we can have some failure templates (like missing mandatory fields, cyclic graph) which would through a BAD REQUEST. It's fine to handle it in a separate PR.

Signed-off-by: Joshua Palis <[email protected]>
@joshpalis
Copy link
Member Author

The comment was for rest integration tests itself, we can have some failure templates (like missing mandatory fields, cyclic graph) which would through a BAD REQUEST. It's fine to handle it in a separate PR.

I have added tests for invalid graph/ invalid inputs for this PR

@dbwiddis dbwiddis linked an issue Dec 19, 2023 that may be closed by this pull request
2 tasks
@dbwiddis dbwiddis merged commit 7986cbf into opensearch-project:main Dec 19, 2023
19 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Dec 19, 2023
* Setting up rest integration tests

Signed-off-by: Joshua Palis <[email protected]>

* removing stray log

Signed-off-by: Joshua Palis <[email protected]>

* Cleaning up integration test example

Signed-off-by: Joshua Palis <[email protected]>

* Fixing provision transport action to respond only after state has been updated to PROVISIONING

Signed-off-by: Joshua Palis <[email protected]>

* Fixing flaky encryption test

Signed-off-by: Joshua Palis <[email protected]>

* cleaning up old logs

Signed-off-by: Joshua Palis <[email protected]>

* Added helper methods to retrieve state and resources created, fixed integration test set up to wait for ml config index to become created, fixed settings update to oly occur once

Signed-off-by: Joshua Palis <[email protected]>

* Adding another test for update API, input validation, local model registration. Persiting cluster settings between test runs to ensure plugin apis are enabled. Cleaning up resources after all test runs complete, rather than between test runs

Signed-off-by: Joshua Palis <[email protected]>

* Adding test for search workflows API, ensures that returned credentials are encrypted

Signed-off-by: Joshua Palis <[email protected]>

* Update integ test TODOs to match current development progress

Signed-off-by: Daniel Widdis <[email protected]>

* Model Group step is not yet implemented

Signed-off-by: Daniel Widdis <[email protected]>

* Comment out tests for incomplete register local model implementation

Signed-off-by: Daniel Widdis <[email protected]>

* Fix unit tests broken with changes to fix integ tests

Signed-off-by: Daniel Widdis <[email protected]>

---------

Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
Co-authored-by: Daniel Widdis <[email protected]>
(cherry picked from commit 7986cbf)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport PRs to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE] Create Integration Test framework
4 participants