SOLR-18317: Admin UI no longer sends nodes=all in standalone mode

[nick-boss-tech]

🤖 AI text below 🤖 (posted on behalf of Nick Shanin)

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

What happens today

In standalone (user-managed) mode, the Admin UI logging page sent nodes=all to /admin/info/logging. Standalone mode has no ZkController, so the request hit a NullPointerException server-side instead of being handled sensibly. The parameter only makes sense in SolrCloud mode, where it broadcasts the logging change to all nodes.

What this change does

Admin UI only. This PR previously also carried a server-side change; that part was split out at reviewer request (see Limits).

• services.js: the Logging resource factory no longer hardcodes nodes: 'all'.
• controllers/logging.js: LoggingLevelController.setLevel waits for isCloudEnabled to settle (the same $watch pattern used in paramsets.js), sends nodes: 'all' only in SolrCloud mode, and omits the parameter entirely in standalone mode.
• app.js: when the system-info request behind the UI mode check fails, isCloudEnabled now settles to false instead of leaving the mode unresolved, so the logging page does not wait indefinitely on a failed probe.

Proof

There is no JavaScript test harness for the legacy Admin UI in the build, so this change has no automated proof; that is stated plainly rather than worked around. Of the three JavaScript files, logging.js and services.js are unchanged from the earlier head of this PR; the app.js settle-on-failure change is new in this cut. The controller change follows the existing $watch pattern already used elsewhere in the same UI. Manual validation in a standalone instance is the check, and the reviewer has offered to do it.

Limits

• Server-side behavior is unchanged by this PR. A direct nodes=all call to the logging or system-info endpoints in standalone mode still reaches the old code path. The server-side hardening that degraded such calls to the local node was split out at reviewer request and is preserved intact on the fork branch solr-18317-server-submit, to be opened as its own PR.
• The app.js settle change is not local to the logging page: paramsets.js and query.js watch the same isCloudEnabled flag with the same wait-until-defined pattern. After a failed system-info probe on a SolrCloud node, those pages now proceed with the cores index type instead of waiting for the flag to be defined. On the logging page itself, a level change after a failed probe is sent without nodes=all, so in a cloud cluster it applies to the receiving node only, with no message at the click; the probe failure itself is surfaced by the error handler.
• This PR fixes the logging page's caller, which is where the hardcoded nodes: 'all' was.

Changelog: changelog/unreleased/SOLR-18317.yml (fixed)

AI assistance

AI agents assisted with research, implementation, review, and drafting. Nick Shanin directed the work and takes responsibility for this contribution.

[epugh]

What if we just fix the Solr Admin UI? Is nodes=all a common parameter that lots of places ahve that could potentially be fixed through this approach? There is a lot of mock tests and more checks, and I just wonder if we made the one place that causes this issue to be a bit smarter on the caller?

Currently in the Solr Admin UI we have lots of places where we consult to decide "am I making this url with a core int he name or a collection in the name"... So understnadin that is there. I'd rather just reuse that logic and then not pass nodes=all to a standalone, after all, that just doens't make any sense in user-managed (standalone) mode. (gotta love how we use two names for the same thing everywhere).

[nick-boss-tech]

🤖 AI text below 🤖 (posted on behalf of Nick Shanin)

Thanks, Eric. You were right that the caller was the real problem; my first version reached for a shared server-side change that was broader than the bug. I reworked it along the lines you suggested (latest commit f887bed):

• The Admin UI is now the primary fix. LoggingLevelController.setLevel waits for isCloudEnabled to settle (the same $watch pattern paramsets.js uses to decide cores vs collections), sends nodes: 'all' only in SolrCloud mode, and omits the parameter entirely in standalone mode. The Logging $resource factory no longer hardcodes nodes:'all'.
• I kept a much narrower server-side guard, because a direct /admin/info/logging?nodes=all call in standalone mode still NPEd on the missing ZooKeeper handle before this PR. The "all means this node" behavior is an opt-in that both proxy base classes leave off by default; the three broadcast-style endpoints (logging and system info on v1, node system info on v2) enable it where they construct their proxies. Explicit node names in standalone mode still get a clear 400, and the v2 handler no longer wraps that 400 into a 500.
• On your question about how common nodes=all is: inside this proxy framework it is only the logging and system-info endpoints on the plural nodes parameter, plus metrics on the singular node. For metrics I deliberately did not extend the "all means local" meaning to standalone mode. node selects destination nodes for a metrics scrape rather than broadcasting a change, so in standalone mode it now gets a clean 400 ("Parameter 'node' requires SolrCloud") instead of the old NPE. (In SolrCloud, node=all fans out to all live nodes via the shared validator; that is unchanged.) That decision is explicit in MetricsHandler.createMetricProxy and pinned by new tests in MetricsHandlerTest.

Tests: the two proxy test classes, LoggingHandlerTest (with the original standalone regression), and the new metrics tests all pass (32 tests), and :solr:core:check -x test is green. For the UI change, since the legacy Admin UI has no JS test harness in the repo, I ran the real controllers/logging.js in Node with the Angular plumbing stubbed: cloud mode sends nodes: 'all', standalone omits it, and an unresolved mode flag sends nothing until it settles (12 assertions, all passing; the harness is ad hoc and uncommitted, happy to include it if there is a preferred place).

Happy to drop the server-side guard entirely if you would rather the API stay strict now that the UI no longer sends the parameter in standalone mode.

[epugh]

Can we split out the Admin UI fix into a new PR? I can pretty easily test and validate your fix if it's just the Admin UI change... I'm not sure I'm ready to dig into all that proxy stuff ;-). Or drop the proxy stuff....

I don't use the Standalone mode, and I want it to go away in favour of single node SolrCloud w/ embedded ZK, so I'm not really motivated to dig into making it better (though other committers might want to!), but I'm happy to test and validate the small Admin UI fix.

[nick-boss-tech]

🤖 AI text below 🤖 (posted on behalf of Nick Shanin)

Done: this PR is now narrowed to the Admin UI change only, and the description is updated to match. The server-side proxy changes are preserved on a separate branch for a later PR. Thank you for the suggestion, and for offering to validate the fix.