CP-49147: Reduce size of the pool record (uefi_certificates)

[edwintorok]

This field is very big and part of every pool.get_all_records() call (done by SM), which is currently >64KiB in size.

TODO: the Changed field needs to match the (future!) tag that this will receive. Hence draft.

[lindig]

The idea is that very few clients will need the certificate and can use a new API call to obtain it. The existing field is replaced by a digest.

This seems fine with me as long as we control the relevant clients.

[lindig]

I would like to see this merged because UEFI takes up a lot of data in records.

[contificate]

I'd also like to see this change merged.

[contificate]

Upon a second look over this PR, I am concerned about the change to "internal only". My understanding is that internal only fields affect what is seen in snapshots.

The API records manipulated internally comprise every field, but my understanding is that DB actions (generated) for get_record etc. exclude those fields (since they're typically things like related session references).

We should change the meaning of the field, but not its visibility to API users (which I fear this PR may have done accidentally).

[robhoes]

I agree with @contificate. Making existing fields internal_only:true is similar to removing the field from the API and is backwards incompatible. It may also break rolling updates.

[edwintorok]

I'll need to find another method than internal_only:true to allow overriding get_. DynamicRO doesn't work either.
Might need to add another bool to field to allow overriding get.

[contificate]

I'll need to find another method than internal_only:true to allow overriding get_. DynamicRO doesn't work either. Might need to add another bool to field to allow overriding get.

A simpler semantics may be to make it so that overloading just works: if you specify a custom implementation, it will avoid endowing the object with implicitly-generated messages of the same name.

[robhoes]

Is this field is present on each host, with the same (large) content?

So we can't simply remove the pool field completely, as it will break many things (clients and server), even when clients are not interested in the field at all. A more limited way of breaking compatibility is to change the contents of a single field, as you have done by hashing the value. I think we should leave it at that, and not try to fake the API getter of the field, which would be inconsistent with what you get back from pool.get_record, event.from etc. We'll just have to take the hit of the change in semantics of that field.

If this makes it somehow hard for a client to get the right data, then we can still add a function to the pool class, but with a name that is different from the default getter.

[edwintorok]

w hard for a client to get the right data

We've deprecated the per-host field though, pointing to the Pool field instead, so it is a bit awkward, not sure whether you can change them per host anymore. In fact we raise an exception if you try to set it per host, pointing you to use 'Pool.set_custom_uefi_certificates instead'.
Pool.set_uefi_certificates is deprecated too, pointing to Pool.set_custom_uefi_certificates.

So actually one solution here would be to replace this field with the static value deprecated. If someone sets a custom uefi certificate that will be present in custom_uefi_certificates field, not this one.

And normally you wouldn't have custom uefi certificates set, and just use the defaults.

You can still query what the UEFI certificate actually is by quering the host object.

We need to be careful that internally we don't accidentally fetch the certificate from the pool (where we only have the digest) instead of the host (where we have the real value), otherwise the VM won't actually have something valid.
I've done that in Helpers.ml though.

[robhoes]

Then we should revert the deprecation status of host.uefi_certificates.

[robhoes]

Do you want to replace the contents of the field when updating the pool?

[edwintorok]

There was a bug here when the host certificate was empty, the pool certificate was still set to non-empty string, that should be fixed now.

I don't think we need any special upgrade code, because this seems to be called from the startup steps of XAPI on each host, so a toolstack restart suffices to get the new values.

[contificate]

Appears to have pulled in other commits, but the relevant one (ba202ba) seems like it ought to work.