Skip to content

Conversation

koooosh
Copy link
Contributor

@koooosh koooosh commented Sep 24, 2025

Issue number:

Related to: #3788

Description of changes:

Add migration for pid setting under kube-reserved and system-reserved settings:

Testing done:

Migration testing:

  • Pre-upgrade (expected failure)
apiclient set -j '{
 "settings": {
    "kubernetes": {
      "kube-reserved": {
        "pid": "9999",
        "cpu": "10m",
        "ephemeral-storage": "1Gi", 
        "memory": "100Mi"
      },
      "system-reserved": {
        "pid": "1000",
        "cpu": "10m",
        "ephemeral-storage": "1Gi", 
        "memory": "100Mi"
      }
    }
  }
}'

=> Failed to change settings: Failed PATCH request to '/settings?tx=apiclient-set-tK69YYxBmTQgmkyl': Status 400 when PATCHing /settings?tx=apiclient-set-tK69YYxBmTQgmkyl: Json deserialize error: Unable to deserialize into KubernetesReservedResourceKey: Invalid input for field Reserved sources key: unknown variant `pid`, expected one of `cpu`, `memory`, `ephemeral-storage` at line 1 column 37
  • Upgrade
[ssm-user@control]$ apiclient get settings.kubernetes.kube-reserved
{
  "settings": {
    "kubernetes": {
      "kube-reserved": {
        "cpu": "10m",
        "ephemeral-storage": "1Gi",
        "memory": "100Mi",
        "pid": "9999"
      }
    }
  }
}
[ssm-user@control]$ apiclient get settings.kubernetes.system-reserved
{
  "settings": {
    "kubernetes": {
      "system-reserved": {
        "cpu": "10m",
        "ephemeral-storage": "1Gi",
        "memory": "100Mi",
        "pid": "1000"
      }
    }
  }
}
 
=> See the following configs in /etc/kubernetes/kubelet/config where pid is added
kubeReserved:
  cpu: "10m"
  memory: "100Mi"
  ephemeral-storage: "1Gi"
  pid: "9999"
systemReserved:
  cpu: "10m"
  ephemeral-storage: "1Gi"
  memory: "100Mi"
  pid: "1000"
  • Downgrade
[ssm-user@control]$ apiclient get settings.kubernetes.system-reserved
{
  "settings": {
    "kubernetes": {
      "system-reserved": {
        "cpu": "10m",
        "ephemeral-storage": "1Gi",
        "memory": "100Mi"
      }
    }
  }
}
[ssm-user@control]$ apiclient get settings.kubernetes.kube-reserved
{
  "settings": {
    "kubernetes": {
      "kube-reserved": {
        "cpu": "10m",
        "ephemeral-storage": "1Gi",
        "memory": "100Mi"
      }
    }
  }
}

=> See the following configs in /etc/kubernetes/kubelet/config where pid is removed
kubeReserved:
  cpu: "10m"
  memory: "100Mi"
  ephemeral-storage: "1Gi"
systemReserved:
  cpu: "10m"
  ephemeral-storage: "1Gi"
  memory: "100Mi"

Terms of contribution:

By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.

@koooosh koooosh force-pushed the add-reserve-pid-migration branch from df9c12c to cd1d10d Compare September 24, 2025 20:45
@koooosh
Copy link
Contributor Author

koooosh commented Sep 24, 2025

^ Force push fixes a clippy error


// We added new kubernetes settings to reserve pids for kubernetes and system components.
fn run() -> Result<()> {
migrate(AddPrefixesMigration(vec![
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add context (maybe to the comment) why we are using AddPrefixesMigration vs the usual AddSettingsMigration for a setting.

I know it was called out here:
bottlerocket-os/bottlerocket-settings-sdk#98 (review)

But I am curious on the behavior.

Copy link
Contributor Author

@koooosh koooosh Oct 3, 2025

Choose a reason for hiding this comment

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

Thanks for the callout! I reran migration testing using AddSettingsMigration instead and it worked -- the results were the same.

Although AddPrefixesMigration technically works, I believe AddSettingsMigration is the correct helper here since the pid setting is supposed to be set to a single value (so not a prefix for other settings). If pid had it's own map, then AddPrefixesMigration would be the right choice. I've fixed this in the latest push.

@koooosh koooosh force-pushed the add-reserve-pid-migration branch from cd1d10d to 6e71b07 Compare October 3, 2025 08:14
@koooosh
Copy link
Contributor Author

koooosh commented Oct 3, 2025

^ Force push replaces AddPrefixesMigration helper to AddSettingsMigration

@koooosh koooosh requested a review from KCSesh October 3, 2025 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants