Change root_maxkeys#23182
Conversation
There was a problem hiding this comment.
@crosbymichael I don't like this, because we're changing global distribution policy. There's almost certainly some side-effects -- I'd prefer if we made the keyring joining optional (as you mentioned in opencontainers/runc#818).
There was a problem hiding this comment.
optional but default so we still need something like this .
There was a problem hiding this comment.
Basically the kernel shipped with the wrong policy and fixed it, but old distros won't change, so we should just fix it.
|
@justincormack what do you think about doing this change? |
|
I wonder if the rpm or deb for the affected distros could have a file in
|
|
@justincormack Changing max file descriptors isn't a global option. The root key limit is a global option. We'd probably be fine with packaging an |
|
Yes true, it is overriding policy set for our process though because we are
|
|
We change global network options at boot. |
|
What is a better alternative then? I'm not sure i see it being too big of a problem because other distros are shipping with these limits, it seems like something is just being overlooked. What if we only bump with from 200 to something like 1024 and provide a flag on the daemon to change it?
I don't think going from an overlooked 200 to 1k-2k is not going to destroy the world, no one even knew this existed until we turned on the feature and containers were capped. |
|
Alternatively, we could output a warning when running |
|
Yes, lets just do this, the other options are worse. SGTM. |
There was a problem hiding this comment.
missing a space "at least"
crosbymichael
force-pushed
the
maxkeys
branch
2 times, most recently
from
8463943 to
1ce7c58
Compare
|
Modified this to also update the maxbytes for the root keys because you need both. The multiplier is a common 25 bytes per key |
|
I think we can move this to code review as i was the only person unsure and I changed my mind... |
|
LGTM when green |
|
|
|
looks good to me |
There was a problem hiding this comment.
nit: s/rootKeyByteMultiplyer/rootKeyByteMultiplier/ ?
|
one nit, but LGTM otherwise |
Most modern distros have the limit for the maximum root keys at 1000000 but some do not. Because we are creating a new key for each container we need to bump this up as the older distros are having this limit at 200. Using 1000000 as the limit because that is that most distros are setting this to now. If someone has this value configured over that we do not change it. Signed-off-by: Michael Crosby <[email protected]>
|
@thaJeztah fixed |
thaJeztah
added
status/4-merge
process/cherry-pick
and removed
status/2-code-review
labels
|
experimental is actually 💚 (that false reporting should be fixed by #24002) |
|
you're right, let's merge |
8 participants
Fixes #22865
Most modern distros have the limit for the maximum root keys at 1000000
but some do not. Because we are creating a new key for each container
we need to bump this up as the older distros are having this limit at
200.
Using 1000000 as the limit because that is that most distros are setting
this to now. If someone has this value configured over that we do not
change it.
Signed-off-by: Michael Crosby [email protected]