[ARVADOS] created: c447d4a79f653bbf2c172a0a715d30db896a4a32
git at public.curoverse.com
git at public.curoverse.com
Thu Sep 17 15:13:34 EDT 2015
at c447d4a79f653bbf2c172a0a715d30db896a4a32 (commit)
commit c447d4a79f653bbf2c172a0a715d30db896a4a32
Author: radhika <radhika at curoverse.com>
Date: Thu Sep 17 15:03:59 2015 -0400
7329: It appears that UnixVolume -> Compare method is falling in infinite loop due to the fact that EOF is not returned when reading an empty file.
Due to this, PutBlock for an EmptyHash is resulting in infinite loop when an EmptyBlock already exists in Keep.
Added a quick fix in the Compare method for unix volume implementation. We can try to see why EOF is not returned and put in a better fix,
but for the time being this helps with unblocking tests.
Added several tests for empty block. Most importantly, added an keep integration test with UnixVolume so that we have integration tests covering some
of these code paths. More tests are definitely called for in this intergration test for better code coverage.
Also did gofmt and golint updates.
diff --git a/services/keepstore/collision.go b/services/keepstore/collision.go
index 210286a..be26514 100644
--- a/services/keepstore/collision.go
+++ b/services/keepstore/collision.go
@@ -35,7 +35,7 @@ func collisionOrCorrupt(expectMD5 string, buf1, buf2 []byte, rdr io.Reader) erro
}
var err error
for rdr != nil && err == nil {
- buf := make([]byte, 1 << 18)
+ buf := make([]byte, 1<<18)
var n int
n, err = rdr.Read(buf)
data <- buf[:n]
diff --git a/services/keepstore/keepstore.go b/services/keepstore/keepstore.go
index ec11af5..3e360e1 100644
--- a/services/keepstore/keepstore.go
+++ b/services/keepstore/keepstore.go
@@ -37,8 +37,8 @@ const BlockSize = 64 * 1024 * 1024
const MinFreeKilobytes = BlockSize / 1024
// Until #6221 is resolved, never_delete must be true.
-// However, allow it to be false in testing with TEST_DATA_MANAGER_TOKEN
-const TEST_DATA_MANAGER_TOKEN = "4axaw8zxe0qm22wa6urpp5nskcne8z88cvbupv653y1njyi05h"
+// However, allow it to be false in testing with TestDataManagerToken
+const TestDataManagerToken = "4axaw8zxe0qm22wa6urpp5nskcne8z88cvbupv653y1njyi05h"
// ProcMounts /proc/mounts
var ProcMounts = "/proc/mounts"
@@ -348,7 +348,7 @@ func main() {
}
}
- if neverDelete != true && dataManagerToken != TEST_DATA_MANAGER_TOKEN {
+ if neverDelete != true && dataManagerToken != TestDataManagerToken {
log.Fatal("never_delete must be true, see #6221")
}
diff --git a/services/keepstore/keepstore_integration_unix_volume_test.go b/services/keepstore/keepstore_integration_unix_volume_test.go
new file mode 100644
index 0000000..affbe9d
--- /dev/null
+++ b/services/keepstore/keepstore_integration_unix_volume_test.go
@@ -0,0 +1,113 @@
+package main
+
+import (
+ "bytes"
+ "os"
+ "testing"
+
+ "git.curoverse.com/arvados.git/sdk/go/arvadostest"
+)
+
+func SetupKeepStoreUnixVolumeIntegrationTest(t *testing.T) {
+ os.Setenv("ARVADOS_API_HOST_INSECURE", "true")
+
+ // Set up Keep unix volumes
+ KeepVM = MakeTestUnixVolumeManager(t, 2)
+ defer KeepVM.Close()
+
+ // Start api and keep servers
+ arvadostest.StartAPI()
+ arvadostest.StartKeep()
+}
+
+// MakeTestUnixVolumeManager returns a RRVolumeManager
+// with the specified number of UnixVolumes.
+var testableUnixVols []*TestableUnixVolume
+
+func MakeTestUnixVolumeManager(t *testing.T, numVolumes int) VolumeManager {
+ vols := make([]Volume, numVolumes)
+ testableUnixVols = make([]*TestableUnixVolume, numVolumes)
+
+ for i := range vols {
+ v := NewTestableUnixVolume(t, false, false)
+ vols[i] = v
+ testableUnixVols[i] = v
+ }
+ return MakeRRVolumeManager(vols)
+}
+
+// Put TestBlock and Get it
+func TestPutTestBlock(t *testing.T) {
+ SetupKeepStoreUnixVolumeIntegrationTest(t)
+
+ // Check that PutBlock succeeds
+ if err := PutBlock(TestBlock, TestHash); err != nil {
+ t.Fatalf("Error during PutBlock: %s", err)
+ }
+
+ // Check that PutBlock succeeds again even after CompareAndTouch
+ if err := PutBlock(TestBlock, TestHash); err != nil {
+ t.Fatalf("Error during PutBlock: %s", err)
+ }
+
+ // Check that PutBlock stored the data as expected
+ buf, err := GetBlock(TestHash)
+ if err != nil {
+ t.Fatalf("Error during GetBlock for %q: %s", TestHash, err)
+ } else if bytes.Compare(buf, TestBlock) != 0 {
+ t.Errorf("Get response incorrect. Expected %q; found %q", TestBlock, buf)
+ }
+}
+
+// UnixVolume -> Compare is falling in infinite loop since EOF is not being
+// returned by reader.Read() for empty block resulting in issue #7329.
+// Hence invoke PutBlock twice to test that path involving CompareAndTouch
+func TestPutEmptyBlock(t *testing.T) {
+ SetupKeepStoreUnixVolumeIntegrationTest(t)
+
+ // Check that PutBlock succeeds
+ if err := PutBlock(EmptyBlock, EmptyHash); err != nil {
+ t.Fatalf("Error during PutBlock: %s", err)
+ }
+
+ // Check that PutBlock succeeds again even after CompareAndTouch
+ // With #7329 unresovled, this falls in infinite loop in UnixVolume -> Compare method
+ if err := PutBlock(EmptyBlock, EmptyHash); err != nil {
+ t.Fatalf("Error during PutBlock: %s", err)
+ }
+
+ // Check that PutBlock stored the data as expected
+ buf, err := GetBlock(EmptyHash)
+ if err != nil {
+ t.Fatalf("Error during GetBlock for %q: %s", EmptyHash, err)
+ } else if bytes.Compare(buf, EmptyBlock) != 0 {
+ t.Errorf("Get response incorrect. Expected %q; found %q", EmptyBlock, buf)
+ }
+}
+
+// PutRaw EmptyHash with bad data (which bypasses hash check)
+// and then invoke PutBlock with the correct EmptyBlock.
+// Put should succeed and next Get should return EmptyBlock
+func TestPutEmptyBlockDiskHashError(t *testing.T) {
+ SetupKeepStoreUnixVolumeIntegrationTest(t)
+
+ badEmptyBlock := []byte("verybaddata")
+
+ // Put bad data for EmptyHash in both volumes
+ testableUnixVols[0].PutRaw(EmptyHash, badEmptyBlock)
+ testableUnixVols[1].PutRaw(EmptyHash, badEmptyBlock)
+
+ // Check that PutBlock with good data succeeds
+ if err := PutBlock(EmptyBlock, EmptyHash); err != nil {
+ t.Fatalf("Error during PutBlock for %q: %s", EmptyHash, err)
+ }
+
+ // Put succeeded and overwrote the badEmptyBlock in one volume,
+ // and Get should return the EmptyBlock now, ignoring the bad data.
+ buf, err := GetBlock(EmptyHash)
+ if err != nil {
+ t.Fatalf("Error during GetBlock for %q: %s", EmptyHash, err)
+ } else if bytes.Compare(buf, EmptyBlock) != 0 {
+ t.Errorf("Get response incorrect. Expected %q; found %q", TestBlock, buf)
+ }
+}
diff --git a/services/keepstore/keepstore_test.go b/services/keepstore/keepstore_test.go
index 3807317..7a882e0 100644
--- a/services/keepstore/keepstore_test.go
+++ b/services/keepstore/keepstore_test.go
@@ -26,6 +26,10 @@ var TestHash3 = "eed29bbffbc2dbe5e5ee0bb71888e61f"
// It must not match any test hashes.
var BadBlock = []byte("The magic words are squeamish ossifrage.")
+// Empty block
+var EmptyHash = "d41d8cd98f00b204e9800998ecf8427e"
+var EmptyBlock = []byte("")
+
// TODO(twp): Tests still to be written
//
// * TestPutBlockFull
@@ -414,6 +418,44 @@ func TestIndex(t *testing.T) {
}
}
+// TestKeepStoreGetEmptyBlock
+func TestKeepStoreGetEmptyBlock(t *testing.T) {
+ defer teardown()
+
+ // Prepare two mock volumes
+ KeepVM = MakeTestVolumeManager(2)
+ defer KeepVM.Close()
+
+ vols := KeepVM.AllWritable()
+ if err := vols[0].Put(EmptyHash, EmptyBlock); err != nil {
+ t.Error("Error putting empty block: %s", err)
+ }
+
+ // Check that GetBlock returns success.
+ result, err := GetBlock(EmptyHash)
+ if err != nil {
+ t.Errorf("Get error for empty hash: %s", err)
+ }
+ if bytes.Compare(result, EmptyBlock) != 0 {
+ t.Errorf("Get response incorrect. Expected %q; found %q", EmptyBlock, result)
+ }
+}
+
+// TestKeepStoreGetEmptyBlockNotExists that does not exist
+func TestKeepStoreGetEmptyBlockNotExists(t *testing.T) {
+ defer teardown()
+
+ // Prepare two mock volumes
+ KeepVM = MakeTestVolumeManager(2)
+ defer KeepVM.Close()
+
+ // Check that GetBlock returns error.
+ _, err := GetBlock(EmptyHash)
+ if err == nil {
+ t.Errorf("Expected error when getting non-existing empty block")
+ }
+}
+
// ========================================
// Helper functions for unit tests.
// ========================================
diff --git a/services/keepstore/mock_mutex_for_test.go b/services/keepstore/mock_mutex_for_test.go
index e75d910..24b549c 100644
--- a/services/keepstore/mock_mutex_for_test.go
+++ b/services/keepstore/mock_mutex_for_test.go
@@ -7,17 +7,17 @@ type MockMutex struct {
func NewMockMutex() *MockMutex {
return &MockMutex{
- AllowLock: make(chan struct{}),
+ AllowLock: make(chan struct{}),
AllowUnlock: make(chan struct{}),
}
}
// Lock waits for someone to send to AllowLock.
func (m *MockMutex) Lock() {
- <- m.AllowLock
+ <-m.AllowLock
}
// Unlock waits for someone to send to AllowUnlock.
func (m *MockMutex) Unlock() {
- <- m.AllowUnlock
+ <-m.AllowUnlock
}
diff --git a/services/keepstore/volume_unix.go b/services/keepstore/volume_unix.go
index afae279..bc6a6e0 100644
--- a/services/keepstore/volume_unix.go
+++ b/services/keepstore/volume_unix.go
@@ -143,6 +143,11 @@ func (v *UnixVolume) Compare(loc string, expect []byte) error {
return nil
} else if err != nil {
return err
+ } else if len(expect) == 0 && n == 0 && bytes.Compare(buf, expect) == 0 {
+ // When reading an empty block, EOF is not returned. Probably a Go issue.
+ // This is resulting in an infinite loop resulting in #7329
+ // Handle empty block scenario explicitly until the EOF issue is resolved.
+ return nil
}
}
})
diff --git a/services/keepstore/volume_unix_test.go b/services/keepstore/volume_unix_test.go
index 4f1e84c..35cf457 100644
--- a/services/keepstore/volume_unix_test.go
+++ b/services/keepstore/volume_unix_test.go
@@ -291,6 +291,65 @@ func TestUnixVolumeCompare(t *testing.T) {
}
}
+// Put an EmptyBlock and get and compare for EmptyHash
+// With #7329 unresolved, Compare falls in infinite loop
+func TestGetAndCompareEmptyBlock(t *testing.T) {
+ v := NewTestableUnixVolume(t, false, false)
+ defer v.Teardown()
+
+ v.Put(EmptyHash, EmptyBlock)
+
+ buf, err := v.Get(EmptyHash)
+ if err != nil {
+ t.Errorf("Error during Get for %q: %s", EmptyHash, err)
+ }
+
+ err = v.Compare(EmptyHash, buf)
+ if err != nil {
+ t.Errorf("Error during Compare for %q: %s", EmptyHash, err)
+ }
+}
+
+// Put baddata for EmptyHash. Get will succeed, but Compare will raise DiskHashError
+func TestGetAndCompareEmptyHashWithDiskHashError(t *testing.T) {
+ v := NewTestableUnixVolume(t, false, false)
+ defer v.Teardown()
+
+ v.PutRaw(EmptyHash, []byte("baddata"))
+
+ _, err := v.Get(EmptyHash)
+ if err != nil {
+ t.Errorf("Unexpected error after PutRaw EmptyHash with baddata")
+ }
+
+ err = v.Compare(EmptyHash, EmptyBlock)
+ if err != DiskHashError {
+ t.Errorf("Expected DiskHashError when comparing EmptyHash with bad data. Got %s", err)
+ }
+}
+
+// Get non existing empty block; should fail with file not found error.
+func TestGetEmptyBlockNonExisting(t *testing.T) {
+ v := NewTestableUnixVolume(t, false, false)
+ defer v.Teardown()
+
+ buf, err := v.Get(EmptyHash)
+ if err == nil {
+ t.Errorf("Get non existing empty hash should have failed, instead got %q", buf)
+ }
+}
+
+// Compare non existing empty hash should fail with file not found error.
+func TestCompareEmptyHashNonExisting(t *testing.T) {
+ v := NewTestableUnixVolume(t, false, false)
+ defer v.Teardown()
+
+ err := v.Compare(EmptyHash, EmptyBlock)
+ if err == nil {
+ t.Errorf("Expected error no such file. But got no error.")
+ }
+}
+
// TODO(twp): show that the underlying Read/Write operations executed
// serially and not concurrently. The easiest way to do this is
// probably to activate verbose or debug logging, capture log output
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list