Skip to content

[crio_swap1g.ign]: Use cgroup v2 instead of v1 #30242

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
Merged
merged 2 commits into from
Aug 7, 2023
Merged

[crio_swap1g.ign]: Use cgroup v2 instead of v1 #30242

merged 2 commits into from
Aug 7, 2023

Conversation

iholder101
Copy link
Contributor

@iholder101 iholder101 commented Jul 30, 2023 •
edited
Loading

Since in Beta1 NodeSwap does not support cgroup v1, the jobs using crio_swap1g.ign should be using cgroup v2. See kubernetes/kubernetes#118764 for more info.

This PR is a pre-requisite for kubernetes/kubernetes#119327, which aims to fail kubelet when swap is being used with cgroup v1. As of now, jobs like pull-kubernetes-node-swap-fedora, which use crio_swap1g.ign, fail instantly with this PR since they use cgroup v1. With this PR merged, I wish all tests would pass with the above PR.

Fixes kubernetes/kubernetes#119726

Here's what I did in order to make sure all of the jobs that use swap will now be set with cgroup v2.

First, from this repo's root, I execute:

> grep -l "NodeSwap=true" . -R
./config/jobs/kubernetes/sig-node/containerd.yaml
./config/jobs/kubernetes/sig-node/node-kubelet.yaml
./config/jobs/kubernetes/sig-node/sig-node-presubmit.yaml

These files define jobs with the NodeSwap feature-gate on. For each such job, I've inspected the file that's used as a parameter for --node-args. For example:

- --node-args=--image-config-file=/workspace/test-infra/jobs/e2e_node/swap/image-config-swap-fedora.yaml

This file refers to other files, such as crio_swap1g.ign. I've tried to search this files for cgoup definitions. In crio_swap1g.ign's example, I saw:

  "kernelArguments": {
    "shouldExist": [
      "systemd.unified_cgroup_hierarchy=0"
    ]
  },

which is deleted by this PR.

As far as I'm aware, the affected jobs are:

  • pull-kubernetes-node-swap-fedora
  • pull-kubernetes-node-swap-fedora-serial

@k8s-ci-robot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jul 30, 2023
@iholder101 iholder101 changed the title [crio_swap1g.ign]: Use cgroup v2 instead of v1 [WIP] [crio_swap1g.ign]: Use cgroup v2 instead of v1 Jul 30, 2023
@iholder101 iholder101 mentioned this pull request Aug 2, 2023
Closed
2 tasks
@iholder101 iholder101 changed the title [WIP] [crio_swap1g.ign]: Use cgroup v2 instead of v1 [crio_swap1g.ign]: Use cgroup v2 instead of v1 Aug 2, 2023
@iholder101 iholder101 marked this pull request as ready for review August 2, 2023 08:07
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 2, 2023
@k8s-ci-robot k8s-ci-robot requested a review from pacoxu August 2, 2023 08:07
@iholder101
Copy link
Contributor Author

/cc @harche

@k8s-ci-robot k8s-ci-robot requested a review from harche August 2, 2023 08:08
@harche
Copy link
Contributor

harche commented Aug 2, 2023

@iholder101 Thanks for this PR. The ignition files cannot be modified directly. They get auto generated from the templates. Check out https://github.com/kubernetes/test-infra/blob/6e07bfd40c34b051012306635cb139412db1f249/jobs/e2e_node/crio/README.md for more info.

@iholder101
Copy link
Contributor Author

@iholder101 Thanks for this PR. The ignition files cannot be modified directly. They get auto generated from the templates. Check out https://github.com/kubernetes/test-infra/blob/6e07bfd40c34b051012306635cb139412db1f249/jobs/e2e_node/crio/README.md for more info.

Oh, good to know :)
Thanks for directing me to the right place.

I'll make this PR a draft until I make the changes.

@iholder101 iholder101 marked this pull request as draft August 2, 2023 15:44
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 2, 2023
@iholder101
crio_swap1g.ign: use cgroup v2 instead of v1
8466f0b 
Since Beta1 NodeSwap does not support cgroup v1,
therefore the jobs using crio_swap1g.ign should
use cgroup v2

Signed-off-by: Itamar Holder <[email protected]>
@iholder101
auto-generate crio ignition file
3b8e848 
Signed-off-by: Itamar Holder <[email protected]>
@iholder101 iholder101 force-pushed the crio_swap1g/set-cgroupv2 branch from 7e38185 to 3b8e848 Compare August 3, 2023 17:59
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 3, 2023
@iholder101
Copy link
Contributor Author

/test all

@harche
Copy link
Contributor

harche commented Aug 3, 2023

cc @saschagrunert for changes in the templates.

@iholder101 iholder101 marked this pull request as ready for review August 3, 2023 18:07
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 3, 2023
@k8s-ci-robot k8s-ci-robot requested a review from sjenning August 3, 2023 18:07
@iholder101
Copy link
Contributor Author

@harche I think it's done correctly this time. Thanks again for pointing me to the right place.
PTAL

@iholder101
Copy link
Contributor Author

/cc @saschagrunert

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: iholder101, saschagrunert

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

The pull request process is described here

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 7, 2023
@k8s-ci-robot k8s-ci-robot merged commit 1f78017 into kubernetes:master Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saschagrunert saschagrunert saschagrunert approved these changes

@ConnorDoyle ConnorDoyle Awaiting requested review from ConnorDoyle

@endocrimes endocrimes Awaiting requested review from endocrimes

@pacoxu pacoxu Awaiting requested review from pacoxu

@harche harche Awaiting requested review from harche

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@sjenning sjenning Awaiting requested review from sjenning

Assignees

@saschagrunert saschagrunert

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. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
SIG Node CI/Test Board
Archived in project
SIG Node: code and documentation PRs
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adapt swap jobs to run with cgroup v2
4 participants
@iholder101 @k8s-ci-robot @harche @saschagrunert