Only support compressed reads if the compression setting is present #8238
Conversation
|
I'd like most of the changes of this PR to be temporary: that is, eventually the compression setting should disappear and we assume any blob larger than 256 MiB must be a compressed blob. That change can happen however months in the future. |
3000 tests run: 2885 passed, 0 failed, 115 skipped (full report)Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
4220e2f at 2024-07-02T17:57:34.054Z :recycle: |
| if compression_bits > BYTE_UNCOMPRESSED { | ||
| warn!("reading key above future limit ({len} bytes)"); | ||
| } |
There was a problem hiding this comment.
Doesn't this mean that we are reading a previously compressed as uncompressed?
Shouldn't we also or alternatively check if the read bytes start with the zstd fourcc/magic?
There was a problem hiding this comment.
Anyway, I am having a hard time parsing this compression_bits. I get it that here it can be the u32 after anding it with 0x0f or 0x7f which means -- aha, it is never masked.
Related note: On line L332R341 the LEN_COMPRESSION_BIT_MASK is used as literal 0xf0:
-assert_eq!(len_buf[0] & 0xf0, 0);
+assert_eq!(len_buf[0] & LEN_COMPRESSION_BIT_MASK, 0);Ok... So perhaps I know understand. Possible compression_bits are:
match compression_bits >> 4 {
0 => /* image layer written before the compression support or small value? */,
1..8 => /* reserved */,
8 => /* uncompressed blob */,
9 => /* zstd */,
10..=15 => /* undefined or written before compression support and too large, which we warn here? */
_ => unreachable!("u4"),
}If this is correctly understood, then okay maybe ... The compression_bits > BYTE_COMPRESSED just looks so off, in my mind a bitfield doesn't support ordered comparison. It'd be nice to have enums and matches for these. Err nope that cannot be correct.
Did you test that this warning is produced with some hand-crafted image file?
EDIT: rust snippet had 1..8 and 10..=15 wrong way around, possibly.
There was a problem hiding this comment.
Doesn't this mean that we are reading a previously compressed as uncompressed?
If the compression setting is disabled, yes. This has the consequence that we can't turn off compression easily any more, but I think it's okay to have it for a while, after which point we'll (mostly) revert this PR.
It'd be nice to have enums and matches for these
match is not good for that as FOOBAR => is equivalent to a variable capture.
There was a problem hiding this comment.
Is my match correct? Did you test if this is hit? Will that be used as a success criteria for the compression support? If so, what is the plan to read all image layers?
Other questions remain, magic/fourcc instead of reserving more bits?
There was a problem hiding this comment.
Is my match correct?
almost correct, the 1..8 range doesn't have the highest bit set so it's an indicator for small uncompressed values.
match compression_bits >> 4 {
0..8 => /* small, uncompressed value below 128 bytes */,
8 => /* uncompressed blob */,
9 => /* zstd */,
10..=15 => /* reserved or written before compression support and too large, which we warn here */
_ => unreachable!("u4"),
}Will that be used as a success criteria for the compression support?
What do you mean, can you expand?
If so, what is the plan to read all image layers?
??
magic/fourcc instead of reserving more bits?
What do you mean by that? I do not think we should autodetect zstd magic here.
There was a problem hiding this comment.
Christian wants this to be merged as-is, doing that now.
There was a problem hiding this comment.
success criteria
plan to read all image layers
Christian mentioned these in #8238 (comment). I was wondering similarly what are the next steps.
magic/fourcc
What do you mean by that? I do not think we should autodetect zstd magic here.
zstd always starts the compressed bytes with the same 4 magic bytes. I was thinking if we should instead use that knowledge instead of awkwardly reserving bits, as I had no idea what was the plan for the next step.. But yeah, seems there is a plan after all.
There was a problem hiding this comment.
For future reference, the slack thread where I proposed to merge asap is https://neondb.slack.com/archives/C074JQU7NER/p1720021276202079
There was a problem hiding this comment.
side question: why is PageServerConf::image_compression an Option? Like with EvictionPolicy, a simple ImageCompressionAlgorithm::Disabled and been designed as the #[default].
To make deploying compression less dangerous, we therefore only assume a blob is compressed if the compression setting is present in the config.
This also means that we can't back out of compression once we enabled it.
I'm confused by this.
IMO once we have written the first compressed blob to S3, we'll have to support decompressing layers with compressed blobs into perpituity. The PageServerConf::image_compression conflig flag should only be controlling whether we write new blobs with compression or not. But not affect reading of exisitng blobs.
So, I guess I'm rejecting the fundamental idea behind this PR. Or I'm missing something.
That's what the option type is for, to indicate None. But of course it can be also inlined.
yes, this PR is only meant for a temporary period until compression is rolled out everywhere. at that point, I'd like to revert most of it. Of course, for the transition period, and maybe also beyond, it might make sense to have the ability to easily revert the config setting. The risk is that we might be reading 256 MiB large blobs in production right now, and on deploy of current main we yield corrupted data. This PR is just to avoid that situation. Maybe both of your suggestions could be combined, and one could change enum ImageCompressionAlgorithm {
DisabledNoDecompress,
Disabled,
Zstd { level: Option<i8> },
}
How does that sound? |
|
Ok, thanks for clarifying. I think it has to go like so:
But let's take that discussion into Slack. Regarding this PR, I now understand its importance and we should definitely merge it before the code from #8106 hits prod, because that would mean doing (4) before invariant (3) is established. |
As per @koivunej 's request in #8238 (comment) , use a runtime param instead of monomorphizing the function based on the value. Part of #5431
This flattens the compression algorithm setting, removing the `Option<_>` wrapping layer and making handling of the setting easier. It also adds a specific setting for *disabled* compression with the continued ability to read copmressed data, giving us the option to more easily back out of a compression rollout, should the need arise, which was one of the limitations of #8238. Implements my suggestion from #8238 (comment) , inspired by Christian's review in #8238 (review) . Part of #5431
…8238) PR #8106 was created with the assumption that no blob is larger than `256 MiB`. Due to #7852 we have checking for *writes* of blobs larger than that limit, but we didn't have checking for *reads* of such large blobs: in theory, we could be reading these blobs every day but we just don't happen to write the blobs for some reason. Therefore, we now add a warning for *reads* of such large blobs as well. To make deploying compression less dangerous, we therefore only assume a blob is compressed if the compression setting is present in the config. This also means that we can't back out of compression once we enabled it. Part of #5431
As per @koivunej 's request in #8238 (comment) , use a runtime param instead of monomorphizing the function based on the value. Part of #5431
This flattens the compression algorithm setting, removing the `Option<_>` wrapping layer and making handling of the setting easier. It also adds a specific setting for *disabled* compression with the continued ability to read copmressed data, giving us the option to more easily back out of a compression rollout, should the need arise, which was one of the limitations of #8238. Implements my suggestion from #8238 (comment) , inspired by Christian's review in #8238 (review) . Part of #5431
…8238) PR #8106 was created with the assumption that no blob is larger than `256 MiB`. Due to #7852 we have checking for *writes* of blobs larger than that limit, but we didn't have checking for *reads* of such large blobs: in theory, we could be reading these blobs every day but we just don't happen to write the blobs for some reason. Therefore, we now add a warning for *reads* of such large blobs as well. To make deploying compression less dangerous, we therefore only assume a blob is compressed if the compression setting is present in the config. This also means that we can't back out of compression once we enabled it. Part of #5431
As per @koivunej 's request in #8238 (comment) , use a runtime param instead of monomorphizing the function based on the value. Part of #5431
This flattens the compression algorithm setting, removing the `Option<_>` wrapping layer and making handling of the setting easier. It also adds a specific setting for *disabled* compression with the continued ability to read copmressed data, giving us the option to more easily back out of a compression rollout, should the need arise, which was one of the limitations of #8238. Implements my suggestion from #8238 (comment) , inspired by Christian's review in #8238 (review) . Part of #5431
…8238) PR #8106 was created with the assumption that no blob is larger than `256 MiB`. Due to #7852 we have checking for *writes* of blobs larger than that limit, but we didn't have checking for *reads* of such large blobs: in theory, we could be reading these blobs every day but we just don't happen to write the blobs for some reason. Therefore, we now add a warning for *reads* of such large blobs as well. To make deploying compression less dangerous, we therefore only assume a blob is compressed if the compression setting is present in the config. This also means that we can't back out of compression once we enabled it. Part of #5431
As per @koivunej 's request in #8238 (comment) , use a runtime param instead of monomorphizing the function based on the value. Part of #5431
This flattens the compression algorithm setting, removing the `Option<_>` wrapping layer and making handling of the setting easier. It also adds a specific setting for *disabled* compression with the continued ability to read copmressed data, giving us the option to more easily back out of a compression rollout, should the need arise, which was one of the limitations of #8238. Implements my suggestion from #8238 (comment) , inspired by Christian's review in #8238 (review) . Part of #5431
…8238) PR #8106 was created with the assumption that no blob is larger than `256 MiB`. Due to #7852 we have checking for *writes* of blobs larger than that limit, but we didn't have checking for *reads* of such large blobs: in theory, we could be reading these blobs every day but we just don't happen to write the blobs for some reason. Therefore, we now add a warning for *reads* of such large blobs as well. To make deploying compression less dangerous, we therefore only assume a blob is compressed if the compression setting is present in the config. This also means that we can't back out of compression once we enabled it. Part of #5431
As per @koivunej 's request in #8238 (comment) , use a runtime param instead of monomorphizing the function based on the value. Part of #5431
This flattens the compression algorithm setting, removing the `Option<_>` wrapping layer and making handling of the setting easier. It also adds a specific setting for *disabled* compression with the continued ability to read copmressed data, giving us the option to more easily back out of a compression rollout, should the need arise, which was one of the limitations of #8238. Implements my suggestion from #8238 (comment) , inspired by Christian's review in #8238 (review) . Part of #5431
…8238) PR #8106 was created with the assumption that no blob is larger than `256 MiB`. Due to #7852 we have checking for *writes* of blobs larger than that limit, but we didn't have checking for *reads* of such large blobs: in theory, we could be reading these blobs every day but we just don't happen to write the blobs for some reason. Therefore, we now add a warning for *reads* of such large blobs as well. To make deploying compression less dangerous, we therefore only assume a blob is compressed if the compression setting is present in the config. This also means that we can't back out of compression once we enabled it. Part of #5431
As per @koivunej 's request in #8238 (comment) , use a runtime param instead of monomorphizing the function based on the value. Part of #5431
This flattens the compression algorithm setting, removing the `Option<_>` wrapping layer and making handling of the setting easier. It also adds a specific setting for *disabled* compression with the continued ability to read copmressed data, giving us the option to more easily back out of a compression rollout, should the need arise, which was one of the limitations of #8238. Implements my suggestion from #8238 (comment) , inspired by Christian's review in #8238 (review) . Part of #5431
…8238) PR #8106 was created with the assumption that no blob is larger than `256 MiB`. Due to #7852 we have checking for *writes* of blobs larger than that limit, but we didn't have checking for *reads* of such large blobs: in theory, we could be reading these blobs every day but we just don't happen to write the blobs for some reason. Therefore, we now add a warning for *reads* of such large blobs as well. To make deploying compression less dangerous, we therefore only assume a blob is compressed if the compression setting is present in the config. This also means that we can't back out of compression once we enabled it. Part of #5431
As per @koivunej 's request in #8238 (comment) , use a runtime param instead of monomorphizing the function based on the value. Part of #5431
This flattens the compression algorithm setting, removing the `Option<_>` wrapping layer and making handling of the setting easier. It also adds a specific setting for *disabled* compression with the continued ability to read copmressed data, giving us the option to more easily back out of a compression rollout, should the need arise, which was one of the limitations of #8238. Implements my suggestion from #8238 (comment) , inspired by Christian's review in #8238 (review) . Part of #5431
Removes the `ImageCompressionAlgorithm::DisabledNoDecompress` variant. We now assume any blob with the specific bits set is actually a compressed blob. The `ImageCompressionAlgorithm::Disabled` variant still remains and is the new default. Reverts large parts of #8238 , as originally intended in that PR. Part of #5431
Removes the `ImageCompressionAlgorithm::DisabledNoDecompress` variant. We now assume any blob with the specific bits set is actually a compressed blob. The `ImageCompressionAlgorithm::Disabled` variant still remains and is the new default. Reverts large parts of #8238 , as originally intended in that PR. Part of #5431
PR #8106 was created with the assumption that no blob is larger than
256 MiB. Due to #7852 we have checking for writes of blobs larger than that limit, but we didn't have checking for reads of such large blobs: in theory, we could be reading these blobs every day but we just don't happen to write the blobs for some reason.Therefore, we now add a warning for reads of such large blobs as well.
To make deploying compression less dangerous, we therefore only assume a blob is compressed if the compression setting is present in the config. This also means that we can't back out of compression once we enabled it.
Part of #5431