mholt
commented
5 months ago Ah, yeah, we do currently expect instance IDs to persist... that makes sense though and we should probably do something to manage this.
I think your suggestion makes the most sense. Thinking on it, it's probably what I would come up with. In fact, we kind of do this when reading for distributed rate limiting:
(We just ignore it, lol, maybe we should delete the file then.)
This reminds me, I need to get onto designing Storage v2... the current APIs have been very strained and limiting.
We have deployed Caddy, along with caddy-ratelimit and caddy-storage-redis, in a containerized environment, with Redis being our only permanent storage. As a result, each time the deployment scales up, the new Caddy processes pick new random instance IDs. We've noticed that the list of
rlStateentries in Redis have been adding up; in one case we have four thousand. The distributed rate limiting read loop has been using more and more CPU as this has grown, and profiling shows this is mainly being spent in the Redis driver and Gob decoding.I noticed that this plugin doesn't delete entries from storage. This storage growth could be fixed if
syncDistributedRead()also deleted entries whererlState.Timestampwas very old during its scan.In our case, it would be nice if we could make use of Redis's expiration feature to automatically delete old entries. However, the Storage interface doesn't have a place for extra metadata when storing a value, and caddy-storage-redis currently passes an expiration time of zero with all writes. Moreover, a deployment using ephemeral Caddy instances and an NFS storage backend would have the same issue with unbounded storage growth.