Arvados

Review notes:

• PutBlock logic is not quite right. It shouldn't write to all Keep volumes it has. Only to one. You should assume there will be many keep servers, each with a number of disks, and the client is responsible for writing copies to as many keep servers as appropriate. Each Keep server should never have more than one copy of a block. Fixing this also sorts out the ambiguity of the 503 return code: that code should mean "I don't have any space to write this block".

Other than that, I think this looks fine to merge. The tests pass, and the code builds.

Review 2:

• About your TODO: check for a full volume before trying to write. Please make this compatible with the existing Keepv1 logic (i.e. a symlink called 'full' that points to a timestamp (non-existant file)). Have a look in the Keepv1 codebase.

Reasoning: I want Keepv2 to be as compatible as possible with Keepv1 is that I really want to be able to have it act as a drop-in replacement.

• About your TODO: should we check here whether the file on disk matches the file we were asked to store?

Yes - that would be good I think. This is an interesting scenario: we've had data corruption on disk, but someone is offering us a new copy of the data, allowing us to repair the corruption. The correct course of action is probably to delete the corrupted file on disk (or mv it to hash.corrupted?) and write the new file.

Otherwise, this looks ready to merge I think. Tests and build pass.

Looking at this diff

• Avoid leaving bogus data on disk if process/machine dies mid-write: Write to {hash}.tmp, then rename it to {hash} if everything is still OK after flush.
• Avoid race condition in write: use O_EXCL when opening .tmp file
• Avoid leaving bogus data on disk if write fails: check errors from f.Close(), and remove .tmp file if Write() or Close() fails
• L241 looks like it finds corrupt data on disk but reports "Collision". If this is actually the right thing to do, maybe add a comment saying why
• Use statfs instead of `df` to get free disk space

Tim Pierce wrote:

• Avoid leaving bogus data on disk if process/machine dies mid-write: Write to {hash}.tmp, then rename it to {hash} if everything is still OK after flush.

Good thought. I am using ioutil.TempFile with a filename prefix of "tmp{hash}" instead, so that the tmp files will not show up if someone requests a file index. That seems to me like the desired behavior; please let me know if you don't agree.

Agree.

• Avoid race condition in write: use O_EXCL when opening .tmp file

I think using ioutil.TempFile is sufficient to avoid this race, but again, let me know if you disagree.

Agree. (Docs seem quite clear that ioutil.TempFile guarantees no races.)

• L241 looks like it finds corrupt data on disk but reports "Collision". If this is actually the right thing to do, maybe add a comment saying why

Per discussion on IRC: the desired behavior here is to report the corrupt block internally, then replace it with the good block we have on hand. I've modified this code and its test accordingly.

Looks good

• Use statfs instead of `df` to get free disk space

Done.

Nice

Here's an MD5 collision, if you feel like testing that behavior:

foo = '\x0e0eaU\x9a\xa7\x87\xd0\x0b\xc6\xf7\x0b\xbd\xfe4\x04\xcf\x03e\x9epO\x854\xc0\x0f\xfbe\x9cL\x87@\xcc\x94/\xeb-\xa1\x15\xa3\xf4\x15\\\xbb\x86\x07Is\x86em}\x1f4\xa4 Y\xd7\x8fZ\x8d\xd1\xef'
bar = '\x0e0eaU\x9a\xa7\x87\xd0\x0b\xc6\xf7\x0b\xbd\xfe4\x04\xcf\x03e\x9etO\x854\xc0\x0f\xfbe\x9cL\x87@\xcc\x94/\xeb-\xa1\x15\xa3\xf4\x15\xdc\xbb\x86\x07Is\x86em}\x1f4\xa4 Y\xd7\x8fZ\x8d\xd1\xef'
foo == bar
# => False
import hashlib
hashlib.md5(foo).hexdigest()
# => 'cee9a457e790cf20d4bdaa6d69f01e41'
hashlib.md5(bar).hexdigest()
# => 'cee9a457e790cf20d4bdaa6d69f01e41'

Another thought for "tests todo": Write fail despite disk reporting plenty of space. (Perhaps test by using a small tmpfs + lowering MIN_FREE_KILOBYTES?)

Just checked "git diff --check" and it complains about a lot of "tab in indent". gofmt seems to have an "indent with spaces" mode. Is there a strong preference for tabs in the Go world? (If not, we should probably stick to our own coding standards)

I haven't spotted any other problems.

Tom Clegg wrote:

Here's an MD5 collision, if you feel like testing that behavior:

[...]

Thanks, I've added TestPutBlockCollision.

Another thought for "tests todo": Write fail despite disk reporting plenty of space. (Perhaps test by using a small tmpfs + lowering MIN_FREE_KILOBYTES?)

Good thought. Might be easier to use an interface to mock ioutil.TempFile. I've added a TODO.

Just checked "git diff --check" and it complains about a lot of "tab in indent". gofmt seems to have an "indent with spaces" mode. Is there a strong preference for tabs in the Go world? (If not, we should probably stick to our own coding standards)

There actually is a strong preference, even a formally recommended one: http://golang.org/doc/effective_go.html#formatting

We use tabs for indentation and gofmt emits them by default. Use spaces only if you must.

Tim Pierce wrote:

Thanks, I've added TestPutBlockCollision.

Looks great

Another thought for "tests todo": Write fail despite disk reporting plenty of space. (Perhaps test by using a small tmpfs + lowering MIN_FREE_KILOBYTES?)

Good thought. Might be easier to use an interface to mock ioutil.TempFile. I've added a TODO.

Great. (But I think the todo comment says "high MIN_FREE" where it should say "low MIN_FREE"?)

Just checked "git diff --check" and it complains about a lot of "tab in indent". gofmt seems to have an "indent with spaces" mode. Is there a strong preference for tabs in the Go world? (If not, we should probably stick to our own coding standards)

There actually is a strong preference, even a formally recommended one: http://golang.org/doc/effective_go.html#formatting

We use tabs for indentation and gofmt emits them by default. Use spaces only if you must.

Indeed. Lovely, now we have to figure out how to make git complain/prohibit tabs in most, but not all, parts of the source tree. (I wouldn't say that qualifies us for the "if you must" clause...)

Comments relating to 2449-keep-index-status-handlers:

• Apply filter so that /index doesn't list files that are not keep blocks, such as *.meta, .lock, last_error, etc
• Add a "device" field to /status.json so the reader can distinguish physical volumes from mount points.

Peter Amstutz wrote:

Comments relating to 2449-keep-index-status-handlers:

• Apply filter so that /index doesn't list files that are not keep blocks, such as *.meta, .lock, last_error, etc

Good thought, added an IsValidLocator method to filter out these files and updated the unity test to see that .meta files are properly excluded.

• Add a "device" field to /status.json so the reader can distinguish physical volumes from mount points.

Added device_num to status.json, populated with the stat(2) st_dev field for the Keep volume directory.

• /index looks good
• I was envisioning providing the device id, e.g. "/dev/sda3" which would be more useful than fsid, but fsid is good enough to distinguish the physical volumes. Ideally we want to provide quite a bit more useful information about the disk, such as which drive bay the disk is in, but that's a story for another sprint.

I ran ./keep -volumes=~/keep0
This fails because ~ is not expanded, but Keep doesn't report an error until you try to access the volume, e.g.:
IndexHandler: ~/keep0: walking to ~/keep0: lstat ~/keep0: no such file or directory

It should check that the volume exists and is accessible and warn or fail if not.

Peter Amstutz wrote:

• /index looks good
• I was envisioning providing the device id, e.g. "/dev/sda3" which would be more useful than fsid, but fsid is good enough to distinguish the physical volumes. Ideally we want to provide quite a bit more useful information about the disk, such as which drive bay the disk is in, but that's a story for another sprint.

That would probably be more helpful, agreed.

It's not clear to me how much of this makes sense to make Keep's responsibility to track. If a disk is going bad and it's throwing write errors, the paths to the problem files/volumes will be recorded in the Keep error logs and can be used by a sysadmin to diagnose which disk needs replacement. It's not clear to me that there's value in exposing that information in the status handler as well. But yes, we should review these issues in the next sprint or two.

I ran ./keep -volumes=~/keep0
This fails because ~ is not expanded, but Keep doesn't report an error until you try to access the volume, e.g.:
IndexHandler: ~/keep0: walking to ~/keep0: lstat ~/keep0: no such file or directory

It should check that the volume exists and is accessible and warn or fail if not.

Good call. I've added a check for this:

twp@yclept:~/arvados/services/keep$ go run keep.go -volumes=/dsfksjhdf,/lkjshdksd,/kdsfks
2014/04/16 13:41:36 bad Keep volume: stat /dsfksjhdf: no such file or directory
2014/04/16 13:41:36 bad Keep volume: stat /lkjshdksd: no such file or directory
2014/04/16 13:41:36 bad Keep volume: stat /kdsfks: no such file or directory
2014/04/16 13:41:36 could not find any keep volumes
exit status 1

If at least one valid Keep volume is found, the server will launch. It will only die with a fatal error if no valid volumes are specified.