Skip to content

SOLR-15823: Broadcast V2 logging level changes to named nodes and migrate the Admin UI to V2 - #5030

Open
nick-boss-tech wants to merge 5 commits into
apache:mainfrom
nick-boss-tech:solr-15823-submit
Open

nick-boss-tech wants to merge 5 commits into
apache:mainfrom
nick-boss-tech:solr-15823-submit

Conversation

@nick-boss-tech

@nick-boss-tech nick-boss-tech commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

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

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

What happens today

The V2 logging endpoint PUT /api/node/logging/levels only ever applies level changes on the node that receives the request (NodeLogging.java:87-99); NodeLogging carries a TODO to add a nodes parameter once SOLR-16738 lands (NodeLogging.java:47), and that proxy machinery has since landed (the node system info endpoint uses it (GetNodeSystemInfo.java:70-83)). Because of the gap, the Admin UI logging screen still sets levels through the V1 handler in SolrCloud mode (logging.js:164-175), since only V1 understands nodes=all.

The V1 handler has a second route into the same code: it applies level changes whenever a request carries set parameters (LoggingHandler.java:75-79), including on GET requests. Eric Pugh noted on the ticket (2026-10-05) that he stumbled over this while testing: he went looking for his POST or PUT in the browser's network log and realized the GET was changing the log level. This PR does not change that behavior; every GET works exactly as it does today.

What this change does

The set-level endpoint accepts a nodes query parameter (NodeLoggingApis.java:41-51). When it is present, NodeLogging fans the request out to the named nodes (or every live node for nodes=all) through V2SolrRequestBasedProxy (NodeLogging.java:123-133), mirroring GetNodeSystemInfo (GetNodeSystemInfo.java:70-83), and the response carries one result per node, keyed by node name, plus a failedNodes list naming any requested nodes that did not respond (LoggingResponse.java:31-56). A named node that is not part of the cluster is rejected with a 400 before anything is sent (NodeLogging.java:134-145), so an unknown name cannot half-apply the change. A node that leaves the live set while a broadcast is in flight is a narrower case the pre-check cannot cover; it is in Limits. Using nodes on a standalone node is also a 400, not an NPE (NodeLogging.java:118-121). When nodes is absent the local-only path is unchanged (NodeLogging.java:100-108), and, again mirroring system info, the receiving node does not also apply the change locally when nodes is present; nodes=all covers it because the resolved set includes it (NodeLogging.java:111-114). The Admin UI logging screen now calls the V2 endpoint in both modes (nodes: "all" only in cloud mode) (logging.js:163-174), and the V1 Logging factory is deleted (commit a76e30e48db). The reference guide's logging page documents the parameter (configuring-logging.adoc:105-116).

Proof

Verified 2026-10-05 at head 85d8df8 (base e432df1). The top commit adds test coverage only, on top of 100ad2e; no production or UI code changed in it.

  • NodeLoggingNodesSolrCloudTest (new, 2-node cluster) (NodeLoggingNodesSolrCloudTest.java): 4/4 with this change. Against base production code the same tests fail 3 of 4 for the stated reason: no broadcast happens, the response covers only the receiving node, and an unknown node name is silently treated as a local request. The fourth test, which pins the no-nodes local response shape, passes on base too. Assertions run against the response body, since the test cluster's nodes share one JVM and log levels are JVM-wide, so level state is not observable per node. The broadcast case also checks the listing afterwards and finds the new level in effect (JVM-wide is the only way it is observable in this cluster), then restores the logger to unset.
  • NodeLoggingAPITest (NodeLoggingAPITest.java): 8/8, including the standalone nodes guard, the unchanged local response shape, and a broadcast case in which one node's proxied request fails: the response carries the answering node's result and names the failed node in failedNodes.
  • LoggingHandlerTest (LoggingHandlerTest.java): 1/1 (the V1 handler passes a null nodes (LoggingHandler.java:77-78), so V1 does not broadcast twice).
  • :solr:core:check and :solr:api:check -x test pass; Error Prone compile is clean. The generated SolrJ client exposes setNodes and forwards the request body through the proxy (NodeLogging.java:123-125, V2SolrRequestBasedProxy.java:58-71), which the cloud test exercises end to end.
  • The Selenium suites for this screen passed at 100ad2e (run 2026-10-05 on Windows with Chrome 153, -Ptests.selenium=true); the one commit since then adds tests only, and the UI code is unchanged: AdminUiLoggingScreenTest (AdminUiLoggingScreenTest.java) 3/3 and AdminUiLoggingStandaloneTest (AdminUiLoggingStandaloneTest.java) 1/1. Both include testChangeLogLevelViaUi, which clicks a level in the UI and waits for the server to report it, so the UI's request is exercised end to end in cloud and standalone mode.

Choices to check

  1. One PR, or split? This PR lands the API change and the UI migration together, as the ticket's acceptance criteria list both. Our position: one PR, because the UI switch is small and is the point of the API change. We are happy to split it into an API PR and a UI PR that follows if reviewers prefer.
  2. Broadcast response shape on partial failure. We return a per-node result mirroring the system info response, with nodes that did not respond named in failedNodes; the alternative is a single overall status. Our position: the per-node shape, because the caller needs to know which nodes still run the old levels. Happy to change the shape if another form is preferred.
  3. Scope of nodes. Only the set-level PUT honors nodes here, as the ticket names. The other endpoints (the level and message GETs and the message threshold PUT (NodeLoggingApis.java:34-65)) could take it too. One caution from the ticket discussion: in V1 the GET is itself a level-changing route when it carries set parameters (LoggingHandler.java:75-79), which Eric Pugh flagged after stumbling over it in testing, and the ticket's verb-split question (changes on PUT or POST, retrieval on GET) is still open. We kept the ticket's scope, left every GET behaving exactly as it does today, and did not let this PR become the place that settles the GET question. Our plan for the read side: the levels GET, which is the read half of the screen this PR migrates and the one place V1 with nodes=all already aggregates, is open as #5031, a separate follow-up PR stacked on this one; the messages GET and the threshold PUT stay out unless maintainers ask for them, and the follow-up PR names them in its Limits. If you would rather see the levels GET in this PR, say so and we will fold it in.

Were these the right calls?

Limits

  • Authorization for a broadcast is checked exactly once, on the node that receives the request: this endpoint requires config-edit permission there (NodeLogging.java:91-92). Node-to-node calls then go through the container's internal client with PKI authentication (RemoteRequestProxy.java:135-137), the same path the system info read uses. On that path the proxied requests present the sending node's own identity rather than the original caller's, and a node that receives one does not re-authorize it (PKIAuthenticationPlugin.java:371-372): the receiving node runs no config-edit check of its own for a change the broadcast delivers to it. A permission rule that is not cluster-wide is therefore effectively widened by the broadcast: a caller allowed config-edit on the receiving node alone can change levels on every node the request names. The broadcast has not been tested on a cluster with authentication and authorization enabled.
  • When a broadcast only partly succeeds, the Admin UI logging screen does not surface the nodes that failed; it refreshes exactly as it did under V1 (logging.js:171-174). For the other nodes that matches how V1 behaved, but not for the node the browser is talking to: V1 applied the change on that node locally before it broadcast (LoggingHandler.java:75-79), so that node always changed. Under V2 it changes only through the broadcast's call to itself, so if the self-call fails, the node is named in failedNodes, the UI ignores the list, and the screen keeps showing the old level. API callers do get the failedNodes list.
  • A node that is live when the request is validated but stops responding during the broadcast is reported in failedNodes after the fact (NodeLogging.java:145-151); the change may already have been applied on the nodes that did respond. There is no rollback, matching how the V1 broadcast behaved. A node that drops out of the live set mid-request is a harder version of the same case: the proxy re-checks membership as it sends (RemoteRequestProxy.java:121-131) and fails the request at that point, after earlier nodes may already have applied the change, and the caller gets that error rather than a failedNodes list.

Changelog: changelog/unreleased/SOLR-15823.yml

AI assistance

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

PUT /api/node/logging/levels now accepts a 'nodes' query parameter.
When present, NodeLogging fans the request out to the named nodes (or
every live node for nodes=all) through V2SolrRequestBasedProxy, the
same machinery GetNodeSystemInfo uses, and the response carries one
result per node plus the names of any requested nodes that did not
respond. Unknown node names are rejected before anything is sent, and
using 'nodes' outside SolrCloud mode is a 400 rather than an NPE. The
V1 handler now calls the endpoint method with a null nodes value so it
does not broadcast twice.
…oint

LoggingLevelController.setLevel now calls
LoggingV2.modifyLocalLogLevel unconditionally, passing nodes=all only
in SolrCloud mode, and the V1 Logging factory is deleted from
services.js.
NodeLoggingAPITest gains a broadcast case where one node's proxied
request fails: the response must carry a result for the node that
answered and name the other in failedNodes. The cloud broadcast test
now also checks the JVM-wide level afterwards, showing the payload
was applied rather than only delivered, and restores the logger to
unset when it finishes.
…y have applied the change

The response can time out after a node accepted the set-level request, so a node named in failedNodes is not guaranteed to be unchanged. The configuring-logging page now says so next to the failedNodes description.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

admin-ui cat:api documentation Improvements or additions to documentation tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant