Skip to content

Commit 8ab220a

Browse files
ameowliaameowlia
authored
Merge pull request #1163 from cloudfoundry/TNZ-111552
Fix download cache bloat and stale disk capacity reporting causing Diego cell disk full
2 parents ec2a26a + a9ee371 commit 8ab220a

19 files changed

Lines changed: 272 additions & 110 deletions

File tree

‎.github/workflows/tests-workflow.yml‎

Lines changed: 0 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -138,12 +138,6 @@ jobs:
138138
- name: build binaries
139139
run: |
140140
repo/.github/helpers/build.bash ${{ github.workspace }} "$BUILD_IMAGE"
141-
- name: bbs-mysql
142-
env:
143-
DIR: src/code.cloudfoundry.org/bbs
144-
DB: mysql
145-
run: |
146-
repo/.github/helpers/test.bash ${{ github.workspace }} "$BUILD_IMAGE"
147141
- name: auctioneer-mysql
148142
env:
149143
DIR: src/code.cloudfoundry.org/auctioneer
@@ -162,12 +156,6 @@ jobs:
162156
DB: mysql
163157
run: |
164158
repo/.github/helpers/test.bash ${{ github.workspace }} "$BUILD_IMAGE"
165-
- name: locket-mysql
166-
env:
167-
DIR: src/code.cloudfoundry.org/locket
168-
DB: mysql
169-
run: |
170-
repo/.github/helpers/test.bash ${{ github.workspace }} "$BUILD_IMAGE"
171159
- name: cfdot-mysql
172160
env:
173161
DIR: src/code.cloudfoundry.org/cfdot
@@ -239,12 +227,6 @@ jobs:
239227
- name: build binaries
240228
run: |
241229
repo/.github/helpers/build.bash ${{ github.workspace }} "$BUILD_IMAGE"
242-
- name: bbs-postgres
243-
env:
244-
DIR: src/code.cloudfoundry.org/bbs
245-
DB: postgres
246-
run: |
247-
repo/.github/helpers/test.bash ${{ github.workspace }} "$BUILD_IMAGE"
248230
- name: auctioneer-postgres
249231
env:
250232
DIR: src/code.cloudfoundry.org/auctioneer
@@ -263,12 +245,6 @@ jobs:
263245
DB: postgres
264246
run: |
265247
repo/.github/helpers/test.bash ${{ github.workspace }} "$BUILD_IMAGE"
266-
- name: locket-postgres
267-
env:
268-
DIR: src/code.cloudfoundry.org/locket
269-
DB: postgres
270-
run: |
271-
repo/.github/helpers/test.bash ${{ github.workspace }} "$BUILD_IMAGE"
272248
- name: cfdot-postgres
273249
env:
274250
DIR: src/code.cloudfoundry.org/cfdot
@@ -296,12 +272,6 @@ jobs:
296272
- name: build binaries
297273
run: |
298274
repo/.github/helpers/build.bash ${{ github.workspace }} "$BUILD_IMAGE"
299-
- name: bbs-mysql
300-
env:
301-
DIR: src/code.cloudfoundry.org/bbs
302-
DB: mysql
303-
run: |
304-
repo/.github/helpers/test.bash ${{ github.workspace }} "$BUILD_IMAGE"
305275
- name: auctioneer-mysql
306276
env:
307277
DIR: src/code.cloudfoundry.org/auctioneer
@@ -320,12 +290,6 @@ jobs:
320290
DB: mysql
321291
run: |
322292
repo/.github/helpers/test.bash ${{ github.workspace }} "$BUILD_IMAGE"
323-
- name: locket-mysql
324-
env:
325-
DIR: src/code.cloudfoundry.org/locket
326-
DB: mysql
327-
run: |
328-
repo/.github/helpers/test.bash ${{ github.workspace }} "$BUILD_IMAGE"
329293
- name: cfdot-mysql
330294
env:
331295
DIR: src/code.cloudfoundry.org/cfdot

‎jobs/rep/spec‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -147,6 +147,9 @@ properties:
147147
diego.executor.max_cache_size_in_bytes:
148148
description: "maximum size of the cache in bytes - this should leave a healthy overhead for temporary items, etc."
149149
default: 10000000000
150+
diego.executor.min_cache_partition_free_bytes:
151+
description: "Minimum free space in bytes that must remain on the disk where the download cache lives. When actual disk free space falls below this threshold, the cache deletes old downloaded files to recover space. Set to 0 to disable this behavior."
152+
default: 5368709120
150153
diego.executor.use_schedulable_disk_size:
151154
description: "Use total space available to containers reported by Garden. If false the total size of image plugin store minus max_cache_size_in_bytes is used."
152155
default: false

‎jobs/rep/templates/rep.json.erb‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,8 @@
125125
raise "additional_memory_allocation_mb cannot be negative"
126126
end
127127

128+
config[:min_cache_partition_free_bytes] = p("diego.executor.min_cache_partition_free_bytes")
129+
128130
if_p("diego.executor.volman.driver_paths") do |value|
129131
config[:volman_driver_paths] = value
130132
end

‎jobs/rep_windows/spec‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,9 @@ properties:
129129
diego.executor.max_cache_size_in_bytes:
130130
description: "maximum size of the cache in bytes - this should leave a healthy overhead for temporary items, etc."
131131
default: 10000000000
132+
diego.executor.min_cache_partition_free_bytes:
133+
description: "EXPERIMENTAL: Additional memory allocated to each container for the envoy proxy. This must not be negative. Currently doesn't work on windows cells but left here for compatability with the linux Rep."
134+
default: 5368709120
132135
diego.executor.use_schedulable_disk_size:
133136
description: "Use total space available to containers reported by Garden. If false the total size of image plugin store minus max_cache_size_in_bytes is used."
134137
default: false

‎jobs/rep_windows/templates/rep.json.erb‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,8 @@
122122
raise "additional_memory_allocation_mb cannot be negative"
123123
end
124124

125+
config[:min_cache_partition_free_bytes] = p("diego.executor.min_cache_partition_free_bytes")
126+
125127
if_p("diego.executor.volman.driver_paths") do |value|
126128
config[:volman_driver_paths] = value
127129
end

‎spec/rep_template_spec.rb‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,17 @@
123123
expect(JSON.parse(rendered_template)['sidecar_root_fs']).to eq('cflinuxfs4')
124124
end
125125
end
126+
127+
context 'min_cache_partition_free_bytes' do
128+
it 'defaults to 5368709120 (5GB)' do
129+
expect(JSON.parse(rendered_template)['min_cache_partition_free_bytes']).to eq(5_368_709_120)
130+
end
131+
132+
it 'is configurable' do
133+
deployment_manifest_fragment['diego']['executor']['min_cache_partition_free_bytes'] = 1_073_741_824
134+
expect(JSON.parse(rendered_template)['min_cache_partition_free_bytes']).to eq(1_073_741_824)
135+
end
136+
end
126137
end
127138

128139
describe 'setup_mounted_data_dirs.erb' do

‎spec/rep_windows_template_spec.rb‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,5 +123,16 @@
123123
expect(JSON.parse(rendered_template)['sidecar_root_fs']).to eq('cflinuxfs4')
124124
end
125125
end
126+
127+
context 'min_cache_partition_free_bytes' do
128+
it 'defaults to 5368709120 (5GB)' do
129+
expect(JSON.parse(rendered_template)['min_cache_partition_free_bytes']).to eq(5_368_709_120)
130+
end
131+
132+
it 'is configurable' do
133+
deployment_manifest_fragment['diego']['executor']['min_cache_partition_free_bytes'] = 1_073_741_824
134+
expect(JSON.parse(rendered_template)['min_cache_partition_free_bytes']).to eq(1_073_741_824)
135+
end
136+
end
126137
end
127138
end

‎src/code.cloudfoundry.org/cacheddownloader/cached_downloader_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ var _ = Describe("File cache", func() {
6969

7070
transformer = cacheddownloader.NoopTransform
7171

72-
cache = cacheddownloader.NewCache(cachedPath, maxSizeInBytes)
72+
cache = cacheddownloader.NewCache(cachedPath, maxSizeInBytes, 0)
7373
downloader = cacheddownloader.NewDownloader(1*time.Second, MAX_CONCURRENT_DOWNLOADS, nil)
7474
cachedDownloader, err = cacheddownloader.New(downloader, cache, transformer)
7575
Expect(err).NotTo(HaveOccurred())
@@ -89,7 +89,7 @@ var _ = Describe("File cache", func() {
8989
Describe("When a new CachedDownloader fails to create a temp directory for the cache", func() {
9090
It("returns an error", func() {
9191
cachedPath = ""
92-
cache = cacheddownloader.NewCache(cachedPath, maxSizeInBytes)
92+
cache = cacheddownloader.NewCache(cachedPath, maxSizeInBytes, 0)
9393
downloader = cacheddownloader.NewDownloader(1*time.Second, MAX_CONCURRENT_DOWNLOADS, nil)
9494
cachedDownloader, err = cacheddownloader.New(downloader, cache, transformer)
9595
Expect(err).To(HaveOccurred())
@@ -1048,7 +1048,7 @@ var _ = Describe("File cache", func() {
10481048
ghttp.RespondWith(http.StatusOK, string(downloadContent), returnedHeader),
10491049
))
10501050

1051-
cache = cacheddownloader.NewCache(cachedPath, maxSizeInBytes)
1051+
cache = cacheddownloader.NewCache(cachedPath, maxSizeInBytes, 0)
10521052
cachedDownloader, err = cacheddownloader.New(downloader, cache, cacheddownloader.TarTransform)
10531053
Expect(err).ToNot(HaveOccurred())
10541054

@@ -1321,7 +1321,7 @@ var _ = Describe("File cache", func() {
13211321
Expect(cachedDownloader.SaveState(logger)).To(Succeed())
13221322

13231323
maxSizeInBytes = 32
1324-
cache = cacheddownloader.NewCache(cachedPath, maxSizeInBytes)
1324+
cache = cacheddownloader.NewCache(cachedPath, maxSizeInBytes, 0)
13251325
})
13261326

13271327
It("should evict old entries from the cache", func() {

‎src/code.cloudfoundry.org/cacheddownloader/file_cache.go‎

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,13 @@ var (
2323
)
2424

2525
type FileCache struct {
26-
CachedPath string
27-
maxSizeInBytes int64
28-
minFreeBytes int64
29-
Entries map[string]*FileCacheEntry
30-
OldEntries map[string]*FileCacheEntry
31-
Seq uint64
26+
CachedPath string
27+
maxSizeInBytes int64
28+
minimumPartitionFreeBytes int64
29+
FreeSpaceFunc func(path string) int64 `json:"-"`
30+
Entries map[string]*FileCacheEntry
31+
OldEntries map[string]*FileCacheEntry
32+
Seq uint64
3233
}
3334

3435
type FileCacheEntry struct {
@@ -41,18 +42,15 @@ type FileCacheEntry struct {
4142
fileInUseCount int
4243
}
4344

44-
func NewCache(dir string, maxSizeInBytes int64, minFreeBytes ...int64) *FileCache {
45-
var minFree int64
46-
if len(minFreeBytes) > 0 {
47-
minFree = minFreeBytes[0]
48-
}
45+
func NewCache(dir string, maxSizeInBytes, minimumPartitionFreeBytes int64) *FileCache {
4946
return &FileCache{
50-
CachedPath: dir,
51-
maxSizeInBytes: maxSizeInBytes,
52-
minFreeBytes: minFree,
53-
Entries: map[string]*FileCacheEntry{},
54-
OldEntries: map[string]*FileCacheEntry{},
55-
Seq: 0,
47+
CachedPath: dir,
48+
maxSizeInBytes: maxSizeInBytes,
49+
minimumPartitionFreeBytes: minimumPartitionFreeBytes,
50+
FreeSpaceFunc: defaultFreeSpaceOnPartition,
51+
Entries: map[string]*FileCacheEntry{},
52+
OldEntries: map[string]*FileCacheEntry{},
53+
Seq: 0,
5654
}
5755
}
5856

@@ -193,6 +191,11 @@ func (e *FileCacheEntry) expandedDirectory() (string, error) {
193191
err = os.RemoveAll(e.FilePath)
194192
if err != nil {
195193
fmt.Fprintln(os.Stderr, "Unable to delete the cached file", err)
194+
} else {
195+
// GetDirectory doubled Size before calling here; tar is now gone so halve it
196+
// back to reflect only the .tar.d on disk. Mirrors the symmetric halving in
197+
// decrementFileInUseCount() which fires when the tar is deleted later.
198+
e.Size = e.Size / 2
196199
}
197200
}
198201
}
@@ -384,7 +387,7 @@ func (c *FileCache) updateOldEntries(logger lager.Logger, cacheKey string, entry
384387

385388
func (c *FileCache) makeRoom(logger lager.Logger, size int64, excludedCacheKey string) {
386389
usedSpace := c.usedSpace(logger)
387-
for c.maxSizeInBytes < usedSpace+size {
390+
for c.maxSizeInBytes < usedSpace+size || (c.minimumPartitionFreeBytes > 0 && c.FreeSpaceFunc(c.CachedPath) < c.minimumPartitionFreeBytes) {
388391
var oldestEntry *FileCacheEntry
389392
oldestAccessTime, oldestCacheKey := maxTime(), ""
390393
for ck, f := range c.Entries {

0 commit comments

Comments
 (0)