add MoveBucket to support moving a sub-bucket from one bucket to anot…

[Elbehery]

This PR is a replacement to #625

cc @ahrtr @fuweid

[Elbehery]

WIP, needs

• tackle review
• tackle review

[Elbehery]

@ahrtr to follow up on testing this and according to #625 (comment)

i can use

func randKeys() [][]byte {
	var keys [][]byte
	var count = rand.Intn(2) + 2
	for i := 0; i < count; i++ {
		keys = append(keys, randKey())
	}
	return keys
}

func randValue() []byte {
	n := rand.Intn(8192)
	b := make([]byte, n)
	for i := 0; i < n; i++ {
		b[i] = byte(rand.Intn(255))
	}
	return b
}

to generate random keys & values .. also can generate random bucket depth and large keys & values as requested

would this be ok ?

[Elbehery]

also @fuweid suggested #625 (comment)

this is another direction, please let me know

[ahrtr]

The test case is big enough, suggest to get all MoveBucket related test cases defined in a separate file e.g. movebucket_test.go

[Elbehery]

The test case is big enough, suggest to get all MoveBucket related test cases defined in a separate file e.g. movebucket_test.go

I separated the test into a separate file

i am stuck with dumping the bucket content into a file and comparing it, i will tag you on the code

[ahrtr]

The test case is big enough, suggest to get all MoveBucket related test cases defined in a separate file e.g. movebucket_test.go

I see that some tests are still in tx_test.go. Please get all moveBucket related test cases included in the new file.

[Elbehery]

@ahrtr @fuweid i finished the testing on Bucket layer

to do

• refactor testing on tx layer
• bucket content verification

[Elbehery]

the failing test is not related to this change

=== RUN   ExampleTx_Rollback
--- PASS: ExampleTx_Rollback (0.01s)
=== RUN   ExampleTx_CopyFile
--- PASS: ExampleTx_CopyFile (0.01s)
FAIL
exit status 1
FAIL	go.etcd.io/bbolt	1202.726s
make: *** [Makefile:45: test] Error 1
Error: Process completed with exit code 2.

[ahrtr]

--- FAIL: TestTx_MoveBucket (4.45s)
    --- PASS: TestTx_MoveBucket/happy_path (0.60s)
    --- PASS: TestTx_MoveBucket/bucketToMove_not_exist_in_srcBucket (0.40s)
    --- PASS: TestTx_MoveBucket/bucketToMove_exist_in_dstBucket (0.48s)
    --- PASS: TestTx_MoveBucket/bucketToMove_key_exist_in_srcBucket,_but_no_subBucket_value (0.31s)
    --- PASS: TestTx_MoveBucket/bucketToMove_key_exist_in_dstBucket,_but_no_subBucket_value (0.16s)
    --- PASS: TestTx_MoveBucket/srcBucket_is_rootBucket (0.38s)
    --- PASS: TestTx_MoveBucket/dstBucket_is_rootBucket (0.19s)
    --- PASS: TestTx_MoveBucket/srcBucket_is_rootBucket,_and_dstBucket_is_rootBucket (0.14s)

[Elbehery]

@ahrtr it is the cleanUp on Windows :/

    testing.go:1206: TempDir RemoveAll cleanup: remove C:\Users\RUNNER~1\AppData\Local\Temp\TestTx_MoveBucket2465631041\002\beforeBucket.db: Access is denied.

[Elbehery]

this should have been fixed in golang/go#50051

the fix is retrying to remove using this

/ removeAll is like os.RemoveAll, but retries Windows "Access is denied."
// errors up to an arbitrary timeout.
//
// Those errors have been known to occur spuriously on at least the
// windows-amd64-2012 builder (https://go.dev/issue/50051), and can only occur
// legitimately if the test leaves behind a temp file that either is still open
// or the test otherwise lacks permission to delete. In the case of legitimate
// failures, a failing test may take a bit longer to fail, but once the test is
// fixed the extra latency will go away.
func removeAll(path string) error {

two option, either I fall back to tempFile() from bbolt testing, or rerun the test and hope :D

[ahrtr]

Note that we have lots of usage of t.TempDir() in the test, we have never seen this issue on the master/main branch so far. Also the golang issue you mentioned had already been fixed in go 1.18. Can you figure out the root cause?

[ahrtr]

In the example in #635 (comment), the parameter paths should be [sb1, sb2] or [db1, db2]

Suggested change

	func createBucketIfNotExist(t testing.TB, tx *bbolt.Tx, incompatibleKey bool, paths ...string) *bbolt.Bucket {
	func openBuckets(t testing.TB, tx *bbolt.Tx, paths ...string) *bbolt.Bucket {

The high level algorithm is something like below,

• Create the bucket hirarchy structure
  • if incompatibleKeyInSrc is true, then create a normal key; otherwise, create a sub-bucket
  • the same thing for the incompatibleKeyInDst
• Move the bucket
  • open the bucket: reuse openBuckets
  • call tx.MoveBucket(key, srcBucket, dstBucket)
• Verify

[Elbehery]

done

[Elbehery]

Note that we have lots of usage of t.TempDir() in the test, we have never seen this issue on the master/main branch so far. Also the golang issue you mentioned had already been fixed in go 1.18. Can you figure out the root cause?

yes i see many times in the code it uses it exactly as I do

filepath.Join(t.TempDir(), "db")

i really dont know why it fails now, the issue is access denied, somehow the cleanUp routine can not access the file :/

[ahrtr]

Talked with @Elbehery in today's meeting, and I added two commits in this PR.

Please take a look, and let me know if you have any comment on this PR. @fuweid @Elbehery

Note I also manually verified the new functionality, and confirmed that it is working as expected.

[fuweid]

LGTM

[fuweid]

It seems like we should limit both src and dst are in that same db.

[ahrtr]

yes, it's a good point.

We can add protection something like below in (*Bucket)MoveBucket,

	if b.tx.db.Path() != dstBucket.tx.db.Path() || b.tx != dstBucket.tx {
		// return error
	}

We also need to add a test

@Elbehery can you take care of it in a following up PR?