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

Update PluginStats with new field in response optional_extended_plugins and upgrade to 2.19.0 #814

Merged
merged 20 commits into from
Feb 28, 2025

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Feb 12, 2025

Description

A new field was added to the output of the response to GET _cat/plugins for optional_extended_plugins in 2.19.

See opensearch-project/OpenSearch#16909 (comment) and opensearch-project/opensearch-go#669

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: Craig Perkins <cwperx@amazon.com>
Copy link
Contributor

github-actions bot commented Feb 12, 2025

Changes Analysis

Commit SHA: 68942c6
Comparing To SHA: 5ed668d

API Changes

Summary

├─┬Paths
│ ├─┬/_plugins/_flow_framework/workflow/{workflow_id}
│ │ └─┬DELETE
│ │   └─┬Responses
│ │     └─┬200
│ │       └─┬application/json
│ │         └─┬Schema
│ │           └──[🔀] $ref (36804:13)❌ 
│ └─┬/_plugins/_knn/models/_search
│   ├─┬GET
│   │ ├─┬Parameters
│   │ │ └─┬Schema
│   │ │   └──[🔀] $ref (36216:9)❌ 
│   │ ├─┬Parameters
│   │ │ └─┬Schema
│   │ │   └──[🔀] $ref (36589:13)❌ 
│   │ └─┬Parameters
│   │   └─┬Schema
│   │     └──[🔀] $ref (44523:13)❌ 
│   └─┬POST
│     ├─┬Parameters
│     │ └─┬Schema
│     │   └──[🔀] $ref (36589:13)❌ 
│     ├─┬Parameters
│     │ └─┬Schema
│     │   └──[🔀] $ref (44523:13)❌ 
│     └─┬Parameters
│       └─┬Schema
│         └──[🔀] $ref (36216:9)❌ 
└─┬Components
  ├──[➖] schemas (55908:7)❌ 
  ├──[➖] schemas (56051:7)❌ 
  ├──[➖] schemas (56028:7)❌ 
  ├──[➖] schemas (51772:7)❌ 
  ├──[➖] schemas (51665:7)❌ 
  ├──[➕] schemas (55951:7)
  ├──[➕] schemas (51754:7)
  ├──[➕] schemas (55263:7)
  ├─┬flow_framework.common___SearchWorkflowRequest
  │ └─┬query
  │   └──[🔀] $ref (44642:13)❌ 
  ├─┬knn._common___TrainedModel
  │ └─┬method
  │   └──[🔀] $ref (55951:13)❌ 
  ├─┬flow_framework.common___SearchStateResponse
  │ ├──[➕] properties (51777:9)
  │ ├──[➕] properties (51775:9)
  │ ├──[➕] properties (51779:9)
  │ ├─┬state
  │ │ └──[🔀] $ref (34678:20)❌ 
  │ ├─┬user
  │ │ └──[🔀] $ref (51770:11)❌ 
  │ ├─┬workflow_id
  │ │ └──[🔀] $ref (51754:13)❌ 
  │ └─┬provisioning_progress
  │   └──[🔀] $ref (34678:20)❌ 
  ├─┬_common___PluginStats
  │ ├──[➕] properties (35621:9)
  │ └─┬version
  │   └──[🔀] $ref (35622:11)❌ 
  └─┬insights._common___TopQuery
    ├──[➕] properties (55454:9)
    ├──[➕] properties (55452:9)
    ├─┬query_hashcode
    │ └──[🔀] description (55481:24)
    ├─┬node_id
    │ ├──[🔀] type (55471:17)❌ 
    │ └──[🔀] description (55472:24)
    ├─┬search_type
    │ ├──[🔀] type (55488:17)❌ 
    │ └──[➖] description (55476:24)
    ├─┬total_shards
    │ ├──[➕] items (55465:13)❌ 
    │ ├──[🔀] type (55463:17)❌ 
    │ └──[➖] description (55462:24)
    ├─┬labels
    │ ├──[🔀] type (55453:17)❌ 
    │ └──[➖] description (55473:24)
    ├─┬measurements
    │ └──[🔀] $ref (55483:11)❌ 
    ├─┬timestamp
    │ └──[🔀] $ref (55291:13)❌ 
    ├─┬source
    │ └──[🔀] $ref (55468:11)❌ 
    ├─┬indices
    │ └──[🔀] $ref (55263:13)❌ 
    ├─┬task_resource_usages
    │ ├──[➖] items (55466:13)❌ 
    │ ├──[🔀] type (55474:17)❌ 
    │ └──[➕] description (55475:24)
    └─┬phase_latency_map
      └──[🔀] $ref (55279:13)❌ 

Document Element Total Changes Breaking Changes
paths 7 7
components 39 24
  • BREAKING Changes: 31 out of 46
  • Modifications: 26
  • Removals: 9
  • Additions: 11
  • Breaking Removals: 6
  • Breaking Modifications: 24
  • Breaking Additions: 1

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/13576974296/artifacts/2666903685

API Coverage

Before After Δ
Covered (%) 663 (64.94 %) 663 (64.94 %) 0 (0 %)
Uncovered (%) 358 (35.06 %) 358 (35.06 %) 0 (0 %)
Unknown 45 45 0

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@dblock
Copy link
Member

dblock commented Feb 12, 2025

See tests failing please.

Any way we can actually test this field in a response? We have a way to install plugins, you can inspire yourself from https://github.com/opensearch-project/opensearch-api-specification/tree/main/tests/plugins.

Update https://github.com/opensearch-project/opensearch-api-specification/blob/main/.github/workflows/test-spec.yml to use 2.19 as stable (replace all 2.18s). Use a 2.20 (even thought there's no plan for such a thing) from dockerhub for the next version.

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@cwperks
Copy link
Member Author

cwperks commented Feb 13, 2025

See tests failing please.

Any way we can actually test this field in a response? We have a way to install plugins, you can inspire yourself from https://github.com/opensearch-project/opensearch-api-specification/tree/main/tests/plugins.

Update https://github.com/opensearch-project/opensearch-api-specification/blob/main/.github/workflows/test-spec.yml to use 2.19 as stable (replace all 2.18s). Use a 2.20 (even thought there's no plan for such a thing) from dockerhub for the next version.

There is an existing GET _cat/plugins test, but the only assertion is 200 for the status code of the response.

I'll see if I can strengthen the existing assertion.

@cwperks
Copy link
Member Author

cwperks commented Feb 13, 2025

Is the default test group supposed to contain tests for knn plugin?

ERROR   knn/models.yaml (tests/default/knn/models.yaml)
    ERROR   PROLOGUES
        PASSED  PUT /movies
        PASSED  POST /_bulk
        ERROR   POST /_plugins/_knn/models/model-1/_train (Validation Failed: 1: Number of training points should be greater than 1000;)

ref: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/13298096662/job/37134385075?pr=814

@dblock
Copy link
Member

dblock commented Feb 13, 2025

There is an existing GET _cat/plugins test, but the only assertion is 200 for the status code of the response.

These tests check all schema. Does the test fail against 2.19 without your change? If so you're all set.

@dblock
Copy link
Member

dblock commented Feb 13, 2025

Is the default test group supposed to contain tests for knn plugin?

Yes, the different groups are just different prerequisites.

Signed-off-by: Craig Perkins <cwperx@amazon.com>
payload:
nodes:
plugins:
- name: opensearch-cross-cluster-replication
Copy link
Member Author

Choose a reason for hiding this comment

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

@dblock I can't find a good way to test this. Is there a way I can assert that the analysis-phonenumber plugin exists in this list w/o caring about the other entries?

Similarly, how can I make the version, opensearch_version and java_version assertions to be agnostic here?

Copy link
Member

@dblock dblock Feb 14, 2025

Choose a reason for hiding this comment

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

I don't think the tooling can do this today, I guess don't try too hard. Open an issue for any missing features that could help in the tester.

Should be able to include the type of the expectation, not just the value, e.g. to test that version is present and is a string:

info:
   version:
      type: string
   additionalProperties: true

fun one to code btw :)

Copy link
Member Author

@cwperks cwperks Feb 20, 2025

Choose a reason for hiding this comment

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

@dblock would there be a way to build a generic contains assertion into the framework? I'm imagining something very generic like: this response contains the strings analysis-phonenumber AND optional_extended_plugins in the body of the response.

Copy link
Member

@dblock dblock Feb 21, 2025

Choose a reason for hiding this comment

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

We do a lot of JSON schema validation here which is the example I gave above. I would not try to implement another style validation because it would be reinventing the wheel, so instead I suggest extending the existing exact check to support a specific sub-schema.

Copy link
Member

@dblock dblock Feb 21, 2025

Choose a reason for hiding this comment

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

btw, @cwperks if you want to try and implement this, start by adding a test to tools/tests/tester/ResponsePayloadEvaluator.test.ts.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was just looking into that class to see how the payload evaluator work. Scoping out the effort now and hopefully will push another commit later today where we can reliably assert that the new field exists on the response.

… values

Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

I am opposed to the “contains” implementation because it invents a whole new DSL and we already have something better that can assert contains, called json schema validation.

Check whether the response matches exactly (what we do today), or whether it also matches another schema: declared in the test file.

Feel free to open an issue on needing to support contains and split that from the PR.

This reverts commit e555bc2.

Signed-off-by: Craig Perkins <cwperx@amazon.com>
…f target values"

This reverts commit e9c9eb1.

Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Needs a little bit of work to turn green.

@dblock
Copy link
Member

dblock commented Feb 24, 2025

@cwperks Looks like more failures in other plugins associated with the version upgrade. I would do the 2.19 upgrade separately, maybe @nhtruong or @Xtansia can take it, ya'll talk.

@cwperks
Copy link
Member Author

cwperks commented Feb 26, 2025

@cwperks Looks like more failures in other plugins associated with the version upgrade. I would do the 2.19 upgrade separately, maybe @nhtruong or @Xtansia can take it, ya'll talk.

@nhtruong @Xtansia is there a separate effort for the version upgrade in this repo? I'd offer to lend a hand, but I'm not as familiar with the components that have failing tests.

@Xtansia
Copy link
Collaborator

Xtansia commented Feb 26, 2025

@cwperks I can take a look at it

Signed-off-by: Thomas Farr <tsfarr@amazon.com>
Signed-off-by: Thomas Farr <tsfarr@amazon.com>
@Xtansia Xtansia force-pushed the optional-extended-plugins branch from da71086 to 6f7acf8 Compare February 27, 2025 00:42
Signed-off-by: Thomas Farr <tsfarr@amazon.com>
Signed-off-by: Thomas Farr <tsfarr@amazon.com>
@Xtansia Xtansia force-pushed the optional-extended-plugins branch from 8c03125 to fe82e28 Compare February 27, 2025 02:12
Signed-off-by: Thomas Farr <tsfarr@amazon.com>
@Xtansia Xtansia force-pushed the optional-extended-plugins branch 2 times, most recently from 6e627f3 to 490f994 Compare February 27, 2025 03:25
Signed-off-by: Thomas Farr <tsfarr@amazon.com>
@Xtansia Xtansia force-pushed the optional-extended-plugins branch from 0b7859c to 50f81b7 Compare February 27, 2025 04:07
@cwperks cwperks changed the title Update PluginStats with new field in response optional_extended_plugins Update PluginStats with new field in response optional_extended_plugins and upgrade to 2.19.0 Feb 27, 2025
Signed-off-by: Thomas Farr <tsfarr@amazon.com>
Copy link
Contributor

Spec Test Coverage Analysis

Total Tested
595 593 (99.66 %)

@Xtansia Xtansia requested a review from dblock February 27, 2025 21:22
@Xtansia
Copy link
Collaborator

Xtansia commented Feb 27, 2025

@dblock @nhtruong Can one of you please give this a review?

nhtruong
nhtruong previously approved these changes Feb 27, 2025
Signed-off-by: Thomas Farr <tsfarr@amazon.com>
@dblock dblock merged commit fcbfc10 into opensearch-project:main Feb 28, 2025
32 checks passed
@Xtansia Xtansia deleted the optional-extended-plugins branch March 2, 2025 20:53
@Xtansia Xtansia restored the optional-extended-plugins branch March 2, 2025 20:54
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