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

Forward port integ test fixes to main branch #4867

Merged
merged 6 commits into from
Nov 1, 2024

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Oct 31, 2024

Description

This PR includes various fixes to stabilize tests in the integrationTest sourceSet. These fixes have already been merged in the 2.18 branch in an effort to get flaky tests to 0. The impetus for fixing the flaky tests, was that this PR was included in the 2.18 branch which triggered the tests in the integrationTest sourceSet to be run with the release tests. These tests were run without retry which differs from how they are run as a PR check where we allow a test to retry twice until success.

Changes included:

  • Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Test fix

Check List

  • New functionality includes testing
  • New functionality has been documented
  • New Roles/Permissions have a corresponding security dashboards plugin PR
  • API changes companion pull request created
  • 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.

cwperks and others added 5 commits October 31, 2024 10:46
…ed with DISABLE_RETRY (opensearch-project#4855)

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Signed-off-by: Craig Perkins <[email protected]>
willyborankin
willyborankin previously approved these changes Oct 31, 2024
@cwperks
Copy link
Member Author

cwperks commented Oct 31, 2024

Looking into test failures. Possibly related to change that went into core: opensearch-project/OpenSearch#16418

@cwperks
Copy link
Member Author

cwperks commented Oct 31, 2024

Looking into test failures. Possibly related to change that went into core: opensearch-project/OpenSearch#16418

Confirmed that this PR is causing test failures in the security repo. I don't understand why just yet. @reta Putting this on your radar.

@cwperks
Copy link
Member Author

cwperks commented Oct 31, 2024

The gist of the issue is that when the security index (or other system index created in test) is created its expected to have a shard on every node. It seems that now a shard is only created on a single node.

This is the logic to create the security index: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/support/SecurityIndexHandler.java#L73-L88

@reta
Copy link
Collaborator

reta commented Oct 31, 2024

Confirmed that this PR is causing test failures in the security repo. I don't understand why just yet. @reta Putting this on your radar.

Thanks @cwperks , taking a look shortly, sorry about that

@cwperks
Copy link
Member Author

cwperks commented Oct 31, 2024

Observation. When I comment these lines out and then put a breakpoint here, this is what I get:

v1Template: [{"default":{"order":-1,"index_patterns":["*"],"settings":{"index":{"number_of_shards":"5","number_of_replicas":"1"}},"mappings":{},"aliases":{}}}]

@reta
Copy link
Collaborator

reta commented Oct 31, 2024

Observation. When I comment these lines out and then put a breakpoint here, this is what I get:

The templates are not applied to system indices after opensearch-project/OpenSearch#16418, seems to be regression.

@cwperks
Copy link
Member Author

cwperks commented Oct 31, 2024

Where is the default template defined in core?

In the debugger, I see this as the default template:

{"default":{"order":-1,"index_patterns":["*"],"settings":{"index":{"number_of_shards":"<number_of_nodes>","number_of_replicas":"1"}},"mappings":{},"aliases":{}}}

@reta
Copy link
Collaborator

reta commented Oct 31, 2024

Where is the default template defined in core?

It is created by security plugin test scaffolding (see please org.opensearch.security.test.helper.cluster.ClusterHelper::startCluster):

        final AcknowledgedResponse templateAck = nodeClient().admin()
            .indices()
            .putTemplate(new PutIndexTemplateRequest("default").source(defaultTemplate, XContentType.JSON))
            .actionGet();

Signed-off-by: Craig Perkins <[email protected]>
Copy link

codecov bot commented Oct 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.89%. Comparing base (42f2332) to head (5cb0a91).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4867      +/-   ##
==========================================
+ Coverage   69.84%   69.89%   +0.04%     
==========================================
  Files         320      320              
  Lines       21674    21688      +14     
  Branches     3457     3460       +3     
==========================================
+ Hits        15139    15159      +20     
+ Misses       4744     4735       -9     
- Partials     1791     1794       +3     

see 3 files with indirect coverage changes

@willyborankin willyborankin merged commit 863b990 into opensearch-project:main Nov 1, 2024
42 checks passed
@willyborankin
Copy link
Collaborator

@cwperks not sure about backporting it in 2.x. It is up to you

@cwperks
Copy link
Member Author

cwperks commented Nov 1, 2024

@cwperks not sure about backporting it in 2.x. It is up to you

I'll create a backport for this to the 2.x branch.

cwperks added a commit that referenced this pull request Nov 1, 2024
…4868)

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: opensearch-trigger-bot[bot] <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com>
Co-authored-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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants