bazel remote cache: add eviction #6812

BenTheElder · 2018-02-14T01:24:03Z

add --remount which will attempt to remount the storage device with strictatime,lazyatime which will have the filesystem track access times for entries with lazy flushing to disk
- this takes advantage of the fact that ext4 supports lazyatime which should have better characteristics than just enabling atime
- We are doing this because Linux defaults to relatime which is not very useful for us since it only updates at most once every 24 hours or when the file is modified, but access time tracking via atime is simple and durable
add eviction based on access time

also vendors a small library for getting atime cross platform, go's stdlib leaves this as entirely system dependent even though most (all?) platforms support this.

xref: #6808

/area bazel
/area kinda-hacky 😬

BenTheElder · 2018-02-14T01:24:40Z

experiment/nursery/BUILD.bazel

@@ -38,6 +38,7 @@ filegroup(

 go_image(
    name = "image",
+    base = "@alpine-base//image",


distroless doesn't have any utilities, I've spun up alpine and it has mount etc.

BenTheElder · 2018-02-14T01:26:08Z

I've SSHed to the cache node and verified that a privileged container can remount with these options enabled and that this does correctly track the access times.

/assign @cjwagner @ixdy @krzyzacy

ixdy · 2018-02-14T01:31:57Z

experiment/nursery/main.go

+// remounts with 'strictatime,lazyatime'
+func remountWithLazyAtime(device, mountPoint string) error {
+	_, err := commandLines([]string{
+		"mount", "-o", "remount,strictatime,lazytime", device, mountPoint,


couldn't you use syscall.Mount and pass syscall.MS_REMOUNT | syscall.MS_STRICTATIME | syscall.MS_LAZYTIME as options?

well, I guess go doesn't know about lazytime yet. lxc/incus@7446f6b

though golang.org/x/sys/unix has it.

I'd also have to parse and plumb through the type, mount is a little more magical

Discussed this offline, shelling out smells a bit gross but syscall.Mount also appears to be a wrapper around mount(2) anyhow.

ixdy · 2018-02-14T01:41:32Z

broader question: is this a better approach than just keeping track of access times via GETs in nursery itself?

BenTheElder · 2018-02-14T01:46:32Z

broader question: is this a better approach than just keeping track of access times via GETs in nursery itself?

I thought about this, implementing this correctly and efficiently is tricky. We're write heavy and we don't want to block when scanning the cache for eviction. Remounting gets us efficient access time tracking for ~free.

More specifically I'm planning to do eviction similar to the kubelet, IE periodically get disk usage and when it passes some threshold(s) scan the cache and start kicking things out.
We could just do something like toss a map of keys to timestamps and mutex in nursery but then we stall while evicting. I'd really like to avoid evicting things unless we're out of disk space and I do want to evict things that are least recently used.

BenTheElder · 2018-02-14T01:59:02Z

Also from offline discussion: the filesystem also gets us free persistence and concurrent write / read. Having lazyatime should help prevent this from being overly abusive to the disk.

Downsides: lazyatime is ext4 specific (thankfully lots of linux things including our local ssd use it anyhow), this also smells a bit of coupling but I've placed the remount code behind a feature flag so this seems OK.

BenTheElder · 2018-02-14T07:21:12Z

This should be ready for review.

I've definitely left room in the API for the cache to track things internally in the future in which case we can remove the remount code.

BenTheElder · 2018-02-14T20:44:06Z

I've deployed this and created / cat-ed a test file, ls -lu shows the timestamps changing 👍

BenTheElder · 2018-02-14T21:37:04Z

I've spoken to @verult on storage about this: ~ next quarter local volumes should be beta (alpha currently) and then we can in theory use PV mount options instead and delete the --remount handling. There are a few other considerations that come with local volumes but we can probably sort those out then.

BenTheElder · 2018-02-14T23:11:18Z

/test pull-test-infra-bazel-canary

cjwagner · 2018-02-15T01:30:40Z

experiment/nursery/diskcache/cache.go

+// PathToKey converts a path on disk to a key, assuming the path is actually
+// under DiskRoot() ...
+func (c *Cache) PathToKey(key string) string {
+	return strings.TrimPrefix(key, c.diskRoot+string(os.PathSeparator))


You should document that c.diskRoot cannot have a trailing slash. Specifying the root dir as the diskRoot could be a little strange too since it would need to be specified as "" instead of "/".

I'll probably just make the constructor handle this instead, good point though.

the constructor now trims os.PathSeperator

cjwagner · 2018-02-15T01:44:21Z

experiment/nursery/main.go

 	flag.Parse()
+	logger := log.WithFields(log.Fields{


You should pass this logger to functions like cacheHandler and monitorDiskAndEvict instead of using the default undecorated logger or adding the field component=nusery in multiple places.

I've been thinking about creating a custom formatter with the default field instead, there are lots of places the subpackages should log things that won't be decorated. We might want to do this in prow as well.

cjwagner · 2018-02-15T01:46:00Z

experiment/nursery/main.go

+			if err != nil {
+				logger.WithError(err).Error("Failed to remount with lazyatime!")
+			}
+			logger.Info("Remount complete")


Only log this if we successfully remount?
Also, should remount failures be fatal?

A) yes, fixing
B) probably not, the cache will still work, the eviction will just be worse

cjwagner · 2018-02-15T01:48:31Z

experiment/nursery/main.go

@@ -142,3 +176,42 @@ func cacheHandler(cache *diskcache.Cache) http.Handler {
 		}
 	})
 }
+
+func monitorDiskAndEvict(cache *diskcache.Cache, interval time.Duration, minBlocksFree, minFilesFree float64) {


Consider moving this to the diskcache package and making it a method of Cache.

cjwagner · 2018-02-15T01:50:25Z

experiment/nursery/main.go

+		blocksFree, filesFree := diskutil.GetDiskUsage(dir)
+		// if we are past either threshold, evict until we are not
+		if blocksFree < minBlocksFree || filesFree < minFilesFree {
+			logger.WithFields(log.Fields{


This might be useful to log every tick instead of only when we actually do evictions. Then we could monitor the disk usage more easily.

cjwagner · 2018-02-15T01:53:02Z

experiment/nursery/main.go

+			// so we can pop entries until we have evicted enough
+			files := cache.GetEntries()
+			sort.Slice(files, func(i, j int) bool {
+				return files[i].LastAccess.After(files[j].LastAccess)


I think you want Before not After?

Yes. Originally I was popping from the end of the slice. Thanks!

krzyzacy

mostly lgtm, a few nit-picks

also what's the expected behavior when the eviction blows away? We probably want to get notified somehow? (though it should not happen?)

krzyzacy · 2018-02-15T03:38:41Z

experiment/nursery/diskcache/cache.go

+					"files-free":  filesFree,
+				})
+				if len(files) < 1 {
+					logger.Fatalf("Failed to find entries to evict!")


nit: nothing to format, just use logger.Fatal

krzyzacy · 2018-02-15T03:48:43Z

experiment/nursery/diskcache/cache.go

+
+// MonitorDiskAndEvict loops monitoring the disk, evicting cache entries
+// when the disk passes either minPercentBlocksFree or minPercentFilesFree
+func (c *Cache) MonitorDiskAndEvict(interval time.Duration, minPercentBlocksFree, minPercentFilesFree float64) {


maybe also add a test for this?

Hmm, how would that work? I'd need to fill up a disk 😬

maybe you can make a fake diskutil?

I don't think tick loops should be unit tested. This was originally in main.go partially due to that.

Discussed offline, adding metrics / monitoring is coming later, and the failure modes for this are such that it's not a real concern (jobs will continue to work, but perhaps slower).

I will add regression tests if anything does go wrong here.

krzyzacy · 2018-02-15T03:52:27Z

experiment/nursery/diskcache/cache.go

+				entry, files = files[0], files[1:]
+				err := c.Delete(c.PathToKey(entry.Path))
+				if err != nil {
+					logger.WithError(err).Error("Error deleting entry")


nit: maybe also log path

krzyzacy · 2018-02-15T03:53:50Z

experiment/nursery/diskutil/diskutil.go

+// GetDiskUsage wraps syscall.Statfs
+func GetDiskUsage(path string) (percentBlocksFree, percentFilesFree float64) {
+	var stat syscall.Statfs_t
+	syscall.Statfs(path, &stat)


check error

done, passed up and checked at call sites / logged

krzyzacy · 2018-02-15T03:55:22Z

experiment/nursery/main.go

+// NOTE: remount is a bit of a hack, unfortunately the kubernetes volumes
+// don't really support this and to cleanly track entry access times we
+// want to use a volume with lazyatime (and not noatime or relatime)
+// so that file access times *are* recorded but are lazily flushed to the disk


can you also link some doc about lazyatime for future reference?

/done, also so/lazyatime/strictatime,lazytime/

krzyzacy · 2018-02-15T03:55:51Z

experiment/nursery/main.go

+var minPercentBlocksFree = flag.Float64("min-percent-blocks-free", 10,
+	"minimum percent of blocks free on --dir's disk before evicting entries")
+var minPercentFilesFree = flag.Float64("min-percent-files-free", 10,
+	"minimum percent of blocks free on --dir's disk before evicting entries")


nit: s/blocks/files?

krzyzacy · 2018-02-15T19:01:18Z

forgot to push?

BenTheElder · 2018-02-15T19:02:40Z

forgot to push?

heh I was just pushing

krzyzacy · 2018-02-15T19:22:44Z

experiment/nursery/diskutil/diskutil.go

+	var stat syscall.Statfs_t
+	err = syscall.Statfs(path, &stat)
+	if err != nil {
+		return 0, 0, nil


0, 0, err?

krzyzacy · 2018-02-15T20:08:22Z

might also worth adding more docs once we start to use it

/lgtm
/hold

k8s-ci-robot · 2018-02-15T20:08:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, krzyzacy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [BenTheElder,krzyzacy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

BenTheElder · 2018-02-15T20:12:11Z

might also worth adding more docs once we start to use it

I will write thorough docs if/when this moves out of experiment/. For now I plan to test against canary jobs and test-infra. If that goes well I'll rename, doc, cleanup, and move in a follow up. Also definitely metrics and a dashboard / alerts. See: #6808

/hold cancel

BenTheElder · 2018-02-15T22:57:05Z

other than needing #6850 to fix the DefaultEntryFormatter this is WAI so far.

BenTheElder commented Feb 14, 2018

View reviewed changes

k8s-ci-robot assigned cjwagner, ixdy and krzyzacy Feb 14, 2018

ixdy reviewed Feb 14, 2018

View reviewed changes

BenTheElder force-pushed the cache-hacks branch from 2961da7 to 66b4059 Compare February 14, 2018 01:33

BenTheElder force-pushed the cache-hacks branch 2 times, most recently from c2e46b3 to 93eae1f Compare February 14, 2018 06:00

BenTheElder force-pushed the cache-hacks branch 2 times, most recently from a0e3048 to 7f9c610 Compare February 14, 2018 06:24

BenTheElder changed the title ~~[WIP] bazel remote cache: add eviction~~ bazel remote cache: add eviction Feb 14, 2018

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 14, 2018

BenTheElder force-pushed the cache-hacks branch 3 times, most recently from 5f9a6b0 to d9d07a4 Compare February 14, 2018 07:00

add remount with lazyatime

30779ae

BenTheElder added 4 commits February 14, 2018 12:33

create diskutil package, implement eviction

7606dc5

update tests

0e0a529

vendor github.com/djherbis/atime

7ece043

use json formatter, log when done remounting

8162bee

BenTheElder force-pushed the cache-hacks branch from d9d07a4 to 8162bee Compare February 14, 2018 20:43

BenTheElder mentioned this pull request Feb 14, 2018

Implement Bazel Remote Caching [Tracking Issue] #6808

Closed

12 tasks

cjwagner reviewed Feb 15, 2018

View reviewed changes

nursery cleanup and fixes

e810a9b

BenTheElder force-pushed the cache-hacks branch from cfd39ca to e810a9b Compare February 15, 2018 03:10

krzyzacy reviewed Feb 15, 2018

View reviewed changes

cleanup / nits

c89cee3

BenTheElder force-pushed the cache-hacks branch from 64de7e9 to c89cee3 Compare February 15, 2018 19:38

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Feb 15, 2018

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 15, 2018

k8s-ci-robot merged commit f5b517c into kubernetes:master Feb 15, 2018

BenTheElder deleted the cache-hacks branch February 15, 2018 20:13

bazel remote cache: add eviction #6812

bazel remote cache: add eviction #6812

Conversation

BenTheElder commented Feb 14, 2018 • edited Loading

Choose a reason for hiding this comment

BenTheElder commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ixdy commented Feb 14, 2018

BenTheElder commented Feb 14, 2018 • edited Loading

BenTheElder commented Feb 14, 2018

BenTheElder commented Feb 14, 2018

BenTheElder commented Feb 14, 2018

BenTheElder commented Feb 14, 2018

BenTheElder commented Feb 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krzyzacy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krzyzacy commented Feb 15, 2018

BenTheElder commented Feb 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krzyzacy commented Feb 15, 2018

k8s-ci-robot commented Feb 15, 2018

BenTheElder commented Feb 15, 2018

BenTheElder commented Feb 15, 2018

BenTheElder commented Feb 14, 2018 •

edited

Loading

BenTheElder commented Feb 14, 2018 •

edited

Loading