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/08/05 16:05:48 UTC
[GitHub] [solr-operator] gerlowskija opened a new pull request #302: Allow users to take GCS-based backups
gerlowskija opened a new pull request #302:
URL: https://github.com/apache/solr-operator/pull/302
This commit adds first-pass support for exposing Solr's
GcsBackupRepository through our operator configuration. This WIP
support has a number of caveats and downsides that'll need agreed
on or fixed prior to merging :
- GCS backups eschew the "persistence" step that currently follows
normal backups
- GCS backups are only included in Solr 8.9+, but there's no check for
this currently.
- operator logic currently assumes that exactly 1 type of backup
config will be provided on a given solrcloud object (i.e. GCS
backups and 'local' PV backups are mutually exclusive for a
solrcloud.
- no automated tests have been added
- no documentation of has been added, beyond the examples on issue
#301
--
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] gerlowskija edited a comment on pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija edited a comment on pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#issuecomment-920864635
Thanks Houston - I saw that 'make lint' advises using 'go fmt' to autocorrect issues as well. Lots of options it looks like!
--
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 #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#discussion_r710167812
##########
File path: controllers/solrcloud_controller.go
##########
@@ -699,6 +695,41 @@ func reconcileCloudStatus(r *SolrCloudReconciler, solrCloud *solr.SolrCloud, log
return outOfDatePods, outOfDatePodsNotStarted, availableUpdatedPodCount, nil
}
+func isPodReadyForBackup(pod corev1.Pod, backupOptions *solr.SolrBackupRestoreOptions) bool {
Review comment:
Yeah I was thinking that the SolrCloud status could start providing a list of ready backup Repos. If you don't think this is very feasible that's fine. We can always add it later.
--
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] gerlowskija commented on a change in pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on a change in pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#discussion_r706189501
##########
File path: controllers/solrcloud_controller.go
##########
@@ -699,6 +695,41 @@ func reconcileCloudStatus(r *SolrCloudReconciler, solrCloud *solr.SolrCloud, log
return outOfDatePods, outOfDatePodsNotStarted, availableUpdatedPodCount, nil
}
+func isPodReadyForBackup(pod corev1.Pod, backupOptions *solr.SolrBackupRestoreOptions) bool {
Review comment:
As written, this method checks if a single pod is ready to handle a backup for any/all of its repositories. This information gets surfaced to consumers under the `backupRestoreReady` boolean field of `SolrCloudStatus`.
We can definitely change it to take a specific repository, but since this call isn't happening at backup-time, we'd still have to call it in a loop for each repository defined on the solrcloud instance and then "AND" each of those results together into the single `backupRestoreReady` value. So I'm not really sure that gets us anything?
Unless you're thinking that we'd change SolrCloudStatus to include a distinct "backupReadiness" status for each repository so that we could check the one we care about at "backup time". I'd be OK with that if that's what you're suggesting, just wanted to double check we're envisioning the same thing 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] gerlowskija commented on a change in pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on a change in pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#discussion_r694263848
##########
File path: controllers/util/backup_util.go
##########
@@ -440,3 +483,13 @@ func RunExecForPod(podName string, namespace string, command []string, config re
return nil
}
+
+// TODO Is the Mozilla license on hashicorp's go-version lib we're using here acceptable by Apache? Is there a better option?
+func SupportsGcsBackups(versionStr string) (bool, error) {
Review comment:
I'd need to double-check the codepath, but this doesn't use the image metadata - it gets the data from Solr itself (using `/solr/admin/info/system` if I remember right).
Does that address your concern about brittle-ness? Or would you still prefer this be removed?
--
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 edited a comment on pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
HoustonPutman edited a comment on pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#issuecomment-926208319
Ok I have refactored to use a new more kube-style way of listing repositories. I've also updated the documentation and examples.
One big difference is we are no longer supporting the legacy location for the backupRepository information. The SolrCloud.withDefaults() will auto-move this information to the `backupRepositories` list. However, we lose the old location where these directories were mounted. This is not an issue, since the Solr Operator will use this new location after it's upgrade. The only issue is when the Solr Operator has upgraded, but the SolrClouds are still in a rolling-restart to update the mountPath of the legacy backupRepo. Users will have to hold off of doing any backups until the SolrClouds they are using have been fully updated. Once this is done, there should be no issues with backwards-compatibility.
I have added a warning for this in the upgrade-notes for `v0.5.0`. I think it's an acceptable tradeoff to not need a lot of custom code. Also we will be able to remove the deprecated option completely in `v0.6.0` without creating an issue for existing clouds (since the deprecated option will have been moved to `backupRepositories`).
I think there are a few things left to do:
- [x] Add helm chart options for the backupRepositories, update Helm documentation
- [x] Add a changelog entry.
- [x] Add assert messages in new tests.
Will try to wrap this up tomorrow. And hopefully do some integration tests afterwards. Would love help with the integration tests if anyone has time for 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
[GitHub] [solr-operator] HoustonPutman commented on a change in pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#discussion_r710166288
##########
File path: api/v1beta1/solrcloud_types.go
##########
@@ -325,20 +329,158 @@ type SolrEphemeralDataStorageOptions struct {
EmptyDir *corev1.EmptyDirVolumeSource `json:"emptyDir,omitempty"`
}
+func (backupOptions *SolrBackupRestoreOptions) IsManaged() bool {
+ return true
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetVolumeSource() corev1.VolumeSource {
+ return *backupOptions.Volume
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetDirectory() string {
+ return backupOptions.Directory
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetSolrMountPath() string {
+ return BaseBackupRestorePath
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetInitPodMountPath() string {
+ return BackupRestoreInitContainerPath
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetBackupPath(backupName string) string {
+ return backupOptions.GetSolrMountPath() + "/backups/" + backupName
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetVolumeName() string {
+ return BackupRestoreVolume
+}
+
type SolrBackupRestoreOptions struct {
+ // TODO Do we need to support this (Volume) here for backcompat, or can it live exclusively in ManagedStorage?
Review comment:
Yes, exactly. We do that already for the SolrCloud and the SolrPrometheusExporter through `WithDefaults()` methods.
--
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 pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#issuecomment-926208319
Ok I have refactored to use a new more kube-style way of listing repositories. I've also updated the documentation and examples.
One big difference is we are no longer supporting the legacy location for the backupRepository information. The SolrCloud.withDefaults() will auto-move this information to the `backupRepositories` list. However, we lose the old location where these directories were mounted. This is not an issue, since the Solr Operator will use this new location after it's upgrade. The only issue is when the Solr Operator has upgraded, but the SolrClouds are still in a rolling-restart to update the mountPath of the legacy backupRepo. Users will have to hold off of doing any backups until the SolrClouds they are using have been fully updated. Once this is done, there should be no issues with backwards-compatibility.
I have added a warning for this in the upgrade-notes for `v0.5.0`. I think it's an acceptable tradeoff to not need a lot of custom code. Also we will be able to remove the deprecated option completely in `v0.6.0` without creating an issue for existing clouds (since the deprecated option will have been moved to `backupRepositories`).
I think there are a few things left to do:
- [ ] Add helm chart options for the backupRepositories, update Helm documentation
- [ ] Add a changelog entry.
- [ ] Add assert messages in new tests.
Will try to wrap this up tomorrow. And hopefully do some integration tests afterwards. Would love help with the integration tests if anyone has time for 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
[GitHub] [solr-operator] HoustonPutman commented on a change in pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#discussion_r714152301
##########
File path: api/v1beta1/solrcloud_types.go
##########
@@ -331,7 +335,50 @@ type SolrBackupRestoreOptions struct {
// Since the volume will be mounted to all solrNodes, it must be able to be written from multiple pods.
// If a PVC reference is given, the PVC must have `accessModes: - ReadWriteMany`.
// Other options are to use a NFS volume.
- Volume corev1.VolumeSource `json:"volume"`
+ // Deprecated: Create an explicit 'managedRepositories' entry instead.
+ Volume *corev1.VolumeSource `json:"volume,omitempty"`
+
+ // Allows specification of multiple different "repositories" for Solr to use when backing up data to GCS.
+ GcsRepositories *[]GcsStorage `json:"gcsRepositories,omitempty"`
Review comment:
Don't need a pointer here, arrays are optional. But `// +optional` should be used.
##########
File path: api/v1beta1/solrcloud_types.go
##########
@@ -331,7 +335,50 @@ type SolrBackupRestoreOptions struct {
// Since the volume will be mounted to all solrNodes, it must be able to be written from multiple pods.
// If a PVC reference is given, the PVC must have `accessModes: - ReadWriteMany`.
// Other options are to use a NFS volume.
- Volume corev1.VolumeSource `json:"volume"`
+ // Deprecated: Create an explicit 'managedRepositories' entry instead.
+ Volume *corev1.VolumeSource `json:"volume,omitempty"`
+
+ // Allows specification of multiple different "repositories" for Solr to use when backing up data to GCS.
+ GcsRepositories *[]GcsStorage `json:"gcsRepositories,omitempty"`
+
+ // Allows specification of multiple different "repositories" for Solr to use when backing up data "locally".
+ // Repositories defined here are considered "managed" and can take advantage of special operator features, such as
+ // post-backup compression.
+ ManagedRepositories *[]ManagedStorage `json:"managedRepositories,omitempty"`
Review comment:
Don't need a pointer here, arrays are optional. But `// +optional` should be used.
##########
File path: controllers/solrcloud_controller.go
##########
@@ -699,6 +694,41 @@ func reconcileCloudStatus(r *SolrCloudReconciler, solrCloud *solr.SolrCloud, log
return outOfDatePods, outOfDatePodsNotStarted, availableUpdatedPodCount, nil
}
+func isPodReadyForBackup(pod corev1.Pod, backupOptions *solr.SolrBackupRestoreOptions) bool {
Review comment:
Maybe instead of all this complex logic, when creating a podTemplate, we add an annotation for all the backupRepositories that pod is ready to handle... That will make it a lot easier to determine what repositories the cloud is "ready" for (just take an intersection of the lists from the pod annotations).
We can do this in a different PR though, don't want to complicate this one further.
##########
File path: api/v1beta1/solrcloud_types.go
##########
@@ -331,7 +335,50 @@ type SolrBackupRestoreOptions struct {
// Since the volume will be mounted to all solrNodes, it must be able to be written from multiple pods.
// If a PVC reference is given, the PVC must have `accessModes: - ReadWriteMany`.
// Other options are to use a NFS volume.
- Volume corev1.VolumeSource `json:"volume"`
+ // Deprecated: Create an explicit 'managedRepositories' entry instead.
+ Volume *corev1.VolumeSource `json:"volume,omitempty"`
+
+ // Allows specification of multiple different "repositories" for Solr to use when backing up data to GCS.
+ GcsRepositories *[]GcsStorage `json:"gcsRepositories,omitempty"`
+
+ // Allows specification of multiple different "repositories" for Solr to use when backing up data "locally".
+ // Repositories defined here are considered "managed" and can take advantage of special operator features, such as
+ // post-backup compression.
+ ManagedRepositories *[]ManagedStorage `json:"managedRepositories,omitempty"`
+
+ // Select a custom directory name to mount the backup/restore data from the given volume.
+ // If not specified, then the name of the solrcloud will be used by default.
+ // Deprecated: Create an explicit 'managedRepositories' entry instead.
+ // +optional
+ Directory string `json:"directory,omitempty"`
+}
+
+type GcsStorage struct {
+ // A name used to identify this GCS storage profile. Values should follow RFC-1123. (See here for more details:
+ // https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names)
+ Name string `json:"name"`
+
+ // The name of the GCS bucket that all backup data will be stored in
+ Bucket string `json:"bucket"`
+
+ // The name of a Kubernetes secret holding a Google cloud service account key
+ GcsCredentialSecret string `json:"gcsCredentialSecret"`
Review comment:
Should this have a secret key as well?
##########
File path: controllers/solrbackup_controller.go
##########
@@ -89,6 +90,7 @@ func (r *SolrBackupReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error)
solrCloud, allCollectionsComplete, collectionActionTaken, err := reconcileSolrCloudBackup(r, backup)
if err != nil {
+ // TODO Should we be failing the backup for some sub-set of errors here?
Review comment:
Yeah since we know the Solr errors, it would probably be good to do stuff here in case. But I think we can do it in a future Issue/PR.
##########
File path: api/v1beta1/solrcloud_types.go
##########
@@ -331,7 +335,50 @@ type SolrBackupRestoreOptions struct {
// Since the volume will be mounted to all solrNodes, it must be able to be written from multiple pods.
// If a PVC reference is given, the PVC must have `accessModes: - ReadWriteMany`.
// Other options are to use a NFS volume.
- Volume corev1.VolumeSource `json:"volume"`
+ // Deprecated: Create an explicit 'managedRepositories' entry instead.
Review comment:
Should have `// +optional` since this is optional now.
##########
File path: controllers/util/solr_util.go
##########
@@ -215,16 +216,63 @@ func GenerateStatefulSet(solrCloud *solr.SolrCloud, solrCloudStatus *solr.SolrCl
}
// Add backup volumes
if solrCloud.Spec.StorageOptions.BackupRestoreOptions != nil {
- solrVolumes = append(solrVolumes, corev1.Volume{
- Name: BackupRestoreVolume,
- VolumeSource: solrCloud.Spec.StorageOptions.BackupRestoreOptions.Volume,
- })
- volumeMounts = append(volumeMounts, corev1.VolumeMount{
- Name: BackupRestoreVolume,
- MountPath: BaseBackupRestorePath,
- SubPath: BackupRestoreSubPathForCloud(solrCloud.Spec.StorageOptions.BackupRestoreOptions.Directory, solrCloud.Name),
- ReadOnly: false,
- })
+ backupOptions := solrCloud.Spec.StorageOptions.BackupRestoreOptions
+ // Backup volumes can come from three sources: a singleton volume for local backups (legacy), a list of "managed"
+ // (i.e. local) repositories, and a list of gcs repositories. All three of these require a volume and
+ // accompanying mount: either for credentials to reach GCS or for the data itself.
+
+ // Handle the "singleton" local volume first
+ if backupOptions.Volume != nil {
+ solrVolumes = append(solrVolumes, corev1.Volume{
+ Name: solr.BackupRestoreVolume,
+ VolumeSource: *backupOptions.Volume,
+ })
+ volumeMounts = append(volumeMounts, corev1.VolumeMount{
+ Name: solr.BackupRestoreVolume,
+ MountPath: solr.BaseBackupRestorePath,
+ SubPath: BackupRestoreSubPathForCloud(backupOptions.Directory, solrCloud.Name),
+ ReadOnly: false,
+ })
+ }
+
+ // Then handle any other managed volumes
+ if backupOptions.ManagedRepositories != nil {
+ for _, managedRepository := range *backupOptions.ManagedRepositories {
+ solrVolumes = append(solrVolumes, corev1.Volume{
+ Name: managedRepository.GetVolumeName(),
+ VolumeSource: *managedRepository.Volume,
+ })
+ volumeMounts = append(volumeMounts, corev1.VolumeMount{
+ Name: managedRepository.GetVolumeName(),
+ MountPath: managedRepository.GetSolrMountPath(),
+ SubPath: BackupRestoreSubPathForCloud(managedRepository.Directory, solrCloud.Name),
+ ReadOnly: false,
+ })
+ }
+ }
+
+ // Lastly, handle the volumes needed for any GCS credentials.
+ if backupOptions.GcsRepositories != nil {
+ for _, gcsRepository := range *backupOptions.GcsRepositories {
+ fals := false
+ solrVolumes = append(solrVolumes, corev1.Volume{
+ Name: gcsRepository.GetVolumeName(),
+ VolumeSource: corev1.VolumeSource{
Review comment:
We might need to worry about restarting the Solr Pods when these secrets update. But again we can have a separate issue/PR for that.
##########
File path: controllers/util/backup_util_test.go
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package util
+
+import (
+ solr "github.com/apache/solr-operator/api/v1beta1"
+ "github.com/stretchr/testify/assert"
+ corev1 "k8s.io/api/core/v1"
+ metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+ "testing"
+)
+
+func TestSolrBackupApiParamsForLegacyBackup(t *testing.T) {
+ legacyRepository := solr.SolrBackupRestoreOptions{
+ Volume: &corev1.VolumeSource{}, // Actual volume info doesn't matter here
+ Directory: "/somedirectory",
+ }
+ backupConfig := solr.SolrBackup{
+ ObjectMeta: metav1.ObjectMeta{
+ Name: "somebackupname",
+ },
+ Spec: solr.SolrBackupSpec{
+ SolrCloud: "solrcloudcluster",
+ RepositoryName: "legacy_local_repository",
+ Collections: []string{"col1", "col2"},
+ },
+ }
+
+ queryParams := GenerateQueryParamsForBackup(&legacyRepository, &backupConfig, "col2")
+
+ assert.Equal(t, "BACKUP", queryParams.Get("action"))
Review comment:
Messages on the asserts would be helpful when debugging 🙂
--
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] gerlowskija removed a comment on pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija removed a comment on pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#issuecomment-920860482
Actually - I see there are a few errors in the PR build that come out of 'make lint'. It looks like the 'lint' output proposes a fix in each line that has bad indentation etc. which is really cool. Just wanted to check before I fix those: is there a programmatic way to apply those fixes, or is "by hand" the best approach for these 'make lint' errors?
--
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] gerlowskija commented on a change in pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on a change in pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#discussion_r706108934
##########
File path: controllers/util/backup_util.go
##########
@@ -440,3 +483,13 @@ func RunExecForPod(podName string, namespace string, command []string, config re
return nil
}
+
+// TODO Is the Mozilla license on hashicorp's go-version lib we're using here acceptable by Apache? Is there a better option?
+func SupportsGcsBackups(versionStr string) (bool, error) {
Review comment:
Just wanted to check in on your concerns here @HoustonPutman
--
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] gerlowskija edited a comment on pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija edited a comment on pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#issuecomment-925988303
> Instead of lists for each individual repository type, we structure it more like Volumes
Houston and I discussed this a bit offline and I'm 👍 on the alternate syntax he proposed. All of his inline comments look good to me as well.
(I'm mostly away from my computer on some paternity leave, but I'll try to stay up to date here. If I go quiet though, anyone should feel free to make whatever changes seem necessary and merge when finished)
--
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] gerlowskija commented on pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#issuecomment-925988303
> Instead of lists for each individual repository type, we structure it more like Volumes
Houston and I discussed this a bit offline and I'm 👍 on the alternate syntax he proposed.
--
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] gerlowskija commented on pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#issuecomment-931230479
Thanks a ton for seeing this through in my absence @HoustonPutman . Will be awesome to see this in 0.5.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
[GitHub] [solr-operator] gerlowskija commented on pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#issuecomment-915142671
Haven't touched this in a few weeks. Both because of some competing priorities where I work, and because I've been scrambling to get things done before I disappear for an upcoming paternity leave.
But just wanted to say that this is still on my radar and I'm hoping to return to it as soon as I can.
--
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] gerlowskija commented on pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#issuecomment-920864635
Thanks Houston - I saw that 'make lint' advises using 'go fmt' to autocorrect issues as well. Lots of options it looks like haha.
--
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] gerlowskija commented on a change in pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on a change in pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#discussion_r694263848
##########
File path: controllers/util/backup_util.go
##########
@@ -440,3 +483,13 @@ func RunExecForPod(podName string, namespace string, command []string, config re
return nil
}
+
+// TODO Is the Mozilla license on hashicorp's go-version lib we're using here acceptable by Apache? Is there a better option?
+func SupportsGcsBackups(versionStr string) (bool, error) {
Review comment:
I'd need to double-check the codepath, but this doesn't use the image tag - it gets the data from Solr itself (I _think_ using `/solr/admin/info/system`)
--
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 edited a comment on pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
HoustonPutman edited a comment on pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#issuecomment-926208319
Ok I have refactored to use a new more kube-style way of listing repositories. I've also updated the documentation and examples.
One big difference is we are no longer supporting the legacy location for the backupRepository information. The SolrCloud.withDefaults() will auto-move this information to the `backupRepositories` list. However, we lose the old location where these directories were mounted. This is not an issue, since the Solr Operator will use this new location after it's upgrade. The only issue is when the Solr Operator has upgraded, but the SolrClouds are still in a rolling-restart to update the mountPath of the legacy backupRepo. Users will have to hold off of doing any backups until the SolrClouds they are using have been fully updated. Once this is done, there should be no issues with backwards-compatibility.
I have added a warning for this in the upgrade-notes for `v0.5.0`. I think it's an acceptable tradeoff to not need a lot of custom code. Also we will be able to remove the deprecated option completely in `v0.6.0` without creating an issue for existing clouds (since the deprecated option will have been moved to `backupRepositories`).
I think there are a few things left to do:
- [ ] Add helm chart options for the backupRepositories, update Helm documentation
- [x] Add a changelog entry.
- [x] Add assert messages in new tests.
Will try to wrap this up tomorrow. And hopefully do some integration tests afterwards. Would love help with the integration tests if anyone has time for 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
[GitHub] [solr-operator] HoustonPutman commented on pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#issuecomment-920864016
@gerlowskija you can run ‘make fmt’ to fix the formatting issues or ‘make prepare’ to auto-fix all possible linting issues that can be auto-fixed
--
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] gerlowskija commented on a change in pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on a change in pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#discussion_r706189501
##########
File path: controllers/solrcloud_controller.go
##########
@@ -699,6 +695,41 @@ func reconcileCloudStatus(r *SolrCloudReconciler, solrCloud *solr.SolrCloud, log
return outOfDatePods, outOfDatePodsNotStarted, availableUpdatedPodCount, nil
}
+func isPodReadyForBackup(pod corev1.Pod, backupOptions *solr.SolrBackupRestoreOptions) bool {
Review comment:
As written, this method checks if a single pod is ready to handle a backup for any/all of its repositories. This information gets surfaced to consumers under the `backupRestoreReady` boolean field of `SolrCloudStatus`.
We can definitely change it to take a specific repository, but since this call isn't happening at backup-time, we'd still have to call it in a loop for each repository defined on the solrcloud instance and then "AND" each of those results together to get the single `backupRestoreReady` value. So I'm not really sure that gets us anything?
Unless you're thinking that we'd change SolrCloudStatus to include a distinct "backupReadiness" status for each repository so that we could check the one we care about at "backup time". I'd be OK with that if that's what you're suggesting, just wanted to double check we're envisioning the same thing here as it's a broader change than just the function-signature.
--
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] gerlowskija commented on pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#issuecomment-920860482
Actually - I see there are a few errors in the PR build that come out of 'make lint'. It looks like the 'lint' output proposes a fix in each line that has bad indentation etc. which is really cool. Just wanted to check before I fix those: is there a programmatic way to apply those fixes, or is "by hand" the best approach for these 'make lint' errors?
--
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] gerlowskija commented on a change in pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on a change in pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#discussion_r706111480
##########
File path: controllers/util/backup_util.go
##########
@@ -440,3 +483,13 @@ func RunExecForPod(podName string, namespace string, command []string, config re
return nil
}
+
+// TODO Is the Mozilla license on hashicorp's go-version lib we're using here acceptable by Apache? Is there a better option?
+func SupportsGcsBackups(versionStr string) (bool, error) {
Review comment:
Gah, nvm. I just traced through where the Solr "version" comes from and you were correct that it's coming from the image tag and not anything more reliable. Not sure why I thought it came from a Solr API iself.
This definitely needs to go then. Though maybe that's an area for improvement in the future - we could use something more reliable than the tag down the road.
--
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] gerlowskija commented on a change in pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on a change in pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#discussion_r707291123
##########
File path: api/v1beta1/solrcloud_types.go
##########
@@ -325,20 +329,158 @@ type SolrEphemeralDataStorageOptions struct {
EmptyDir *corev1.EmptyDirVolumeSource `json:"emptyDir,omitempty"`
}
+func (backupOptions *SolrBackupRestoreOptions) IsManaged() bool {
+ return true
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetVolumeSource() corev1.VolumeSource {
+ return *backupOptions.Volume
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetDirectory() string {
+ return backupOptions.Directory
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetSolrMountPath() string {
+ return BaseBackupRestorePath
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetInitPodMountPath() string {
+ return BackupRestoreInitContainerPath
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetBackupPath(backupName string) string {
+ return backupOptions.GetSolrMountPath() + "/backups/" + backupName
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetVolumeName() string {
+ return BackupRestoreVolume
+}
+
type SolrBackupRestoreOptions struct {
+ // TODO Do we need to support this (Volume) here for backcompat, or can it live exclusively in ManagedStorage?
+
// This is a volumeSource for a volume that will be mounted to all solrNodes to store backups and load restores.
// The data within the volume will be namespaces for this instance, so feel free to use the same volume for multiple clouds.
// Since the volume will be mounted to all solrNodes, it must be able to be written from multiple pods.
// If a PVC reference is given, the PVC must have `accessModes: - ReadWriteMany`.
// Other options are to use a NFS volume.
- Volume corev1.VolumeSource `json:"volume"`
+ Volume *corev1.VolumeSource `json:"volume,omitempty"`
+
+ // Allows specification of multiple different "repositories" for Solr to use when backing up data to GCS.
+ GcsRepositories *[]GcsStorage `json:"gcsRepositories,omitempty"`
+ // Allows specification of multiple different "repositories" for Solr to use when backing up data "locally".
+ // Repositories defined here are considered "managed" and can take advantage of special operator features, such as
+ // post-backup compression.
+ ManagedRepositories *[]ManagedStorage `json:"managedRepositories,omitempty""`
+
+ // TODO Do we need to support this here for backcompat, or can it live exclusively in ManagedStorage
// Select a custom directory name to mount the backup/restore data from the given volume.
// If not specified, then the name of the solrcloud will be used by default.
// +optional
Directory string `json:"directory,omitempty"`
}
+type GcsStorage struct {
+ // A name used to identify this GCS storage profile.
+ Name string `json:"name"`
+
+ // The name of the GCS bucket that all backup data will be stored in
+ Bucket string `json:"bucket"`
+
+ // The name of a Kubernetes secret holding a Google cloud service account key
+ GcsCredentialSecret string `json:"gcsCredentialSecret"`
+ // JEGERLOW TODO Should 'baseLocation' be optional?
+ // A chroot within the bucket to store data in. If specified this should already exist
+ BaseLocation string `json:"baseLocation"`
+}
+
+func VolumeExistsWithName(needle string, haystack []corev1.Volume) bool {
+ for _, volume := range haystack {
+ if volume.Name == needle {
+ return true
+ }
+ }
+ return false
+}
+
+func (gcsRepository *GcsStorage) IsManaged() bool { return false }
+
+func (gcsRepository *GcsStorage) GetSolrMountPath() string {
+ return fmt.Sprintf("%s-%s-%s", BaseBackupRestorePath, "gcscredential", gcsRepository.Name)
+}
+
+func (gcsRepository *GcsStorage) GetInitPodMountPath() string {
+ return ""
+}
+
+func (gcsRepository *GcsStorage) GetBackupPath(backupName string) string {
+ if gcsRepository.BaseLocation != "" {
+ return gcsRepository.BaseLocation
+ }
+ return "/"
+}
+
+func (gcsRepository *GcsStorage) GetVolumeName() string {
+ return fmt.Sprintf("%s-%s", gcsRepository.Name, BackupRestoreCredentialVolume)
+}
+
+func (gcs *GcsStorage) IsCredentialVolumePresent(volumes []corev1.Volume) bool {
+ expectedVolumeName := gcs.GetVolumeName()
+ return VolumeExistsWithName(expectedVolumeName, volumes)
+}
+
+type ManagedStorage struct {
+ // A name used to identify this local storage profile.
+ Name string `json:"name"`
+
+ // This is a volumeSource for a volume that will be mounted to all solrNodes to store backups and load restores.
+ // The data within the volume will be namespaces for this instance, so feel free to use the same volume for multiple clouds.
+ // Since the volume will be mounted to all solrNodes, it must be able to be written from multiple pods.
+ // If a PVC reference is given, the PVC must have `accessModes: - ReadWriteMany`.
+ // Other options are to use a NFS volume.
+ Volume *corev1.VolumeSource `json:"volume,omitempty"`
+
+ // Select a custom directory name to mount the backup/restore data from the given volume.
+ // If not specified, then the name of the solrcloud will be used by default.
+ // +optional
+ Directory string `json:"directory,omitempty"`
+}
+
+func (managedRepository *ManagedStorage) IsManaged() bool { return true }
Review comment:
AFAICT, golang "receiver functions" need to be defined in the same package as the type they're received by. So I was able to move these out to a new file (solrcloud_type_helpers.go), but it's still in the API package by necessity.
Hope that's what you were looking for!
--
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 edited a comment on pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
HoustonPutman edited a comment on pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#issuecomment-926208319
Ok I have refactored to use a new more kube-style way of listing repositories. I've also updated the documentation and examples.
One big difference is we are no longer supporting the legacy location for the backupRepository information. The SolrCloud.withDefaults() will auto-move this information to the `backupRepositories` list. However, we lose the old location where these directories were mounted. This is not an issue, since the Solr Operator will use this new location after it's upgrade. The only issue is when the Solr Operator has upgraded, but the SolrClouds are still in a rolling-restart to update the mountPath of the legacy backupRepo. Users will have to hold off of doing any backups until the SolrClouds they are using have been fully updated. Once this is done, there should be no issues with backwards-compatibility.
I have added a warning for this in the upgrade-notes for `v0.5.0`. I think it's an acceptable tradeoff to not need a lot of custom code. Also we will be able to remove the deprecated option completely in `v0.6.0` without creating an issue for existing clouds (since the deprecated option will have been moved to `backupRepositories`).
I think there are a few things left to do:
- [ ] Add helm chart options for the backupRepositories, update Helm documentation
- [ ] Add a changelog entry.
- [x] Add assert messages in new tests.
Will try to wrap this up tomorrow. And hopefully do some integration tests afterwards. Would love help with the integration tests if anyone has time for 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
[GitHub] [solr-operator] HoustonPutman merged pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
HoustonPutman merged pull request #302:
URL: https://github.com/apache/solr-operator/pull/302
--
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] gerlowskija commented on pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#issuecomment-904077839
Thanks for the review Houston. Just getting back from some time off now; gonna try to address your comments in the next day or two, as well as getting the docs and tests in order.
--
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 #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on a change in pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#discussion_r691586870
##########
File path: api/v1beta1/solrbackup_types.go
##########
@@ -38,12 +38,16 @@ type SolrBackupSpec struct {
// A reference to the SolrCloud to create a backup for
SolrCloud string `json:"solrCloud"`
+ // The name of the repository to use for the backup. Defaults to "legacy_local_repository" if not specified (the
+ // auto-configured repository for legacy singleton volumes).
+ RepositoryName string `json:"repositoryName,omitempty"`
Review comment:
```suggestion
// +optional
RepositoryName string `json:"repositoryName,omitempty"`
```
##########
File path: api/v1beta1/solrcloud_types.go
##########
@@ -325,20 +329,158 @@ type SolrEphemeralDataStorageOptions struct {
EmptyDir *corev1.EmptyDirVolumeSource `json:"emptyDir,omitempty"`
}
+func (backupOptions *SolrBackupRestoreOptions) IsManaged() bool {
+ return true
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetVolumeSource() corev1.VolumeSource {
+ return *backupOptions.Volume
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetDirectory() string {
+ return backupOptions.Directory
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetSolrMountPath() string {
+ return BaseBackupRestorePath
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetInitPodMountPath() string {
+ return BackupRestoreInitContainerPath
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetBackupPath(backupName string) string {
+ return backupOptions.GetSolrMountPath() + "/backups/" + backupName
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetVolumeName() string {
+ return BackupRestoreVolume
+}
+
type SolrBackupRestoreOptions struct {
+ // TODO Do we need to support this (Volume) here for backcompat, or can it live exclusively in ManagedStorage?
Review comment:
Volume and Directory do need to stay for back-compat, but we can deprecate them and auto-move them over to the new location.
##########
File path: controllers/util/backup_util.go
##########
@@ -440,3 +483,13 @@ func RunExecForPod(podName string, namespace string, command []string, config re
return nil
}
+
+// TODO Is the Mozilla license on hashicorp's go-version lib we're using here acceptable by Apache? Is there a better option?
+func SupportsGcsBackups(versionStr string) (bool, error) {
Review comment:
I'm not sure we even need to do this.... We should definitely **heavily** document the constraint, but there is no guarantee that the tag or version of the solr image will reflect this. I would like to ensure we never infer the version of a solr image by its tag, I think that can get us into a tricky place.
##########
File path: api/v1beta1/solrbackup_types.go
##########
@@ -38,12 +38,16 @@ type SolrBackupSpec struct {
// A reference to the SolrCloud to create a backup for
SolrCloud string `json:"solrCloud"`
+ // The name of the repository to use for the backup. Defaults to "legacy_local_repository" if not specified (the
+ // auto-configured repository for legacy singleton volumes).
+ RepositoryName string `json:"repositoryName,omitempty"`
+
// The list of collections to backup. If empty, all collections in the cloud will be backed up.
// +optional
Collections []string `json:"collections,omitempty"`
// Persistence is the specification on how to persist the backup data.
- Persistence PersistenceSource `json:"persistence"`
+ Persistence PersistenceSource `json:"persistence,omitempty"`
Review comment:
```suggestion
// +optional
Persistence *PersistenceSource `json:"persistence,omitempty"`
```
This will make the field _more_ optional. Though it will require code changes, since this is a pointer now.
##########
File path: api/v1beta1/solrcloud_types.go
##########
@@ -325,20 +329,158 @@ type SolrEphemeralDataStorageOptions struct {
EmptyDir *corev1.EmptyDirVolumeSource `json:"emptyDir,omitempty"`
}
+func (backupOptions *SolrBackupRestoreOptions) IsManaged() bool {
+ return true
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetVolumeSource() corev1.VolumeSource {
+ return *backupOptions.Volume
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetDirectory() string {
+ return backupOptions.Directory
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetSolrMountPath() string {
+ return BaseBackupRestorePath
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetInitPodMountPath() string {
+ return BackupRestoreInitContainerPath
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetBackupPath(backupName string) string {
+ return backupOptions.GetSolrMountPath() + "/backups/" + backupName
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetVolumeName() string {
+ return BackupRestoreVolume
+}
+
type SolrBackupRestoreOptions struct {
+ // TODO Do we need to support this (Volume) here for backcompat, or can it live exclusively in ManagedStorage?
+
// This is a volumeSource for a volume that will be mounted to all solrNodes to store backups and load restores.
// The data within the volume will be namespaces for this instance, so feel free to use the same volume for multiple clouds.
// Since the volume will be mounted to all solrNodes, it must be able to be written from multiple pods.
// If a PVC reference is given, the PVC must have `accessModes: - ReadWriteMany`.
// Other options are to use a NFS volume.
- Volume corev1.VolumeSource `json:"volume"`
+ Volume *corev1.VolumeSource `json:"volume,omitempty"`
+
+ // Allows specification of multiple different "repositories" for Solr to use when backing up data to GCS.
+ GcsRepositories *[]GcsStorage `json:"gcsRepositories,omitempty"`
+ // Allows specification of multiple different "repositories" for Solr to use when backing up data "locally".
+ // Repositories defined here are considered "managed" and can take advantage of special operator features, such as
+ // post-backup compression.
+ ManagedRepositories *[]ManagedStorage `json:"managedRepositories,omitempty""`
+
+ // TODO Do we need to support this here for backcompat, or can it live exclusively in ManagedStorage
// Select a custom directory name to mount the backup/restore data from the given volume.
// If not specified, then the name of the solrcloud will be used by default.
// +optional
Directory string `json:"directory,omitempty"`
}
+type GcsStorage struct {
+ // A name used to identify this GCS storage profile.
+ Name string `json:"name"`
+
+ // The name of the GCS bucket that all backup data will be stored in
+ Bucket string `json:"bucket"`
+
+ // The name of a Kubernetes secret holding a Google cloud service account key
+ GcsCredentialSecret string `json:"gcsCredentialSecret"`
+ // JEGERLOW TODO Should 'baseLocation' be optional?
+ // A chroot within the bucket to store data in. If specified this should already exist
+ BaseLocation string `json:"baseLocation"`
Review comment:
yeah, this should be optional.
##########
File path: api/v1beta1/solrcloud_types.go
##########
@@ -325,20 +329,158 @@ type SolrEphemeralDataStorageOptions struct {
EmptyDir *corev1.EmptyDirVolumeSource `json:"emptyDir,omitempty"`
}
+func (backupOptions *SolrBackupRestoreOptions) IsManaged() bool {
+ return true
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetVolumeSource() corev1.VolumeSource {
+ return *backupOptions.Volume
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetDirectory() string {
+ return backupOptions.Directory
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetSolrMountPath() string {
+ return BaseBackupRestorePath
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetInitPodMountPath() string {
+ return BackupRestoreInitContainerPath
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetBackupPath(backupName string) string {
+ return backupOptions.GetSolrMountPath() + "/backups/" + backupName
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetVolumeName() string {
+ return BackupRestoreVolume
+}
+
type SolrBackupRestoreOptions struct {
+ // TODO Do we need to support this (Volume) here for backcompat, or can it live exclusively in ManagedStorage?
+
// This is a volumeSource for a volume that will be mounted to all solrNodes to store backups and load restores.
// The data within the volume will be namespaces for this instance, so feel free to use the same volume for multiple clouds.
// Since the volume will be mounted to all solrNodes, it must be able to be written from multiple pods.
// If a PVC reference is given, the PVC must have `accessModes: - ReadWriteMany`.
// Other options are to use a NFS volume.
- Volume corev1.VolumeSource `json:"volume"`
+ Volume *corev1.VolumeSource `json:"volume,omitempty"`
+
+ // Allows specification of multiple different "repositories" for Solr to use when backing up data to GCS.
+ GcsRepositories *[]GcsStorage `json:"gcsRepositories,omitempty"`
+ // Allows specification of multiple different "repositories" for Solr to use when backing up data "locally".
+ // Repositories defined here are considered "managed" and can take advantage of special operator features, such as
+ // post-backup compression.
+ ManagedRepositories *[]ManagedStorage `json:"managedRepositories,omitempty""`
+
+ // TODO Do we need to support this here for backcompat, or can it live exclusively in ManagedStorage
// Select a custom directory name to mount the backup/restore data from the given volume.
// If not specified, then the name of the solrcloud will be used by default.
// +optional
Directory string `json:"directory,omitempty"`
}
+type GcsStorage struct {
+ // A name used to identify this GCS storage profile.
+ Name string `json:"name"`
+
+ // The name of the GCS bucket that all backup data will be stored in
+ Bucket string `json:"bucket"`
+
+ // The name of a Kubernetes secret holding a Google cloud service account key
+ GcsCredentialSecret string `json:"gcsCredentialSecret"`
+ // JEGERLOW TODO Should 'baseLocation' be optional?
+ // A chroot within the bucket to store data in. If specified this should already exist
+ BaseLocation string `json:"baseLocation"`
+}
+
+func VolumeExistsWithName(needle string, haystack []corev1.Volume) bool {
+ for _, volume := range haystack {
+ if volume.Name == needle {
+ return true
+ }
+ }
+ return false
+}
+
+func (gcsRepository *GcsStorage) IsManaged() bool { return false }
+
+func (gcsRepository *GcsStorage) GetSolrMountPath() string {
+ return fmt.Sprintf("%s-%s-%s", BaseBackupRestorePath, "gcscredential", gcsRepository.Name)
+}
+
+func (gcsRepository *GcsStorage) GetInitPodMountPath() string {
+ return ""
+}
+
+func (gcsRepository *GcsStorage) GetBackupPath(backupName string) string {
+ if gcsRepository.BaseLocation != "" {
+ return gcsRepository.BaseLocation
+ }
+ return "/"
+}
+
+func (gcsRepository *GcsStorage) GetVolumeName() string {
+ return fmt.Sprintf("%s-%s", gcsRepository.Name, BackupRestoreCredentialVolume)
+}
+
+func (gcs *GcsStorage) IsCredentialVolumePresent(volumes []corev1.Volume) bool {
+ expectedVolumeName := gcs.GetVolumeName()
+ return VolumeExistsWithName(expectedVolumeName, volumes)
+}
+
+type ManagedStorage struct {
+ // A name used to identify this local storage profile.
+ Name string `json:"name"`
+
+ // This is a volumeSource for a volume that will be mounted to all solrNodes to store backups and load restores.
+ // The data within the volume will be namespaces for this instance, so feel free to use the same volume for multiple clouds.
+ // Since the volume will be mounted to all solrNodes, it must be able to be written from multiple pods.
+ // If a PVC reference is given, the PVC must have `accessModes: - ReadWriteMany`.
+ // Other options are to use a NFS volume.
+ Volume *corev1.VolumeSource `json:"volume,omitempty"`
Review comment:
We don't want this to be optional right? If it's a managed backup, they need a readWriteMany volume.
##########
File path: controllers/solrcloud_controller.go
##########
@@ -699,6 +695,41 @@ func reconcileCloudStatus(r *SolrCloudReconciler, solrCloud *solr.SolrCloud, log
return outOfDatePods, outOfDatePodsNotStarted, availableUpdatedPodCount, nil
}
+func isPodReadyForBackup(pod corev1.Pod, backupOptions *solr.SolrBackupRestoreOptions) bool {
Review comment:
I would think that this method would take a `repository`. Because a pod might be ready for one repo, but not another.
##########
File path: controllers/solrbackup_controller.go
##########
@@ -253,7 +283,21 @@ func persistSolrCloudBackups(r *SolrBackupReconciler, backup *solrv1beta1.SolrBa
}
now := metav1.Now()
- persistenceJob := util.GenerateBackupPersistenceJobForCloud(backup, solrCloud)
+ backupRepository, found := util.GetBackupRepositoryByName(solrCloud.Spec.StorageOptions.BackupRestoreOptions, backup.Spec.RepositoryName)
+ if ! found {
+ err = fmt.Errorf("Unable to find backup repository to use for backup [%s] (which specified the repository" +
+ "[%s]). solrcloud must define a repository matching that name (or have only 1 repository defined).",
+ backup.Name, backup.Spec.RepositoryName)
+ return err
+ }
+
+ managedBackupRepository, ok := backupRepository.(util.ManagedBackupRepository)
+ if ! ok {
Review comment:
I'd rather the persistence stuff go in this `if ok {`, otherwise it's a little hard to follow.
##########
File path: api/v1beta1/solrcloud_types.go
##########
@@ -325,20 +329,158 @@ type SolrEphemeralDataStorageOptions struct {
EmptyDir *corev1.EmptyDirVolumeSource `json:"emptyDir,omitempty"`
}
+func (backupOptions *SolrBackupRestoreOptions) IsManaged() bool {
+ return true
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetVolumeSource() corev1.VolumeSource {
+ return *backupOptions.Volume
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetDirectory() string {
+ return backupOptions.Directory
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetSolrMountPath() string {
+ return BaseBackupRestorePath
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetInitPodMountPath() string {
+ return BackupRestoreInitContainerPath
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetBackupPath(backupName string) string {
+ return backupOptions.GetSolrMountPath() + "/backups/" + backupName
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetVolumeName() string {
+ return BackupRestoreVolume
+}
+
type SolrBackupRestoreOptions struct {
+ // TODO Do we need to support this (Volume) here for backcompat, or can it live exclusively in ManagedStorage?
+
// This is a volumeSource for a volume that will be mounted to all solrNodes to store backups and load restores.
// The data within the volume will be namespaces for this instance, so feel free to use the same volume for multiple clouds.
// Since the volume will be mounted to all solrNodes, it must be able to be written from multiple pods.
// If a PVC reference is given, the PVC must have `accessModes: - ReadWriteMany`.
// Other options are to use a NFS volume.
- Volume corev1.VolumeSource `json:"volume"`
+ Volume *corev1.VolumeSource `json:"volume,omitempty"`
+
+ // Allows specification of multiple different "repositories" for Solr to use when backing up data to GCS.
+ GcsRepositories *[]GcsStorage `json:"gcsRepositories,omitempty"`
+ // Allows specification of multiple different "repositories" for Solr to use when backing up data "locally".
+ // Repositories defined here are considered "managed" and can take advantage of special operator features, such as
+ // post-backup compression.
+ ManagedRepositories *[]ManagedStorage `json:"managedRepositories,omitempty""`
+
+ // TODO Do we need to support this here for backcompat, or can it live exclusively in ManagedStorage
// Select a custom directory name to mount the backup/restore data from the given volume.
// If not specified, then the name of the solrcloud will be used by default.
// +optional
Directory string `json:"directory,omitempty"`
}
+type GcsStorage struct {
+ // A name used to identify this GCS storage profile.
+ Name string `json:"name"`
+
+ // The name of the GCS bucket that all backup data will be stored in
+ Bucket string `json:"bucket"`
+
+ // The name of a Kubernetes secret holding a Google cloud service account key
+ GcsCredentialSecret string `json:"gcsCredentialSecret"`
+ // JEGERLOW TODO Should 'baseLocation' be optional?
+ // A chroot within the bucket to store data in. If specified this should already exist
+ BaseLocation string `json:"baseLocation"`
+}
+
+func VolumeExistsWithName(needle string, haystack []corev1.Volume) bool {
+ for _, volume := range haystack {
+ if volume.Name == needle {
+ return true
+ }
+ }
+ return false
+}
+
+func (gcsRepository *GcsStorage) IsManaged() bool { return false }
+
+func (gcsRepository *GcsStorage) GetSolrMountPath() string {
+ return fmt.Sprintf("%s-%s-%s", BaseBackupRestorePath, "gcscredential", gcsRepository.Name)
+}
+
+func (gcsRepository *GcsStorage) GetInitPodMountPath() string {
+ return ""
+}
+
+func (gcsRepository *GcsStorage) GetBackupPath(backupName string) string {
+ if gcsRepository.BaseLocation != "" {
+ return gcsRepository.BaseLocation
+ }
+ return "/"
+}
+
+func (gcsRepository *GcsStorage) GetVolumeName() string {
+ return fmt.Sprintf("%s-%s", gcsRepository.Name, BackupRestoreCredentialVolume)
+}
+
+func (gcs *GcsStorage) IsCredentialVolumePresent(volumes []corev1.Volume) bool {
+ expectedVolumeName := gcs.GetVolumeName()
+ return VolumeExistsWithName(expectedVolumeName, volumes)
+}
+
+type ManagedStorage struct {
+ // A name used to identify this local storage profile.
+ Name string `json:"name"`
+
+ // This is a volumeSource for a volume that will be mounted to all solrNodes to store backups and load restores.
+ // The data within the volume will be namespaces for this instance, so feel free to use the same volume for multiple clouds.
+ // Since the volume will be mounted to all solrNodes, it must be able to be written from multiple pods.
+ // If a PVC reference is given, the PVC must have `accessModes: - ReadWriteMany`.
+ // Other options are to use a NFS volume.
+ Volume *corev1.VolumeSource `json:"volume,omitempty"`
+
+ // Select a custom directory name to mount the backup/restore data from the given volume.
+ // If not specified, then the name of the solrcloud will be used by default.
+ // +optional
+ Directory string `json:"directory,omitempty"`
+}
+
+func (managedRepository *ManagedStorage) IsManaged() bool { return true }
Review comment:
I think it would be good to get a fair number of these methods out of the API part and move them somewhere else. I know there are a lot of similar examples in here already, but I need to go and refactor that sometime.
--
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 pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
HoustonPutman commented on pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#issuecomment-915351296
> before I disappear for an upcoming paternity leave
Congrats!!! 🎉
No worries, I've been busy on the S3 Repo stuff and the v0.4.0 release. I do plan on taking this forward over the next few weeks. Mind if I make some changes while you are working on other stuff?
--
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] gerlowskija commented on a change in pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on a change in pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#discussion_r706104773
##########
File path: api/v1beta1/solrcloud_types.go
##########
@@ -325,20 +329,158 @@ type SolrEphemeralDataStorageOptions struct {
EmptyDir *corev1.EmptyDirVolumeSource `json:"emptyDir,omitempty"`
}
+func (backupOptions *SolrBackupRestoreOptions) IsManaged() bool {
+ return true
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetVolumeSource() corev1.VolumeSource {
+ return *backupOptions.Volume
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetDirectory() string {
+ return backupOptions.Directory
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetSolrMountPath() string {
+ return BaseBackupRestorePath
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetInitPodMountPath() string {
+ return BackupRestoreInitContainerPath
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetBackupPath(backupName string) string {
+ return backupOptions.GetSolrMountPath() + "/backups/" + backupName
+}
+
+func (backupOptions *SolrBackupRestoreOptions) GetVolumeName() string {
+ return BackupRestoreVolume
+}
+
type SolrBackupRestoreOptions struct {
+ // TODO Do we need to support this (Volume) here for backcompat, or can it live exclusively in ManagedStorage?
Review comment:
Just to double-check: when you say "auto-move" you're suggesting that early in its processing loop the operator would restructure and 'SolrBackupRestoreOptions' it sees so that any value held by 'Value' here is moved over to be a 'ManagedRepository' entry. (And that way the rest of the operator can ignore the 'Volume' field).
--
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] gerlowskija commented on a change in pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on a change in pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#discussion_r706189501
##########
File path: controllers/solrcloud_controller.go
##########
@@ -699,6 +695,41 @@ func reconcileCloudStatus(r *SolrCloudReconciler, solrCloud *solr.SolrCloud, log
return outOfDatePods, outOfDatePodsNotStarted, availableUpdatedPodCount, nil
}
+func isPodReadyForBackup(pod corev1.Pod, backupOptions *solr.SolrBackupRestoreOptions) bool {
Review comment:
As written, this method checks if a single pod is ready to handle a backup for any/all of its repositories. This information gets surfaced to consumers under the `backupRestoreReady` boolean field of `SolrCloudStatus`.
We can definitely change it to take a specific repository, but since this call isn't happening at backup-time, we'd still have to call it in a loop for each repository defined on the solrcloud instance and then "AND" each of those results together to get the single `backupRestoreReady` value. So I'm not really sure that gets us anything?
Unless you're thinking that we'd change SolrCloudStatus to include a distinct "backupReadiness" status for each repository so that we could check the one we care about at "backup time". I'd be OK with that if that's what you're suggesting, just wanted to double check we're envisioning the same thing 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] gerlowskija commented on pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#issuecomment-916986870
Thanks!
> Mind if I make some changes while you are working on other stuff?
Not at all, thanks for the help! My plate cleared up a bit after posting the other day, so I'll actually be picking this up again in the short term (until paternity takes me away again). So if I'm unable to finish, hopefully I can at least whittle down what'll be left when you turn your attention to 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] gerlowskija commented on a change in pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on a change in pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#discussion_r694262621
##########
File path: api/v1beta1/solrbackup_types.go
##########
@@ -38,12 +38,16 @@ type SolrBackupSpec struct {
// A reference to the SolrCloud to create a backup for
SolrCloud string `json:"solrCloud"`
+ // The name of the repository to use for the backup. Defaults to "legacy_local_repository" if not specified (the
+ // auto-configured repository for legacy singleton volumes).
+ RepositoryName string `json:"repositoryName,omitempty"`
+
// The list of collections to backup. If empty, all collections in the cloud will be backed up.
// +optional
Collections []string `json:"collections,omitempty"`
// Persistence is the specification on how to persist the backup data.
- Persistence PersistenceSource `json:"persistence"`
+ Persistence PersistenceSource `json:"persistence,omitempty"`
Review comment:
(I hadn't seen this "Suggested Change" feature of Github's - very cool!)
I'm not against having a pointer here - I'll have to read the docs a bit on what you mean by "more optional" haha, but sounds good to 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.
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] gerlowskija commented on a change in pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on a change in pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#discussion_r706111480
##########
File path: controllers/util/backup_util.go
##########
@@ -440,3 +483,13 @@ func RunExecForPod(podName string, namespace string, command []string, config re
return nil
}
+
+// TODO Is the Mozilla license on hashicorp's go-version lib we're using here acceptable by Apache? Is there a better option?
+func SupportsGcsBackups(versionStr string) (bool, error) {
Review comment:
Gah, nvm. I just traced through where the Solr "version" comes from and you were correct that it's coming from the image tag and not anything more reliable. Not sure why I thought it came from a Solr API.
This definitely needs to go then. Though maybe that's an area for improvement in the future - we could use something more reliable than the tag down the road.
--
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] gerlowskija commented on pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#issuecomment-898666314
I've updated this to use different CRD's (see the latest comment on the related issue #301). This also adds support for defining multiple backup repositories in your solrcloud definition which is pretty cool.
Still needs tests, docs for the new CRD fields, and there's currently a bug where GCS backups succeed but this success is never persisted in the kube status. Hope to fix those shortly.
--
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] gerlowskija commented on pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
gerlowskija commented on pull request #302:
URL: https://github.com/apache/solr-operator/pull/302#issuecomment-920856983
Alright, quick update on my progress here. Since I last posted, I've
- addressed all of Houston's requests (except two where I asked for clarification on what he wanted done)
- fixed a bug mentioned earlier around updating backup status when persistence was skipped.
- added some unit tests
- added examples (under `examples/`) and re-wrote the backup docs (in `docs/solr-backup/README.md`)
- done misc cleanup (e.g. removing personal debug logging, etc.)
That's everything I had planned to do here, unless @HoustonPutman has other feedback or gets a chance to see my responses from his earlier review. But otherwise I think this is ready 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
[GitHub] [solr-operator] HoustonPutman merged pull request #302: Allow users to take GCS-based backups
Posted by GitBox <gi...@apache.org>.
HoustonPutman merged pull request #302:
URL: https://github.com/apache/solr-operator/pull/302
--
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