cipd: instance cache

cipd: instance cache

Cache package instances on file system in a shared cache, as specified
by -cache-dir flag. Use LRU for GC.

The implementation is similar to the one of tag cache: a binary-protobuf
file with cache state is loaded/saved every time it is accessed. The state contains last access time of
instances.

If the state file is not found, corrupted or was not synced with fs for
a long time, the state is synced. Newly discovered instances are
considered last accessed at EPOCH.

GC happens on sync or when an instance is added: cutoff date is
determined and first N instances that were accessed before cutoff date
are removed, where N is the number of instance beyond cache limit.

R=vadimsh@chromium.org
BUG=601017
Committed: https://github.com/luci/luci-go/commit/6c20df86be89df4837ccf85f19827e69b1cf584c

[nodir, 2016-04-08 22:35:10]

PTAL. Tested locally by doing
cipd init -cache-dir ~/cipd-cache
cipd install packages...

Will write unit tests.

[nodir, 2016-04-11 14:35:39]

I will remove dep on local.FileSystem. It is an overkill

[Vadim Sh., 2016-04-11 16:29:13]

it is not, but you are not using it correctly

you shouldn't be using ioutil.WriteFile: it is racy. You should be using
FileSystem.EnsureFile - it is less racy (doing atomic rename).

[Vadim Sh., 2016-04-11 16:52:03]

also mention in the comments that cache doesn't validate hash of files (and
FetchInstance too) and it's caller responsibility to do so

https://codereview.chromium.org/1870263002/diff/20001/client/cipd/client.go
File client/cipd/client.go (right):

https://codereview.chromium.org/1870263002/diff/20001/client/cipd/client.go#n...
client/cipd/client.go:804: if _, err := output.Seek(0, 0); err != nil {
well, it may not be enough. We also need to truncate the file to zero length.
Otherwise we may end up with "<valid data>|<garbage left from reading cached
file>".

https://codereview.chromium.org/1870263002/diff/20001/client/cipd/client.go#n...
client/cipd/client.go:827: if _, err := output.Seek(0, 0); err != nil {
It may be simpler to always download into cache, and then do io.Copy from the
cache file into output. That way you don't need to change FetchInstance
signature (and no need to truncate files).

https://codereview.chromium.org/1870263002/diff/20001/client/cipd/internal/in...
File client/cipd/internal/instancecache.go (right):

https://codereview.chromium.org/1870263002/diff/20001/client/cipd/internal/in...
client/cipd/internal/instancecache.go:36: path   local.FileSystem
fs

it is not path

https://codereview.chromium.org/1870263002/diff/20001/client/cipd/internal/in...
client/cipd/internal/instancecache.go:110: if _, err = io.Copy(f, contents); err
!= nil {
use FileSystem.EnsureFile to avoid:

1) two cipd processing stepping on each others toes.
2) killed cipd process leaving invalid file with valid name in the cache dir.

https://codereview.chromium.org/1870263002/diff/20001/client/cipd/internal/in...
client/cipd/internal/instancecache.go:277: stateBytes, err :=
proto.Marshal(&c.state)
please use BlobWithSHA1 too:
https://github.com/luci/luci-go/blob/master/client/cipd/internal/checksum.go

https://codereview.chromium.org/1870263002/diff/20001/client/cipd/internal/in...
client/cipd/internal/instancecache.go:285: if err :=
ioutil.WriteFile(c.statePath(), stateBytes, 0666); err != nil {
use fs.EnsureFile

https://codereview.chromium.org/1870263002/diff/20001/client/cmd/cipd/main.go
File client/cmd/cipd/main.go (right):

https://codereview.chromium.org/1870263002/diff/20001/client/cmd/cipd/main.go...
client/cmd/cipd/main.go:1675: rand.Seed(time.Now().Unix())
I was bit in the ass by this construct. Multiple processes may start in exact
same second.

Seed from crypto/rand instead, e.g.

	var seed int64
	if err := binary.Read(cryptorand.Reader, binary.LittleEndian, &seed); err !=
nil {
		panic(err)
	}
	rand.Seed(seed)

https://codereview.chromium.org/1870263002/diff/20001/common/proto/google/uti...
File common/proto/google/util.go (right):

https://codereview.chromium.org/1870263002/diff/20001/common/proto/google/uti...
common/proto/google/util.go:28: func (t *Timestamp) FromTime(other time.Time) {
what if 'other' is Zero?

other.Unix() returns some nonsense in that case

I'd prefer NewTimestamp to be used always, since in proto fields we usually have
*Timestamp, and doing proto.timestamp = google.NewTimestamp(...) deals correctly
with zero time.

[nodir, 2016-04-11 22:04:38]

Patchset #3 (id:40001) has been deleted

[nodir, 2016-04-11 22:16:01]

Patchset #3 (id:60001) has been deleted

[nodir, 2016-04-11 22:16:17]

https://codereview.chromium.org/1870263002/diff/20001/client/cipd/client.go
File client/cipd/client.go (right):

https://codereview.chromium.org/1870263002/diff/20001/client/cipd/client.go#n...
client/cipd/client.go:804: if _, err := output.Seek(0, 0); err != nil {
On 2016/04/11 16:52:02, Vadim Sh. wrote:
> well, it may not be enough. We also need to truncate the file to zero length.
> Otherwise we may end up with "<valid data>|<garbage left from reading cached
> file>".

Done.

https://codereview.chromium.org/1870263002/diff/20001/client/cipd/client.go#n...
client/cipd/client.go:827: if _, err := output.Seek(0, 0); err != nil {
On 2016/04/11 16:52:02, Vadim Sh. wrote:
> It may be simpler to always download into cache, and then do io.Copy from the
> cache file into output. That way you don't need to change FetchInstance
> signature (and no need to truncate files).

Done.

https://codereview.chromium.org/1870263002/diff/20001/client/cipd/internal/in...
File client/cipd/internal/instancecache.go (right):

https://codereview.chromium.org/1870263002/diff/20001/client/cipd/internal/in...
client/cipd/internal/instancecache.go:36: path   local.FileSystem
On 2016/04/11 16:52:03, Vadim Sh. wrote:
> fs
> 
> it is not path

Done.

https://codereview.chromium.org/1870263002/diff/20001/client/cipd/internal/in...
client/cipd/internal/instancecache.go:110: if _, err = io.Copy(f, contents); err
!= nil {
On 2016/04/11 16:52:03, Vadim Sh. wrote:
> use FileSystem.EnsureFile to avoid:
> 
> 1) two cipd processing stepping on each others toes.
> 2) killed cipd process leaving invalid file with valid name in the cache dir.

Done.

https://codereview.chromium.org/1870263002/diff/20001/client/cipd/internal/in...
client/cipd/internal/instancecache.go:277: stateBytes, err :=
proto.Marshal(&c.state)
On 2016/04/11 16:52:03, Vadim Sh. wrote:
> please use BlobWithSHA1 too:
> https://github.com/luci/luci-go/blob/master/client/cipd/internal/checksum.go

Done.

https://codereview.chromium.org/1870263002/diff/20001/client/cipd/internal/in...
client/cipd/internal/instancecache.go:285: if err :=
ioutil.WriteFile(c.statePath(), stateBytes, 0666); err != nil {
On 2016/04/11 16:52:03, Vadim Sh. wrote:
> use fs.EnsureFile

Done.

https://codereview.chromium.org/1870263002/diff/20001/client/cmd/cipd/main.go
File client/cmd/cipd/main.go (right):

https://codereview.chromium.org/1870263002/diff/20001/client/cmd/cipd/main.go...
client/cmd/cipd/main.go:1675: rand.Seed(time.Now().Unix())
On 2016/04/11 16:52:03, Vadim Sh. wrote:
> I was bit in the ass by this construct. Multiple processes may start in exact
> same second.
> 
> Seed from crypto/rand instead, e.g.
> 
> 	var seed int64
> 	if err := binary.Read(cryptorand.Reader, binary.LittleEndian, &seed); err !=
> nil {
> 		panic(err)
> 	}
> 	rand.Seed(seed)

Done.

https://codereview.chromium.org/1870263002/diff/20001/common/proto/google/uti...
File common/proto/google/util.go (right):

https://codereview.chromium.org/1870263002/diff/20001/common/proto/google/uti...
common/proto/google/util.go:28: func (t *Timestamp) FromTime(other time.Time) {
On 2016/04/11 16:52:03, Vadim Sh. wrote:
> what if 'other' is Zero?
> 
> other.Unix() returns some nonsense in that case
> 
> I'd prefer NewTimestamp to be used always, since in proto fields we usually
have
> *Timestamp, and doing proto.timestamp = google.NewTimestamp(...) deals
correctly
> with zero time.

Done.

[Vadim Sh., 2016-04-11 23:47:50]

Consider also reducing time a cache state is open, e.g. instead of doing:

1) Open.
2) Fetch instance.
3) Fetch instance.
4) Close.

Do

1) Open. Fetch instance. Close.
2) Open. Fetch instance. Close.

or even: Fetch Instace, Open, update state file, Close.

It significantly reduces chance of state file overwrite (since it will be open
for shorter time).

I didn't bother doing it with tag cache because it wasn't supposed to be used
globally. (It should be done with tag cache too...)

https://codereview.chromium.org/1870263002/diff/80001/client/cipd/client.go
File client/cipd/client.go (right):

https://codereview.chromium.org/1870263002/diff/80001/client/cipd/client.go#n...
client/cipd/client.go:788: func (client *clientImpl) FetchInstance(pin
common.Pin, output io.WriteSeeker) error {
I think I prefer 

func ... FetchInstance(...) ... {
   ..

  cache := ...
  if cache == nil {
    return client.fetchInstanceNoCache(...)
  } else {
    return client.fetchInstanceWithCache(...)
  }
}

instead of bunch of back-and-forth ifs.

[nodir, 2016-04-11 23:51:42]

sigh, I did that and then saw tagcache reads and writes once per run

[nodir, 2016-04-13 00:02:19]

Patchset #4 (id:100001) has been deleted

[nodir, 2016-04-13 00:43:16]

Patchset #4 (id:120001) has been deleted

[nodir, 2016-04-13 01:53:02]

Patchset #4 (id:140001) has been deleted

[nodir, 2016-04-13 02:05:30]

PTAL

https://codereview.chromium.org/1870263002/diff/80001/client/cipd/client.go
File client/cipd/client.go (right):

https://codereview.chromium.org/1870263002/diff/80001/client/cipd/client.go#n...
client/cipd/client.go:788: func (client *clientImpl) FetchInstance(pin
common.Pin, output io.WriteSeeker) error {
On 2016/04/11 23:47:50, Vadim Sh. wrote:
> I think I prefer 
> 
> func ... FetchInstance(...) ... {
>    ..
> 
>   cache := ...
>   if cache == nil {
>     return client.fetchInstanceNoCache(...)
>   } else {
>     return client.fetchInstanceWithCache(...)
>   }
> }
> 
> instead of bunch of back-and-forth ifs.

Done.

[nodir, 2016-04-13 02:08:56]

Description was changed from

==========
cipd: instance cache

Cache package instances on file system in a shared cache, as specified
by -cache-dir flag. Use LRU for GC.

The implementation is similar to the one of tag cache: a binary-protobuf
file with cache state is loaded on client creation and saved back
when the client is closed. The state contains last access time of
instances.

If file is not found, corrupted or was not synced with fs for
a long time, the state is synced. Newly discovered instances are
considered last accessed at EPOCH.

GC happens when on loading or when an instance is added: cutoff date is
determined and first N instances that were accessed before cutoff date
are removed, where N is the number of instance beyond cache limit.

R=vadimsh@chromium.org
BUG=601017
==========

to

==========
cipd: instance cache

Cache package instances on file system in a shared cache, as specified
by -cache-dir flag. Use LRU for GC.

The implementation is similar to the one of tag cache: a binary-protobuf
file with cache state is loaded/saved every time it is accessed. The state
contains last access time of
instances.

If the state file is not found, corrupted or was not synced with fs for
a long time, the state is synced. Newly discovered instances are
considered last accessed at EPOCH.

GC happens on sync or when an instance is added: cutoff date is
determined and first N instances that were accessed before cutoff date
are removed, where N is the number of instance beyond cache limit.

R=vadimsh@chromium.org
BUG=601017
==========

[nodir, 2016-04-13 19:15:26]

Patchset #5 (id:180001) has been deleted

[nodir, 2016-04-13 19:15:47]

Added tests, ready for review. PTAL!

[Vadim Sh., 2016-04-13 21:49:02]

lgtm

https://codereview.chromium.org/1870263002/diff/200001/client/cipd/client.go
File client/cipd/client.go (left):

https://codereview.chromium.org/1870263002/diff/200001/client/cipd/client.go#...
client/cipd/client.go:761: err := common.ValidatePin(pin)
you lost this check

https://codereview.chromium.org/1870263002/diff/200001/client/cipd/client.go
File client/cipd/client.go (right):

https://codereview.chromium.org/1870263002/diff/200001/client/cipd/client.go#...
client/cipd/client.go:803: // Output may be corrupted. Rewinding back.
you still need to truncate the file :(

https://codereview.chromium.org/1870263002/diff/200001/client/cipd/client.go#...
client/cipd/client.go:823: if _, err := io.Copy(output, f); err != nil {
eh.. probably good enough for now, but it is a bit wasteful

in most cases 'output' here is actually some temp file that is thrown away after
it is unzipped

it would be good to be able to unzip directly from a file in cache (to avoid
copying the file)

https://codereview.chromium.org/1870263002/diff/200001/client/cipd/internal/i...
File client/cipd/internal/instancecache.go (right):

https://codereview.chromium.org/1870263002/diff/200001/client/cipd/internal/i...
client/cipd/internal/instancecache.go:86: func (c *InstanceCache) Put(pin
common.Pin, now time.Time, write func(*os.File) error) error {
btw, why *os.File and not io.WriteSeeker?

https://codereview.chromium.org/1870263002/diff/200001/client/cipd/internal/i...
client/cipd/internal/instancecache.go:269: return local.EnsureFile(c.fs,
statePath, bytes.NewReader(stateBytes))
we will need to teach EnsureFile to retry access errors a bunch of times for
Windows' sake... but not in this CL

[nodir, 2016-04-13 23:23:52]

https://codereview.chromium.org/1870263002/diff/200001/client/cipd/client.go
File client/cipd/client.go (left):

https://codereview.chromium.org/1870263002/diff/200001/client/cipd/client.go#...
client/cipd/client.go:761: err := common.ValidatePin(pin)
On 2016/04/13 21:49:02, Vadim Sh. wrote:
> you lost this check

Done.

https://codereview.chromium.org/1870263002/diff/200001/client/cipd/client.go
File client/cipd/client.go (right):

https://codereview.chromium.org/1870263002/diff/200001/client/cipd/client.go#...
client/cipd/client.go:803: // Output may be corrupted. Rewinding back.
On 2016/04/13 21:49:02, Vadim Sh. wrote:
> you still need to truncate the file :(

it is not required. I've improved the comment

https://codereview.chromium.org/1870263002/diff/200001/client/cipd/client.go#...
client/cipd/client.go:823: if _, err := io.Copy(output, f); err != nil {
On 2016/04/13 21:49:02, Vadim Sh. wrote:
> eh.. probably good enough for now, but it is a bit wasteful
> 
> in most cases 'output' here is actually some temp file that is thrown away
after
> it is unzipped
> 
> it would be good to be able to unzip directly from a file in cache (to avoid
> copying the file)

Acknowledged.

https://codereview.chromium.org/1870263002/diff/200001/client/cipd/internal/i...
File client/cipd/internal/instancecache.go (right):

https://codereview.chromium.org/1870263002/diff/200001/client/cipd/internal/i...
client/cipd/internal/instancecache.go:86: func (c *InstanceCache) Put(pin
common.Pin, now time.Time, write func(*os.File) error) error {
On 2016/04/13 21:49:02, Vadim Sh. wrote:
> btw, why *os.File and not io.WriteSeeker?

because *os.File is what Put provides. 
In general, types of provided values (e.g. return values) should be concrete,
and types of required values (e.g. parameters) should be abstract. Here file is
a provided value.

https://codereview.chromium.org/1870263002/diff/200001/client/cipd/internal/i...
client/cipd/internal/instancecache.go:269: return local.EnsureFile(c.fs,
statePath, bytes.NewReader(stateBytes))
On 2016/04/13 21:49:02, Vadim Sh. wrote:
> we will need to teach EnsureFile to retry access errors a bunch of times for
> Windows' sake... but not in this CL

yeah, that's what I thought when I saw your CL

[nodir, 2016-04-13 23:36:53]

The CQ bit was checked by nodir@chromium.org

[nodir, 2016-04-13 23:36:54]

The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org
Link to the patchset: https://codereview.chromium.org/1870263002/#ps220001
(title: "restore check, improve comment")

[commit-bot: I haz the power, 2016-04-13 23:36:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1870263002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1870263002/220001

[commit-bot: I haz the power, 2016-04-13 23:40:55]

Description was changed from

==========
cipd: instance cache

Cache package instances on file system in a shared cache, as specified
by -cache-dir flag. Use LRU for GC.

The implementation is similar to the one of tag cache: a binary-protobuf
file with cache state is loaded/saved every time it is accessed. The state
contains last access time of
instances.

If the state file is not found, corrupted or was not synced with fs for
a long time, the state is synced. Newly discovered instances are
considered last accessed at EPOCH.

GC happens on sync or when an instance is added: cutoff date is
determined and first N instances that were accessed before cutoff date
are removed, where N is the number of instance beyond cache limit.

R=vadimsh@chromium.org
BUG=601017
==========

to

==========
cipd: instance cache

Cache package instances on file system in a shared cache, as specified
by -cache-dir flag. Use LRU for GC.

The implementation is similar to the one of tag cache: a binary-protobuf
file with cache state is loaded/saved every time it is accessed. The state
contains last access time of
instances.

If the state file is not found, corrupted or was not synced with fs for
a long time, the state is synced. Newly discovered instances are
considered last accessed at EPOCH.

GC happens on sync or when an instance is added: cutoff date is
determined and first N instances that were accessed before cutoff date
are removed, where N is the number of instance beyond cache limit.

R=vadimsh@chromium.org
BUG=601017

Committed:
https://github.com/luci/luci-go/commit/6c20df86be89df4837ccf85f19827e69b1cf584c
==========

[commit-bot: I haz the power, 2016-04-13 23:40:56]

Committed patchset #6 (id:220001) as
https://github.com/luci/luci-go/commit/6c20df86be89df4837ccf85f19827e69b1cf584c