*: add ABS support for backup and restore #1842

rjtsdl · 2018-01-09T23:52:00Z

Add ABS support for backup-operator and restore-operator
Fix #1784

etcd-bot · 2018-01-09T23:52:04Z

Can one of the admins verify this patch?

etcd-bot · 2018-01-09T23:52:07Z

Can one of the admins verify this patch?

etcd-bot · 2018-01-09T23:53:01Z

Can one of the admins verify this patch?

hongchaodeng · 2018-01-10T00:02:04Z

Our backup test right now is a bit flaky: #1825

We are focusing on that and need to fix it first. We have rules to prioritize making testing stable, fixing bugs over adding features. Will review this PR once it is fixed.

rjtsdl · 2018-01-10T00:03:10Z

@hongchaodeng SGTM !

fanminshi · 2018-01-10T00:03:42Z

pkg/backup/writer/abs_writer.go

+
+	blob := containerRef.GetBlobReference(key)
+	putBlobOpts := storage.PutBlobOptions{}
+	err = blob.CreateBlockBlobFromReader(r, &putBlobOpts)


inline putBlobOpts to
err = blob.CreateBlockBlobFromReader(r, &storage.PutBlobOptions{})

fanminshi · 2018-01-10T00:04:01Z

pkg/backup/writer/abs_writer.go

+	}
+
+	getBlobOpts := &storage.GetBlobOptions{}
+	_, err = blob.Get(getBlobOpts)


inline getBlobOpts to
blob.Get(&storage.GetBlobOptions{})

fanminshi · 2018-01-10T00:06:05Z

pkg/controller/restore-operator/http.go

+		if err != nil {
+			return fmt.Errorf("failed to create ABS client: %v", err)
+		}
+		// Nothing to Close for absCli yet


when should absCli be closed?

it is simply an object. We don't need to explicit call Close. It would be GC when no reference :)

@rjtsdl sometime a client can create gorutines that must be closed to avoid any go routines leaks. not sure if that's the case for abs client.

absCli doesn't do that. :)

good to know.

fanminshi · 2018-01-10T00:08:28Z

pkg/util/azureutil/absfactory/client.go

+
+	abs := bc.GetBlobService()
+	w.ABS = &abs
+	return w, nil


inline w as
return &ABSClient{ABS: &bc.GetBlobService()}

It throw error "cannot take address of bc.GetBlobService()" So I can only do small refactoring

xiang90 · 2018-01-16T23:29:59Z

pkg/backup/reader/abs_reader.go

+	abs *storage.BlobStorageClient
+}
+
+func NewABSReader(abs *storage.BlobStorageClient) Reader {


doc string on public func.

@rjtsdl ping? fix the above?

xiang90 · 2018-01-16T23:30:29Z

pkg/backup/writer/abs_writer.go

+	"io"
+
+	"github.com/Azure/azure-sdk-for-go/storage"
+	"github.com/coreos/etcd-operator/pkg/backup/util"


separate internal and external import

change the import according to what @xiang90's suggestion?

e.g:

"bytes" "encoding/base64" "fmt" "github.com/coreos/etcd-operator/pkg/backup/util" "github.com/Azure/azure-sdk-for-go/storage" "github.com/pborman/uuid"

xiang90 · 2018-01-16T23:31:46Z

@rjtsdl what is the test plan for this?

fanminshi · 2018-01-16T23:34:47Z

pkg/backup/reader/abs_reader.go

+
+	blob := containerRef.GetBlobReference(key)
+	getBlobOpts := &storage.GetBlobOptions{}
+	return blob.Get(getBlobOpts)


inline getBlobOpts? to
return blob.Get( &storage.GetBlobOptions{})

@rjtsdl ping? fix the above?

rjtsdl · 2018-01-17T00:26:00Z

@xiang90 currently I don't have unittests for this. Mostly it is me, building custom image, and try it out with ABS specified.

What's the plan for different storage types? ABS should have no difference.

xiang90 · 2018-01-17T00:27:53Z

What's the plan for different storage types? ABS should have no difference.

There is an e2e test for S3 storage. I am fine if ABS starts with replicating that test.

fanminshi · 2018-01-17T00:33:18Z

@rjtsdl take a look of how TestBackupAndRestore in e2eslow work. try to see if you can get it work on ABS.

rjtsdl · 2018-01-17T00:59:52Z

@fanminshi @xiang90 thx for the pointer. I will add test in this PR. It may not be very soon. But bear with me :)

fanminshi · 2018-01-17T01:03:57Z

@rjtsdl take your time. feel free to ask if you have any questions or concerns.

rjtsdl · 2018-01-22T18:39:23Z

@shrutir25 we need to rebase/merge with the latest master :)

rjtsdl · 2018-01-22T22:51:24Z

@shrutir25 added tests for ABS.
@fanminshi @xiang90 TAL. Let me know if you want to have an ABS secrets setup for your test pipeline. We may able to help. :)

xiang90 · 2018-01-22T22:53:05Z

@rjtsdl The API parts and the overall workflow look good to me. Need @hongchaodeng and @fanminshi to look through the implementation and tests. Thanks for the contribution!

hongchaodeng · 2018-01-22T23:01:04Z

test/container/run

@@ -20,7 +20,10 @@ echo "TEST_NAMESPACE: ${TEST_NAMESPACE}"
 echo "OPERATOR_IMAGE: ${OPERATOR_IMAGE}"

 export TEST_AWS_SECRET="aws"
+export TEST_ABS_SECRET="abs"


Please don't change this file test/container/run

hongchaodeng · 2018-01-22T23:01:56Z

test/e2e/e2eslow/backup_restore_test.go

@@ -81,6 +82,49 @@ func TestBackupAndRestore(t *testing.T) {
 	testEtcdRestoreOperatorForS3Source(t, clusterName, s3Path)
 }

+// TestBackupAndRestoreABS runs the backup test first, and only runs the restore test after if the backup test succeeds and sets the ABS path
+func TestBackupAndRestoreABS(t *testing.T) {


Most of the code is duplicate to TestBackupAndRestoreS3().
Can you refactor it to reduce duplication?

hongchaodeng · 2018-01-22T23:02:32Z

test/e2e/e2eutil/spec_util.go

@@ -59,6 +59,30 @@ func NewS3Backup(endpoints []string, clusterName, path, secret, clientTLSSecret
 	}
 }

+// NewABSBackup creates a EtcdBackup object using clusterName.
+func NewABSBackup(endpoints []string, clusterName, path, secret, clientTLSSecret string) *api.EtcdBackup {


Most of the code are duplicate to NewS3Backup().
Can you refactor it?

hongchaodeng · 2018-01-22T23:04:10Z

pkg/controller/backup-operator/abs_backup.go

+	}
+
+	var tlsConfig *tls.Config
+	if len(clientTLSSecret) != 0 {


This code is duplicate to handleS3(). Can you refactor it?

I think we can just extract line 39 - 49 to something like generateTLSConfig ?

hongchaodeng · 2018-01-22T23:25:19Z

Can you split this PR into two:

API changes and implementation. We will review and rely on you to manually test it.
Testing. We will work together to setup test credentials to pass the test.

fanminshi · 2018-01-22T23:25:04Z

pkg/backup/writer/abs_writer.go

+
+const (
+	// AzureBlobBlockChunkLimitInBytes 100MiB is the limit
+	AzureBlobBlockChunkLimitInBytes = 104857600


prefer AzureBlobBlockChunkLimitInBytes = 100 * 1024 * 1024 for readability; It is pretty clear that 100 * 1024 * 1024 is 100MiB

fanminshi · 2018-01-22T23:26:52Z

pkg/backup/writer/abs_writer.go

+	"io"
+
+	"github.com/Azure/azure-sdk-for-go/storage"
+	"github.com/coreos/etcd-operator/pkg/backup/util"


change the import according to what @xiang90's suggestion?

e.g:

"bytes" "encoding/base64" "fmt" "github.com/coreos/etcd-operator/pkg/backup/util" "github.com/Azure/azure-sdk-for-go/storage" "github.com/pborman/uuid"

fanminshi · 2018-01-22T23:29:14Z

pkg/backup/writer/abs_writer.go

+	}
+
+	buf := new(bytes.Buffer)
+	buf.ReadFrom(r)


what if reader has a 2 GB size? Will ReadFrom return ErrTooLarge on system that has smaller memory or disk space?
I think we need to figure out a way to read 100MB into buffer and then write 100MB buffer into ABS and repeat.

@rjtsdl the suggestion is just an optimization and it will not affect the correctness. we can optimize the uploading logic in a future pr.

I will submit this optimization in the next pr since I dont want to block the merging of this current pr

Thx, we can do it in another PR. :)

fanminshi · 2018-01-22T23:39:32Z

@rjtsdl most of code looks good. I just have few concern with abs uploading logic. the test can be refactor a bit more but let's do that in a separate pr like @hongchaodeng suggested.

shrutir25 · 2018-01-23T18:58:23Z

@fanminshi @hongchaodeng - thanks for reviewing the PR ! I will make all the changes as mentioned and submit out a new PR for the tests.

shrutir25 · 2018-01-24T01:55:40Z

@rjtsdl @hongchaodeng @fanminshi @khenidak - I have made the changes as per the review comments. PTAL :)

rjtsdl · 2018-01-24T01:57:13Z

pkg/controller/backup-operator/abs_backup.go

+	}
+
+	var tlsConfig *tls.Config
+	if tlsConfig, err := generateTLSConfig(kubecli, clientTLSSecret, namespace); err != nil {


replace := with =

hongchaodeng

Code looks good in general.

hongchaodeng · 2018-01-24T03:43:58Z

test/e2e/e2eslow/backup_restore_test.go

@@ -39,7 +39,7 @@ func init() {
 	rand.Seed(time.Now().UTC().UnixNano())
 }

-// TestBackupAndRestore runs the backup test first, and only runs the restore test after if the backup test succeeds and sets the S3 path
+// TestBackupAndRestoreS3 runs the backup test first, and only runs the restore test after if the backup test succeeds and sets the S3 path


revert this?

hongchaodeng · 2018-01-24T03:44:54Z

test/pod/README.md

@@ -6,6 +6,10 @@ The scripts at `test/pod` can be used to package and run the e2e tests inside a

 The e2e tests need access to an S3 bucket for testing. Create a secret containing the aws credentials and config files in the same namespace that the test-pod will run in. Consult the [backup-operator guide][setup-aws-secret] on how to do so.

+## Create the ABS secret


In test/pod/, it is testing related.
Can you revert this and put the changes into upcoming testing PR?

sure I will add it to the test PR.

hongchaodeng · 2018-01-24T03:46:07Z

Gopkg.lock

@@ -6,6 +6,18 @@
  packages = ["compute/metadata","internal"]
  revision = "3b1ae45394a234c385be014e9a488f2bb6eef821"

+[[projects]]


Don't you need to update Gopkg.toml?
How could you have changes in Gopkg.lock while Gopkg.toml remains unchanged?

@rjtsdl - is there a change that needs to be made in Gopkg.toml ?

i saw "github.com/Azure/azure-sdk-for-go" being added. how come Gopkg.toml has not changed?

Added it :)

hongchaodeng · 2018-01-24T17:25:35Z

@etcd-bot ok to test

fanminshi · 2018-01-24T23:37:22Z

please also squash the commits into just few relevant ones:

all the go dep change can be in one commit.
all apis updates can be in one commit.
the abs_reader.go, abs_writer.go can be in one commit.
all controller changes can be in one commit.

I hope you get the idea.

hongchaodeng · 2018-01-25T17:21:20Z

Gopkg.toml

+  name = "github.com/Azure/azure-sdk-for-go"
+  version = "v11.3.0-beta"
+
+[[constraint]]


These three deps aren't imported directly in the code. Can you get rid of them:

[[constraint]] name = "github.com/Azure/go-autorest" version = "v9.6.0" [[constraint]] name = "github.com/dgrijalva/jwt-go" [[constraint]] name = "github.com/satori/uuid" version = "v1.1.0

hongchaodeng · 2018-01-25T17:22:53Z

pkg/controller/backup-operator/abs_backup.go

+	"k8s.io/client-go/kubernetes"
+)
+
+// TODO: replace this with generic backend interface for other options (PV, Azure)


remove this TODO.

hongchaodeng · 2018-01-25T17:24:11Z

test/pod/README.md

@@ -21,6 +21,7 @@ The [test-pod-tmpl.yaml](./test-pod-tmpl.yaml) can be used to define the test-po
 - `OPERATOR_IMAGE` is the etcd-operator image used for testing
 - `TEST_S3_BUCKET` is the S3 bucket name used for testing
 - `TEST_AWS_SECRET` is the secret name containing the aws credentials/config files.
+- `TEST_ABS_SECRET` is the secret name containing the abs credentials


Can you revert the changes related under test/pod/. These would come into testing PR later.

hongchaodeng · 2018-01-25T17:58:17Z

Please also update generated code:
https://github.com/coreos/etcd-operator/tree/master/hack/k8s/codegen

./hack/k8s/codegen/update-generated.sh

hongchaodeng · 2018-01-25T18:07:50Z

pkg/util/azureutil/absfactory/client.go:28:2: const tmpdir is unused

fanminshi · 2018-01-25T18:35:16Z

@rjtsdl please also rebase the commits as well. probably merge remove unused const into the commit that introduces azureutil/absfactory/client.go.

hongchaodeng · 2018-01-25T18:37:24Z

No need to worry the commits.
When merging PR, maintainers should use "Squash and merge" and make sure the commit message is great.

rjtsdl · 2018-01-25T18:38:43Z

@hongchaodeng @fanminshi

I rebased it a bit.
Yeah, squash and merge should do the work :)

fanminshi · 2018-01-25T18:42:51Z

@rjtsdl a bit nit on the commit msg. usually, we want the format to be <pkg>: commit msg.

in your case, pkg can be pkg/apis: ... for the all api changes and pkg/backup: for backup changes, and etc. if the commit contains mutiple pkgs, then use *: ... instead. I hope you get the idea.

fanminshi · 2018-01-25T18:43:34Z

@rjtsdl more info on https://github.com/coreos/etcd-operator/blob/master/CONTRIBUTING.md

hongchaodeng · 2018-01-25T18:44:54Z

@fanminshi
I will help correct the corresponding commits when merging. No need to worry about that.

fanminshi · 2018-01-25T18:45:52Z

@hongchaodeng sure thing. @rjtsdl you can follow my advice on future pr then.

fanminshi reviewed Jan 10, 2018

View reviewed changes

rjtsdl force-pushed the jiren-addabssupport branch from a2249d9 to efa6712 Compare January 11, 2018 03:42

xiang90 reviewed Jan 16, 2018

View reviewed changes

fanminshi reviewed Jan 16, 2018

View reviewed changes

hongchaodeng reviewed Jan 22, 2018

View reviewed changes

fanminshi reviewed Jan 22, 2018

View reviewed changes

fanminshi changed the title ~~add ABS support for backup and restore~~ *: add ABS support for backup and restore Jan 23, 2018

rjtsdl commented Jan 24, 2018

View reviewed changes

hongchaodeng reviewed Jan 24, 2018

View reviewed changes

rjtsdl force-pushed the jiren-addabssupport branch from e1af336 to b61134b Compare January 25, 2018 01:41

Jingtao Ren added 2 commits January 24, 2018 17:44

add ABS support for backup and restore

5f2cb2d

support large azure blob block size

daaea5d

rjtsdl force-pushed the jiren-addabssupport branch from b61134b to 69fcda8 Compare January 25, 2018 01:44

hongchaodeng reviewed Jan 25, 2018

View reviewed changes

rjtsdl force-pushed the jiren-addabssupport branch 3 times, most recently from ec07d43 to 01cd7ff Compare January 25, 2018 17:51

refactoring and address comment

e26c421

rjtsdl force-pushed the jiren-addabssupport branch from 5b030c7 to e26c421 Compare January 25, 2018 18:37

hongchaodeng self-assigned this Jan 25, 2018

hongchaodeng merged commit 8339b61 into coreos:master Jan 25, 2018

rjtsdl deleted the jiren-addabssupport branch January 25, 2018 18:50

hongchaodeng mentioned this pull request Jan 25, 2018

backup to Azure #1110

Closed

feiskyer mentioned this pull request Aug 2, 2018

Add cloud environment support for ABS backup #1981

Merged

		@@ -6,6 +6,10 @@ The scripts at `test/pod` can be used to package and run the e2e tests inside a

		The e2e tests need access to an S3 bucket for testing. Create a secret containing the aws credentials and config files in the same namespace that the test-pod will run in. Consult the [backup-operator guide][setup-aws-secret] on how to do so.

		## Create the ABS secret

*: add ABS support for backup and restore #1842

*: add ABS support for backup and restore #1842

Conversation

rjtsdl commented Jan 9, 2018 • edited Loading

etcd-bot commented Jan 9, 2018

etcd-bot commented Jan 9, 2018

etcd-bot commented Jan 9, 2018

hongchaodeng commented Jan 10, 2018

rjtsdl commented Jan 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanminshi Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiang90 commented Jan 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjtsdl commented Jan 17, 2018

xiang90 commented Jan 17, 2018

fanminshi commented Jan 17, 2018

rjtsdl commented Jan 17, 2018

fanminshi commented Jan 17, 2018

rjtsdl commented Jan 22, 2018

rjtsdl commented Jan 22, 2018

xiang90 commented Jan 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongchaodeng commented Jan 22, 2018

fanminshi Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanminshi Jan 22, 2018 • edited Loading

Choose a reason for hiding this comment

fanminshi Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanminshi commented Jan 22, 2018

shrutir25 commented Jan 23, 2018 • edited Loading

shrutir25 commented Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

hongchaodeng left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fanminshi Jan 24, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongchaodeng commented Jan 24, 2018

fanminshi commented Jan 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hongchaodeng commented Jan 25, 2018

hongchaodeng commented Jan 25, 2018

fanminshi commented Jan 25, 2018

hongchaodeng commented Jan 25, 2018 • edited Loading

rjtsdl commented Jan 25, 2018

fanminshi commented Jan 25, 2018

fanminshi commented Jan 25, 2018

hongchaodeng commented Jan 25, 2018

fanminshi commented Jan 25, 2018

rjtsdl commented Jan 9, 2018 •

edited

Loading

fanminshi Jan 23, 2018 •

edited

Loading

fanminshi Jan 22, 2018 •

edited

Loading

fanminshi Jan 22, 2018 •

edited

Loading

fanminshi Jan 23, 2018 •

edited

Loading

shrutir25 commented Jan 23, 2018 •

edited

Loading

shrutir25 commented Jan 24, 2018 •

edited

Loading

fanminshi Jan 24, 2018 •

edited

Loading

hongchaodeng commented Jan 25, 2018 •

edited

Loading