-
Notifications
You must be signed in to change notification settings - Fork 741
backend: add Azure Blob Storage (ABS) backup support #1237
Conversation
901f1b5
to
b6313c1
Compare
Thanks for the contribution. |
b6313c1
to
3af6935
Compare
@hongchaodeng design document added in latest push; please see |
doc/design/abs_backup.md
Outdated
|
||
When starting etcd operator, we need to provide flags in order to access the ABS storage account: | ||
```bash | ||
$ etcd-operator --backup-abs-secret ${secret_name} --backup-abs-container ${container_name} ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if both s3 and abs options are given, users can choose from s3 or abs? they are not mutual-exclusive, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct.
pkg/backup/abs.go
Outdated
"io/ioutil" | ||
|
||
"github.com/Sirupsen/logrus" | ||
"github.com/coreos/etcd-operator/pkg/backup/abs" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
separate internal and external imports
func (ab *absBackend) getBlobSize(key string) (int64, error) { | ||
rc, err := ab.open(key) | ||
|
||
b, err := ioutil.ReadAll(rc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need to close rc on error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated to close rc on error and otherwise return ioutil.Close(rc)
as the error returned from the function -- how does this sound?
pkg/backup/abs/abs.go
Outdated
containerExists, err := containerRef.Exists() | ||
if err != nil { | ||
return nil, err | ||
} else if !containerExists { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change this to if !containerExists
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else is not needed.
pkg/backup/abs/abs.go
Outdated
opts := &storage.DeleteBlobOptions{} | ||
err := blob.Delete(opts) | ||
|
||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return blob.Delete(&storage.DeleteBlobOptions{})
pkg/backup/abs/abs.go
Outdated
blob := w.container.GetBlobReference(blobName) | ||
|
||
opts := &storage.GetBlobOptions{} | ||
resp, err := blob.Get(opts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return blob.Get(&storage.GetBlobOptions{})
pkg/backup/abs_backend_test.go
Outdated
t.Fatal(err) | ||
} | ||
if err.Error() != "missing required environment variable of AZURE_STORAGE_ACCOUNT" { | ||
t.Fatal(err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
t.Error("explain the problem here")
@vdice Looks good in general. @hongchaodeng needs to approve this. |
pkg/backup/abs/abs.go
Outdated
return fmt.Errorf("create block blob from reader failed: %v", err) | ||
} | ||
|
||
return err |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should probably just be return nil
Code freeze right now. K8s 1.7 is released and it breaks a lot of our tests. We need to fix failures and make tests stable before merging any feature. |
2c8a0c2
to
ebdb5e1
Compare
ebdb5e1
to
3d081d3
Compare
PR updated/rebased from master; also added changelog addition. PTAL @hongchaodeng |
We won't merge any feature for a while. |
3d081d3
to
f5e5227
Compare
f5e5227
to
ce02710
Compare
@hongchaodeng rebased off master and tested to success on a (minikube) k8s 1.7.0 cluster -- please let me know if anything looks awry, when convenient. Thank you! |
type: Opaque | ||
stringData: | ||
storage-account: <storage-account-name> | ||
storage-key: <storage-key> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line?
## the section below can be added to restore from a pre-existing cluster backup | ||
restore: | ||
backupClusterName: "cluster-a" | ||
storageType: "ABS" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new line
I feel this is OK to merge. I am OK with adding Azure/AWS/GCP support in tree initially. Then we need to figure out a way to add out of tree support for backup storage. I give this pr a quick look. It seems OK. defer to @hongchaodeng, @hasbro17 and @fanminshi for a detailed review. |
Yeah. I agree that we should merge this and move forward. |
cmd/operator/main.go
Outdated
@@ -76,6 +79,9 @@ func init() { | |||
flag.StringVar(&awsConfig, "backup-aws-config", "", | |||
"The name of the kube configmap object that stores the AWS config file. The file name must be 'config'.") | |||
flag.StringVar(&s3Bucket, "backup-s3-bucket", "", "The name of the AWS S3 bucket to store backups in.") | |||
flag.StringVar(&absSecret, "backup-abs-secret", "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will deprecate the the operator wide flag for backup storage options.
Each cluster's backup storage option should sit in its own cluster spec.
@@ -0,0 +1,19 @@ | |||
apiVersion: "etcd.database.coreos.com/v1beta2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example in spec_examples.md is good enough.
To keep it consistent, no duplicate example here again.
doc/user/backup_config.md
Outdated
|
||
See the [ABS backup with cluster specific configuration](spec_examples.md#abs-backup-and-cluster-specific-abs-configuration) spec to see what the cluster's `spec.backup` field should be configured as to set a cluster specific ABS backup configuration. The following additional fields need to be set under the cluster spec's `spec.backup.abs` field: | ||
- `absContainer`: The name of the ABS container to store backups in. | ||
- `absSecret`: The secret object name which should contain two key value pairs for the ABS account name and key. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please mention the file layout of the secret:
storage-account:
storage-key:
@@ -0,0 +1,8 @@ | |||
apiVersion: v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put this under example/azure/ ?
glide.yaml
Outdated
@@ -25,3 +25,5 @@ import: | |||
- package: github.com/jpillora/go-ogle-analytics | |||
- package: golang.org/x/net | |||
- package: golang.org/x/time | |||
- package: github.com/Azure/azure-sdk-for-go/storage |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put glide.yaml, glide.lock change in separate commit ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is no different to vendor the entire repo or specific dir under the repo.
Just
package: github.com/Azure/azure-sdk-for-go
version ...
BTW, it's v10.3.0-beta
now :)
@vdice
|
@hongchaodeng Great news! I'll get to work addressing code comments and breaking this up into separate PRs as requested. Will have an update (and new PRs) by tomorrow. |
@hongchaodeng pushed an additional commit that hopefully addresses all feedback. Wanted to make sure I'm on the right track prior to breaking these changes out. In particular, hoping for confirmation that I fully removed operator flag/backup customization in anticipation of the deprecation you've mentioned. Thanks! |
a5c48b3
to
05c91cf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The direction looks good. Just some implementation detail issues. But we can fix them along the way in individual PRs.
Thanks @vdice for the fast work.
doc/design/abs_backup.md
Outdated
|
||
When starting etcd operator, we need to provide flags in order to access the ABS storage account: | ||
```bash | ||
$ etcd-operator --backup-abs-secret ${secret_name} --backup-abs-container ${container_name} ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the flag?
return err | ||
} | ||
|
||
os.Setenv(env.ABSStorageAccount, string(se.Data[spec.ABSStorageAccount])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't set env. This will affect other clusters.
doc/user/backup_config.md
Outdated
@@ -4,6 +4,7 @@ In etcd operator, we provide the following options to save cluster backups to: | |||
- Persistent Volume (PV) on GCE or AWS | |||
- Persistent Volume (PV) with custom StorageClasses | |||
- S3 bucket on AWS | |||
- ABS container on Azure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Azure Blob Storage (ABS) container
05c91cf
to
6296c5b
Compare
(work split up into PRs linked above -- follow those for progress) |
This PR adds Azure Blob Storage (ABS) backup support.
Accomplishes part of #1110
[skip ci]