feat(Upgrade): Users can now upgrade to v20.07.0

[abhimanyusinghgaur]

This PR adds the capability to upgrade to v20.07.0 from v20.03.0+
It has only considered the breaking changes introduced in #5185.

It also introduces a generic system to handle upgrades.

At present, upgrade tool expects to handle only breaking ACL changes, and does not expect to handle badger data format changes. The way it is supposed to work is by updating the Dgraph binary to new version, start Dgraph cluster with existing data directories (which could also have been restored by enterprise backup/restore), and then running the upgrade tool by specifying the version you want to upgrade from and the version you want to upgrade to.

Fixes #DGRAPH-1617.

---

This change is 

Docs Preview:

[abhimanyusinghgaur]

linking #5016 for context.

[pawanrawal]

Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

---

upgrade/change_list.go, line 45 at r4 (raw file):

				},
				{
					name: "Upgrade non-predefined types/predicates using reserved namespace",

Aren't we going to remove this and the related code?

---

upgrade/change_v20.03.0.go, line 31 at r1 (raw file):

	queryACLGroupsBefore_20_03_0 = `
		{
			rules(func: type(Group)) @filter(has(dgraph.group.acl)) {

did this query change?

[abhimanyusinghgaur]

Reviewable status: 0 of 8 files reviewed, 2 unresolved discussions (waiting on @manishrjain, @pawanrawal, and @vvbalaji-dgraph)

---

upgrade/change_list.go, line 45 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Aren't we going to remove this and the related code?

Done.

---

upgrade/change_v20.03.0.go, line 31 at r1 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

did this query change?

Yes, this query had got changed during the reserved namespace change. So, I put back the old name for Group type as in v20.03.0 and added a has filter just to be sure if that node belongs to ACL Group and not to some user-defined Group type.

[pawanrawal]

Reviewed 1 of 9 files at r2.
Reviewable status: 1 of 8 files reviewed, 6 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, and @vvbalaji-dgraph)

---

upgrade/change_list.go, line 22 at r5 (raw file):

	allChanges = changeList{
		{
			introducedIn: &version{major: 20, minor: 3, patch: 0},

Should be verifying the different cases here or mention them to @lgalatin

1.2.2 => 20.3.0
1.2.2 => 20.7.0
20.3.0 => 20.7.0

---

upgrade/change_v20.07.0.go, line 34 at r4 (raw file):

	queryACLUsersBefore_v20_07_0 = `
		{
			nodes(func: has(dgraph.xid)) @filter(type(User)) {

Why don't we apply the type(User) as root func before and the dgraph.xid check later? Similarly for other queries.

---

upgrade/change_v20.07.0.go, line 102 at r5 (raw file):

			dropOldTypeIfUnused: true,
			predsToRemoveFromOldType: map[string]struct{}{
				"dgraph.xid":        {},

If the old type itself is dropped, do we need to drop the preds from the type too?

---

upgrade/change_v20.07.0.go, line 111 at r5 (raw file):

			newTypeName:         "dgraph.type.Group",
			oldUidNodeQuery:     queryACLGroupsBefore_v20_07_0,
			dropOldTypeIfUnused: true,

This seems to be true for all the types so do we even need it?

---

upgrade/upgrade.go, line 316 at r5 (raw file):

	fmt.Println()
	fmt.Println("Upgrade finished!!!")

We don't need three !
Also this can just be fmt.Printf("\nUpgrade finished!")

---

upgrade/change_v20.03.0.go, line 84 at r4 (raw file):

					Subject:   newRuleStr,
					Predicate: "dgraph.type",
					ObjectId:  "Rule", // the name of the type was Rule in v20.03.0

Are we verifying that this continues to work, we should mention the scenarios it is supposed to work in to Leyla so that this can be tested properly before the release.

[martinmr]

Added some comments. It looks good for the most part but we should test this on a real dataset to make sure this works.

My one concert is that there are some reserved predicates that cannot be modified via mutations (dgraph.graphql.schema for example). Just make sure this tool is not affected by those limitations.

Reviewed 4 of 9 files at r2, 1 of 1 files at r4, 5 of 5 files at r5.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @abhimanyusinghgaur, @manishrjain, and @vvbalaji-dgraph)

---

upgrade/change_v20.07.0.go, line 177 at r5 (raw file):

Quoted 18 lines of code…

	// build NQuads for changing old type name to new name
	var setNQuads, delNQuads []*api.NQuad
	for _, node := range oldQueryRes.Nodes {
		setNQuads = append(setNQuads, &api.NQuad{
			Subject:   node.Uid,
			Predicate: "dgraph.type",
			ObjectValue: &api.Value{
				Val: &api.Value_StrVal{StrVal: t.newTypeName},
			},
		})
		delNQuads = append(delNQuads, &api.NQuad{
			Subject:   node.Uid,
			Predicate: "dgraph.type",
			ObjectValue: &api.Value{
				Val: &api.Value_StrVal{StrVal: t.oldTypeName},
			},
		})
	}

The parts where you build the nquads could go into its own function so you can write unit tests for them (no need to send the triples to Dgraph, just verifying that the nquads are what you expect).

Similar comment for other places where you generate nquads.

---

upgrade/utils.go, line 62 at r5 (raw file):

// getQueryResult executes the given query and unmarshals the result in given pointer queryResPtr.
// If any error is encountered, it returns the error.
func getQueryResult(dg *dgo.Dgraph, query string, queryResPtr interface{}) error {

This function should also retry just like mutate and alter below. For simplicity, create a new transaction every time there's a retry.

[abhimanyusinghgaur]

Reviewable status: 2 of 8 files reviewed, 8 unresolved discussions (waiting on @abhimanyusinghgaur, @lgalatin, @manishrjain, @martinmr, @pawanrawal, and @vvbalaji-dgraph)

---

upgrade/change_list.go, line 22 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Should be verifying the different cases here or mention them to @lgalatin

1.2.2 => 20.3.0
1.2.2 => 20.7.0
20.3.0 => 20.7.0

Added dry-run, also, will mention this to @lgalatin about how to test.

---

upgrade/change_v20.07.0.go, line 34 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Why don't we apply the type(User) as root func before and the dgraph.xid check later? Similarly for other queries.

Done.

---

upgrade/change_v20.07.0.go, line 102 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

If the old type itself is dropped, do we need to drop the preds from the type too?

no, preds are only removed from the type if the type can't be dropped.

---

upgrade/change_v20.07.0.go, line 111 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

This seems to be true for all the types so do we even need it?

updated

---

upgrade/change_v20.07.0.go, line 177 at r5 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

	// build NQuads for changing old type name to new name
	var setNQuads, delNQuads []*api.NQuad
	for _, node := range oldQueryRes.Nodes {
		setNQuads = append(setNQuads, &api.NQuad{
			Subject:   node.Uid,
			Predicate: "dgraph.type",
			ObjectValue: &api.Value{
				Val: &api.Value_StrVal{StrVal: t.newTypeName},
			},
		})
		delNQuads = append(delNQuads, &api.NQuad{
			Subject:   node.Uid,
			Predicate: "dgraph.type",
			ObjectValue: &api.Value{
				Val: &api.Value_StrVal{StrVal: t.oldTypeName},
			},
		})
	}

The parts where you build the nquads could go into its own function so you can write unit tests for them (no need to send the triples to Dgraph, just verifying that the nquads are what you expect).

Similar comment for other places where you generate nquads.

Done.

---

upgrade/upgrade.go, line 316 at r5 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

We don't need three !
Also this can just be fmt.Printf("\nUpgrade finished!")

Done.

---

upgrade/utils.go, line 62 at r5 (raw file):

Previously, martinmr (Martin Martinez Rivera) wrote…

This function should also retry just like mutate and alter below. For simplicity, create a new transaction every time there's a retry.

Done.

---

upgrade/change_v20.03.0.go, line 84 at r4 (raw file):

Previously, pawanrawal (Pawan Rawal) wrote…

Are we verifying that this continues to work, we should mention the scenarios it is supposed to work in to Leyla so that this can be tested properly before the release.

Will do

[pawanrawal]

Mostly looking good. Please add a --dryrun mode which can print the changelists that would be applied. That can be used to quickly eyeball the changes.

Reviewed 1 of 9 files at r2, 1 of 5 files at r5, 6 of 6 files at r6.
Reviewable status: 7 of 8 files reviewed, 8 unresolved discussions (waiting on @abhimanyusinghgaur, @lgalatin, @manishrjain, @martinmr, @pawanrawal, and @vvbalaji-dgraph)

[abhimanyusinghgaur]

Done

Reviewable status: 7 of 8 files reviewed, 8 unresolved discussions (waiting on @abhimanyusinghgaur, @lgalatin, @manishrjain, @martinmr, @pawanrawal, and @vvbalaji-dgraph)