Add API function TryUpdatingCache

[saschagrunert]

The function can be used to reload the registries without harming the
cache on failure. On success, the cache will be updated as intended.
This function can be reused by the current GetRegistries API.

[vrothberg]

This seems redundant to invalidating the cache and then calling GetRegistries() (minus the error case). Is there a concrete use case for that? I prefer keeping the API small but if there's a non-hypothetical use case, I would not block.

[saschagrunert]

This seems redundant to invalidating the cache and then calling GetRegistries() (minus the error case). Is there a concrete use case for that? I prefer keeping the API small but if there's a non-hypothetical use case, I would not block.

My use case is that I only want to override the cache on successful parsing of the configuration, not in every case.

[TomSweeneyRedHat]

nit; s/memoized/memorized/ or perhaps "added"

[mtrmac]

https://en.wikipedia.org/wiki/Memoization ; though maybe that indicates that using simpler words would be easier to understand.

[saschagrunert]

Yes, that's what I meant, but I changed to to added.

[TomSweeneyRedHat]

LGTM, will let @vrothberg and @mtrmac decide if the function should be added or not.

[vrothberg]

My use case is that I only want to override the cache on successful parsing of the configuration, not in every case.

Who would be the consumer of the API?

[saschagrunert]

My use case is that I only want to override the cache on successful parsing of the configuration, not in every case.

Who would be the consumer of the API?

The CRI-O live config reload feature. The thing is that I don't want to destroy the cache unless the config is valid.

[vrothberg]

Thinking about it, I guess you're pointing to CRI-O's reload function where it could make sense. If @mtrmac is on board, I would prefer to add a parameter to GetRegistries(..) (e.g., updateCache bool). With the next release of c/image, consumers of sysregistriesv2 are very likely to be broken (s/URL/Location and friends).

[vrothberg]

... [to be broken], so we could add the parameter.

[mtrmac]

Thinking about it, I guess you're pointing to CRI-O's reload function where it could make sense. If @mtrmac is on board, I would prefer to add a parameter to GetRegistries(..) (e.g., updateCache bool).

Neither of the two APIs match how I imagine the callers work (nor how I see CRI-O working today).

Most callers should never call GetRegistries; they would (have c/image/docker implicitly) call FindRegistry; what value of updateCache should FindRegistry use for GetRegistries(updateCache)?

For a daemon, I imagine there’s a SIGHUP or similar mechanism to reload configuration (or, perhaps, “reload before starting to process every request, but not every single time while processing it); in that case “reload” is what the caller wants, “reload and return the results” seems like pointless extra return value.

Alternatively, I could imagine a “never cache, reload all the time if the file is valid” configuration — besides being a bit risky, that looks like an extra bit in SystemContext, so that it applies to all the implicit FindRegistry calls as well.

What precisely is the caller doing and what does it want?

[saschagrunert]

For a daemon, I imagine there’s a SIGHUP or similar mechanism to reload configuration (or, perhaps, “reload before starting to process every request, but not every single time while processing it); in that case “reload” is what the caller wants, “reload and return the results” seems like pointless extra return value.

This API would be for the configuration reload of registries.conf during CRI-O runtime. This works by listening on SIGHUP as you already mentioned. My initial approach was to call InvalidateCache(), but this API does not return any result and immediately destroys the cache. Afterwards I would have to call GetRegistries(), which is currently the only function which repopulates the cache and loads the config from the TOML file. For debugging purposes I would give the user the feedback that the registries are verified and which registries were found. As a user I would like to see that my newly configured registry has been added successfully.

What precisely is the caller doing and what does it want?

The caller mainly wants to recreate the registries cache, but only on successful parsing of registries.conf. Beside this, the function should return a meaningful error and the result of the registry cache repopulation.

[vrothberg]

This API would be for the configuration reload of registries.conf during CRI-O runtime. This works by listening on SIGHUP as you already mentioned. My initial approach was to call InvalidateCache(), but this API does not return any result and immediately destroys the cache. Afterwards I would have to call GetRegistries(), which is currently the only function which repopulates the cache and loads the config from the TOML file. For debugging purposes I would give the user the feedback that the registries are verified and which registries were found. As a user I would like to see that my newly configured registry has been added successfully.

That's pretty much how the API is intended to be used.

The caller mainly wants to recreate the registries cache, but only on successful parsing of registries.conf.

That is the key difference to the current API but the question remains why that is needed instead of using the current API? Could an empty cache lead to issues?

In case of CRI-O, I can imagine that the SIGHUP reload aims at try-reload semantics, meaning that the data will only be updated if it's valid and without error to avoid race-conditions.

[saschagrunert]

That is the key difference to the current API but the question remains why that is needed instead of using the current API? Could an empty cache lead to issues?

Yes, I think so. Lets imagine the following scenario:

1. Kubernetes pulls images via CRI-O, whereas the cache of registries.conf is populated and everything works as intended.
2. The admin changes registries.conf, all background pulls are fine because the cache serves the registries
3. The admin sends SIGHUP to CRI-O.

• If we now invalidate the cache without pre-checking the configuration, then all pulls from that point in time are failing if the configuration is invalid.
• If we pre-check the configuration and only invalidate the cache on successful validation, then everything should be fine.

In case of CRI-O, I can imagine that the SIGHUP reload aims at try-reload semantics, meaning that the data will only be updated if it's valid and without error to avoid race-conditions.

Yes, that's exactly the case. I really want to be sure that everything works before changing the internal application state. For the log_level it works the same way right now:
https://github.com/cri-o/cri-o/blob/0c50025d5f0351ae9acbc1fc46d1afd38c25f953/server/config.go#L218-L232

[mtrmac]

For a daemon, I imagine there’s a SIGHUP or similar mechanism to reload configuration (or, perhaps, “reload before starting to process every request, but not every single time while processing it); in that case “reload” is what the caller wants, “reload and return the results” seems like pointless extra return value.

This API would be for the configuration reload of registries.conf during CRI-O runtime. This works by listening on SIGHUP as you already mentioned. My initial approach was to call InvalidateCache(), but this API does not return any result and immediately destroys the cache. Afterwards I would have to call GetRegistries(), which is currently the only function which repopulates the cache and loads the config from the TOML file. For debugging purposes I would give the user the feedback that the registries are verified and which registries were found. As a user I would like to see that my newly configured registry has been added successfully.

So, maybe: TryReloadingConfiguration (TryRefreshingCache? whatever, naming is hard) which either reloads the configuration and succeeds (and if we wanted to complicate things significantly, would return also an indication whether the configuration has changed[1]); or keeps the current cached configuration, if any, and fails.

On success, the code can optionally call GetRegistries to see what the changed configuration is, and log that.

---

[1] This is not something an external caller can quite trivially do, because adding an extra member to a structure does not break the API/ABI, but it can invalidate external code which tries to compare two Registry values.

[mtrmac]

A further thought — do we even want InvalidateCache to exist at all, or should there only be that Try… API, which preserves the previous cache on failure?

I can see an argument for actually strictly&reliably failing if the configuration is currently invalid, instead of continuing to use some hard-to-describe old configuration and hiding the problem. It’s not very strong, though.

I suppose keeping InvalidateCache around doesn’t hurt, so we can default to keeping it, and give callers both options.

[saschagrunert]

So, maybe: TryReloadingConfiguration (TryRefreshingCache? whatever, naming is hard) which either reloads the configuration and succeeds (...); or keeps the current cached configuration, if any, and fails.

Yeah, that's what the current implementaion already does, right? Or should I remove the []Registries from the return parameters? (From my POV this would over complicate things and double the work done internally.)

I suppose keeping InvalidateCache around doesn’t hurt, so we can default to keeping it, and give callers both options.

I agree and would not remove it.

[vrothberg]

Yeah, that's what the current implementaion already does, right? Or should I remove the []Registries from the return parameters? (From my POV this would over complicate things and double the work done internally.)

I'd go with a TryUpdatingCache(..) ([]Registries, error). Callers can still ignore the first return value if they don't need it.

I also agree with keeping InvalidateCache(). It doesn't hurt to keep it around.

[saschagrunert]

I'd go with a TryUpdatingCache(..) ([]Registries, error). Callers can still ignore the first return value if they don't need it.

Renamed the function according to your suggestion.

[vrothberg]

LGTM but needs a rebase

[saschagrunert]

Rebased on top of the latest master branch.

[vrothberg]

@mrunalp @mtrmac PTAL

[rhatdan]

@saschagrunert Are you still interested in this, can you rebase?

[saschagrunert]

@saschagrunert Are you still interested in this, can you rebase?

Yes, rebased on top of the latest master branch

[rhatdan]

@mtrmac @vrothberg PTAL

[vrothberg]

LGTM

[mtrmac]

ACK on the API (though note the V2RegistriesConf comment); I’m afraid the implementation needs more updates for the interim code changes.

[saschagrunert]

The CI needs #670 to succeed.

[saschagrunert]

Rebased.

[vrothberg]

LGTM, let's get this into the v3.0.0 release.

[vrothberg]

Green, pulling the trigger. Thanks @saschagrunert 👍