SOLR-15748 : Created v2 equivalent of v1 'CLUSTERSTATUS' command #1061

joshgog · 2022-10-10T22:18:59Z

https://issues.apache.org/jira/browse/SOLR-15748

Description

Create a v2 equivalent of 'CLUSTERSTATUS' command under the v1 /solr/admin/collections endpoint.

Solution

Added an endpoint @endpoint(method = GET, path = "/cluster", permission = COLL_READ_PERM) to the ClusterAPI

Tests

Tested mapping to the API endpoint.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

sonatype-lift · 2022-10-10T23:07:28Z

⚠️ 314 God Classes were detected by Lift in this project. Visit the Lift web console for more details.

This reverts commit 32a37f6.

gerlowskija

Looks great; except for one question/suggestion I had. Take a look when you get a chance and let me know what you think.

gerlowskija · 2022-10-12T15:35:27Z

solr/core/src/java/org/apache/solr/handler/ClusterAPI.java

@@ -261,6 +263,13 @@ public void getNodes(SolrQueryRequest req, SolrQueryResponse rsp) {
    rsp.add("nodes", getCoreContainer().getZkController().getClusterState().getLiveNodes());
  }

+  @EndPoint(method = GET, path = "/cluster/cluster-status",  permission = COLL_READ_PERM)


[+1] The method body itself here looks great 👍

[Q] My only question is about the path string you chose: "/cluster/cluster-status".

Did you pick this because "/cluster" was already taken by the getCluster method down below? Or was there another reason?

Assuming the main factor was the conflict with getCluster, I'd vote to nuke that method and change the path string here to be "/cluster".

For reasons that I don't quite understand, the v2 API has two endpoints that expose the "List Collections" functionality: ClusterAPI.getCluster which we see below uses the "/cluster" path, and CollectionsAPI.getCollections here which uses the path "/collections".

There's absolutely no reason for that afaict, and "cluster status" is a very natural thing to expose at "/cluster". So IMO we should be safe to delete getCluster in this PR.

Did you pick this because "/cluster" was already taken by the getCluster method down below? Or was there another reason?

Yes that's the main reason, "/cluster" had already been mapped to a method.

For reasons that I don't quite understand, the v2 API has two endpoints that expose the "List Collections" functionality: ClusterAPI.getCluster which we see below uses the "/cluster" path, and CollectionsAPI.getCollections here which uses the path "/collections".

Exactly. It was a challenge understing the same while working on the issue.

There's absolutely no reason for that afaict, and "cluster status" is a very natural thing to expose at "/cluster". So IMO we should be safe to delete getCluster in this PR.

Will do that

gerlowskija · 2022-10-12T16:01:06Z

Hey @joshgog - thanks for the PR; looks good so far! A few quick questions for you:

I noticed you left the ./gradlew check box unchecked on the checklist in the PR description. Running check isn't strictly required, as most committers will run it themselves before merging code. But it does help give your reviewer a bit of extra confidence as it signals: "Hey, the code quality checks and tests and all that passed for me, so this should be ready to review". So if you're willing, feel free to give that a try when you get a few minutes and let me know how it went.
End-user-visible changes in Solr usually get mentioned in a "change-log" file that lives at solr/CHANGES.txt. This file exists both to give users an idea of what might've changed release-to-release, but also as a way to give credit and recognition to all the contributors like you who helped. If you get a few minutes, could you document this change by adding a bullet point under the "Improvements" section for the 9.2.0 release? Alternately, if you don't have time or aren't sure what to write, I'm happy to draft that myself if you can tell me what name you'd like to be credited under. (Some people use their full name, some people use a pseudonym or username, etc.)
Lastly, since we're changing a Solr API we should probably update the documentation anywhere it might be mentioned. Solr's main documentation, the "Solr Reference Guide" lives alongside the code in this repo at solr/solr-ref-guide. It can be built using the command ./gradlew buildLocalSite - more detail on this in the README here. The one place in particular that I know needs updated is the section here. Could you take a crack at updating that and let me know how it goes?

joshgog · 2022-10-13T02:09:55Z

I noticed you left the ./gradlew check box unchecked on the checklist in the PR description. Running check isn't strictly required, as most committers will run it themselves before merging code. But it does help give your reviewer a bit of extra confidence as it signals: "Hey, the code quality checks and tests and all that passed for me, so this should be ready to review". So if you're willing, feel free to give that a try when you get a few minutes and let me know how it went.

There was one failed test. JWT security issues. I'm rerunning the check to note the details of the error. Where is the test log located? To avoid rerunning the check.

2. End-user-visible changes in Solr usually get mentioned in a "change-log" file that lives at solr/CHANGES.txt. This file exists both to give users an idea of what might've changed release-to-release, but also as a way to give credit and recognition to all the contributors like you who helped. If you get a few minutes, could you document this change by adding a bullet point under the "Improvements" section for the 9.2.0 release? Alternately, if you don't have time or aren't sure what to write, I'm happy to draft that myself if you can tell me what name you'd like to be credited under. (Some people use their full name, some people use a pseudonym or username, etc.)

I have documented the changes.

3. Lastly, since we're changing a Solr API we should probably update the documentation anywhere it might be mentioned. Solr's main documentation, the "Solr Reference Guide" lives alongside the code in this repo at solr/solr-ref-guide. It can be built using the command ./gradlew buildLocalSite - more detail on this in the README here. The one place in particular that I know needs updated is the section here. Could you take a crack at updating that and let me know how it goes?

I have updated the reference guide

joshgog · 2022-10-13T02:47:35Z

There was one failed test. JWT security issues. I'm rerunning the check to note the details of the error. Where is the test log located? To avoid rerunning the check.

The ./gradlew check rerun was successful. No failed tests

janhoy · 2022-10-13T08:08:41Z

solr/core/src/java/org/apache/solr/handler/ClusterAPI.java

@@ -261,6 +257,13 @@ public void getNodes(SolrQueryRequest req, SolrQueryResponse rsp) {
    rsp.add("nodes", getCoreContainer().getZkController().getClusterState().getLiveNodes());
  }

+  @EndPoint(method = GET, path = "/cluster", permission = COLL_READ_PERM)


The COLL_READ_PERM (collection-admin-read) permission here stems from when CLUSTERSTATUS was a collection-api command.

The content of clusterstatus is mostly collection data, i.e. a list of collections, their shards and replicas and their status. So I'm OK with keeping this permission although it is now located under/api/cluster. I'm thinking of the CloudSolrClient SolrJ client that will read cluster status in order to route index/search requests.

Any thoughts?

Since I'm still learning the code base and how Solr works, I'll have to look into how the CloudSolrClient works to better understand your point and put across my thoughts.

I'm not expecting you to have an opinion on this detail. Solr's security framework is a bit involved :) But if a committer has an opinion I'm happy to hear it.

Are you getting at the fact that COLL_READ_PERM/collection-admin-read is a bad name in the v2 world where /admin/collections no longer exists? Or are you suggesting that it might be nice to give "cluster status" a different permission group so that CSC's can be setup to require fewer permissions? Or something else altogether?

If the former, then I totally agree. I think it still makes sense to have a permission group like COLL_READ_PERM going forward, but the name would need to be rethought. Regardless of the underlying api paths or the permission name though, I think users will still find it valuable to group various admin-read functionality in a single permission-group.

In terms of having a "slimmer" predefined permission group for HttpClusterStateProvider-based CloudSolrClients to use, that seems like it might be useful. Though, to play devil's advocate, IMO HttpClusterStateProvider is sort of a minority use-case, and users that want to run a HCSP without giving it full COLL_READ_PERMs can already do so by defining "custom permissions". So I could go either way I guess 🤷

In either case - IMO making changes to our predefined permissions is definitely a bigger task that we'd want to be a separate JIRA for sure.

gerlowskija

No failed tests ... I have updated the reference guide ... I have documented the changes.

🎉 Awesome, thanks for the quick follow up! I made some minor tweaks to the CHANGES.txt wording - primarily to mention where the collection-listing functionality that used to be at /api/cluster can still be found.

But everything else looks great. I'm going to run tests locally as a double-check, but then I think this is ready to merge. I'll target early next week - should give Jan or whoever else is interested in reviewing a chance to take a look if they want to.

gerlowskija · 2022-10-13T17:22:35Z

(Tests and 'check' pass for me 👍 )

epugh

LGTM

epugh · 2022-10-13T18:29:10Z

I did also check, the ref guide does mention /api/collections for listing out collections!

joshgog · 2022-10-13T18:39:33Z

It seems the action LIST and action CLUSTERSTATUS return the same result. No?

gerlowskija · 2022-10-13T19:31:40Z

CLUSTERSTATUS returns a superset of the information returned by list. That is, LIST returns the collection names only. CLUSTERSTATUS returns not just the collection names, but the shards and replicas that make them up, each of their "statuses", the nodes that make up the cluster, etc.

Are you seeing them return the same info? If so, can you share curl commands or something similar to test out?

joshgog · 2022-10-14T01:47:05Z

Are you seeing them return the same info?

II had not tested LIST. They do return different info

joshgog added 2 commits October 12, 2022 07:01

Created v2 equivalent of v1 'CLUSTERSTATUS' command

32a37f6

Added endpoint to ClusterAPI

8c58a8e

joshgog force-pushed the SOLR-15748 branch from fb50537 to 8c58a8e Compare October 12, 2022 10:46

Revert "Created v2 equivalent of v1 'CLUSTERSTATUS' command"

a3d617d

This reverts commit 32a37f6.

gerlowskija requested changes Oct 12, 2022

View reviewed changes

joshgog marked this pull request as draft October 12, 2022 16:06

joshgog added 4 commits October 12, 2022 20:03

Updated endpoint path

5fada81

Documented the changes

7c92693

Added v2 API to ref-doc

bdee3fa

gradle tidy

6d0d404

joshgog marked this pull request as ready for review October 13, 2022 02:10

joshgog requested a review from gerlowskija October 13, 2022 04:51

janhoy reviewed Oct 13, 2022

View reviewed changes

gerlowskija approved these changes Oct 13, 2022

View reviewed changes

Amend CHANGES.txt entry

5c58784

epugh approved these changes Oct 13, 2022

View reviewed changes

Merge branch 'main' into SOLR-15748

3765e00

gerlowskija merged commit 79fba55 into apache:main Oct 18, 2022

gerlowskija pushed a commit that referenced this pull request Oct 20, 2022

SOLR-15748 : Created v2 equivalent of v1 'CLUSTERSTATUS' command (#1061)

82e642e

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-15748 : Created v2 equivalent of v1 'CLUSTERSTATUS' command #1061

SOLR-15748 : Created v2 equivalent of v1 'CLUSTERSTATUS' command #1061

joshgog commented Oct 10, 2022 •

edited

sonatype-lift bot commented Oct 10, 2022

gerlowskija left a comment

gerlowskija Oct 12, 2022

joshgog Oct 12, 2022

gerlowskija commented Oct 12, 2022

joshgog commented Oct 13, 2022

joshgog commented Oct 13, 2022

janhoy Oct 13, 2022

joshgog Oct 13, 2022

janhoy Oct 13, 2022

gerlowskija Oct 13, 2022 •

edited

gerlowskija left a comment

gerlowskija commented Oct 13, 2022

epugh left a comment

epugh commented Oct 13, 2022

joshgog commented Oct 13, 2022

gerlowskija commented Oct 13, 2022

joshgog commented Oct 14, 2022

SOLR-15748 : Created v2 equivalent of v1 'CLUSTERSTATUS' command #1061

SOLR-15748 : Created v2 equivalent of v1 'CLUSTERSTATUS' command #1061

Conversation

joshgog commented Oct 10, 2022 • edited

Description

Solution

Tests

Checklist

sonatype-lift bot commented Oct 10, 2022

gerlowskija left a comment

Choose a reason for hiding this comment

gerlowskija Oct 12, 2022

Choose a reason for hiding this comment

joshgog Oct 12, 2022

Choose a reason for hiding this comment

gerlowskija commented Oct 12, 2022

joshgog commented Oct 13, 2022

joshgog commented Oct 13, 2022

janhoy Oct 13, 2022

Choose a reason for hiding this comment

joshgog Oct 13, 2022

Choose a reason for hiding this comment

janhoy Oct 13, 2022

Choose a reason for hiding this comment

gerlowskija Oct 13, 2022 • edited

Choose a reason for hiding this comment

gerlowskija left a comment

Choose a reason for hiding this comment

gerlowskija commented Oct 13, 2022

epugh left a comment

Choose a reason for hiding this comment

epugh commented Oct 13, 2022

joshgog commented Oct 13, 2022

gerlowskija commented Oct 13, 2022

joshgog commented Oct 14, 2022

joshgog commented Oct 10, 2022 •

edited

gerlowskija Oct 13, 2022 •

edited