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/04/26 17:02:21 UTC
[GitHub] [solr-operator] jward-bw opened a new pull request #265: Implement support for hostpath solr data volumes.
jward-bw opened a new pull request #265:
URL: https://github.com/apache/solr-operator/pull/265
This PR adds a new option for ephemeral data storage. Instead of the emptyDir you can use a `hostPath` volume. The default behaviour for ephemeral storage is still emptyDir, but specifying `HostPath` will mean hostPath will be used instead of emptyDir.
--
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.
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] jward-bw commented on pull request #265: Implement support for hostpath solr data volumes.
Posted by GitBox <gi...@apache.org>.
jward-bw commented on pull request #265:
URL: https://github.com/apache/solr-operator/pull/265#issuecomment-827419574
Interestingly I removed both and re-ran make mod-tidy, and the `github.com/elazarl/goproxy/ext` one came back, but the other didn't. I've removed both anyway
--
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.
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 #265: Implement support for hostpath solr data volumes.
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #265:
URL: https://github.com/apache/solr-operator/pull/265#discussion_r622301461
##########
File path: controllers/solrcloud_controller_storage_test.go
##########
@@ -329,6 +449,58 @@ func TestEphemeralStorageWithSpecs(t *testing.T) {
},
}
+ testHostPathSpecs(t, g, instance)
+}
+
+func testHostPathSpecs(t *testing.T, g *gomega.GomegaWithT, instance *solr.SolrCloud) {
+
+ // Setup the Manager and Controller. Wrap the Controller Reconcile function so it writes each request to a
+ // channel when it is finished.
+ mgr, err := manager.New(testCfg, manager.Options{})
+ g.Expect(err).NotTo(gomega.HaveOccurred())
+ testClient = mgr.GetClient()
+
+ solrCloudReconciler := &SolrCloudReconciler{
+ Client: testClient,
+ Log: ctrl.Log.WithName("controllers").WithName("SolrCloud"),
+ }
+ newRec, requests := SetupTestReconcile(solrCloudReconciler)
+ g.Expect(solrCloudReconciler.SetupWithManagerAndReconciler(mgr, newRec)).NotTo(gomega.HaveOccurred())
+
+ stopMgr, mgrStopped := StartTestManager(mgr, g)
+
+ defer func() {
+ close(stopMgr)
+ mgrStopped.Wait()
+ }()
+
+ cleanupTest(g, instance.Namespace)
+
+ // Create the SolrCloud object and expect the Reconcile and StatefulSet to be created
+ err = testClient.Create(context.TODO(), instance)
+ g.Expect(err).NotTo(gomega.HaveOccurred())
+ defer testClient.Delete(context.TODO(), instance)
+ g.Eventually(requests, timeout).Should(gomega.Receive(gomega.Equal(expectedCloudRequest)))
Review comment:
(Also no need to force push each time, I'll squash-merge in the end.)
--
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.
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] jward-bw commented on pull request #265: Implement support for hostpath solr data volumes.
Posted by GitBox <gi...@apache.org>.
jward-bw commented on pull request #265:
URL: https://github.com/apache/solr-operator/pull/265#issuecomment-827004493
@HoustonPutman I'm not sure what this error means, any ideas?
https://github.com/apache/solr-operator/pull/265/checks?check_run_id=2440577547
--
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.
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] jward-bw commented on a change in pull request #265: Implement support for hostpath solr data volumes.
Posted by GitBox <gi...@apache.org>.
jward-bw commented on a change in pull request #265:
URL: https://github.com/apache/solr-operator/pull/265#discussion_r622306087
##########
File path: controllers/solrcloud_controller_storage_test.go
##########
@@ -329,6 +449,58 @@ func TestEphemeralStorageWithSpecs(t *testing.T) {
},
}
+ testHostPathSpecs(t, g, instance)
+}
+
+func testHostPathSpecs(t *testing.T, g *gomega.GomegaWithT, instance *solr.SolrCloud) {
+
+ // Setup the Manager and Controller. Wrap the Controller Reconcile function so it writes each request to a
+ // channel when it is finished.
+ mgr, err := manager.New(testCfg, manager.Options{})
+ g.Expect(err).NotTo(gomega.HaveOccurred())
+ testClient = mgr.GetClient()
+
+ solrCloudReconciler := &SolrCloudReconciler{
+ Client: testClient,
+ Log: ctrl.Log.WithName("controllers").WithName("SolrCloud"),
+ }
+ newRec, requests := SetupTestReconcile(solrCloudReconciler)
+ g.Expect(solrCloudReconciler.SetupWithManagerAndReconciler(mgr, newRec)).NotTo(gomega.HaveOccurred())
+
+ stopMgr, mgrStopped := StartTestManager(mgr, g)
+
+ defer func() {
+ close(stopMgr)
+ mgrStopped.Wait()
+ }()
+
+ cleanupTest(g, instance.Namespace)
+
+ // Create the SolrCloud object and expect the Reconcile and StatefulSet to be created
+ err = testClient.Create(context.TODO(), instance)
+ g.Expect(err).NotTo(gomega.HaveOccurred())
+ defer testClient.Delete(context.TODO(), instance)
+ g.Eventually(requests, timeout).Should(gomega.Receive(gomega.Equal(expectedCloudRequest)))
Review comment:
Ok thanks, I've been on repos before where they like to have everything in a single commit :)
--
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.
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] jward-bw commented on pull request #265: Implement support for hostpath solr data volumes.
Posted by GitBox <gi...@apache.org>.
jward-bw commented on pull request #265:
URL: https://github.com/apache/solr-operator/pull/265#issuecomment-827421441
I don't use go regularly, so I shouldn't have any weird config. I did recently update to the latest version, so I wonder if that's caused a change in behaviour fo rme?
--
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.
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 #265: Implement support for hostpath solr data volumes.
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #265:
URL: https://github.com/apache/solr-operator/pull/265#discussion_r622296816
##########
File path: controllers/solrcloud_controller_storage_test.go
##########
@@ -329,6 +449,58 @@ func TestEphemeralStorageWithSpecs(t *testing.T) {
},
}
+ testHostPathSpecs(t, g, instance)
+}
+
+func testHostPathSpecs(t *testing.T, g *gomega.GomegaWithT, instance *solr.SolrCloud) {
+
+ // Setup the Manager and Controller. Wrap the Controller Reconcile function so it writes each request to a
+ // channel when it is finished.
+ mgr, err := manager.New(testCfg, manager.Options{})
+ g.Expect(err).NotTo(gomega.HaveOccurred())
+ testClient = mgr.GetClient()
+
+ solrCloudReconciler := &SolrCloudReconciler{
+ Client: testClient,
+ Log: ctrl.Log.WithName("controllers").WithName("SolrCloud"),
+ }
+ newRec, requests := SetupTestReconcile(solrCloudReconciler)
+ g.Expect(solrCloudReconciler.SetupWithManagerAndReconciler(mgr, newRec)).NotTo(gomega.HaveOccurred())
+
+ stopMgr, mgrStopped := StartTestManager(mgr, g)
+
+ defer func() {
+ close(stopMgr)
+ mgrStopped.Wait()
+ }()
+
+ cleanupTest(g, instance.Namespace)
+
+ // Create the SolrCloud object and expect the Reconcile and StatefulSet to be created
+ err = testClient.Create(context.TODO(), instance)
+ g.Expect(err).NotTo(gomega.HaveOccurred())
+ defer testClient.Delete(context.TODO(), instance)
+ g.Eventually(requests, timeout).Should(gomega.Receive(gomega.Equal(expectedCloudRequest)))
Review comment:
You might want to copy this line twice. So two or three of the same line in a row. I think the tests will be flakey otherwise. This will help the failure that you see in the github actions.
--
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.
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] jward-bw commented on a change in pull request #265: Implement support for hostpath solr data volumes.
Posted by GitBox <gi...@apache.org>.
jward-bw commented on a change in pull request #265:
URL: https://github.com/apache/solr-operator/pull/265#discussion_r622292273
##########
File path: controllers/util/solr_util.go
##########
@@ -214,16 +214,20 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
},
}
} else {
- emptyDirVolume := corev1.Volume{
- Name: solrDataVolumeName,
- VolumeSource: corev1.VolumeSource{
- EmptyDir: &corev1.EmptyDirVolumeSource{},
- },
+ ephemeralVolume := corev1.Volume{
+ Name: solrDataVolumeName,
+ VolumeSource: corev1.VolumeSource{},
}
if solrCloud.Spec.StorageOptions.EphemeralStorage != nil {
Review comment:
I might have misunderstood what you meant, but I've added a check for if EmptyDir is nil, and a corresponding test. There was already a test for if EphemeralStorage is empty though (if that's what you meant?):
https://github.com/apache/solr-operator/blob/c78b2b80ab097cb7f1b573f4fca8903ba7f63eb9/controllers/solrcloud_controller_storage_test.go#L240
--
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.
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] jward-bw commented on pull request #265: Implement support for hostpath solr data volumes.
Posted by GitBox <gi...@apache.org>.
jward-bw commented on pull request #265:
URL: https://github.com/apache/solr-operator/pull/265#issuecomment-827030163
They don't go away. Should I just remove the changes to go.mod?
--
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.
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 #265: Implement support for hostpath solr data volumes.
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #265:
URL: https://github.com/apache/solr-operator/pull/265#discussion_r622295063
##########
File path: controllers/util/solr_util.go
##########
@@ -214,16 +214,20 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
},
}
} else {
- emptyDirVolume := corev1.Volume{
- Name: solrDataVolumeName,
- VolumeSource: corev1.VolumeSource{
- EmptyDir: &corev1.EmptyDirVolumeSource{},
- },
+ ephemeralVolume := corev1.Volume{
+ Name: solrDataVolumeName,
+ VolumeSource: corev1.VolumeSource{},
}
if solrCloud.Spec.StorageOptions.EphemeralStorage != nil {
Review comment:
Ahh wonderful. Must have missed the test. But good change on the code 👍
--
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.
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 #265: Implement support for hostpath solr data volumes.
Posted by GitBox <gi...@apache.org>.
HoustonPutman merged pull request #265:
URL: https://github.com/apache/solr-operator/pull/265
--
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.
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 pull request #265: Implement support for hostpath solr data volumes.
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #265:
URL: https://github.com/apache/solr-operator/pull/265#issuecomment-827005617
> @HoustonPutman I'm not sure what this error means, any ideas?
> https://github.com/apache/solr-operator/pull/265/checks?check_run_id=2440577547
That is saying that you have two extra lines in your go.mod.
Try running `make mod-tidy` and see if those go away.
--
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.
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] jward-bw commented on pull request #265: Implement support for hostpath solr data volumes.
Posted by GitBox <gi...@apache.org>.
jward-bw commented on pull request #265:
URL: https://github.com/apache/solr-operator/pull/265#issuecomment-829094594
> The looks good to me!
>
> One final request:
> Would you mind adding this option to the "Data Storage" section in `docs/solr-cloud/solr-cloud-crd.md`?
Done! Sorry I missed that yesterday.
--
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.
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] jward-bw edited a comment on pull request #265: Implement support for hostpath solr data volumes.
Posted by GitBox <gi...@apache.org>.
jward-bw edited a comment on pull request #265:
URL: https://github.com/apache/solr-operator/pull/265#issuecomment-827421441
I don't use go regularly, so I shouldn't have any weird config. I did recently update to the latest version, so I wonder if that's caused a change in behaviour for me? 🤷
--
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.
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 pull request #265: Implement support for hostpath solr data volumes.
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #265:
URL: https://github.com/apache/solr-operator/pull/265#issuecomment-827040716
That's a good idea. I pulled your PR and ran `make mod-tidy`, and those two lines were removed. Maybe just a local thing for you?
--
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.
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 #265: Implement support for hostpath solr data volumes.
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #265:
URL: https://github.com/apache/solr-operator/pull/265#discussion_r621422387
##########
File path: controllers/util/solr_util.go
##########
@@ -214,16 +214,20 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
},
}
} else {
- emptyDirVolume := corev1.Volume{
- Name: solrDataVolumeName,
- VolumeSource: corev1.VolumeSource{
- EmptyDir: &corev1.EmptyDirVolumeSource{},
- },
+ ephemeralVolume := corev1.Volume{
+ Name: solrDataVolumeName,
+ VolumeSource: corev1.VolumeSource{},
}
if solrCloud.Spec.StorageOptions.EphemeralStorage != nil {
Review comment:
If `solrCloud.Spec.StorageOptions.EphemeralStorage` is empty, but not `nil`, then I think we will have an empty VolumeSource. We want to default to a plain EmptyDir in this case as well.
--
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.
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