kubeadm: keep dns resourceRequirements on upgrade

[maxl99]

What happened?

After an upgrade with kubeadm the resourceRequirements will be reset.
Currently the memory limit is hardcoded to 170M in the template.

In our big prod cluster (1000 nodes,5k services, 20k pods) coreDNS needs more than 170M.
So we increased the mem limit in the deployment after an outage (all coredns pods got OOM killed)
After kubeadm upgrade the deployment gets updated to 170M
-> Due to that all coredns pods rotate(during startup the probes are sucessful) and will get oomkilled after loading/caching all dns entries

What did you expect to happen?

keep resourceRequests after kubeadm upgrade

How can we reproduce it (as minimally and precisely as possible)?

1. change deployment (increase resourceRequests)
2. run kubeadm upgrade

Anything else we need to know?

No response

Kubernetes version

$ kubectl version
# paste output here

Cloud provider

OS version

# On Linux:
$ cat /etc/os-release
# paste output here
$ uname -a
# paste output here

# On Windows:
C:\> wmic os get Caption, Version, BuildNumber, OSArchitecture
# paste output here

Install tools

kubeadm

Container runtime (CRI) and version (if applicable)

Related plugins (CNI, CSI, ...) and versions (if applicable)

[k8s-ci-robot]

There are no sig labels on this issue. Please add an appropriate label by using one of the following commands:

• /sig <group-name>
• /wg <group-name>
• /committee <group-name>

Please see the group list for a listing of the SIGs, working groups, and committees available.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[neolit123]

/transfer kubeadm

[neolit123]

@maxl99 the unwritten kubeadm policy about coredns is the following, roughly - if you'd like customizations you can deploy your own coredns and manage it. that said we did add the feature to preserve the deployment replica count, so maybe we can also preserve other aspects of the existing deployment. but not such that would block the coredns migrator code (like "image" IIRC).

cc @pacoxu @SataQiu WDYT?

your PR comes 6 days before code freeze for 1.30 so i think it's a problem merging it without discussion:
kubernetes/kubernetes#123586

[neolit123]

/remove-kind bug
/kind feature
(not a bug)

[maxl99]

@neolit123 In my opinion it would be nice to preserve some specs in the deployment/read them from the existing k8s deplyoment. It is really bad to hardcode mem limits like in coredns (170M) without an option to change it.
This caused an outage in our main prod cluster...DNS always has a big impact :D
This would happen again and again with every "kubeadm upgrade"

[neolit123]

can we understand better what else can be preserved from the deployment and handle it all in a single pr?

[maxl99]

In our env we touched only the resourceRequirements and replicas(already handled by kubernetes/kubernetes#85837).
So for me it would be fine to do the same with the resourceRequirements.

But I imagine that there are also some other points for other people.

[SataQiu]

that said we did add the feature to preserve the deployment replica count, so maybe we can also preserve other aspects of the existing deployment. but not such that would block the coredns migrator code (like "image" IIRC).

IMO, if we supported every feature(replica、resource、label、annotation ...) this way, it would make the code difficult to maintain.
Perhaps we should support applying custom patches on addons in the v1beta4 API, even during the upgrade process.

[pacoxu]

Can we support kube-proxy and coredns patches like control plane components patches in https://kubernetes.io/docs/reference/config-api/kubeadm-config.v1beta3/#kubeadm-k8s-io-v1beta3-Patches?

BTW, can we skip dns upgrade like etcd?

      --etcd-upgrade                       Perform the upgrade of etcd. (default true)

[neolit123]

Can we support kube-proxy and coredns patches like control plane components patches in https://kubernetes.io/docs/reference/config-api/kubeadm-config.v1beta3/#kubeadm-k8s-io-v1beta3-Patches?
BTW, can we skip dns upgrade like etcd?

kube-proxy is a DS so it's controlled by a single config (which we create on init), arguably doesn't need patches.
the addons are skipped during upgrade if they are not found, we also have e2e for that. so users can use addons with different CM names today.

IMO, if we supported every feature(replica、resource、label、annotation ...) this way, it would make the code difficult to maintain.
Perhaps we should support applying custom patches on addons in the v1beta4 API, even during the upgrade process.

sadly our dns code is already a mess because of the coredns migration integration.
i was thinking that maybe a DeepCopy of the active Deployment + some edits will not be a lot of code.
generally agree that a patch might be cleaner and less code for us to own.

is "coredns" as a patch target OK, or should it be something like "corednsdeployment"?

@maxl99
we are discussing allowing users to patch coredns instead of auto preserving options
https://kubernetes.io/docs/setup/production-environment/tools/kubeadm/control-plane-flags/#patches

[neolit123]

@SataQiu @pacoxu

any concerns about adding this patch target?
1.31 will open soon and i think i can try finding time for it.

is "coredns" as a patch target OK, or should it be something like "corednsdeployment"?

best to be explicit about the object type corednsdeployment, because patches target a single object and this leaves room if later we want to add e.g. corednsservice.

[SataQiu]

With kubernetes/kubernetes#124820, you can configure coredns resource requirements in the following way:

$ mkdir patches
$ vim ./patches/corednsdeployment+strategic.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: coredns
  namespace: kube-system
spec:
  template:
    spec:
      containers:
      - name: coredns
        resources:
          limits:
            memory: 2Gi
            cpu: 2
          requests:
             memory: 1Gi
             cpu: 1
$ kubeadm upgrade apply <version> --patches ./patches

[neolit123]

closing as completed. 1.31 has the new feature to patch corednsdeployment