From a38c219cf38180dbef9a61002c4f00bea7ce5e82 Mon Sep 17 00:00:00 2001 From: Seebs Date: Thu, 11 Nov 2021 13:54:57 -0600 Subject: [PATCH 1/5] Add test for RBF failures This test case triggers a failure in RBF, it's a separate patch to make it easier to see the failure. --- rbf/tx_test.go | 90 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/rbf/tx_test.go b/rbf/tx_test.go index f7cd78d6a..05cd76df0 100644 --- a/rbf/tx_test.go +++ b/rbf/tx_test.go @@ -219,6 +219,96 @@ func TestTx_DeleteBitmap(t *testing.T) { } } +// deallocateTree had a bug which caused a page to be marked neither free +// nor in-use. +func TestTx_DeallocateTree(t *testing.T) { + db := MustOpenDB(t) + defer MustCloseDB(t, db) + tx := MustBegin(t, db, true) + defer tx.Rollback() + var err error + + // Create bitmap & add value. + if err = tx.CreateBitmap("x"); err != nil { + t.Fatal(err) + } + const N = 315 + slots := make([]uint64, N) + for i := range slots { + slots[i] = uint64(i) << 20 + } + if _, err = tx.Add("x", slots...); err != nil { + t.Fatal(err) + } + if err = tx.Check(); err != nil { + t.Fatalf("check: %v", err) + } + if err = tx.DeleteBitmap("x"); err != nil { + t.Fatal(err) + } else { + if ok, err := tx.Contains("x", 0); err != nil { + t.Fatal(err) + } else if ok { + t.Fatal("expected no value in recreated bitmap") + } + } + if err = tx.Check(); err != nil { + t.Fatalf("check: %v", err) + } +} + +func TestTx_RecreateBitmap(t *testing.T) { + db := MustOpenDB(t) + defer MustCloseDB(t, db) + tx := MustBegin(t, db, true) + defer tx.Rollback() + + // Create bitmap & add value. + if err := tx.CreateBitmap("x"); err != nil { + t.Fatal(err) + } + const N = 825000 + slots := make([]uint64, N) + for i := range slots { + slots[i] = uint64(i) << 20 + } + if _, err := tx.Add("x", slots...); err != nil { + t.Fatal(err) + } + err := tx.Commit() + if err != nil { + t.Fatal(err) + } + tx = MustBegin(t, db, true) + defer tx.Rollback() + // Delete bitmap, verifying that it's gone. + if err := tx.DeleteBitmap("x"); err != nil { + t.Fatal(err) + } else { + if ok, err := tx.Contains("x", 0); err != nil { + t.Fatal(err) + } else if ok { + t.Fatal("expected no value in recreated bitmap") + } + } + err = tx.Commit() + if err != nil { + t.Fatal(err) + } + tx = MustBegin(t, db, true) + defer tx.Rollback() + if err := tx.CreateBitmap("x"); err != nil { + t.Fatal(err) + } + if _, err := tx.Add("x", slots...); err != nil { + t.Fatal(err) + } + err = tx.Commit() + if err != nil { + t.Fatal(err) + } +} + func TestTx_RenameBitmap(t *testing.T) { db := MustOpenDB(t) defer MustCloseDB(t, db) From 9c0c0ec8c135f44d9c0593aa449a34db9a9e51fa Mon Sep 17 00:00:00 2001 From: Seebs Date: Wed, 10 Nov 2021 17:27:12 -0600 Subject: [PATCH 2/5] There are two related bugs here. First, it is possible for us to end up allocating *or freeing* pages during a modification of the free list, in a way such that the change to the free list means that when we finish the modification which caused the allocate or free, we've overwritten the inner change. Second, when deallocating trees, we don't actually deallocate the branch nodes themselves. The former causes potentially severe data corruption. The latter causes us to gradually leak pages in a way that we don't notice because we only run those tests during the RBF tests. The fix for this is surprisingly intricate, because of the counterintuitive fact that *allocating* a page means *removing* things from the free list (and thus potentially deallocating free list pages), while *freeing* a page means *adding* things to the free list (and thus potentially needing to allocate pages for the free list). While modifying the free list, any allocations we need always just come from the end of the file; we don't try to reuse free pages. If a page becomes *deallocated* by a free list modification, we don't annotate it in the free list at the instant that it happens; we stash that information until the current modification of the free list happens, then iterate through any such pages. I am pretty sure there's virtually never more than one, and I don't actually know that I can create a case wherein we'd end up with the nested case firing, wherein removing a page from the free list causes us to remove another page, but I think if the free list got large and cluttered and needed rebalancing or something it could maybe happen. --- rbf/tx.go | 93 +++++++++++++++++++++++++++++++++++++++++++++++--- rbf/tx_test.go | 89 +++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 171 insertions(+), 11 deletions(-) diff --git a/rbf/tx.go b/rbf/tx.go index 9ecbe9a02..812d35738 100644 --- a/rbf/tx.go +++ b/rbf/tx.go @@ -56,6 +56,27 @@ type Tx struct { // behavior where an existing container has all its bits cleared // but still sticks around in the database. DeleteEmptyContainer bool + + // It is possible for a modification of the free list to cause a page to + // be allocated or deallocated, which would modify the free list. + // + // For the case where pages need to be allocated during free list + // changes, we can trivially just allocate new pages and not use the + // free list. That's the simple case... + modifyingFreelist bool + // But removals can't be deferred/not-done like that. If a free list + // change causes us to deallocate a page (such as if we're *allocating* + // a page, which causes it to be *removed* from the free list), we really + // do need to record that, but if we try to do it during the update + // process, things could go horribly wrong. So, we have a transient list + // of page numbers which have been deallocated, but it happened *during* + // the modification of the free list. The top-level modification then + // processes them on its way out, using a defer. During the processing + // of this list, we *still* have the flag set, and we make a new list + // while processing the list, so if somehow a pending add to the list + // manages to trigger a *deallocation* (which I don't think should be + // happening), we'll process that one after the current list is processed. + pendingFreelistAdds []uint32 } func (tx *Tx) DBPath() string { @@ -903,16 +924,67 @@ func (tx *Tx) walkTree(pgno, parent uint32, fn func(pgno, parent, typ uint32) er } } +// freelistCleanup handles things which we need to add to the free list, +// because they became free during the process of modifying the free list. +// It also marks us as done modifying the free list. Expected usage is that +// you set modifyingFreelist to true, then defer this. +// +// if you are modifying the free list, we can't further change the free +// list during that modification. for page allocations, we can just skip +// the free list check. for deallocations, though, we do need to mark them +// as freed at some point. so, if we're modifying the free list when +// a new freePgno happens, we stash the new pages in here, then apply +// them afterwards. so far as i know, this can actually only happen +// during an allocate, when we're removing entries from the free list, and +// the add path doesn't ever trigger it. so, when we remove entries from +// the free list, it's possible that doing so frees up pages that were +// part of the free list, and we then add them. but we don't have to worry +// about that removing things from the free list, because the add logic +// already just uses new pages rather than trying to use the free list +// when it knows the free list is involved. +func (tx *Tx) freelistCleanup(outErr *error) { + defer func() { + // no matter what, we're done with this after this, but we still + // want it set *while* we do this so nothing we do will have side + // effects that collide with what we're doing. + tx.modifyingFreelist = false + }() + if len(tx.pendingFreelistAdds) == 0 { + return + } + c := Cursor{tx: tx} + c.stack.elems[0] = stackElem{pgno: readMetaFreelistPageNo(tx.meta[:])} + for len(tx.pendingFreelistAdds) > 0 { + var pass []uint32 + pass, tx.pendingFreelistAdds = tx.pendingFreelistAdds, nil + for _, pgno := range pass { + if changed, err := c.Add(uint64(pgno)); err != nil { + if outErr != nil && *outErr == nil { + *outErr = err + } + return + } else if !changed { + PanicOn(fmt.Sprintf("rbf.Tx.freePgno(): double free: %d", tx.pendingFreelistAdds)) + } + } + } +} + // allocatePgno returns a page number for a new available page. This page may be // pulled from the free list or, if no free pages are available, it will be // created by extending the file size. -func (tx *Tx) allocatePgno() (uint32, error) { +func (tx *Tx) allocatePgno() (_ uint32, outErr error) { + if tx.modifyingFreelist { + return tx.allocateNewPgno(), nil + } // Attempt to find page in freelist. pgno, err := tx.nextFreelistPageNo() if err != nil { return 0, err } else if pgno != 0 { + tx.modifyingFreelist = true + defer tx.freelistCleanup(&outErr) c := Cursor{tx: tx} c.stack.elems[0] = stackElem{pgno: readMetaFreelistPageNo(tx.meta[:])} if changed, err := c.Remove(uint64(pgno)); err != nil { @@ -922,11 +994,16 @@ func (tx *Tx) allocatePgno() (uint32, error) { } return pgno, nil } + // no freelist pages, fall back + return tx.allocateNewPgno(), nil +} +// allocateNewPgno requests a new page unconditionally, ignoring the free list. +func (tx *Tx) allocateNewPgno() uint32 { // Increment the total page count by one and return the last page. - pgno = readMetaPageN(tx.meta[:]) + pgno := readMetaPageN(tx.meta[:]) writeMetaPageN(tx.meta[:], pgno+1) - return pgno, nil + return pgno } func (tx *Tx) nextFreelistPageNo() (uint32, error) { @@ -952,10 +1029,16 @@ func (tx *Tx) nextFreelistPageNo() (uint32, error) { } // deallocate releases a page number to the freelist. -func (tx *Tx) freePgno(pgno uint32) error { +func (tx *Tx) freePgno(pgno uint32) (outErr error) { + if tx.modifyingFreelist { + tx.pendingFreelistAdds = append(tx.pendingFreelistAdds, pgno) + return nil + } c := Cursor{tx: tx} c.stack.elems[0] = stackElem{pgno: readMetaFreelistPageNo(tx.meta[:])} + tx.modifyingFreelist = true + defer tx.freelistCleanup(&outErr) if changed, err := c.Add(uint64(pgno)); err != nil { return err } else if !changed { @@ -979,7 +1062,7 @@ func (tx *Tx) deallocateTree(pgno uint32) error { return err } } - return nil + return tx.freePgno(pgno) case PageTypeLeaf: return tx.freePgno(pgno) diff --git a/rbf/tx_test.go b/rbf/tx_test.go index 05cd76df0..e4b68f4b6 100644 --- a/rbf/tx_test.go +++ b/rbf/tx_test.go @@ -22,6 +22,7 @@ import ( "time" "github.com/molecula/featurebase/v2/rbf" + "github.com/molecula/featurebase/v2/roaring" ) func TestTx_CommitRollback(t *testing.T) { @@ -245,12 +246,11 @@ func TestTx_DeallocateTree(t *testing.T) { } if err = tx.DeleteBitmap("x"); err != nil { t.Fatal(err) - } else { - if ok, err := tx.Contains("x", 0); err != nil { - t.Fatal(err) - } else if ok { - t.Fatal("expected no value in recreated bitmap") - } + } + if ok, err := tx.Contains("x", 0); err != nil { + t.Fatal(err) + } else if ok { + t.Fatal("expected no value in recreated bitmap") } if err = tx.Check(); err != nil { t.Fatalf("check: %v", err) @@ -369,6 +369,83 @@ func TestTx_Add_Quick(t *testing.T) { }) } +func TestTx_DeallocateToFreeList(t *testing.T) { + db := MustOpenDB(t) + defer MustCloseDB(t, db) + tx := MustBegin(t, db, true) + defer tx.Rollback() + var err error + + // Create bitmap & add value. + if err = tx.CreateBitmap("x"); err != nil { + t.Fatal(err) + } + if err = tx.CreateBitmap("y"); err != nil { + t.Fatal(err) + } + const N = 12274831 + slots := make([]uint64, N) + for i := range slots { + slots[i] = uint64(i) << 10 + } + bm := roaring.NewBitmap(slots...) + if _, err = tx.AddRoaring("x", bm); err != nil { + t.Fatal(err) + } + if err = tx.Check(); err != nil { + t.Fatal(err) + } + for i := 0; i < 500; i++ { + if _, err := tx.Add("y", uint64(i)<<16); err != nil { + t.Fatal(err) + } + } + if err = tx.Check(); err != nil { + t.Fatal(err) + } + if err := tx.DeleteBitmap("y"); err != nil { + t.Fatal(err) + } + if err = tx.Check(); err != nil { + t.Fatal(err) + } + if err = tx.Commit(); err != nil { + t.Fatal(err) + } + tx = MustBegin(t, db, true) + defer tx.Rollback() + // Delete bitmap, verifying that it's gone. + if err := tx.DeleteBitmap("x"); err != nil { + t.Fatal(err) + } else { + if ok, err := tx.Contains("x", 0); err != nil { + t.Fatal(err) + } else if ok { + t.Fatal("expected no value in recreated bitmap") + } + } + if err = tx.Check(); err != nil { + t.Fatal(err) + } + if err = tx.Commit(); err != nil { + t.Fatal(err) + } + tx = MustBegin(t, db, true) + defer tx.Rollback() + if err := tx.CreateBitmap("x"); err != nil { + t.Fatal(err) + } + if _, err := tx.AddRoaring("x", bm); err != nil { + t.Fatal(err) + } + if err = tx.Check(); err != nil { + t.Fatal(err) + } + if err = tx.Commit(); err != nil { + t.Fatal(err) + } +} + func TestTx_AddRemove_Quick(t *testing.T) { if testing.Short() { t.Skip("-short enabled, skipping") From eb717f568dcee4f7801655d9ace3029158b4df28 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Thu, 18 Nov 2021 11:49:38 -0600 Subject: [PATCH 3/5] fix for ec2 ip filtering --- .gitlab/.gitlab-ci.yml | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/.gitlab/.gitlab-ci.yml b/.gitlab/.gitlab-ci.yml index ccd710889..385e66eab 100644 --- a/.gitlab/.gitlab-ci.yml +++ b/.gitlab/.gitlab-ci.yml @@ -221,16 +221,17 @@ deploy node for linux amd64: - echo "$AWS_SSH_PRIVATE_KEY" | ssh-add - - chmod 700 /root/.ssh - '[[ -f /.dockerenv ]] && echo -e "Host *\n\tStrictHostKeyChecking no\n\n" > ~/.ssh/config' + - apt install jq script: - AMI=$(aws ssm get-parameters --names /aws/service/ami-amazon-linux-latest/amzn2-ami-hvm-x86_64-ebs --query 'Parameters[0].[Value]' --output text --profile $PROFILE) - SECURITY_GROUP=$(aws ec2 describe-security-groups --filters Name=vpc-id,Values=vpc-03a4ba3d5b7c8f978 Name=group-name,Values=default --query 'SecurityGroups[*].[GroupId]' --output text --profile $PROFILE) - SUBNET_ID=$(aws ec2 describe-subnets --filters 'Name=vpc-id,Values=vpc-03a4ba3d5b7c8f978' 'Name=availability-zone,Values=us-east-2a' --query 'Subnets[0].SubnetId' --output text --profile $PROFILE) - - aws ec2 run-instances --image-id $AMI --instance-type $INSTANCE --security-group-ids $SECURITY_GROUP --subnet-id $SUBNET_ID --key-name gitlab-featurebase-dev --tag-specifications 'ResourceType=instance,Tags=[{Key=Name,Value=linux-amd64-node}]' --profile $PROFILE --user-data file://.gitlab/cloud-init.sh --iam-instance-profile Name=featurebase-dev-ssm - - sleep 120 # Need to wait for the EC2 instance to launch and run the initialization commands passed through user-data - - PUBLIC_IP=$(aws ec2 describe-instances --filters 'Name=tag:Name, Values=linux-amd64-node' 'Name=instance-state-name, Values=running' --query 'Reservations[*].Instances[*].PublicIpAddress' --output text --profile $PROFILE) - - echo $PUBLIC_IP - - INSTANCE_ID=$(aws ec2 describe-instances --filters 'Name=tag:Name, Values=linux-amd64-node' 'Name=instance-state-name, Values=running' --query 'Reservations[*].Instances[*].InstanceId' --output text --profile $PROFILE) + - aws ec2 run-instances --image-id $AMI --instance-type $INSTANCE --security-group-ids $SECURITY_GROUP --subnet-id $SUBNET_ID --key-name gitlab-featurebase-dev --tag-specifications 'ResourceType=instance,Tags=[{Key=Name,Value=linux-amd64-node}]' --profile $PROFILE --user-data file://.gitlab/cloud-init.sh --iam-instance-profile Name=featurebase-dev-ssm > config.json + - INSTANCE_ID=$(jq '.Instances | .[] |.InstanceId' config.json) - echo $INSTANCE_ID > linux-amd64-instance.txt + - sleep 120 # Need to wait for the EC2 instance to launch and run the initialization commands passed through user-data + - PUBLIC_IP=$(aws ec2 describe-instances --instance-ids $INSTANCE_ID --filters 'Name=instance-state-name, Values=running' --query 'Reservations[*].Instances[*].PublicIpAddress' --output text --profile $PROFILE) + - echo $PUBLIC_IP - scp -o StrictHostKeyChecking=no -i gitlab-featurebase-dev.pem featurebase_linux_amd64 ec2-user@$PUBLIC_IP:. - scp -o StrictHostKeyChecking=no -i gitlab-featurebase-dev.pem .gitlab/featurebase.conf .gitlab/featurebase.service ec2-user@$PUBLIC_IP:. - aws ssm send-command --document-name "AWS-RunShellScript" --instance-ids $INSTANCE_ID --cli-input-json file://.gitlab/configureFeatureBase.json --profile $PROFILE --region us-east-2 From 5d489198bde46044cb265719dcdd3deb15426e4a Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Thu, 18 Nov 2021 12:11:48 -0600 Subject: [PATCH 4/5] fixed apt install bug --- .gitlab/.gitlab-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitlab/.gitlab-ci.yml b/.gitlab/.gitlab-ci.yml index 385e66eab..5ab7d3216 100644 --- a/.gitlab/.gitlab-ci.yml +++ b/.gitlab/.gitlab-ci.yml @@ -221,7 +221,7 @@ deploy node for linux amd64: - echo "$AWS_SSH_PRIVATE_KEY" | ssh-add - - chmod 700 /root/.ssh - '[[ -f /.dockerenv ]] && echo -e "Host *\n\tStrictHostKeyChecking no\n\n" > ~/.ssh/config' - - apt install jq + - apt update && apt -y install jq script: - AMI=$(aws ssm get-parameters --names /aws/service/ami-amazon-linux-latest/amzn2-ami-hvm-x86_64-ebs --query 'Parameters[0].[Value]' --output text --profile $PROFILE) - SECURITY_GROUP=$(aws ec2 describe-security-groups --filters Name=vpc-id,Values=vpc-03a4ba3d5b7c8f978 Name=group-name,Values=default --query 'SecurityGroups[*].[GroupId]' --output text --profile $PROFILE) From 4e3f31471cee66357b3c2d141952b3c2ecdd7484 Mon Sep 17 00:00:00 2001 From: Souhaila Noor Date: Thu, 18 Nov 2021 12:42:09 -0600 Subject: [PATCH 5/5] remove double quotes from variable name --- .gitlab/.gitlab-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitlab/.gitlab-ci.yml b/.gitlab/.gitlab-ci.yml index 5ab7d3216..0dda19409 100644 --- a/.gitlab/.gitlab-ci.yml +++ b/.gitlab/.gitlab-ci.yml @@ -227,7 +227,7 @@ deploy node for linux amd64: - SECURITY_GROUP=$(aws ec2 describe-security-groups --filters Name=vpc-id,Values=vpc-03a4ba3d5b7c8f978 Name=group-name,Values=default --query 'SecurityGroups[*].[GroupId]' --output text --profile $PROFILE) - SUBNET_ID=$(aws ec2 describe-subnets --filters 'Name=vpc-id,Values=vpc-03a4ba3d5b7c8f978' 'Name=availability-zone,Values=us-east-2a' --query 'Subnets[0].SubnetId' --output text --profile $PROFILE) - aws ec2 run-instances --image-id $AMI --instance-type $INSTANCE --security-group-ids $SECURITY_GROUP --subnet-id $SUBNET_ID --key-name gitlab-featurebase-dev --tag-specifications 'ResourceType=instance,Tags=[{Key=Name,Value=linux-amd64-node}]' --profile $PROFILE --user-data file://.gitlab/cloud-init.sh --iam-instance-profile Name=featurebase-dev-ssm > config.json - - INSTANCE_ID=$(jq '.Instances | .[] |.InstanceId' config.json) + - INSTANCE_ID=$(jq '.Instances | .[] |.InstanceId' config.json | tr -d '"') - echo $INSTANCE_ID > linux-amd64-instance.txt - sleep 120 # Need to wait for the EC2 instance to launch and run the initialization commands passed through user-data - PUBLIC_IP=$(aws ec2 describe-instances --instance-ids $INSTANCE_ID --filters 'Name=instance-state-name, Values=running' --query 'Reservations[*].Instances[*].PublicIpAddress' --output text --profile $PROFILE)