Skip to content

Conversation

@marquiz
Copy link
Contributor

@marquiz marquiz commented Jul 24, 2023

Documentation for KEP-4033.

Document the KubeletCgroupDriverFromCRI feature gate. Also, add notes of this feature in parts of the documentation that describe cgroup driver configuration.

Refs:

NOTE: no container runtime supports this new feature, yet, so we cannot refer to specific versions of runtimes (yet)

NOTE: replaces/re-submits #42091 which was unwittingly submitted against the main branch

Document the KubeletCgroupDriverFromCRI feature gate. Also, add notes of
this feature in parts of the documentation that describe cgroup driver
configuration.
@k8s-ci-robot k8s-ci-robot added this to the 1.28 milestone Jul 24, 2023
@netlify
Copy link

netlify bot commented Jul 24, 2023

👷 Deploy Preview for kubernetes-io-vnext-staging processing.

Name Link
🔨 Latest commit fa73830
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-vnext-staging/deploys/64ccdbffbad5fd00083ee14f

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 24, 2023
@k8s-ci-robot k8s-ci-robot added the language/en Issues or PRs related to English language label Jul 24, 2023
@k8s-ci-robot k8s-ci-robot requested a review from vincepri July 24, 2023 07:49
@k8s-ci-robot k8s-ci-robot added sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 24, 2023
@sftim
Copy link
Contributor

sftim commented Jul 24, 2023

/sig node

@k8s-ci-robot k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Jul 24, 2023
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Thanks

I've added feedback to align this with our style guide

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 24, 2023
@marquiz marquiz force-pushed the devel/cgroup-driver-autoconfig-dev-1.28 branch from f32795c to b177d37 Compare July 24, 2023 19:10
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 24, 2023
@marquiz marquiz force-pushed the devel/cgroup-driver-autoconfig-dev-1.28 branch from b177d37 to 7e208b0 Compare July 24, 2023 19:12
@marquiz
Copy link
Contributor Author

marquiz commented Jul 24, 2023

I've added feedback to align this with our style guide

Thanks @sftim for the review. I incorporated the changes you suggested as a single commit.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

Looks good for docs.

Can we get a tech review from SIG Node?

@marquiz
Copy link
Contributor Author

marquiz commented Jul 25, 2023

Can we get a tech review from SIG Node?

/cc @mikebrow @haircommander

@marquiz
Copy link
Contributor Author

marquiz commented Jul 25, 2023

/cc mrunalp

@k8s-ci-robot k8s-ci-robot requested a review from mrunalp July 25, 2023 05:00
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

see comments, cheers

the `cgroupDriver` field under `KubeletConfiguration`, kubeadm defaults it to `systemd`.
{{< /note >}}

For Kubernetes v1.28 and later, with the `KubeletCgroupDriverFromCRI`
Copy link
Member

Choose a reason for hiding this comment

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

bit of a tldr issue I believe.. giving the instructions above for kubelet config driven and default if not configured.. then ending with 1.28 alpha feature gate info.

Suggest mentioning the feature gate above in direct association with the kubelet config steps / options. Something like unless the alpha level feature gate is set to automatically retrieve the appropriate cgroup driver from the runtime, additional configuration may be 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'm a bit cautious on this ("advertising" too much) as this is an alpha feature and there are no released container runtime supporting this, yet. When we get to beta, sure. WDYT @sftim

Copy link
Member

Choose a reason for hiding this comment

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

good point...

Copy link
Contributor

Choose a reason for hiding this comment

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

We should document how Kubernetes behaves, even if the compatible runtimes don't exist on release day.

When using kubeadm, manually configure the
[cgroup driver for kubelet](/docs/tasks/administer-cluster/kubeadm/configure-cgroup-driver/#configuring-the-kubelet-cgroup-driver).

Starting with v1.28 and later, you can enable automatic detection of the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Starting with v1.28 and later, you can enable automatic detection of the
Starting with Kubernetes v1.28 and later, you can enable automatic detection of the

Copy link
Contributor

Choose a reason for hiding this comment

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

Aside, but also a bit relevant:
I'm slightly wary of saying “and later” about alpha features that we can't prove will graduate to stable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, have to agree on that. I now changed all of these to In Kubernetes {{< skew currentVersion >}}...

ok @sftim @mikebrow ?

Copy link
Member

Choose a reason for hiding this comment

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

given the concern over skew "and later" comments perhaps.. for Kubernetes v1.2.8, you can..

cgroup driver configuration of the kubelet (usually done via kubeadm) and CRI-O
in sync.

Starting with v1.28 and later, you can enable automatic detection of the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Starting with v1.28 and later, you can enable automatic detection of the
Starting with Kubernetes v1.28 and later, you can enable automatic detection of the

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with In Kubernetes {{< skew currentVersion >}}

In v1.22 and later, if the user does not set the `cgroupDriver` field under `KubeletConfiguration`,
kubeadm defaults it to `systemd`.

Starting with v1.28 and later, you can enable automatic detection of the
Copy link
Member

Choose a reason for hiding this comment

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

see tldr comment

@sftim
Copy link
Contributor

sftim commented Jul 25, 2023

Wording looks fine for alpha, what this really needs now is tech signoff.

@mikebrow
Copy link
Member

Wording looks fine for alpha, what this really needs now is tech signoff.

to that end for containerd .. I'm reviewing @marquiz impl in containerd containerd/containerd#8722, would like to see that merged before providing my signoff. Cheers!

the `cgroupDriver` field under `KubeletConfiguration`, kubeadm defaults it to `systemd`.
{{< /note >}}

In Kubernetes {{< skew currentVersion >}}, with the `KubeletCgroupDriverFromCRI`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suppose we are in v1.29 now, the above line will be rendered into something like
"In Kubernetes 1.29, with the KubeletCgroupDriverFromCRI".
Is this statement still appropriate?
Suppose we are in v1.34 now and the feature gate has been GA'ed and removed,
the above line will be rendered into something like
"In Kubernetes 1.34, with the KubeletCgroupDriverFromCRI".
Is this statement still appropriate?

Clearly stating that this feature is in Alpha starting 1.28 is good enough.
Why do we use the skew shortcode the make this release number a moving target?

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'm indifferent on this, can change it to v1.29. This (In Kubernetes {{< skew currentVersion >}}) was something that @sftim suggested in one of his earlier comments so I took it from there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suppose we are in v1.34 now and the feature gate has been GA'ed and removed,

When we change Kubernetes, we should remove the docs. If we don't trust ourselves to do that, I would focus on fixing that lack of trust.

If we could trust the release docs process, I think this change would be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As a contributor I'm inclined to agree with @sftim about the docs process here. Especially with a KEP involved, the process should ensure that the documentation is changed when the feature is moved to beta and later GA (or removed before graduating further). WDYT @tengqm?

Copy link
Contributor

Choose a reason for hiding this comment

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

Per #42160 (comment), let's follow the path of least risk to the release, at least for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

K, I changed this now to In Kubernetes v1.28, ... (not to Starting with v1.28 ... as per #42160 (comment))

PTAL @sftim @tengqm

When using kubeadm, manually configure the
[cgroup driver for kubelet](/docs/tasks/administer-cluster/kubeadm/configure-cgroup-driver/#configuring-the-kubelet-cgroup-driver).

In Kubernetes {{< skew currentVersion >}}, you can enable automatic detection of the
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

In addition to the above confusion, using this skew shortcode makes it difficult
to remove contents that are no longer relevant in the future.

cgroup driver configuration of the kubelet (usually done via kubeadm) and CRI-O
in sync.

In Kubernetes {{< skew currentVersion >}}, you can enable automatic detection of the
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

In v1.22 and later, if the user does not set the `cgroupDriver` field under `KubeletConfiguration`,
kubeadm defaults it to `systemd`.

In Kubernetes {{< skew currentVersion >}}, you can enable automatic detection of the
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here.

@marquiz
Copy link
Contributor Author

marquiz commented Aug 4, 2023

ping @mikebrow the containerd implementation (containerd/containerd#8722) is now merged

ping @sftim what's your take on using {{< skew currentVersion >}} (#42160 (comment))?

@sftim
Copy link
Contributor

sftim commented Aug 4, 2023

ping @sftim what's your take on using {{< skew currentVersion >}} (#42160 (comment))?

I very much like like timeless documentation, but I also like having the docs ready for release. I'm reluctantly OK with not using that shortcode if it unblocks approval. After the release, I could propose putting it back.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

DetailsGit tree hash: 81a58ecd287120b77b79111f9812be9dff4008cd

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sftim

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 5, 2023
@k8s-ci-robot k8s-ci-robot merged commit 8478392 into kubernetes:dev-1.28 Aug 5, 2023
@marquiz marquiz deleted the devel/cgroup-driver-autoconfig-dev-1.28 branch August 7, 2023 05:22
Rishit-dagli pushed a commit to Rishit-dagli/website that referenced this pull request Aug 12, 2023
…autoconfig-dev-1.28

docs: document kubelet cgroup driver detection from the runtime
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/en Issues or PRs related to English language lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

Development

Successfully merging this pull request may close these issues.

5 participants