GraphQL Admin API: Support Backup operation #4706

pawanrawal · 2020-01-31T14:02:47Z

This PR adds support for doing backups using the GraphQL Admin API. A backup can be initiated with the following mutation

mutation {
  backup(input: {destination: "/tmp", accessKey: "xxx", secretKey: "xxx", 
    sessionToken: "xxx", anonymous: true, forceFull: true}) {
    
    response {
      code
      message
    }
  }
}

The options for the backup are the same as those that we have in the HTTP API. Common code for backup has been moved to worker.ProcessBackupRequest so that it can be called from both /admin/backup and from the graphql admin package.

This change is

…graphql/admin and ee package

golangcibot · 2020-01-31T14:06:01Z

ee/backup/backup.go

@@ -297,3 +297,4 @@ func writeKVList(list *bpb.KVList, w io.Writer) error {
 	_, err = w.Write(buf)
 	return err
 }
+


File is not gofmt-ed with -s (from gofmt)

Suggested change

golangcibot · 2020-01-31T14:11:54Z

worker/backup_ee.go

+
+func ProcessBackupRequest(ctx context.Context, req pb.BackupRequest, forceFull bool) error {
+	if !EnterpriseEnabled() {
+		return errors.Errorf("You must enable enterprise features first. "+


printf: Errorf call has arguments but no formatting directives (from govet)

golangcibot · 2020-01-31T14:20:10Z

worker/backup_ee.go

+
+func ProcessBackupRequest(ctx context.Context, req *pb.BackupRequest, forceFull bool) error {
+	if !EnterpriseEnabled() {
+		return errors.Errorf("You must enable enterprise features first. "+


printf: Errorf call has arguments but no formatting directives (from govet)

golangcibot · 2020-01-31T14:20:10Z

worker/backup_ee.go

+		}
+	}
+
+	glog.Infof("Created backup request: %s. Groups=%v\n", &req, groups)


printf: Infof format %s has arg &req of wrong type **github.com/dgraph-io/dgraph/protos/pb.BackupRequest (from govet)

martinmr

Reviewed 1 of 6 files at r1, 1 of 1 files at r2, 2 of 4 files at r3, 3 of 3 files at r5.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @manishrjain, @MichaelJCompton, and @pawanrawal)

ee/backup/tests/minio-large/backup_test.go, line 211 at r5 (raw file):

		dstDir := backupDir + "/" + object.Key
		err := os.MkdirAll(dstDir, os.ModePerm)
		require.NoError(t, err)

you can write this as require.NoError(t, os.MkdirAll(....))

graphql/admin/admin.go, line 285 at r5 (raw file):

Quoted 5 lines of code…

			return resolve.NewMutationResolver(
				backup,
				backup,
				backup,
				resolve.StdMutationCompletion(m.ResponseName()))

Maybe add a comment about what the arguments mean? It's unclear why backup is passed three times.

graphql/admin/backup.go, line 38 at r5 (raw file):

type backupInput struct {
	Destination  string

if the type is not exported the fields probably shouldn't either.

worker/backup_ee.go, line 80 at r5 (raw file):

func ProcessBackupRequest(ctx context.Context, req *pb.BackupRequest, forceFull bool) error {
	if !EnterpriseEnabled() {
		return errors.New("You must enable enterprise features first. " +

start error message with lowercase

worker/backup_ee.go, line 85 at r5 (raw file):

	if req.Destination == "" {
		return errors.Errorf("You must specify a 'destination' value")

start error messages with lowercase

pawanrawal

Reviewable status: 3 of 6 files reviewed, 9 unresolved discussions (waiting on @golangcibot, @manishrjain, @martinmr, and @MichaelJCompton)

ee/backup/backup.go, line 300 at r1 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

File is not gofmt-ed with -s (from gofmt)

Done.

ee/backup/tests/minio-large/backup_test.go, line 211 at r5 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

you can write this as require.NoError(t, os.MkdirAll(....))

Done.

graphql/admin/admin.go, line 285 at r5 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

			return resolve.NewMutationResolver(
				backup,
				backup,
				backup,
				resolve.StdMutationCompletion(m.ResponseName()))
Maybe add a comment about what the arguments mean? It's unclear why backup is passed three times.

Done.

graphql/admin/backup.go, line 38 at r5 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

if the type is not exported the fields probably shouldn't either.

I am using this type with json.Unmarshal and that won't work if fields are not exported.

worker/backup_ee.go, line 79 at r2 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

printf: Errorf call has arguments but no formatting directives (from govet)

Done.

worker/backup_ee.go, line 79 at r3 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

printf: Errorf call has arguments but no formatting directives (from govet)

Done.

worker/backup_ee.go, line 137 at r3 (raw file):

Previously, golangcibot (Bot from GolangCI) wrote…

printf: Infof format %s has arg &req of wrong type **github.com/dgraph-io/dgraph/protos/pb.BackupRequest (from govet)

Done.

worker/backup_ee.go, line 80 at r5 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

start error message with lowercase

done

worker/backup_ee.go, line 85 at r5 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

start error messages with lowercase

done

martinmr

Reviewed 3 of 3 files at r6.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @golangcibot, @manishrjain, @MichaelJCompton, and @pawanrawal)

ee/backup/tests/minio-large/backup_test.go, line 143 at r6 (raw file):

	resp, err := http.PostForm("http://localhost:8180/admin/backup", url.Values{
		"destination": []string{backupDestination},
	})

is this still working? will the http and graphql interface work side by side.

If so, there should be a basic test to verify the graphql requests work. It doesn't need to be done in this PR.

MichaelJCompton

Looks good.

I think we can merge this as is. BUT, we need to check if the intention is to remove /admin/backup completely. If that's the case, we'll have to have a separate PR that removes that and converts any testing to GraphQL.

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @golangcibot, @manishrjain, and @pawanrawal)

graphql/admin/admin.go, line 279 at r6 (raw file):

				})
		}).
		WithMutationResolver("backup", func(m schema.Mutation) resolve.MutationResolver {

can you initiate a backup while the server is starting?

If not, this should be moved to addConnectedAdminResolvers and just register an error resolver like the others here.

It's a little clunky ATM to have to do it twice. We'll smooth that out once we have these admin endpoints done and can see what a nice flow might be.

...um maybe ignore this. Looks like backup does this check anyway.

graphql/admin/backup.go, line 89 at r6 (raw file):

	x.Check2(buf.WriteString(br.mutation.SelectionSet()[0].ResponseName() + `": [{`))

	for i, sel := range br.mutation.SelectionSet()[0].SelectionSet() {

this doesn't quite work if there are aliases in the original query.

Ignore that though. I have a general soln for that on the health endpoint branch, so that aliases just work.

In fact, with what I've done, you can just hardcode

{"backup" : { "code": "Success", "message": "Backup completed" }}

and it'll work out the fields and alias for you. Leave this for now and we'll do a pass through once the health is merged.

pawanrawal

Yes, I'll check with Manish and add a card if that is the case.

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @golangcibot, @manishrjain, and @MichaelJCompton)

ee/backup/tests/minio-large/backup_test.go, line 143 at r6 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

	resp, err := http.PostForm("http://localhost:8180/admin/backup", url.Values{
		"destination": []string{backupDestination},
	})
is this still working? will the http and graphql interface work side by side.

If so, there should be a basic test to verify the graphql requests work. It doesn't need to be done in this PR.

It is working, both scenarios work currently. We'll most probably remove HTTP endpoint though, waiting to confirm with @manishrjain and then will modify the tests or add separate tests for GraphQL endpoints.

graphql/admin/admin.go, line 279 at r6 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

can you initiate a backup while the server is starting?

If not, this should be moved to addConnectedAdminResolvers and just register an error resolver like the others here.

It's a little clunky ATM to have to do it twice. We'll smooth that out once we have these admin endpoints done and can see what a nice flow might be.

...um maybe ignore this. Looks like backup does this check anyway.

Ignoring for now.

graphql/admin/backup.go, line 89 at r6 (raw file):

Previously, MichaelJCompton (Michael Compton) wrote…

this doesn't quite work if there are aliases in the original query.

Ignore that though. I have a general soln for that on the health endpoint branch, so that aliases just work.

In fact, with what I've done, you can just hardcode

{"backup" : { "code": "Success", "message": "Backup completed" }}

and it'll work out the fields and alias for you. Leave this for now and we'll do a pass through once the health is merged.

Ignoring for now, will fix after your PR is merged.

pawanrawal added 4 commits January 31, 2020 17:23

Rearrange backup code to prepare calling it from graphql admin API.

bee9bbf

Move ProcessBackupRequest to worker so that it can be called by both …

26309f6

…graphql/admin and ee package

It works and sends the correct response in case of success.

d34c020

Add a dummy ProcessBackupRequest for oss binary.

b2efa37

pawanrawal requested a review from martinmr January 31, 2020 14:02

pawanrawal requested review from manishrjain and a team as code owners January 31, 2020 14:02

pawanrawal requested a review from MichaelJCompton January 31, 2020 14:02

golangcibot reviewed Jan 31, 2020

View reviewed changes

Run gofmt on backup.go

6c4473f

golangcibot reviewed Jan 31, 2020

View reviewed changes

pawanrawal added 2 commits January 31, 2020 19:46

Pass a pointer around

085246f

Fix another golancibot issue.

9edc429

golangcibot reviewed Jan 31, 2020

View reviewed changes

Fix a log.

149e165

pawanrawal added the area/graphql Issues related to GraphQL support on Dgraph. label Jan 31, 2020

pawanrawal added 2 commits February 3, 2020 16:35

Fix race condition while sending backup request to different groups.

6a69fee

Fix copyright year

7ad48e4

martinmr suggested changes Feb 4, 2020

View reviewed changes

Address comments by Michael.

7598585

pawanrawal commented Feb 6, 2020

View reviewed changes

martinmr approved these changes Feb 6, 2020

View reviewed changes

MichaelJCompton approved these changes Feb 6, 2020

View reviewed changes

pawanrawal commented Feb 7, 2020

View reviewed changes

pawanrawal merged commit 2eca86c into master Feb 7, 2020

pawanrawal deleted the pawanrawal/make-backup-graphql-compatible branch February 7, 2020 07:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GraphQL Admin API: Support Backup operation #4706

GraphQL Admin API: Support Backup operation #4706

pawanrawal commented Jan 31, 2020 •

edited

Loading

golangcibot Jan 31, 2020

golangcibot Jan 31, 2020

golangcibot Jan 31, 2020

golangcibot Jan 31, 2020

martinmr left a comment

pawanrawal left a comment

martinmr left a comment

MichaelJCompton left a comment

pawanrawal left a comment

GraphQL Admin API: Support Backup operation #4706

GraphQL Admin API: Support Backup operation #4706

Conversation

pawanrawal commented Jan 31, 2020 • edited Loading

golangcibot Jan 31, 2020

Choose a reason for hiding this comment

golangcibot Jan 31, 2020

Choose a reason for hiding this comment

golangcibot Jan 31, 2020

Choose a reason for hiding this comment

golangcibot Jan 31, 2020

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

martinmr left a comment

Choose a reason for hiding this comment

MichaelJCompton left a comment

Choose a reason for hiding this comment

pawanrawal left a comment

Choose a reason for hiding this comment

pawanrawal commented Jan 31, 2020 •

edited

Loading