Added CLI-flags to enable restoration for delta snapshots with large amount of data

[abdasgupta]

What this PR does / why we need it:
Added CLI-flags (max-call-send-message-size, max-request-bytes and max-txn-ops) to enable restoration for delta snapshots with large amount of data (large number of events or events with large data).

Which issue(s) this PR fixes:
Fixes #263

Special notes for your reviewer:

Release note:

Added CLI-flags (`max-call-send-message-size`, `max-request-bytes` and `max-txn-ops`) to enable restoration for delta snapshots with large amount of data (large number of events or events with large data).

[amshuman-kr]

LGTM

@abdasgupta I have updated the PR title, description and the releases notes. Please check if it makes sense and see anything needs to be changed. You can follow this kind template in the future PRs.

[amshuman-kr]

The integration test is failing. @shreyas-s-rao any idea why this is?

An error occurred (BucketAlreadyOwnedByYou) when calling the CreateBucket operation: Your previous request to create the named bucket succeeded and you already own it.

[shreyas-s-rao]

@abdasgupta Thanks for the well-written PR! Can you please check why the unit tests for restoration are failing?

@amshuman-kr The issue was with two pipeline jobs running in parallel and trying to access the same bucket. I have temporarily deleted one of the PR jobs for running tm tests, since the tm tests run on head update anyway. I'll refactor the pipeline definitions today and re-enable on-demand tm tests for PRs later.

[abdasgupta]

@shreyas-s-rao I have updated the test cases now

[Gerrit91]

Very useful. Thanks for this PR. 👍

Hope it's fine that I added a review comment.

[amshuman-kr]

Quote from https://etcd.io/docs/v3.4.0/learning/api/#transaction.

A transaction can atomically process multiple requests in a single request. For modifications to the key-value store, this means the store’s revision is incremented only once for the transaction and all events generated by the transaction will have the same revision.

But I doubt all the events in a single delta snapshot ever belong to a single transaction while the changes are originally made on etcd.

After that, there is this 👇 in the same documentation.

However, modifications to the same key multiple times within a single transaction are forbidden.

I am not sure we are checking this either which is again another problem.

The restore operation throws error with revision mismatch.

@abdasgupta I suppose you meant that a revision check after restoration fails with this error. But I don't know if we check after restoration if the restored revision matches the backup latest revision? From the above documentation, it looks like they may not match even based on the current implementation (before this PR)!
I double checked with v3.3.12 documentation to make sure the behaviour has not changed in v.3.4.0.

Anyway, if I understood right, we have the following options. Anything else will be technically incorrect.

1. Continue to use transactions during delta snapshot restoration restoration but keep them faithful (one-to-one) with original source transactions. Maybe there is a way to group events using their ModRevision. This can be complicated because some events of the same source transaction might possibly be split across delta snapshots. But with this approach we can retain the one-to-one mapping of original and restored revisions.
2. Give up on one-to-one mapping of original and restored revisions (i.e. no checks on revisions after restoration) but still continue to use transaction during delta snapshot restoration to potentially speed up delta snapshot restoration.
3. Give up on one-to-one mapping of original and restored revisions (i.e. no checks on revisions after restoration), give up on optimizing speed of delta snapshot restoration and just apply each event in its own transaction.

Or I have misunderstood completely, which is certainly possible 😉

cc @shreyas-s-rao

[shreyas-s-rao]

@amshuman-kr I don't particularly feel comfortable giving up on revision checks during restoration, because right now that's our only safety net against restoration inconsistencies, if at all the snapshot was not uploaded correctly, or something went wrong in the bucket or even during the download of the object.

However, modifications to the same key multiple times within a single transaction are forbidden

I too remember seeing this in the documentation, but there never seemed to an issue regarding this when we applied the transactions during restoration. Surprising indeed.

Since delta snapshots record events (which are either PUT or DELETE), I think we should pursue restoring delta snapshots in the same way, serially applying PUT and DELETE events instead of batching them into transactions. @abdasgupta @amshuman-kr WDYT?

[abdasgupta]

I have tried to do only PUT and DELETE operations, instead of using transactions. First problem is, each operation would commit as I understood. So, for large Delta events, it should take larger time. Second problem, that also gave error of revision mismatch.

The changes I Made:

func applyEventsToEtcd(client *clientv3.Client, events []event) error {
	var (
		lastRev int64
		ops     = []clientv3.Op{}
		ctx     = context.TODO()
	)

	for _, e := range events {
		ev := e.EtcdEvent
		nextRev := ev.Kv.ModRevision
		if lastRev != 0 && nextRev > lastRev {
			if _, err := client.Txn(ctx).Then(ops...).Commit(); err != nil {
				return err
			}
			ops = []clientv3.Op{}
		}
		lastRev = nextRev
		switch ev.Type {
		case mvccpb.PUT:
			//ops = append(ops, clientv3.OpPut(string(ev.Kv.Key), string(ev.Kv.Value))) //, clientv3.WithLease(clientv3.LeaseID(ev.Kv.Lease))))
			_, err := client.Put(ctx, string(ev.Kv.Key), string(ev.Kv.Value))
			if err != nil {
				return err
			}

		case mvccpb.DELETE:
			//ops = append(ops, clientv3.OpDelete(string(ev.Kv.Key)))
			_, err := client.Delete(ctx, string(ev.Kv.Key))
			if err != nil {
				return err
			}
		default:
			return fmt.Errorf("Unexpected event type")
		}
	}
	//_, err := client.Txn(ctx).Then(ops...).Commit()
	return nil
}

The error:

INFO[0148] Cleanup complete                              actor=restorer
ERRO[0148] Restoration failed.                           actor=restorer
{"level":"info","ts":"2020-11-09T22:49:56.595+0530","caller":"etcdserver/server.go:1464","msg":"skipped leadership transfer for single voting member cluster","local-member-id":"8e9e05c52164694d","current-leader-member-id":"8e9e05c52164694d"}
{"level":"info","ts":"2020-11-09T22:49:56.614+0530","caller":"embed/etcd.go:360","msg":"closing etcd server","name":"default","data-dir":"default.etcd","advertise-peer-urls":["http://localhost:0"],"advertise-client-urls":["http://localhost:0"]}
{"level":"info","ts":"2020-11-09T22:49:56.614+0530","caller":"etcdserver/server.go:1464","msg":"skipped leadership transfer for single voting member cluster","local-member-id":"8e9e05c52164694d","current-leader-member-id":"8e9e05c52164694d"}
{"level":"info","ts":"2020-11-09T22:49:56.615+0530","caller":"embed/etcd.go:555","msg":"stopping serving peer traffic","address":"127.0.0.1:51885"}
{"level":"info","ts":"2020-11-09T22:49:56.615+0530","caller":"embed/etcd.go:562","msg":"stopped serving peer traffic","address":"127.0.0.1:51885"}
{"level":"info","ts":"2020-11-09T22:49:56.615+0530","caller":"embed/etcd.go:364","msg":"closed etcd server","name":"default","data-dir":"default.etcd","advertise-peer-urls":["http://localhost:0"],"advertise-client-urls":["http://localhost:0"]}
FATA[0148] Failed to restore snapshot: snapshot revision verification failed for delta snapshot Incr-00008597-00009002-1604909996 : mismatched event revision while applying delta snapshot, expected 9002 but applied 18001

Hope this is what you were suggesting? @shreyas-s-rao

As a reason for this error, I think that the delta snapshot operations are queued to the ETCD server in a order which is not matching with the order the changes were made actually .

[shreyas-s-rao]

@abdasgupta Yes, this is what I was thinking. If I'm right, most of the delta snapshots events are getting applied correctly, but that applying that last event is throwing the error right?

[amshuman-kr]

I don't particularly feel comfortable giving up on revision checks during restoration, because right now that's our only safety net against restoration inconsistencies, if at all the snapshot was not uploaded correctly, or something went wrong in the bucket or even during the download of the object.

I agree.

Since delta snapshots record events (which are either PUT or DELETE), I think we should pursue restoring delta snapshots in the same way, serially applying PUT and DELETE events instead of batching them into transactions.

I am afraid this is not an option. As @abdasgupta showed above, if the original events were done using transaction then not using transaction while restoring will still lead to mismatch in revisions after restoration.

If we do not want to give up on revision check after restoration (and I agree that we should not), then the correct thing is to group the events into transactions only if they share the same ModRevision. It seems to me that generally using transactions during restoration for optimization like we have done so far is incorrect. Generally not using transactions during restoration seems also equally incorrect.

Furthermore, the implementation idea for compaction of compacting the delta snapshots asynchronously by periodically restoring and replacing past delta snapshots with full snapshots would work only if revisions are consistent across restorations. So, we don't really have a choice here. As far as I can see, we have to honour original transaction during restoration. The only tricky part for me is how to ensure events from the same original transaction do not get split into multiple delta snapshot files. If that can be ensured, then the rest is manageable, I think.

[amshuman-kr]

The only tricky part for me is how to ensure events from the same original transaction do not get split into multiple delta snapshot files.

I think this can be achieved if etcd watch returns all the events from the same transaction in one watch response. I am reasonably confident that it will. We need to check if this is guaranteed to be true.

[amshuman-kr]

TL;DR 😉

Transactions are an ETCD functionality designed for use by clients of ETCD however they see fit. It clearly has semantics associated with it and not just an optimization hack. We should not use it for any other purpose. Equally, we should not avoid using it for any other purpose either.

[amshuman-kr]

BTW I am ok merging this PR as is with the additional CLI flags without the chunking changes during restoration because the issue with transactions is clearly a pre-existing condition. We can create a separate issue to track it (and address it in a separate PR).

[abdasgupta]

There is a conflict appearing in this PR due to #275

[shreyas-s-rao]

@abdasgupta Yes, can you please rebase the PR?

[abdasgupta]

@amshuman-kr @shreyas-s-rao Took the liberty of passing RestoreOption as parameter to startEmbeddedEtcd for both the validator and restorer. Please review if I missed something in validator. make test is passing on my system and I checked the restoration operation is also passing.

[shreyas-s-rao]

@abdasgupta Thanks for the PR. LGTM.

[amshuman-kr]

LGTM