- Notifications
You must be signed in to change notification settings - Fork928
chore: don't cache errors in file cache#18555
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Changes fromall commits
780233b
8a6deb1
bf6562a
610740a
ec53459
df7acff
2ecb1d7
0fa1f6b
db89836
aab0335
c0ea08a
78dedaa
File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -5,6 +5,7 @@ import ( | ||||||||||||||||||||||||||||||||||||||||||||
"context" | ||||||||||||||||||||||||||||||||||||||||||||
"io/fs" | ||||||||||||||||||||||||||||||||||||||||||||
"sync" | ||||||||||||||||||||||||||||||||||||||||||||
"sync/atomic" | ||||||||||||||||||||||||||||||||||||||||||||
"github.com/google/uuid" | ||||||||||||||||||||||||||||||||||||||||||||
"github.com/prometheus/client_golang/prometheus" | ||||||||||||||||||||||||||||||||||||||||||||
@@ -25,60 +26,61 @@ type FileAcquirer interface { | ||||||||||||||||||||||||||||||||||||||||||||
// New returns a file cache that will fetch files from a database | ||||||||||||||||||||||||||||||||||||||||||||
func New(registerer prometheus.Registerer, authz rbac.Authorizer) *Cache { | ||||||||||||||||||||||||||||||||||||||||||||
return &Cache{ | ||||||||||||||||||||||||||||||||||||||||||||
lock: sync.Mutex{}, | ||||||||||||||||||||||||||||||||||||||||||||
data: make(map[uuid.UUID]*cacheEntry), | ||||||||||||||||||||||||||||||||||||||||||||
authz: authz, | ||||||||||||||||||||||||||||||||||||||||||||
cacheMetrics: newCacheMetrics(registerer), | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
funcnewCacheMetrics(registerer prometheus.Registerer)cacheMetrics { | ||||||||||||||||||||||||||||||||||||||||||||
aslilac marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||||||||||||||||||||||||||||||||||||||||||||
subsystem := "file_cache" | ||||||||||||||||||||||||||||||||||||||||||||
f := promauto.With(registerer) | ||||||||||||||||||||||||||||||||||||||||||||
return cacheMetrics{ | ||||||||||||||||||||||||||||||||||||||||||||
currentCacheSize: f.NewGauge(prometheus.GaugeOpts{ | ||||||||||||||||||||||||||||||||||||||||||||
Namespace: "coderd", | ||||||||||||||||||||||||||||||||||||||||||||
Subsystem: subsystem, | ||||||||||||||||||||||||||||||||||||||||||||
Name: "open_files_size_bytes_current", | ||||||||||||||||||||||||||||||||||||||||||||
Help: "The current amount of memory of all files currently open in the file cache.", | ||||||||||||||||||||||||||||||||||||||||||||
}), | ||||||||||||||||||||||||||||||||||||||||||||
totalCacheSize: f.NewCounter(prometheus.CounterOpts{ | ||||||||||||||||||||||||||||||||||||||||||||
Namespace: "coderd", | ||||||||||||||||||||||||||||||||||||||||||||
Subsystem: subsystem, | ||||||||||||||||||||||||||||||||||||||||||||
Name: "open_files_size_bytes_total", | ||||||||||||||||||||||||||||||||||||||||||||
Help: "The total amount of memory ever opened in the file cache. This number never decrements.", | ||||||||||||||||||||||||||||||||||||||||||||
}), | ||||||||||||||||||||||||||||||||||||||||||||
currentOpenFiles: f.NewGauge(prometheus.GaugeOpts{ | ||||||||||||||||||||||||||||||||||||||||||||
Namespace: "coderd", | ||||||||||||||||||||||||||||||||||||||||||||
Subsystem: subsystem, | ||||||||||||||||||||||||||||||||||||||||||||
Name: "open_files_current", | ||||||||||||||||||||||||||||||||||||||||||||
Help: "The count of unique files currently open in the file cache.", | ||||||||||||||||||||||||||||||||||||||||||||
}), | ||||||||||||||||||||||||||||||||||||||||||||
totalOpenedFiles: f.NewCounter(prometheus.CounterOpts{ | ||||||||||||||||||||||||||||||||||||||||||||
Namespace: "coderd", | ||||||||||||||||||||||||||||||||||||||||||||
Subsystem: subsystem, | ||||||||||||||||||||||||||||||||||||||||||||
Name: "open_files_total", | ||||||||||||||||||||||||||||||||||||||||||||
Help: "The total count of unique files ever opened in the file cache.", | ||||||||||||||||||||||||||||||||||||||||||||
}), | ||||||||||||||||||||||||||||||||||||||||||||
currentOpenFileReferences: f.NewGauge(prometheus.GaugeOpts{ | ||||||||||||||||||||||||||||||||||||||||||||
Namespace: "coderd", | ||||||||||||||||||||||||||||||||||||||||||||
Subsystem: subsystem, | ||||||||||||||||||||||||||||||||||||||||||||
Name: "open_file_refs_current", | ||||||||||||||||||||||||||||||||||||||||||||
Help: "The count of file references currently open in the file cache. Multiple references can be held for the same file.", | ||||||||||||||||||||||||||||||||||||||||||||
}), | ||||||||||||||||||||||||||||||||||||||||||||
totalOpenFileReferences: f.NewCounterVec(prometheus.CounterOpts{ | ||||||||||||||||||||||||||||||||||||||||||||
Namespace: "coderd", | ||||||||||||||||||||||||||||||||||||||||||||
Subsystem: subsystem, | ||||||||||||||||||||||||||||||||||||||||||||
Name: "open_file_refs_total", | ||||||||||||||||||||||||||||||||||||||||||||
Help: "The total number of file references ever opened in the file cache. The 'hit' label indicates if the file was loaded from the cache.", | ||||||||||||||||||||||||||||||||||||||||||||
}, []string{"hit"}), | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
// Cache persists the files for template versions, and is used by dynamic | ||||||||||||||||||||||||||||||||||||||||||||
@@ -106,18 +108,20 @@ type cacheMetrics struct { | ||||||||||||||||||||||||||||||||||||||||||||
totalCacheSize prometheus.Counter | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
type cacheEntry struct { | ||||||||||||||||||||||||||||||||||||||||||||
refCount atomic.Int32 | ||||||||||||||||||||||||||||||||||||||||||||
value *lazy.ValueWithError[CacheEntryValue] | ||||||||||||||||||||||||||||||||||||||||||||
close func() | ||||||||||||||||||||||||||||||||||||||||||||
purge func() | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
type CacheEntryValue struct { | ||||||||||||||||||||||||||||||||||||||||||||
fs.FS | ||||||||||||||||||||||||||||||||||||||||||||
Object rbac.Object | ||||||||||||||||||||||||||||||||||||||||||||
Size int64 | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
var _ fs.FS = (*CloseFS)(nil) | ||||||||||||||||||||||||||||||||||||||||||||
// CloseFS is a wrapper around fs.FS that implements io.Closer. The Close() | ||||||||||||||||||||||||||||||||||||||||||||
@@ -142,93 +146,121 @@ func (c *Cache) Acquire(ctx context.Context, db database.Store, fileID uuid.UUID | ||||||||||||||||||||||||||||||||||||||||||||
// mutex has been released, or we would continue to hold the lock until the | ||||||||||||||||||||||||||||||||||||||||||||
// entire file has been fetched, which may be slow, and would prevent other | ||||||||||||||||||||||||||||||||||||||||||||
// files from being fetched in parallel. | ||||||||||||||||||||||||||||||||||||||||||||
e := c.prepare(db, fileID) | ||||||||||||||||||||||||||||||||||||||||||||
ev, err := e.value.Load() | ||||||||||||||||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||
e.close() | ||||||||||||||||||||||||||||||||||||||||||||
e.purge() | ||||||||||||||||||||||||||||||||||||||||||||
return nil, err | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
// We always run the fetch under a system context and actor, so we need to check the caller's | ||||||||||||||||||||||||||||||||||||||||||||
// context manually before returning. | ||||||||||||||||||||||||||||||||||||||||||||
// Check if the caller's context was canceled | ||||||||||||||||||||||||||||||||||||||||||||
if err := ctx.Err(); err != nil { | ||||||||||||||||||||||||||||||||||||||||||||
e.close() | ||||||||||||||||||||||||||||||||||||||||||||
return nil, err | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
// Check that the caller is authorized to access the file | ||||||||||||||||||||||||||||||||||||||||||||
subject, ok := dbauthz.ActorFromContext(ctx) | ||||||||||||||||||||||||||||||||||||||||||||
if !ok { | ||||||||||||||||||||||||||||||||||||||||||||
e.close() | ||||||||||||||||||||||||||||||||||||||||||||
return nil, dbauthz.ErrNoActor | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
if err := c.authz.Authorize(ctx, subject, policy.ActionRead, ev.Object); err != nil { | ||||||||||||||||||||||||||||||||||||||||||||
e.close() | ||||||||||||||||||||||||||||||||||||||||||||
return nil, err | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines +172 to 174 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Do not change this, but I forgot something absolutely annoying about file authorizing. coder/coderd/database/dbauthz/dbauthz.go Lines 848 to 868 inaab0335
I really do not want to remedy this atm lol. We use a provisioner context for fetching things from the filecache so far. And is why we avoided it. MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I'm confused here. the comment calls the function | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
varcloseOnce sync.Once | ||||||||||||||||||||||||||||||||||||||||||||
return &CloseFS{ | ||||||||||||||||||||||||||||||||||||||||||||
FS:ev.FS, | ||||||||||||||||||||||||||||||||||||||||||||
close: func() { | ||||||||||||||||||||||||||||||||||||||||||||
// sync.Once makes the Close() idempotent, so we can call it | ||||||||||||||||||||||||||||||||||||||||||||
// multiple times without worrying about double-releasing. | ||||||||||||||||||||||||||||||||||||||||||||
closeOnce.Do(func() { | ||||||||||||||||||||||||||||||||||||||||||||
e.close() | ||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||
}, nil | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
func (c *Cache) prepare(db database.Store, fileID uuid.UUID) *cacheEntry { | ||||||||||||||||||||||||||||||||||||||||||||
c.lock.Lock() | ||||||||||||||||||||||||||||||||||||||||||||
defer c.lock.Unlock() | ||||||||||||||||||||||||||||||||||||||||||||
hitLabel := "true" | ||||||||||||||||||||||||||||||||||||||||||||
entry, ok := c.data[fileID] | ||||||||||||||||||||||||||||||||||||||||||||
if !ok { | ||||||||||||||||||||||||||||||||||||||||||||
hitLabel = "false" | ||||||||||||||||||||||||||||||||||||||||||||
var purgeOnce sync.Once | ||||||||||||||||||||||||||||||||||||||||||||
entry = &cacheEntry{ | ||||||||||||||||||||||||||||||||||||||||||||
value: lazy.NewWithError(func() (CacheEntryValue, error) { | ||||||||||||||||||||||||||||||||||||||||||||
val, err := fetch(db, fileID) | ||||||||||||||||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||
return val, err | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
// Add the size of the file to the cache size metrics. | ||||||||||||||||||||||||||||||||||||||||||||
c.currentCacheSize.Add(float64(val.Size)) | ||||||||||||||||||||||||||||||||||||||||||||
c.totalCacheSize.Add(float64(val.Size)) | ||||||||||||||||||||||||||||||||||||||||||||
return val, err | ||||||||||||||||||||||||||||||||||||||||||||
}), | ||||||||||||||||||||||||||||||||||||||||||||
close: func() { | ||||||||||||||||||||||||||||||||||||||||||||
entry.refCount.Add(-1) | ||||||||||||||||||||||||||||||||||||||||||||
c.currentOpenFileReferences.Dec() | ||||||||||||||||||||||||||||||||||||||||||||
// Safety: Another thread could grab a reference to this value between | ||||||||||||||||||||||||||||||||||||||||||||
// this check and entering `purge`, which will grab the cache lock. This | ||||||||||||||||||||||||||||||||||||||||||||
// is annoying, and may lead to temporary duplication of the file in | ||||||||||||||||||||||||||||||||||||||||||||
// memory, but is better than the deadlocking potential of other | ||||||||||||||||||||||||||||||||||||||||||||
// approaches we tried to solve this. | ||||||||||||||||||||||||||||||||||||||||||||
if entry.refCount.Load() > 0 { | ||||||||||||||||||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
entry.purge() | ||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||
purge: func() { | ||||||||||||||||||||||||||||||||||||||||||||
purgeOnce.Do(func() { | ||||||||||||||||||||||||||||||||||||||||||||
c.purge(fileID) | ||||||||||||||||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
c.data[fileID] = entry | ||||||||||||||||||||||||||||||||||||||||||||
c.currentOpenFiles.Inc() | ||||||||||||||||||||||||||||||||||||||||||||
c.totalOpenedFiles.Inc() | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
c.currentOpenFileReferences.Inc() | ||||||||||||||||||||||||||||||||||||||||||||
c.totalOpenFileReferences.WithLabelValues(hitLabel).Inc() | ||||||||||||||||||||||||||||||||||||||||||||
entry.refCount.Add(1) | ||||||||||||||||||||||||||||||||||||||||||||
return entry | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
// purge immediately removes an entry from the cache, even if it has open | ||||||||||||||||||||||||||||||||||||||||||||
// references. | ||||||||||||||||||||||||||||||||||||||||||||
func (c *Cache) purge(fileID uuid.UUID) { | ||||||||||||||||||||||||||||||||||||||||||||
c.lock.Lock() | ||||||||||||||||||||||||||||||||||||||||||||
defer c.lock.Unlock() | ||||||||||||||||||||||||||||||||||||||||||||
entry, ok := c.data[fileID] | ||||||||||||||||||||||||||||||||||||||||||||
if !ok { | ||||||||||||||||||||||||||||||||||||||||||||
Comment on lines 253 to 254 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. Can this ever happen? I like the defensive code, just wondering if the comment is accurate MemberAuthor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others.Learn more. I don't think it's possible now (god I hope it's not), but it could get messed up later and it would cause book keeping bugs if it continued past here. | ||||||||||||||||||||||||||||||||||||||||||||
// If we land here, it's probably because of a fetch attempt that | ||||||||||||||||||||||||||||||||||||||||||||
// resulted in an error, and got purged already. It may also be an | ||||||||||||||||||||||||||||||||||||||||||||
// erroneous extra close, but we can't really distinguish between those | ||||||||||||||||||||||||||||||||||||||||||||
// two cases currently. | ||||||||||||||||||||||||||||||||||||||||||||
return | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
// Purge the file from the cache. | ||||||||||||||||||||||||||||||||||||||||||||
c.currentOpenFiles.Dec() | ||||||||||||||||||||||||||||||||||||||||||||
ev, err := entry.value.Load() | ||||||||||||||||||||||||||||||||||||||||||||
if err == nil { | ||||||||||||||||||||||||||||||||||||||||||||
c.currentCacheSize.Add(-1 * float64(ev.Size)) | ||||||||||||||||||||||||||||||||||||||||||||
@@ -246,11 +278,18 @@ func (c *Cache) Count() int { | ||||||||||||||||||||||||||||||||||||||||||||
return len(c.data) | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
func fetch(store database.Store, fileID uuid.UUID) (CacheEntryValue, error) { | ||||||||||||||||||||||||||||||||||||||||||||
// Because many callers can be waiting on the same file fetch concurrently, we | ||||||||||||||||||||||||||||||||||||||||||||
// want to prevent any failures that would cause them all to receive errors | ||||||||||||||||||||||||||||||||||||||||||||
// because the caller who initiated the fetch would fail. | ||||||||||||||||||||||||||||||||||||||||||||
// - We always run the fetch with an uncancelable context, and then check | ||||||||||||||||||||||||||||||||||||||||||||
// context cancellation for each acquirer afterwards. | ||||||||||||||||||||||||||||||||||||||||||||
// - We always run the fetch as a system user, and then check authorization | ||||||||||||||||||||||||||||||||||||||||||||
// for each acquirer afterwards. | ||||||||||||||||||||||||||||||||||||||||||||
// This prevents a canceled context or an unauthorized user from "holding up | ||||||||||||||||||||||||||||||||||||||||||||
// the queue". | ||||||||||||||||||||||||||||||||||||||||||||
aslilac marked this conversation as resolved. Show resolvedHide resolvedUh oh!There was an error while loading.Please reload this page. | ||||||||||||||||||||||||||||||||||||||||||||
//nolint:gocritic | ||||||||||||||||||||||||||||||||||||||||||||
file, err := store.GetFileByID(dbauthz.AsFileReader(context.Background()), fileID) | ||||||||||||||||||||||||||||||||||||||||||||
if err != nil { | ||||||||||||||||||||||||||||||||||||||||||||
return CacheEntryValue{}, xerrors.Errorf("failed to read file from database: %w", err) | ||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.