You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "janhoy (via GitHub)" <gi...@apache.org> on 2024/01/03 21:39:47 UTC
[PR] Remove ingress and node-services during reconcile [solr-operator]
janhoy opened a new pull request, #674:
URL: https://github.com/apache/solr-operator/pull/674
Fixes #673
DRAFT, code largely generated by GitHub copilot, just throwing up the idea
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Remove ingress and node-services during reconcile [solr-operator]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman merged PR #674:
URL: https://github.com/apache/solr-operator/pull/674
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Remove ingress and node-services during reconcile [solr-operator]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #674:
URL: https://github.com/apache/solr-operator/pull/674#issuecomment-2593922092
I think this should be good to go
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Remove ingress and node-services during reconcile [solr-operator]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on PR #674:
URL: https://github.com/apache/solr-operator/pull/674#issuecomment-2593149013
> Houston raised some concerns about this possibly deleting services that might still be used by one or more pods
Let’s not over think either. This cleanup is in response to an explicit reconfiguration of a cluster. Can we simply assume that this also implies that there is no longer a need for the ingress or these services?
Can you give a concrete example of legitimate use of these services that warrant this added complexity? In my head we WANT such requests to fail after this reconfiguration?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Remove ingress and node-services during reconcile [solr-operator]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on code in PR #674:
URL: https://github.com/apache/solr-operator/pull/674#discussion_r1503398995
##########
controllers/solrcloud_controller.go:
##########
@@ -167,6 +167,21 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
}
}
}
+ } else {
Review Comment:
This looks pretty straightforward to me. I'd say we should do it for the headless service as well, but honestly we might want to just always make a headless service for simplicity...
##########
controllers/solrcloud_controller.go:
##########
@@ -167,6 +167,21 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
}
}
}
+ } else {
+ // Need no individual services per onde, delete if found
+ for _, nodeName := range solrNodeNames {
+ serviceName := nodeName
+ foundService := &corev1.Service{}
+ err = r.Get(ctx, types.NamespacedName{Name: serviceName, Namespace: instance.Namespace}, foundService)
+ if err == nil {
+ err = r.Delete(ctx, foundService)
Review Comment:
We would likely want to log here that we are deleting the service.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Remove ingress and node-services during reconcile [solr-operator]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #674:
URL: https://github.com/apache/solr-operator/pull/674#issuecomment-2523460698
Trying to follow up on some of these older PRs. AFAICT from the discussion above, the main thing holding up this PR is a test like the one Houston described [in his earlier comment](https://github.com/apache/solr-operator/pull/674#pullrequestreview-1902178369)? Is that right @janhoy ?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Remove ingress and node-services during reconcile [solr-operator]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #674:
URL: https://github.com/apache/solr-operator/pull/674#issuecomment-1967273180
> > switch it to just use the headless and make sure that the node services no longer exist.
>
> I might take a stab at such a test one day. Extending one of the existing, so after verifying that node-services are there, call an "update" on the CRD, then wait for reconsile to happen, and check that they are gone. Is there some test method that waits for reconcile or do we need to do polling with timeout?
If you look at the existing unit tests, that's exactly how all of the `Eventually...()` functions work. They are eventually consistent checks. There should be some similar tests around changing state halfway through, such as creating necessary secrets for basicAuth after the SolrCloud has already been created. I can help you with the test, but obviously if you get started first, it'll help better with the learning 🙂
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Remove ingress and node-services during reconcile [solr-operator]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #674:
URL: https://github.com/apache/solr-operator/pull/674#issuecomment-2000144889
Btw this is going to be impacted by the work in #692 . The two approaches should not necessarily conflict with eachother though. We might not want to prune old node services too heavily, in case the user is using autoscaling. At that point it's likely better to keep the old ones around. (which will result in less rolling restarts due to the hostAliases being updated when the new node services are created.)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Remove ingress and node-services during reconcile [solr-operator]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on PR #674:
URL: https://github.com/apache/solr-operator/pull/674#issuecomment-1966110621
> switch it to just use the headless and make sure that the node services no longer exist.
I might take a stab at such a test one day. Extending one of the existing, so after verifying that node-services are there, call an "update" on the CRD, then wait for reconsile to happen, and check that they are gone. Is there some test method that waits for reconcile or do we need to do polling with timeout?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Remove ingress and node-services during reconcile [solr-operator]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on PR #674:
URL: https://github.com/apache/solr-operator/pull/674#issuecomment-1954190172
Any takers for a review? It works when testing locally :)
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Remove ingress and node-services during reconcile [solr-operator]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on code in PR #674:
URL: https://github.com/apache/solr-operator/pull/674#discussion_r1503423348
##########
controllers/solrcloud_controller.go:
##########
@@ -167,6 +167,21 @@ func (r *SolrCloudReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
}
}
}
+ } else {
+ // Need no individual services per onde, delete if found
+ for _, nodeName := range solrNodeNames {
+ serviceName := nodeName
+ foundService := &corev1.Service{}
+ err = r.Get(ctx, types.NamespacedName{Name: serviceName, Namespace: instance.Namespace}, foundService)
+ if err == nil {
+ err = r.Delete(ctx, foundService)
Review Comment:
Added logging both for service deletion and ingress deletion
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Remove ingress and node-services during reconcile [solr-operator]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #674:
URL: https://github.com/apache/solr-operator/pull/674#issuecomment-2536344844
Alright, rather than introducing a net-new test, I was able to add some assertions into `solrcloud_scaling_test.go` that should validate the functionality here. There's still something not quite right with the test assertions (perhaps we're not waiting long enough to see the pod-services disappear?), but it's a start.
In the meantime, we should decide how we want this behavior to interact with the work in #692. One idea that Houston suggested offline is that rather than deleting orphaned services right away, we could instead give them an annotation that indicates that they should be deleted after X time (say, 30 minutes?). What do we all think about that?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Remove ingress and node-services during reconcile [solr-operator]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on PR #674:
URL: https://github.com/apache/solr-operator/pull/674#issuecomment-2582020954
So shall we try to conclude on this before 0.9? Do I understand correctly that the feature itself looks good, no concerns wrt autoscaling here. And the test is ok, but could have been converted to a unit test?
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Remove ingress and node-services during reconcile [solr-operator]
Posted by "gerlowskija (via GitHub)" <gi...@apache.org>.
gerlowskija commented on PR #674:
URL: https://github.com/apache/solr-operator/pull/674#issuecomment-2588234157
Houston and I spent some time on this today. I've removed the e2e test code and replaced it with a comparable (and now working!) unit-style test covering the ingress-cleanup.
When we got to thinking through the service-cleanup a bit more though, Houston raised some concerns about this possibly deleting services that might still be used by one or more pods. One approach for handling this would be to give each Solr pod an annotation recording the service it uses, and then defer cleanup of each service until it is no longer used by any pods.
This approach is safer but adds enough complexity that we probably shouldn't try to squeeze it into 0.9.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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Remove ingress and node-services during reconcile [solr-operator]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on PR #674:
URL: https://github.com/apache/solr-operator/pull/674#issuecomment-2524716441
Suppose so. I think I got stuck with the test part. Anyone may feel free to grab this, I won't pick it up soonish.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Remove ingress and node-services during reconcile [solr-operator]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #674:
URL: https://github.com/apache/solr-operator/pull/674#issuecomment-2586143084
Yes, we should definitely include this. I'll try to get it merged early this week
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Remove ingress and node-services during reconcile [solr-operator]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #674:
URL: https://github.com/apache/solr-operator/pull/674#issuecomment-2536668005
Ahhh @gerlowskija , I think we were misunderstanding. Also misunderstanding with regards to the test. This is only deleting services/ingresses if the entire externalAddressibility feature is changed. Not when the solrcloud is scaled up/down. This testing can probably be done in the unit tests, and we have no concerns around autoscaling.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Remove ingress and node-services during reconcile [solr-operator]
Posted by "HoustonPutman (via GitHub)" <gi...@apache.org>.
HoustonPutman commented on PR #674:
URL: https://github.com/apache/solr-operator/pull/674#issuecomment-2593608496
So the biggest concern here is the use of node-services instead of the headless service. If we auto-remove the node-services immediately when the user switches away from the ingress option in their SolrCloud resource, the pods will have not had a chance to update themselves to say that they have a different hostname. Therefore inter-node traffic will be halted, which will probably lead to a never-ending rolling restart (because nodes won't become healthy again).
If the idea is that all the data should go away, then the user should delete and recreate.
I'm going to push a commit that does this a bit more cautiously, only deleting the services when we know they aren't in use. Ingresses I'm ok with just deleting.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org
Re: [PR] Remove ingress and node-services during reconcile [solr-operator]
Posted by "janhoy (via GitHub)" <gi...@apache.org>.
janhoy commented on PR #674:
URL: https://github.com/apache/solr-operator/pull/674#issuecomment-2594033477
Ok, I was not aware that inter pod communication preferred node-service and needed a rolling restart to start using headless service instead. Thanks for the diligent review and fix. I have nothing to add. Feel free to merge.
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org