Update downloader's Etag based on bundle activation #2286
Conversation
| @@ -129,8 +139,6 @@ func (d *Downloader) oneShot(ctx context.Context) error { | |||
| d.f(ctx, Update{ETag: etag, Bundle: b, Error: err, Metrics: m}) | |||
| } | |||
|
|
|||
| d.etag = etag | |||
There was a problem hiding this comment.
Just thinking out loud a little bit. I'm wondering if we are maybe leaking too much of the "downloader" details out by making the clients have to set the etag. From a high level I like that we can create a downloader and it will just like do the right thing with regards to cache controls. In this particular case we are kind of working around issues with bundle dependencies by manually setting them to trigger re-downloading and trying to re-activate the bundle.
I guess maybe what I'm leaning towards is having the downloader still automatically set the etag, but maybe just do it before calling the listener so they can clear it in their callback if they need to.
I'm wondering too if we maybe want to just have a like ClearCache() or Reset() or something a little bit more opaque instead of making the bundle and discovery plugins know that setting the etag to "" means it will download it again. I'm less sure about this direction though.. WDYT?
There was a problem hiding this comment.
@patrick-east I like the idea of setting the etag in downloader and clearing it in the client callback if some error occurs. I was thinking if the callback could be updated to return an error and then the downloader would set/reset the etag accordingly. But your approach seems good.
So in the downloader we would be adding a ClearCache() api that would basically set the etag to "" , correct ?
There was a problem hiding this comment.
@patrick-east updated the code as per the above discussion.
ashutosh-narkar
force-pushed
the
bundle-etag
branch
from
2f13e7b to
7668f41
Compare
plugins/bundle/plugin_test.go
Outdated
| @@ -1300,6 +1314,7 @@ func TestUpgradeLegacyBundleToMuiltiBundleNewBundles(t *testing.T) { | |||
|
|
|||
| b.Manifest.Init() | |||
|
|
|||
| //plugin.downloaders[bundleName] = download.New(downloadConf, plugin.manager.Client(serviceName), bundleName) | |||
There was a problem hiding this comment.
nit: Remove commented out line
download/download.go
Outdated
| func (d *Downloader) GetCache() string { | ||
| return d.etag | ||
| } | ||
|
|
There was a problem hiding this comment.
Looks like this API is only for testing, right? I'd maybe either remove it or rename to GetETag().
Being a public API on the golang API's I'm learning to be much more conservative with these. If later on we changed to have the downloader have more caching mechanisms than just an etag this API would need to change (ClearCache() is pretty safe, it can always do whatever it needs to make the downloader clear its cache and the caller doesn't need to know anything about how it was caching).
For the test cases in the bundle/discovery plugins we might want to use the http test server and just use the real mechanism to ensure we get a download the second time rather than add a public API to the downloader and check directly.
We can also easily test that in download_test.go where we already have the kind of test server setup, plus we can look at the private fields for the downloader to validate the etag state before/after clearing the cache (without the public API).
There was a problem hiding this comment.
Made couple of changes:
- Removed the
GetETag()public API. SoClearCache()is the only public API on the downloader - Updated the tests in
download_test.goto cover different etag caching scenarios
LMKWYT.
Earlier the Etag on the downloader would be updated unconditionally after every attempt to download a bundle. This could lead to a situation wherein a bundle fails to activate and would remain in an unactivated state since any subsequent downloads of the same version of the bundle would not trigger the activation process. This change attempts to resolve the issue by allowing the client to reset the Etag on the downloader incase of downloader errors and bundle activation failures. The drawback now is that we could end up re-downloading the same version of a bundle multiple times till it successfully activates. This situtation is likely to occur when using multiple bundle sources where a bundle may depend on some other. Generally using multiple bundle sources isn't recommended so the extra network traffic as a result of the re-downloads although not ideal may not too harmful. Fixes open-policy-agent#2220 Fixes open-policy-agent#2279 Signed-off-by: Ashutosh Narkar <[email protected]>
ashutosh-narkar
force-pushed
the
bundle-etag
branch
from
7668f41 to
78a6f18
Compare
This change updates how the Etag on the downloader object is set. See the commit message for more details.
Fixes #2220
Fixes #2279
Signed-off-by: Ashutosh Narkar [email protected]