cli: add SNP ID block annotations to Pods based on CPU requirements#2214
cli: add SNP ID block annotations to Pods based on CPU requirements#2214daniel-weisse wants to merge 6 commits intomainfrom
Conversation
2287b59 to
6de728d
Compare
3acb557 to
6fd606e
Compare
0304e90 to
f63b942
Compare
f63b942 to
59f9a72
Compare
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
Signed-off-by: Daniel Weiße <dw@edgeless.systems>
59f9a72 to
cf6cec9
Compare
msanft
left a comment
There was a problem hiding this comment.
I think this will work, but I'm a little unsure about the current interface we expose to the user.
| podLevelCPU := getCPUCount(spec.Resources) | ||
|
|
||
| // Convert milliCPUs to number of CPUs (rounding up), and add 1 for hypervisor overhead | ||
| totalMilliCPUs := max(regularContainersCPU, initContainersCPU, podLevelCPU) |
There was a problem hiding this comment.
I wonder if this matches the user's expectations, or what's done by non-Kata Kubernetes here.
There was a problem hiding this comment.
What do you think may be unexpected about this formula? I pointed @daniel-weisse to #2272 for where it comes from.
There was a problem hiding this comment.
The thing I was wary about is the round-up. With cgroups and CPU slices, this isn't something to worry about. But when a user shifts some YAML that worked in his non-Contrast deployment to Contrast, we may try to use more CPUs than physically available due to this. I don't think this is something that would be a realistic scenario, though. LMK
There was a problem hiding this comment.
Understood, thanks. We'll need to document this in https://docs.edgeless.systems/contrast/howto/workload-deployment/deployment-file-preparation#pod-resources before we consider this feature done, yes. I don't see what we could do to not round up, though, since fractional CPUs don't make sense for VMs.
There was a problem hiding this comment.
Scheduler considerations might become interesting, though: I don't think there's a way to tell k8s via runtimeClass to round up the limits.
There was a problem hiding this comment.
Do you have a concrete idea on how to proceed with this? I don't see what we could do either.
There was a problem hiding this comment.
Just document it, recommeding only integral CPU counts. If rounding does not change the number, there are no problems with unexpected counts or scheduling. But if the user decides to go against that recommendation, this code still does the right thing.
| ] | ||
| ++ [ | ||
| "panic=1" | ||
| "nr_cpus=1" |
There was a problem hiding this comment.
Should we just set this to the maximum value of 8 statically?
As per the docs, this is:
Maximum number of processors that an SMP kernel could support
We could also just omit this, as this number can also be resolved dynamically: https://elixir.bootlin.com/linux/v6.19.8/source/kernel/cpu.c#L3153-L3166
There was a problem hiding this comment.
Yes, there's a ticket for removing the param / Markus mentioned this as well yesterday. Thomas at one point suggested setting this to 32.
8 seems a bit low. I'm not super sure if the memory usage increase from removing the limit is a noticeable problem. I'm voting "remove".
There was a problem hiding this comment.
From what I read in the docs, this setting is only relevant for VMs that might have CPUs hot-plugged during runtime, which is not the case for us.
There was a problem hiding this comment.
No, but apparently setting this to a constant < the Kernel max will save some memory 🤷🏼♀️
There was a problem hiding this comment.
Yeah, I think this is only relevant for memory pre-reservation. Perhaps also for hot-plugging, which we don't need to support anyway.
There was a problem hiding this comment.
nr_cpus= [SMP] Maximum number of processors that an SMP kernel
could support. nr_cpus=n : n >= 1 limits the kernel to
support 'n' processors. It could be larger than the
number of already plugged CPU during bootup, later in
runtime you can physically add extra cpu until it reaches
n. So during boot up some boot time memory for per-cpu
variables need be pre-allocated for later physical cpu
hot plugging.
I read this as "the kernel does the right thing for the number of CPUs plugged at boot; if you plan on plugging more later, set this to the number you're aiming for."
There was a problem hiding this comment.
Right, that is also how I understood this, with the caveat that not setting this defaults to the kconfig value of CONFIG_NR_CPUS, which in our case is currently 240. Hence the increased memory pre-reservation if the line is removed altogether.
Not arguing against removing this though, as I said above, I'm for it.
There was a problem hiding this comment.
I read this as "the kernel does the right thing for the number of CPUs plugged at boot; if you plan on plugging more later, set this to the number you're aiming for."
"n >= 1 limits the kernel to support 'n' processors". If you don't set it, you will be able to hot-plug up to CONFIG_NR_CPUS as Charlotte said. And this requires reservation of some memory.
Since we already limit this to 240 (I didn't know that when I commented 32 in the ticket), it should be okay.
Not specifying `nr_cpus` on the command line costs us marginal amounts of memory while saving complexity in the TDX RTMR pre-calculation. By dropping this from the command line, we make the kernel fall back to the `CONFIG_NR_CPUS=240` kconfig variable.
|
@burgerdev, @charludo; Addressed my own feedback, PTAL. |
charludo
left a comment
There was a problem hiding this comment.
fixup changes LGTM; have not looked into the still-open conversation.
contrast generatewith the ID blocks required for the requested CPU amount