Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Enable k8s reserved cpus #3964

Merged
merged 2 commits into from
Jun 13, 2024
Merged

Enable k8s reserved cpus #3964

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
241b187
K8s kubelet reserved-cpus support
james-masson Jun 6, 2024
7cb2390
K8s kubelet reserved-cpus migration
james-masson Jun 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Release.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,4 +319,5 @@ version = "1.21.0"
"migrate_v1.21.0_pod-infra-container-image-remove-settings-generator.lz4",
"migrate_v1.21.0_pod-infra-container-image-affected-services.lz4",
"migrate_v1.21.0_pod-infra-container-image-services.lz4",
"migrate_v1.21.0_k8s-reserved-cpus-v0-1-0.lz4",
]
7 changes: 7 additions & 0 deletions packages/kubernetes-1.23/kubelet-config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,13 +87,17 @@ kubeReserved:
{{/if}}
{{/if}}
ephemeral-storage: "{{default "1Gi" settings.kubernetes.kube-reserved.ephemeral-storage}}"
{{#unless settings.kubernetes.reserved-cpus}}
kubeReservedCgroup: "/runtime"
{{/unless}}
{{#if settings.kubernetes.system-reserved}}
systemReserved:
{{#each settings.kubernetes.system-reserved}}
{{@key}}: "{{this}}"
{{/each}}
{{#unless settings.kubernetes.reserved-cpus}}
systemReservedCgroup: "/system"
{{/unless}}
{{/if}}
cpuCFSQuota: {{default true settings.kubernetes.cpu-cfs-quota-enforced}}
cpuManagerPolicy: {{default "none" settings.kubernetes.cpu-manager-policy}}
Expand Down Expand Up @@ -182,3 +186,6 @@ reservedMemory:
{{/each}}
{{/if}}
{{/if}}
{{#if settings.kubernetes.reserved-cpus}}
reservedSystemCPUs: {{settings.kubernetes.reserved-cpus}}
{{/if}}
7 changes: 7 additions & 0 deletions packages/kubernetes-1.24/kubelet-config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,13 +87,17 @@ kubeReserved:
{{/if}}
{{/if}}
ephemeral-storage: "{{default "1Gi" settings.kubernetes.kube-reserved.ephemeral-storage}}"
{{#unless settings.kubernetes.reserved-cpus}}
kubeReservedCgroup: "/runtime"
{{/unless}}
{{#if settings.kubernetes.system-reserved}}
systemReserved:
{{#each settings.kubernetes.system-reserved}}
{{@key}}: "{{this}}"
{{/each}}
{{#unless settings.kubernetes.reserved-cpus}}
systemReservedCgroup: "/system"
{{/unless}}
{{/if}}
cpuCFSQuota: {{default true settings.kubernetes.cpu-cfs-quota-enforced}}
cpuManagerPolicy: {{default "none" settings.kubernetes.cpu-manager-policy}}
Expand Down Expand Up @@ -181,3 +185,6 @@ reservedMemory:
{{/each}}
{{/if}}
{{/if}}
{{#if settings.kubernetes.reserved-cpus}}
reservedSystemCPUs: {{settings.kubernetes.reserved-cpus}}
{{/if}}
7 changes: 7 additions & 0 deletions packages/kubernetes-1.25/kubelet-config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,13 +87,17 @@ kubeReserved:
{{/if}}
{{/if}}
ephemeral-storage: "{{default "1Gi" settings.kubernetes.kube-reserved.ephemeral-storage}}"
{{#unless settings.kubernetes.reserved-cpus}}
kubeReservedCgroup: "/runtime"
{{/unless}}
{{#if settings.kubernetes.system-reserved}}
systemReserved:
{{#each settings.kubernetes.system-reserved}}
{{@key}}: "{{this}}"
{{/each}}
{{#unless settings.kubernetes.reserved-cpus}}
systemReservedCgroup: "/system"
{{/unless}}
{{/if}}
cpuCFSQuota: {{default true settings.kubernetes.cpu-cfs-quota-enforced}}
cpuManagerPolicy: {{default "none" settings.kubernetes.cpu-manager-policy}}
Expand Down Expand Up @@ -182,3 +186,6 @@ reservedMemory:
{{/each}}
{{/if}}
{{/if}}
{{#if settings.kubernetes.reserved-cpus}}
reservedSystemCPUs: {{settings.kubernetes.reserved-cpus}}
{{/if}}
7 changes: 7 additions & 0 deletions packages/kubernetes-1.26/kubelet-config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,13 +87,17 @@ kubeReserved:
{{/if}}
{{/if}}
ephemeral-storage: "{{default "1Gi" settings.kubernetes.kube-reserved.ephemeral-storage}}"
{{#unless settings.kubernetes.reserved-cpus}}
kubeReservedCgroup: "/runtime"
{{/unless}}
{{#if settings.kubernetes.system-reserved}}
systemReserved:
{{#each settings.kubernetes.system-reserved}}
{{@key}}: "{{this}}"
{{/each}}
{{#unless settings.kubernetes.reserved-cpus}}
systemReservedCgroup: "/system"
{{/unless}}
{{/if}}
cpuCFSQuota: {{default true settings.kubernetes.cpu-cfs-quota-enforced}}
cpuManagerPolicy: {{default "none" settings.kubernetes.cpu-manager-policy}}
Expand Down Expand Up @@ -182,3 +186,6 @@ reservedMemory:
{{/each}}
{{/if}}
{{/if}}
{{#if settings.kubernetes.reserved-cpus}}
reservedSystemCPUs: {{settings.kubernetes.reserved-cpus}}
{{/if}}
7 changes: 7 additions & 0 deletions packages/kubernetes-1.27/kubelet-config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,17 @@ kubeReserved:
{{/if}}
{{/if}}
ephemeral-storage: "{{default "1Gi" settings.kubernetes.kube-reserved.ephemeral-storage}}"
{{#unless settings.kubernetes.reserved-cpus}}
kubeReservedCgroup: "/runtime"
{{/unless}}
{{#if settings.kubernetes.system-reserved}}
systemReserved:
{{#each settings.kubernetes.system-reserved}}
{{@key}}: "{{this}}"
{{/each}}
{{#unless settings.kubernetes.reserved-cpus}}
systemReservedCgroup: "/system"
{{/unless}}
{{/if}}
cpuCFSQuota: {{default true settings.kubernetes.cpu-cfs-quota-enforced}}
cpuManagerPolicy: {{default "none" settings.kubernetes.cpu-manager-policy}}
Expand Down Expand Up @@ -183,3 +187,6 @@ reservedMemory:
{{/each}}
{{/if}}
{{/if}}
{{#if settings.kubernetes.reserved-cpus}}
reservedSystemCPUs: {{settings.kubernetes.reserved-cpus}}
{{/if}}
7 changes: 7 additions & 0 deletions packages/kubernetes-1.28/kubelet-config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,17 @@ kubeReserved:
{{/if}}
{{/if}}
ephemeral-storage: "{{default "1Gi" settings.kubernetes.kube-reserved.ephemeral-storage}}"
{{#unless settings.kubernetes.reserved-cpus}}
kubeReservedCgroup: "/runtime"
{{/unless}}
{{#if settings.kubernetes.system-reserved}}
systemReserved:
{{#each settings.kubernetes.system-reserved}}
{{@key}}: "{{this}}"
{{/each}}
{{#unless settings.kubernetes.reserved-cpus}}
systemReservedCgroup: "/system"
{{/unless}}
{{/if}}
cpuCFSQuota: {{default true settings.kubernetes.cpu-cfs-quota-enforced}}
cpuManagerPolicy: {{default "none" settings.kubernetes.cpu-manager-policy}}
Expand Down Expand Up @@ -181,3 +185,6 @@ reservedMemory:
{{/each}}
{{/if}}
{{/if}}
{{#if settings.kubernetes.reserved-cpus}}
reservedSystemCPUs: {{settings.kubernetes.reserved-cpus}}
{{/if}}
7 changes: 7 additions & 0 deletions packages/kubernetes-1.29/kubelet-config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,17 @@ kubeReserved:
{{/if}}
{{/if}}
ephemeral-storage: "{{default "1Gi" settings.kubernetes.kube-reserved.ephemeral-storage}}"
{{#unless settings.kubernetes.reserved-cpus}}
james-masson marked this conversation as resolved.
Show resolved Hide resolved
kubeReservedCgroup: "/runtime"
{{/unless}}
{{#if settings.kubernetes.system-reserved}}
systemReserved:
{{#each settings.kubernetes.system-reserved}}
{{@key}}: "{{this}}"
{{/each}}
{{#unless settings.kubernetes.reserved-cpus}}
systemReservedCgroup: "/system"
{{/unless}}
{{/if}}
cpuCFSQuota: {{default true settings.kubernetes.cpu-cfs-quota-enforced}}
cpuManagerPolicy: {{default "none" settings.kubernetes.cpu-manager-policy}}
Expand Down Expand Up @@ -181,3 +185,6 @@ reservedMemory:
{{/each}}
{{/if}}
{{/if}}
{{#if settings.kubernetes.reserved-cpus}}
reservedSystemCPUs: {{settings.kubernetes.reserved-cpus}}
{{/if}}
7 changes: 7 additions & 0 deletions packages/kubernetes-1.30/kubelet-config
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,13 +91,17 @@ kubeReserved:
{{/if}}
{{/if}}
ephemeral-storage: "{{default "1Gi" settings.kubernetes.kube-reserved.ephemeral-storage}}"
{{#unless settings.kubernetes.reserved-cpus}}
kubeReservedCgroup: "/runtime"
{{/unless}}
{{#if settings.kubernetes.system-reserved}}
systemReserved:
{{#each settings.kubernetes.system-reserved}}
{{@key}}: "{{this}}"
{{/each}}
{{#unless settings.kubernetes.reserved-cpus}}
systemReservedCgroup: "/system"
{{/unless}}
{{/if}}
cpuCFSQuota: {{default true settings.kubernetes.cpu-cfs-quota-enforced}}
cpuManagerPolicy: {{default "none" settings.kubernetes.cpu-manager-policy}}
Expand Down Expand Up @@ -181,3 +185,6 @@ reservedMemory:
{{/each}}
{{/if}}
{{/if}}
{{#if settings.kubernetes.reserved-cpus}}
reservedSystemCPUs: {{settings.kubernetes.reserved-cpus}}
{{/if}}
7 changes: 7 additions & 0 deletions sources/Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions sources/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ members = [
"api/migration/migrations/v1.21.0/pod-infra-container-image-affected-services",
"api/migration/migrations/v1.21.0/pod-infra-container-image-remove-settings-generator",
"api/migration/migrations/v1.21.0/pod-infra-container-image-services",
"api/migration/migrations/v1.21.0/k8s-reserved-cpus-v0-1-0",

"bloodhound",

Expand Down
10 changes: 10 additions & 0 deletions sources/api/migration/migrations/v1.21.0/k8s-reserved-cpus-v0-1-0/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
[package]
name = "k8s-reserved-cpus-v0-1-0"
version = "0.1.0"
authors = ["James Masson <[email protected]>"]
license = "Apache-2.0 OR MIT"
edition = "2021"
publish = false

[dependencies]
migration-helpers = { path = "../../../migration-helpers", version = "0.1.0" }
18 changes: 18 additions & 0 deletions sources/api/migration/migrations/v1.21.0/k8s-reserved-cpus-v0-1-0/src/main.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
use migration_helpers::common_migrations::AddSettingsMigration;
use migration_helpers::{migrate, Result};
use std::process;

/// Add the option to set Kubernetes reserved-cpus
fn run() -> Result<()> {
migrate(AddSettingsMigration(&["settings.kubernetes.reserved-cpus"]))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we need to limit this for k8s only? And have a conditional here to perform a Noop migration?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find any example of that for an "Add a new setting" migration.

I found

https://github.com/bottlerocket-os/bottlerocket/blob/1.20.x/sources/api/migration/migrations/v1.20.0/add-ntp-default-options-v0-1-0/src/main.rs

and

https://github.com/bottlerocket-os/bottlerocket/blob/1.19.x/sources/api/migration/migrations/v1.19.2/add-ecs-enable-container-metadata/src/main.rs

and

https://github.com/bottlerocket-os/bottlerocket/blob/1.19.x/sources/api/migration/migrations/v1.19.0/add-additional-ecs-settings/src/main.rs

all of which do the same as mine...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bottlerocket/sources/api/migration/migrations/archived/v1.15.0/oci-defaults-docker-setting/src/main.rs

Line 2 in a1bcca5

use migration_helpers::{migrate, Result};
is the inverse of what you need here.

Suggested change
migrate(AddSettingsMigration(&["settings.kubernetes.reserved-cpus"]))
if cfg!(variant_runtime = "k8s") {
migrate(AddSettingsMigration(&["settings.kubernetes.reserved-cpus"]))
} else {
migrate(NoOpMigration);
}

The example I linked to used ? but I don't think it is required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried a few conbinations of this and the other change you linked to.

I'm stuck on...

  #19 0.466 7  | /     if cfg!(variant_runtime = "k8s") {                                                                                                             
  #19 0.466 8  | |         migrate(AddSettingsMigration(&["settings.kubernetes.reserved-cpus"]))
  #19 0.466    | |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `()`, found `Result<(), Error>`                             
  #19 0.466 9  | |     } else {                                                                                                                                       
  #19 0.466 10 | |        migrate(NoOpMigration);                                                                                                                     
  #19 0.466 11 | |     }                                                                                                                                              
  #19 0.466    | |_____- expected this to be `()`                                                                                                                     
  #19 0.466    |
  #19 0.466    = note: expected unit type `()`
  #19 0.466                    found enum `Result<(), migration_helpers::error::Error>`
  #19 0.466 help: consider using a semicolon here
  #19 0.466    |
  #19 0.466 8  |         migrate(AddSettingsMigration(&["settings.kubernetes.reserved-cpus"]));
  #19 0.466    |                                                                              +
  #19 0.466 help: consider using a semicolon here
  #19 0.466    |

... any hints?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, sorry, there was a typo in my suggestion, there should not be a ; at the end of the first migration call to end the statement.

Suggested change
migrate(AddSettingsMigration(&["settings.kubernetes.reserved-cpus"]))
if cfg!(variant_runtime = "k8s") {
migrate(AddSettingsMigration(&["settings.kubernetes.reserved-cpus"]))
} else {
migrate(NoOpMigration)
}

The error is complaining about mixing and matching statements vs an expression. In this case, if you leave off the ; it will return the results of the function call, in this case migration() so want both statements to not have ; but then you also don't want Ok(()) at the end of your run() function. So the code I used to get this to compile looks like this for the entire file:

use migration_helpers::common_migrations::{AddSettingsMigration, NoOpMigration};
use migration_helpers::{migrate, Result};
use std::process;

/// Add the option to set Kubernetes reserved-cpus
fn run() -> Result<()> {
     if cfg!(variant_runtime = "k8s") {
         migrate(AddSettingsMigration(&["settings.kubernetes.reserved-cpus"]))
    } else {
        migrate(NoOpMigration)
    }
}

// Returning a Result from main makes it print a Debug representation of the error, but with Snafu
// we have nice Display representations of the error, so we wrap "main" (run) and print any error.
// https://github.com/shepmaster/snafu/issues/110
fn main() {
    if let Err(e) = run() {
        eprintln!("{}", e);
        process::exit(1);
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either this suggestion, or adding ?; and Ok(()) at the end should be enough.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This migration looks fine to me - it doesn't matter that aws-ecs-* doesn't have these settings, it'll just be a no-op on downgrade because the paths won't exist.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We only need a conditional migration in the case where settings are added in one version for some variants (aws-k8s-*) and then in a subsequent version for other variants (aws-ecs-*). These settings won't ever be added for aws-ecs-* so it's fine.

}

// Returning a Result from main makes it print a Debug representation of the error, but with Snafu
// we have nice Display representations of the error, so we wrap "main" (run) and print any error.
// https://github.com/shepmaster/snafu/issues/110
fn main() {
if let Err(e) = run() {
eprintln!("{}", e);
process::exit(1);
}
}
3 changes: 3 additions & 0 deletions sources/models/modeled-types/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,9 @@ pub mod error {
input: String,
source: std::num::ParseIntError,
},

#[snafu(display("Invalid Kernel CpuSet value '{}'", input))]
InvalidKernelCpuSetValue { input: String },
}

/// Creates a `ValidationError` with a consistent message for strings with regex validations
Expand Down
75 changes: 75 additions & 0 deletions sources/models/modeled-types/src/shared.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1178,3 +1178,78 @@ mod test_positive_integer {
assert!(NonNegativeInteger::try_from(-1).is_err());
}
}

// =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^= =^..^=

/// KernelCpuSetValue represents a string that contains a valid Kernel CpuSet Value from
/// here: https://man7.org/linux/man-pages/man7/cpuset.7.html#FORMATS. This matches the
/// logic from https://github.com/kubernetes/utils/blob/d93618cff8a22d3aea7bf78d9d528fd859720c2d/cpuset/cpuset.go#L203
#[derive(Debug, Clone, Eq, PartialEq, Hash)]
pub struct KernelCpuSetValue {
inner: String,
}

lazy_static! {
pub(crate) static ref KERNAL_CPU_SET_VALUE: Regex =
Regex::new(r"^([0-9]+(-[0-9]+)?,?)*([0-9]+(-[0-9]+)?)+$").unwrap();
}

impl TryFrom<&str> for KernelCpuSetValue {
type Error = error::Error;

fn try_from(input: &str) -> Result<Self, Self::Error> {
ensure!(
!input.is_empty(),
error::InvalidKernelCpuSetValueSnafu { input }
);
ensure!(
KERNAL_CPU_SET_VALUE.is_match(input),
error::InvalidKernelCpuSetValueSnafu { input }
);
Ok(KernelCpuSetValue {
inner: input.to_string(),
})
}
}

string_impls_for!(KernelCpuSetValue, "KernelCpuSetValue");

#[cfg(test)]
mod test_kernel_cpu_set_value {
use super::KernelCpuSetValue;
use std::convert::TryFrom;

#[test]
fn good_tokens() {
for ok in &[
"1",
"0,1,2,3",
"1-4",
"1,6-9",
"1-4,6",
"1-3,6-9,10-15",
"100,101",
"1,2,3,4,5,6,7,8,9,10",
] {
KernelCpuSetValue::try_from(*ok).unwrap();
}
}

#[test]
fn bad_names() {
for err in &[
"",
"100.0",
"0...4",
"1..5",
"ten",
"1-",
"-3",
"9,-10",
"1,2,3,4,5,6,7,8,9,10,",
&"a".repeat(23),
] {
KernelCpuSetValue::try_from(*err).unwrap_err();
}
}
}
15 changes: 8 additions & 7 deletions sources/models/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,13 +48,13 @@ use std::net::IpAddr;
use crate::de::{deserialize_limit, deserialize_mirrors, deserialize_node_taints};
use modeled_types::{
BootConfigKey, BootConfigValue, BootstrapContainerMode, CpuManagerPolicy, CredentialProvider,
DNSDomain, EtcHostsEntries, Identifier, IntegerPercent, KubernetesAuthenticationMode,
KubernetesBootstrapToken, KubernetesCloudProvider, KubernetesClusterDnsIp,
KubernetesClusterName, KubernetesDurationValue, KubernetesLabelKey, KubernetesLabelValue,
KubernetesQuantityValue, KubernetesReservedResourceKey, KubernetesTaintValue,
KubernetesThresholdValue, OciDefaultsCapability, OciDefaultsResourceLimitType,
PemCertificateString, SingleLineString, TopologyManagerPolicy, TopologyManagerScope, Url,
ValidBase64, ValidLinuxHostname,
DNSDomain, EtcHostsEntries, Identifier, IntegerPercent, KernelCpuSetValue,
KubernetesAuthenticationMode, KubernetesBootstrapToken, KubernetesCloudProvider,
KubernetesClusterDnsIp, KubernetesClusterName, KubernetesDurationValue, KubernetesLabelKey,
KubernetesLabelValue, KubernetesQuantityValue, KubernetesReservedResourceKey,
KubernetesTaintValue, KubernetesThresholdValue, OciDefaultsCapability,
OciDefaultsResourceLimitType, PemCertificateString, SingleLineString, TopologyManagerPolicy,
TopologyManagerScope, Url, ValidBase64, ValidLinuxHostname,
};

#[derive(Debug, Clone, PartialEq, Default, Serialize, Deserialize)]
Expand Down Expand Up @@ -137,6 +137,7 @@ struct KubernetesSettings {
shutdown_grace_period_for_critical_pods: KubernetesDurationValue,
memory_manager_reserved_memory: HashMap<Identifier, KubernetesMemoryReservation>,
memory_manager_policy: KubernetesMemoryManagerPolicy,
reserved_cpus: KernelCpuSetValue,

// Settings where we generate a value based on the runtime environment. The user can specify a
// value to override the generated one, but typically would not.
Expand Down