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

[PROPOSAL] Remove waitCluster in Integration Tests #422

Closed
nhtruong opened this issue Mar 9, 2023 · 3 comments · Fixed by #423
Closed

[PROPOSAL] Remove waitCluster in Integration Tests #422

nhtruong opened this issue Mar 9, 2023 · 3 comments · Fixed by #423
Labels
good first issue Good for newcomers

Comments

@nhtruong
Copy link
Collaborator

nhtruong commented Mar 9, 2023

Every integration test starts of by hitting waitCluster method, which waits for the cluster to be up and ready before the test is executed. This is redundant because we have already implemented healthcheck along with autoheal to assure that the OS cluster is up and running before the integration workflow executes these tests.

Moreover, waitCluster is waiting for status of green, which is usually not applicable for local testing because:
Since we're testing the client functionality (mostly API endpoints), the OS cluster only has a single node (for simplicity and it's sufficient for development and testing). However, by default every index created requires 1 replica, which means it needs a 2nd node to deploy a backup copy to. This causes the index to have status of yellow, which leads to waitCluster to wait forever if there's any index already exists on the OS cluster. For most developers, it's normal to have a few existing indices on the local OS cluster for development. But we have to delete these indices or change their replicas_count to 0 if we want to run the integration tests locally.

Note that having a yellow status on single-node cluster is normal. The cluster is still working as intended. You just dont have a backup copy of the indices. I would say this is a bug from the server standpoint: Indices shouldn't have number of replicate set to 1 by default if there's only 1 node in the cluster.

In other words, we should remove waitCluster because:

  • It's redundant
  • It makes it harder to run integration tests locally
@nhtruong
Copy link
Collaborator Author

nhtruong commented Mar 9, 2023

Workarounds to run integration tests locally:

  • Remove all existing indices before running the tests: curl -X DELETE localhost:9200/_all
  • Update settings of existing indices and set number_of_replicas to 0.

@nhtruong nhtruong added good first issue Good for newcomers and removed untriaged labels Mar 9, 2023
Wielmany added a commit to Wielmany/opensearch-js that referenced this issue Mar 9, 2023
Signed-off-by: Didar Tursunov <89772209+Wielmany@users.noreply.github.com>
Wielmany added a commit to Wielmany/opensearch-js that referenced this issue Mar 10, 2023
Wielmany added a commit to Wielmany/opensearch-js that referenced this issue Mar 10, 2023
Signed-off-by: Didar Tursunov <89772209+Wielmany@users.noreply.github.com>
Wielmany added a commit to Wielmany/opensearch-js that referenced this issue Mar 10, 2023
Signed-off-by: Didar Tursunov <89772209+Wielmany@users.noreply.github.com>
@nhtruong nhtruong assigned widdendream and unassigned widdendream Mar 10, 2023
@dblock
Copy link
Member

dblock commented Mar 11, 2023

Workarounds to run integration tests locally:

  • Remove all existing indices before running the tests: curl -X DELETE localhost:9200/_all
  • Update settings of existing indices and set number_of_replicas to 0.

I suggest 1) documenting this in developer guide with something one can just copy-paste, 2) actually fixing integration tests to ignore existing data so that they can run on top of whatever is in an existing instance of OpenSearch (new issue?)

@nhtruong
Copy link
Collaborator Author

@dblock this issue has been resolved in #423. The workarounds were only meant for before #423 was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants