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.

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

fix: set limitnofile #713

Merged
merged 4 commits into from
Mar 3, 2023
Merged

fix: set limitnofile #713

merged 4 commits into from
Mar 3, 2023

Conversation

faiq
Copy link
Collaborator

@faiq faiq commented Mar 2, 2023

What problem does this PR solve?:
https://d2iq.atlassian.net/browse/D2IQ-96107
This change in contained seems to cause problems for workloads that use disk containerd/containerd#4475
Which issue(s) does this PR fix?:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


@github-actions github-actions bot added the fix label Mar 2, 2023
@dkoshkin dkoshkin added the runs-e2e-tests runs e2e tests for GHA label Mar 2, 2023
@dlipovetsky
Copy link
Collaborator

When we upgraded containerd 1.4 to 1.6, the systemd unit changed the file descriptor limit from 1024768 to infinity. This PR effectively returns the limit to what it was in DKP 2.4 and earlier.

@faiq
Copy link
Collaborator Author

faiq commented Mar 3, 2023

Looking upstream this seems to be caused by this PR which sets the containerd limits to unlimited, users who were previously on 1.4.13 version of containerd provided by dkp 2.4 and previous had this limit set. Removing this limit caused our rook deployment to break and we also saw problem with HA proxy as well which had this limit as the root cause. We decided to bring this limit back for a couple of reasons

  1. We don’t know if this can break existing users applications
  2. It breaks our application

@faiq faiq removed the runs-e2e-tests runs e2e tests for GHA label Mar 3, 2023
@faiq faiq force-pushed the faiq/containerd-filelimit branch from bdc833d to d5b0c02 Compare March 3, 2023 15:41
@faiq faiq force-pushed the faiq/containerd-filelimit branch from 7a75cf5 to c2559a9 Compare March 3, 2023 16:03
@faiq faiq added the runs-e2e-tests runs e2e tests for GHA label Mar 3, 2023
@dkoshkin
Copy link
Contributor

dkoshkin commented Mar 3, 2023

@faiq I think we are good to merge this, the 2 other fixes look good:

@faiq faiq merged commit 6477937 into main Mar 3, 2023
@faiq faiq deleted the faiq/containerd-filelimit branch March 3, 2023 17:38
template:
dest: /etc/systemd/system/containerd.service.d/max-files.conf
src: etc/systemd/system/containerd.service.d/max-files.conf
mode: 0644
Copy link
Contributor

Choose a reason for hiding this comment

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

@faiq I started to rebase my molecule test PR #553

it turns out that this part fails with Ubuntu. This is when http_proxy or https_proxyis not set and the first task is not run due to that.

My suggestion:

Move this part after install includes and remove the conditions from directory creation

Copy link
Contributor

Choose a reason for hiding this comment

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

@faiq cherry-picking 137be77 should help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix runs-e2e-tests runs e2e tests for GHA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants