Re: [PR] fix the solr zk invocation [solr-operator]
gerlowskija commented on PR #756: URL: https://github.com/apache/solr-operator/pull/756#issuecomment-2710826399 Will do a sanity-check with 8.11 and start prepping to merge this. Thanks for the quick update @elangelo ! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix the solr zk invocation [solr-operator]
gerlowskija commented on PR #756: URL: https://github.com/apache/solr-operator/pull/756#issuecomment-2711077517 (Confirmed this PR works with Solr 8.11 as well - so we should be covered for the full range of supported Solr versions!) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix the solr zk invocation [solr-operator]
HoustonPutman commented on PR #756: URL: https://github.com/apache/solr-operator/pull/756#issuecomment-2711081795 I will work on adding an integration test, but since you tested it manually, it shouldn't block this PR or a release IMO. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix the solr zk invocation [solr-operator]
gerlowskija merged PR #756: URL: https://github.com/apache/solr-operator/pull/756 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix the solr zk invocation [solr-operator]
gerlowskija commented on PR #756: URL: https://github.com/apache/solr-operator/pull/756#issuecomment-2714450348 Thanks again for all your work on this @elangelo ! -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix the solr zk invocation [solr-operator]
gerlowskija commented on PR #756: URL: https://github.com/apache/solr-operator/pull/756#issuecomment-2714415769 The PR jobs are failing currently due to a setup issue: > Error: This request has been automatically failed because it uses a deprecated version of `actions/cache: v2`. Please update your workflow to use v3/v4 of actions/cache to avoid interruptions. Learn more: https://github.blog/changelog/2024-12-05-notice-of-upcoming-releases-and-breaking-changes-for-github-actions/#actions-cache-v1-v2-and-actions-toolkit-cache-package-closing-down I'll tackle updating this in a separate PR, so it doesn't get bundled in to this. I've run the unit and integration-tests locally, as well as doing a good deal of manual testing, so this should be ready to merge and backport! (I've skipped adding a helm chart changelog entry for this PR, as that would make it look like the fix was first made available in 0.10.0, when really it'll most likely get released in a 0.9.1. I *will* add a changelog when I backport to the `release-0.9` branch however. @HoustonPutman does that sound right to you, or am I missing something?) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix the solr zk invocation [solr-operator]
elangelo commented on PR #756: URL: https://github.com/apache/solr-operator/pull/756#issuecomment-2704474666 > Has anyone tested this with Solr 8.11 (This is still supported for the 0.9 line)  At the very least the help page says `-z` should work. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix the solr zk invocation [solr-operator]
epugh commented on code in PR #756:
URL: https://github.com/apache/solr-operator/pull/756#discussion_r1982211735
##
controllers/solrcloud_controller_basic_auth_test.go:
##
@@ -351,12 +351,12 @@ func expectBasicAuthConfigOnPodTemplateWithGomega(g
Gomega, solrCloud *solrv1bet
func expectPutSecurityJsonInZkCmd(g Gomega, expInitContainer
*corev1.Container) {
g.Expect(expInitContainer).To(Not(BeNil()), "Didn't find the setup-zk
InitContainer in the sts!")
- expCmd := "solr zk cp zk:/security.json /tmp/current_security.json
>/dev/null 2>&1; " +
+ expCmd := "solr zk cp zk:/security.json /tmp/current_security.json
--zk-host $ZK_HOST >/dev/null 2>&1; " +
Review Comment:
Yeah -z has been the short version for forever!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] fix the solr zk invocation [solr-operator]
HoustonPutman commented on PR #756: URL: https://github.com/apache/solr-operator/pull/756#issuecomment-2702120082 Has anyone tested this with Solr 8.11 (This is still supported for the 0.9 line) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix the solr zk invocation [solr-operator]
samuelverstraete commented on code in PR #756:
URL: https://github.com/apache/solr-operator/pull/756#discussion_r1981717896
##
controllers/solrcloud_controller_basic_auth_test.go:
##
@@ -351,12 +351,12 @@ func expectBasicAuthConfigOnPodTemplateWithGomega(g
Gomega, solrCloud *solrv1bet
func expectPutSecurityJsonInZkCmd(g Gomega, expInitContainer
*corev1.Container) {
g.Expect(expInitContainer).To(Not(BeNil()), "Didn't find the setup-zk
InitContainer in the sts!")
- expCmd := "solr zk cp zk:/security.json /tmp/current_security.json
>/dev/null 2>&1; " +
+ expCmd := "solr zk cp zk:/security.json /tmp/current_security.json
--zk-host $ZK_HOST >/dev/null 2>&1; " +
Review Comment:
i changed the invocation to use -z instead. This worked both on solr 9.6.1
and solr 9.8.0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] fix the solr zk invocation [solr-operator]
gerlowskija commented on code in PR #756:
URL: https://github.com/apache/solr-operator/pull/756#discussion_r1981474383
##
controllers/solrcloud_controller_basic_auth_test.go:
##
@@ -351,12 +351,12 @@ func expectBasicAuthConfigOnPodTemplateWithGomega(g
Gomega, solrCloud *solrv1bet
func expectPutSecurityJsonInZkCmd(g Gomega, expInitContainer
*corev1.Container) {
g.Expect(expInitContainer).To(Not(BeNil()), "Didn't find the setup-zk
InitContainer in the sts!")
- expCmd := "solr zk cp zk:/security.json /tmp/current_security.json
>/dev/null 2>&1; " +
+ expCmd := "solr zk cp zk:/security.json /tmp/current_security.json
--zk-host $ZK_HOST >/dev/null 2>&1; " +
Review Comment:
[-1] The `--zk-host` syntax is perfect for Solr 9.8, but I think this
param/flag changed somewhere in the last few Solr versions. So relying on it
in the initContainer would cause failures whenever someone was using e.g. Solr
9.6.
Luckily, the "short" version of the opt (`-z`) has stayed the same - so that
should be a safer option. Can the PR use that "short" syntax instead?
(@epugh - can you double-check me on this. According to the [version matrix
docs
here](https://github.com/apache/solr-operator/blob/main/docs/upgrade-notes.md#solr-versions),
operator 0.9.x currently aims to support >= 8.11, so ideally we'd get a syntax
that works through that range. If there's no single syntax that works for the
whole range, we could always do the old `||` trick with the minimum set of
syntaxes, e.g.
```
solr zk --zk-host $ZK_HOST || solr zk -zkHost $ZK_HOST
```
)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
-
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
Re: [PR] fix the solr zk invocation [solr-operator]
HoustonPutman commented on PR #756: URL: https://github.com/apache/solr-operator/pull/756#issuecomment-2675380584 We also desperately need integration tests for the basic auth stuff. Our biggest testing gap -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix the solr zk invocation [solr-operator]
HoustonPutman commented on PR #756: URL: https://github.com/apache/solr-operator/pull/756#issuecomment-2675378459 Yeah, this will need a bug fix release probably -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix the solr zk invocation [solr-operator]
elangelo commented on PR #756: URL: https://github.com/apache/solr-operator/pull/756#issuecomment-2663001130 @gerlowskija you are right of course... I was using solr-operator 0.9. I initially was doing this with 0.8 but that wasn't working as well. I then upgraded to 0.9 and there found it broken too. I did bring up my own custom container and confirmed it immediately worked if i added --zk-host to the cli. I understand we might want a longer term fix than this and obviously I'm good with that, but short term this means that initializing authentication on solr 9.8 and younger is broken? If we need to fix the solr cli tool we would have to backport it? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix the solr zk invocation [solr-operator]
gerlowskija commented on PR #756: URL: https://github.com/apache/solr-operator/pull/756#issuecomment-2661595935 @epugh - I believe what you're saying about ZK_HOST not being obeyed by `bin/solr` the way we thought, but I'm not sure how to square that with what I saw in my quick repro attempt [above](https://github.com/apache/solr-operator/pull/756#issuecomment-2653981478). It looked very much to me like having a "ZK_HOST" set was impacting things, even if we can't find the codepath by which that happens. But what am I missing something? > personally, when it comes to invoking the SolrCLI, I like the verbosity of passing in all the args, so there is less magic. I like the env-vars, personally. But if you think that's worth pursuing, a dev@ thread might be the best way to weigh support/consensus there? > I was doing this with solr 9.8.0 and solr-operator 0.8 You're sure your deployment used operator-0.8 @elangelo ? The 0.8 operator used a completely different CLI/client for talking to ZK than this code on 'main' currently uses. (The operator used `zkcli.sh` in 0.8, vs `bin/solr zk` in >= 0.9.0). Will try to do a bit more poking around here this week, but appreciate any clarification in the meantime. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix the solr zk invocation [solr-operator]
epugh commented on PR #756: URL: https://github.com/apache/solr-operator/pull/756#issuecomment-2654032493 personally, when it comes to invoking the SolrCLI, I like the verbosity of passing in all the args, so there is less magic. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix the solr zk invocation [solr-operator]
epugh commented on PR #756: URL: https://github.com/apache/solr-operator/pull/756#issuecomment-2654030295 just a bit of poking, and i don't think we look up from env var the ZK_HOST: https://github.com/apache/solr/blob/main/solr/core/src/java/org/apache/solr/cli/CLIUtils.java#L223 eventually, we do have a look up for a Solr URL, but it doesn't use ZK_HOST, instead it uses these processes: https://github.com/apache/solr/blob/main/solr/core/src/java/org/apache/solr/cli/CLIUtils.java#L71 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix the solr zk invocation [solr-operator]
elangelo commented on PR #756: URL: https://github.com/apache/solr-operator/pull/756#issuecomment-2654002210 I was doing this with solr 9.8.0 and solr-operator 0.8 When ZK_HOST is set (which is default in the init container) I always get that error message: `Neither --zk-host or --solr-url parameters provided so assuming solr url is http://localhost:8983` that obviously fails as i'm running this on kubernetes. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
Re: [PR] fix the solr zk invocation [solr-operator]
gerlowskija commented on PR #756: URL: https://github.com/apache/solr-operator/pull/756#issuecomment-2653981478 Hi @elangelo - what Solr version are you running? And are there any particular steps needed to reproduce the issue? I tried playing with this a bit this morning but haven't been able to reproduce on the versions I tried (9.7.0 and the unreleased 10.0): ``` # Try without a ZK_HOST initially $ bin/solr zk cp foo.json zk:/foo.json Neither --zk-host or --solr-url parameters provided so assuming solr url is http://localhost:8983. ERROR: Server refused connection at: http://localhost:8983/solr/admin/info/system ... # With ZK_HOST value $ export ZK_HOST=localhost:9765 $ bin/solr zk cp foo.json zk:/foo.json Copying from 'foo.json' to 'zk:/foo.json'. ZooKeeper at localhost:9765 ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] - To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
