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

cleanup: set pid limit only for nodeserver #3776

Merged
merged 2 commits into from Apr 25, 2023
Merged

Conversation

Madhu-1
Copy link
Collaborator

@Madhu-1 Madhu-1 commented Apr 25, 2023

setting pod limit only for node server for below reasons

  • We dont execute any commands with CLI anymore in the controller service
  • Controller deployment is not privileged enough to set the PID limits.

Added one more commit for doc cleanup

Signed-off-by: Madhu Rajanna madhupr007@gmail.com

@Madhu-1 Madhu-1 requested review from nixpanic and a team April 25, 2023 07:48
@mergify mergify bot added the cleanup label Apr 25, 2023
@Madhu-1 Madhu-1 force-pushed the cleanup branch 2 times, most recently from 063091f to 4932de3 Compare April 25, 2023 08:06
Rakshith-R

This comment was marked as duplicate.

Copy link
Contributor

@Rakshith-R Rakshith-R left a comment

Choose a reason for hiding this comment

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

tiny nit, everything else looks good to me.

// set pidLimit only for NodeServer
// the driver may need a higher PID limit for handling all concurrent requests
if conf.IsNodeServer && conf.PidLimit != 0 {
currentLimit, pidErr := util.GetPIDLimit()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
currentLimit, pidErr := util.GetPIDLimit()
_, pidErr := util.GetPIDLimit()

currentLimit variable is not used anywhere, let's remove it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

its used for logging.

@Madhu-1 Madhu-1 requested a review from Rakshith-R April 25, 2023 08:25
@nixpanic nixpanic added ci/skip/e2e skip running e2e CI jobs ok-to-test Label to trigger E2E tests labels Apr 25, 2023
@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.24

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.25

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.26

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.27

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.25

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.26

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.27

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.25

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.26

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.27

@github-actions
Copy link

/test ci/centos/upgrade-tests-cephfs

@github-actions
Copy link

/test ci/centos/upgrade-tests-rbd

@github-actions github-actions bot removed the ok-to-test Label to trigger E2E tests label Apr 25, 2023
we dont support imageFormat anymore in
cephcsi and default is set to 2, removing
its reference from the repo.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
setting pod limit only for nodeserver for below
reasons

* We dont execute any commands  with CLI
anymore in controller service
* Controller deployment is not privileged
enough to set the pid limits.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Apr 25, 2023
@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.24

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.25

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.26

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.27

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.25

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.26

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.27

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.25

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.26

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.27

@github-actions
Copy link

/test ci/centos/upgrade-tests-cephfs

@github-actions
Copy link

/test ci/centos/upgrade-tests-rbd

@github-actions github-actions bot removed the ok-to-test Label to trigger E2E tests label Apr 25, 2023
@mergify mergify bot merged commit b71beb7 into ceph:devel Apr 25, 2023
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/skip/e2e skip running e2e CI jobs cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants