Add --concurrent-ephemeralvolume-syncs flag for kube-controller-manager #102981

SataQiu · 2021-06-18T08:41:40Z

What type of PR is this?

/kind feature
/kind api-change

What this PR does / why we need it:

While many other controllers like deployment have flag --concurrent-xxx-syncs to set the number of workers, the ephemeral controller doesn't and just hardcoded with 1 worker. It is very slow to deal with a large number of ephemeral volumes in the cluster. This PR exports a flag --concurrent-ephemeralvolume-syncs with the default value of 1 for the ephemeral controller.

Which issue(s) this PR fixes:

xref #79169

Special notes for your reviewer:

Does this PR introduce a user-facing change?

kube-controller-manager supports '--concurrent-ephemeralvolume-syncs' flag to set the number of ephemeral volume controller workers.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

fejta-bot · 2021-06-18T09:42:09Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

SataQiu · 2021-06-18T10:08:28Z

/label api-review

SataQiu · 2021-06-18T10:40:06Z

/test pull-kubernetes-e2e-gce-ubuntu-containerd

fedebongio · 2021-06-22T20:05:30Z

/triage accepted

msau42 · 2021-07-08T21:05:35Z

/assign @pohly @jsafrane

lavalamp · 2021-07-08T23:40:51Z

can someone from sig apps or storage review first?

JamesLaverack · 2021-07-09T02:42:58Z

/milestone clear

pohly · 2021-07-09T06:44:54Z

cmd/kube-controller-manager/app/core.go

@@ -401,8 +401,7 @@ func startEphemeralVolumeController(ctx ControllerContext) (http.Handler, bool,
 		if err != nil {
 			return nil, true, fmt.Errorf("failed to start ephemeral volume controller: %v", err)
 		}
-		// TODO (before beta at the latest): make this configurable similar to the EndpointController
-		go ephemeralController.Run(1 /* int(ctx.ComponentConfig.EphemeralController.ConcurrentEphemeralVolumeSyncs) */, ctx.Stop)
+		go ephemeralController.Run(int(ctx.ComponentConfig.EphemeralController.ConcurrentEphemeralVolumeSyncs), ctx.Stop)
 		return nil, true, nil


Thanks for taking care of this TODO. It never made it into tracking list of opens for the feature and then of course was promptly forgotten - my bad.

Making it configurable makes sense. The rest seems to be mostly cut-and-paste from how it is done elsewhere and looks good to me.

/lgtm

jsafrane · 2021-07-12T17:49:54Z

/approve
from sig-storage perspective

SataQiu · 2021-07-15T15:43:38Z

/assign @liggitt

liggitt · 2021-07-22T15:05:37Z

pkg/controller/apis/config/types.go

@@ -84,6 +85,9 @@ type KubeControllerManagerConfiguration struct {
 	// EndpointSliceMirroringControllerConfiguration holds configuration for
 	// EndpointSliceMirroringController related features.
 	EndpointSliceMirroringController endpointslicemirroringconfig.EndpointSliceMirroringControllerConfiguration
+	// EphemeralControllerConfiguration holds configuration for EphemeralController
+	// related features.
+	EphemeralController ephemeralconfig.EphemeralControllerConfiguration


suggest naming this EphemeralVolumeController for clarity (same for the type)

(also applies to the v1alpha1/types.go file)

liggitt · 2021-07-22T15:07:26Z

pkg/controller/volume/ephemeral/config/v1alpha1/defaults.go

+// run it in your wrapper struct of this type in its `SetDefaults_` method.
+func RecommendedDefaultEphemeralControllerConfiguration(obj *kubectrlmgrconfigv1alpha1.EphemeralControllerConfiguration) {
+	if obj.ConcurrentEphemeralVolumeSyncs == 0 {
+		obj.ConcurrentEphemeralVolumeSyncs = 1


is 1 really the default we want? is there a downside to making this > 1 (most sync loops default to 5)

SataQiu · 2021-07-26T11:34:56Z

/test pull-kubernetes-integration

liggitt

/milestone v1.23

/approve
config API type mechanics lgtm

would like an ack on the thread safety of the controller's sync loop from @pohly

liggitt · 2021-07-27T14:50:46Z

pkg/controller/volume/ephemeral/config/v1alpha1/defaults.go

+// run it in your wrapper struct of this type in its `SetDefaults_` method.
+func RecommendedDefaultEphemeralVolumeControllerConfiguration(obj *kubectrlmgrconfigv1alpha1.EphemeralVolumeControllerConfiguration) {
+	if obj.ConcurrentEphemeralVolumeSyncs == 0 {
+		obj.ConcurrentEphemeralVolumeSyncs = 5


lgtm, would like an ack from @pohly that this controller's sync loop is threadsafe

It certainly was meant to be thread safe, so although that hasn't been tested at scale because it couldn't be enabled earlier, it should be fine.

k8s-ci-robot · 2021-07-27T14:51:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jsafrane, liggitt, SataQiu

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:

~~api/OWNERS~~ [liggitt]
~~cmd/kube-controller-manager/OWNERS~~ [liggitt]
~~pkg/controller/apis/config/OWNERS~~ [liggitt]
~~pkg/controller/volume/ephemeral/OWNERS~~ [jsafrane,liggitt]
~~staging/src/k8s.io/kube-controller-manager/config/OWNERS~~ [liggitt]

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

liggitt · 2021-08-05T14:48:58Z

/lgtm
/hold
API review bits done, pending lgtm by controller author (@pohly). Can unhold once that is done.

pohly · 2021-08-05T15:08:49Z

I has my LGTM (#102981 (comment)).

/hold cancel

pohly · 2021-08-05T15:10:01Z

/hold

Sorry, just noticed the additional questions... let me check.

pohly · 2021-08-05T15:13:20Z

/hold cancel

k8s-triage-robot · 2021-08-05T22:06:55Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-triage-robot · 2021-08-06T01:07:55Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

k8s-ci-robot requested review from lavalamp and mwielgus June 18, 2021 08:42

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. sig/apps Categorizes an issue or PR as relevant to SIG Apps. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jun 18, 2021

SataQiu force-pushed the add-ephemeral-config-v1alpha1 branch from 25d704a to f0ae3d1 Compare June 18, 2021 08:43

k8s-ci-robot added the api-review Categorizes an issue or PR as actively needing an API review. label Jun 18, 2021

liggitt assigned lavalamp Jun 18, 2021

liggitt added this to Assigned in API Reviews Jun 18, 2021

liggitt added this to the v1.22 milestone Jun 18, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 22, 2021

k8s-ci-robot assigned jsafrane and pohly Jul 8, 2021

k8s-ci-robot removed this from the v1.22 milestone Jul 9, 2021

pohly reviewed Jul 9, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 9, 2021

k8s-ci-robot assigned liggitt Jul 15, 2021

liggitt reviewed Jul 22, 2021

View reviewed changes

liggitt moved this from Assigned to Changes requested in API Reviews Jul 22, 2021

add --concurrent-ephemeralvolume-syncs flag for kube-controller-manager

7fa0b9b

SataQiu force-pushed the add-ephemeral-config-v1alpha1 branch from f0ae3d1 to 7fa0b9b Compare July 25, 2021 13:37

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 25, 2021

liggitt reviewed Jul 27, 2021

View reviewed changes

k8s-ci-robot added this to the v1.23 milestone Jul 27, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 27, 2021

liggitt moved this from Changes requested to API review completed, 1.23 in API Reviews Aug 5, 2021

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 5, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 5, 2021

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 5, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 5, 2021

k8s-ci-robot merged commit 7ab3e3c into kubernetes:master Aug 6, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --concurrent-ephemeralvolume-syncs flag for kube-controller-manager #102981

Add --concurrent-ephemeralvolume-syncs flag for kube-controller-manager #102981

SataQiu commented Jun 18, 2021

fejta-bot commented Jun 18, 2021

SataQiu commented Jun 18, 2021

SataQiu commented Jun 18, 2021

fedebongio commented Jun 22, 2021

msau42 commented Jul 8, 2021

lavalamp commented Jul 8, 2021

JamesLaverack commented Jul 9, 2021

pohly Jul 9, 2021

jsafrane commented Jul 12, 2021

SataQiu commented Jul 15, 2021

liggitt Jul 22, 2021

liggitt Jul 22, 2021

SataQiu Jul 25, 2021

liggitt Jul 22, 2021

SataQiu commented Jul 26, 2021

liggitt left a comment

liggitt Jul 27, 2021

pohly Aug 5, 2021

k8s-ci-robot commented Jul 27, 2021

liggitt commented Aug 5, 2021

pohly commented Aug 5, 2021

pohly commented Aug 5, 2021

pohly commented Aug 5, 2021

k8s-triage-robot commented Aug 5, 2021

k8s-triage-robot commented Aug 6, 2021

Add --concurrent-ephemeralvolume-syncs flag for kube-controller-manager #102981

Add --concurrent-ephemeralvolume-syncs flag for kube-controller-manager #102981

Conversation

SataQiu commented Jun 18, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

fejta-bot commented Jun 18, 2021

SataQiu commented Jun 18, 2021

SataQiu commented Jun 18, 2021

fedebongio commented Jun 22, 2021

msau42 commented Jul 8, 2021

lavalamp commented Jul 8, 2021

JamesLaverack commented Jul 9, 2021

pohly Jul 9, 2021

Choose a reason for hiding this comment

jsafrane commented Jul 12, 2021

SataQiu commented Jul 15, 2021

liggitt Jul 22, 2021

Choose a reason for hiding this comment

liggitt Jul 22, 2021

Choose a reason for hiding this comment

SataQiu Jul 25, 2021

Choose a reason for hiding this comment

liggitt Jul 22, 2021

Choose a reason for hiding this comment

SataQiu commented Jul 26, 2021

liggitt left a comment

Choose a reason for hiding this comment

liggitt Jul 27, 2021

Choose a reason for hiding this comment

pohly Aug 5, 2021

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 27, 2021

liggitt commented Aug 5, 2021

pohly commented Aug 5, 2021

pohly commented Aug 5, 2021

pohly commented Aug 5, 2021

k8s-triage-robot commented Aug 5, 2021

k8s-triage-robot commented Aug 6, 2021