Skip to content
This repository has been archived by the owner on Oct 24, 2023. It is now read-only.

chore: disable --pod-max-pids by default #1126

Merged
merged 1 commit into from
Apr 23, 2019

Conversation

jackfrancis
Copy link
Member

Reason for Change:

--pod-max-pids configuration became enabled without --feature-gates opt-in starting in 1.14. Because there are lots of valid workflows that fork a bunch of processes, we want to be careful about setting an upper bound by default. This PR disables the upper limit to be permissive for such workloads.

Issue Fixed:

Fixes #1055

Requirements:

Notes:

@@ -269,7 +269,7 @@ Below is a list of kubelet options that aks-engine will configure by default:
| "--image-gc-low-threshold" | "850" |
| "--non-masquerade-cidr" | "10.0.0.0/8" |
| "--azure-container-registry-config" | "/etc/kubernetes/azure.json" |
| "--pod-max-pids" | "100" (need to activate the feature in --feature-gates=SupportPodPidsLimit=true) |
| "--pod-max-pids" | "-1" (need to activate the feature in --feature-gates=SupportPodPidsLimit=true) |
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 track an issue to make this a sensible default as a follow up item (-1 doesn't really make sense as a default value).

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, but that's what the work-in-progress docs say: kubernetes/website#13806

Copy link
Contributor

Choose a reason for hiding this comment

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

I also asked for clarification on what 0 means. If it disables the feature, then I think we may actually want 0 instead of -1 (node enforceable limit) if we don't set a node enforceable limit today.

Copy link
Contributor

@CecileRobertMichon CecileRobertMichon left a comment

Choose a reason for hiding this comment

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

lgtm

@codecov
Copy link

codecov bot commented Apr 23, 2019

Codecov Report

Merging #1126 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1126   +/-   ##
======================================
  Coverage    74.4%   74.4%           
======================================
  Files         131     131           
  Lines       18286   18286           
======================================
  Hits        13606   13606           
  Misses       3901    3901           
  Partials      779     779

Copy link
Member

@mboersma mboersma left a comment

Choose a reason for hiding this comment

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

/lgtm

@acs-bot
Copy link

acs-bot commented Apr 23, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jackfrancis, mboersma

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:
  • OWNERS [jackfrancis,mboersma]

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

@acs-bot acs-bot merged commit 33f1fa2 into Azure:master Apr 23, 2019
mboersma pushed a commit that referenced this pull request Apr 30, 2019
# Conflicts:
#	pkg/engine/testdata/key-vault-certs/kubernetes.json
@Sahasrara
Copy link

How do I get this change applied to my clusters?

@jackfrancis
Copy link
Member Author

@Sahasrara upgrading your cluster (aks-engine upgrade) using v0.38.0 or later of aks-engine will apply these changes to a pre-existing cluster.

@jackfrancis jackfrancis deleted the max-pods-pids branch July 22, 2019 16:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SupportPodPidsLimit becoming a beta feature breaks some 1.14 deployments
6 participants