You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/09/20 08:43:38 UTC
[GitHub] [solr-operator] vladiceanu opened a new pull request #324: Support Lifecycle input for Solr container
vladiceanu opened a new pull request #324:
URL: https://github.com/apache/solr-operator/pull/324
**Resolves** #322
**Describe your changes**
This PR adds support for custom Lifecycle input for Solr main container. A user should be able to set `lifecycle.preStop` and `lifecycle.postStart` hooks through SolrCloud CR for Solr main container.
This PR also includes Helm chart changes to support lifecycle inputs.
Tested locally on minikube.
--
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
[GitHub] [solr-operator] vladiceanu commented on a change in pull request #324: Support Lifecycle input for Solr container
Posted by GitBox <gi...@apache.org>.
vladiceanu commented on a change in pull request #324:
URL: https://github.com/apache/solr-operator/pull/324#discussion_r715067459
##########
File path: controllers/util/solr_util.go
##########
@@ -327,6 +328,13 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
}
}
+ // Default preStop hook
+ preStop := &corev1.Handler{
Review comment:
Makes sense. I've cleaned up the tests of `preStop` hook that were based on ENV variable `SOLR_PORT` because we're passing different Lifecycle values and that won't work; in any case, we already have a test for ENV variables.
Also, I have added the Lifecycle tests for SolrCloud and PromExporter STS/Deployment.
Please have a look and let me know if I miss anything.
--
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
[GitHub] [solr-operator] HoustonPutman commented on a change in pull request #324: Support Lifecycle input for Solr container
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #324:
URL: https://github.com/apache/solr-operator/pull/324#discussion_r715113610
##########
File path: controllers/util/solr_util.go
##########
@@ -327,6 +328,13 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
}
}
+ // Default preStop hook
+ preStop := &corev1.Handler{
Review comment:
No worries, thanks for the great and thorough PR!
--
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
[GitHub] [solr-operator] HoustonPutman commented on a change in pull request #324: Support Lifecycle input for Solr container
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #324:
URL: https://github.com/apache/solr-operator/pull/324#discussion_r714940753
##########
File path: controllers/util/solr_util.go
##########
@@ -327,6 +328,13 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
}
}
+ // Default preStop hook
+ preStop := &corev1.Handler{
Review comment:
Yeah, if you look at https://github.com/apache/solr-operator/blob/main/controllers/solrcloud_controller_test.go#L174, you'll see we test basically all of the custom options there. Add the custom lifecycle, and test that the podSpec actually uses 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: 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
[GitHub] [solr-operator] HoustonPutman commented on a change in pull request #324: Support Lifecycle input for Solr container
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #324:
URL: https://github.com/apache/solr-operator/pull/324#discussion_r715081864
##########
File path: controllers/util/solr_util.go
##########
@@ -327,6 +328,13 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
}
}
+ // Default preStop hook
+ preStop := &corev1.Handler{
Review comment:
The new tests look great, but why did you remove the old tests from the tests that don't specify a custom lifecycle?
We test those because the ports change between the tests, and we want to make sure the correct port is always used no matter the solrAddressibility options specified.
It looks like we are keeping the same default lifecycle, unless I'm missing something. So those tests wouldn't have to change, I don't think.
--
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
[GitHub] [solr-operator] vladiceanu commented on a change in pull request #324: Support Lifecycle input for Solr container
Posted by GitBox <gi...@apache.org>.
vladiceanu commented on a change in pull request #324:
URL: https://github.com/apache/solr-operator/pull/324#discussion_r715090863
##########
File path: controllers/util/solr_util.go
##########
@@ -327,6 +328,13 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
}
}
+ // Default preStop hook
+ preStop := &corev1.Handler{
Review comment:
oh, yes, I thought it doesn't make sense, but now I see that the point of these tests is to ensure the correctness of the used port. I have added them back.
--
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
[GitHub] [solr-operator] HoustonPutman merged pull request #324: Support custom Lifecycle for Solr & PrometheusExporter containers
Posted by GitBox <gi...@apache.org>.
HoustonPutman merged pull request #324:
URL: https://github.com/apache/solr-operator/pull/324
--
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
[GitHub] [solr-operator] vladiceanu commented on a change in pull request #324: Support Lifecycle input for Solr container
Posted by GitBox <gi...@apache.org>.
vladiceanu commented on a change in pull request #324:
URL: https://github.com/apache/solr-operator/pull/324#discussion_r715113063
##########
File path: controllers/util/solr_util.go
##########
@@ -327,6 +328,13 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
}
}
+ // Default preStop hook
+ preStop := &corev1.Handler{
Review comment:
thank you for all the help, Houston.
--
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
[GitHub] [solr-operator] HoustonPutman commented on a change in pull request #324: Support Lifecycle input for Solr container
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #324:
URL: https://github.com/apache/solr-operator/pull/324#discussion_r714908511
##########
File path: controllers/util/solr_util.go
##########
@@ -327,6 +328,13 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
}
}
+ // Default preStop hook
+ preStop := &corev1.Handler{
Review comment:
Is there a reason you split this out here?
--
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
[GitHub] [solr-operator] vladiceanu commented on a change in pull request #324: Support Lifecycle input for Solr container
Posted by GitBox <gi...@apache.org>.
vladiceanu commented on a change in pull request #324:
URL: https://github.com/apache/solr-operator/pull/324#discussion_r714926359
##########
File path: controllers/util/solr_util.go
##########
@@ -327,6 +328,13 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
}
}
+ // Default preStop hook
+ preStop := &corev1.Handler{
Review comment:
Just for better readability, I guess.
--
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
[GitHub] [solr-operator] vladiceanu commented on a change in pull request #324: Support Lifecycle input for Solr container
Posted by GitBox <gi...@apache.org>.
vladiceanu commented on a change in pull request #324:
URL: https://github.com/apache/solr-operator/pull/324#discussion_r715090863
##########
File path: controllers/util/solr_util.go
##########
@@ -327,6 +328,13 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
}
}
+ // Default preStop hook
+ preStop := &corev1.Handler{
Review comment:
oh, yes, you're right. In the beginning, I thought it doesn't make sense if we add podLifecycle ones, but now I see that the point of these tests is to ensure the correctness of the used port. I have added them back.
--
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
[GitHub] [solr-operator] vladiceanu commented on a change in pull request #324: Support Lifecycle input for Solr container
Posted by GitBox <gi...@apache.org>.
vladiceanu commented on a change in pull request #324:
URL: https://github.com/apache/solr-operator/pull/324#discussion_r714931499
##########
File path: controllers/util/solr_util.go
##########
@@ -327,6 +328,13 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
}
}
+ // Default preStop hook
+ preStop := &corev1.Handler{
Review comment:
> This looks good to me! But I would like to see a test added 🙂
I was checking around and found that we already have a few tests on Lifecycle, i.e. https://github.com/apache/solr-operator/blob/main/controllers/solrcloud_controller_externaldns_test.go#L117, https://github.com/apache/solr-operator/blob/main/controllers/solrcloud_controller_test.go#L300, https://github.com/apache/solr-operator/blob/main/controllers/solrcloud_controller_test.go#L396, and other places.
Are there any other kinds of tests that you think it would make sense to add? Perhaps just a simple custom input, not based on variables?
--
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
[GitHub] [solr-operator] vladiceanu commented on a change in pull request #324: Support Lifecycle input for Solr container
Posted by GitBox <gi...@apache.org>.
vladiceanu commented on a change in pull request #324:
URL: https://github.com/apache/solr-operator/pull/324#discussion_r715067459
##########
File path: controllers/util/solr_util.go
##########
@@ -327,6 +328,13 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
}
}
+ // Default preStop hook
+ preStop := &corev1.Handler{
Review comment:
Make sense. I've cleaned up the tests of `preStop` hook that were based on ENV variable `SOLR_PORT` because we're passing different Lifecycle values and that won't work; in any case, we already have a test for ENV variables.
Also, I have added the Lifecycle tests for SolrCloud and PromExporter STS/Deployment.
Please have a look and let me know if I miss anything.
--
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