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

introduce concurrent flags to controllers #1321

Merged
merged 2 commits into from
Feb 25, 2022

Conversation

pigletfly
Copy link
Contributor

@pigletfly pigletfly commented Jan 26, 2022

Signed-off-by: pigletfly wangbing.adam@gmail.com

What type of PR is this?
/kind feature

What this PR does / why we need it:

Make controller reconcile concurrent configurable

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

`karmada-controller-manager`: Introduced several flags to specify controller's capacities:
- --concurrent-work-syncs
- --concurrent-namespace-syncs
- --concurrent-resource-template-syncs
- --concurrent-cluster-syncs
- --concurrent-clusterresourcebinding-syncs
- --concurrent-resourcebinding-syncs

`karmada-agent`: Introduced several flags to specify controller's capacities:
- --concurrent-work-syncs
- --concurrent-cluster-syncs

The larger number of these flags means faster controller reconciles, but more CPU (and network) load.

@karmada-bot karmada-bot added kind/feature Categorizes issue or PR as related to a new feature. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 26, 2022
@karmada-bot karmada-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 26, 2022
pkg/detector/detector.go Outdated Show resolved Hide resolved
@pigletfly
Copy link
Contributor Author

if numbers of objects are huge,controller can't proccess them in time.In my case, pp objects get reconciled after serveral minutes.

@pigletfly
Copy link
Contributor Author

/release-note-none

@karmada-bot karmada-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Jan 27, 2022
@pigletfly
Copy link
Contributor Author

/priority important-soon

@karmada-bot karmada-bot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jan 28, 2022
@pigletfly
Copy link
Contributor Author

/cc @RainbowMango @Garrybest

@Garrybest
Copy link
Member

Refer to #1260

@Garrybest
Copy link
Member

Hi @pigletfly, what about make it easier by using ControllerConfigurationSpec in when NewManager? Like this.

controllerManager, err := controllerruntime.NewManager(config, controllerruntime.Options{
	Scheme:                     gclient.NewSchema(),
	SyncPeriod:                 &opts.ResyncPeriod.Duration,
	LeaderElection:             opts.LeaderElection.LeaderElect,
	LeaderElectionID:           opts.LeaderElection.ResourceName,
	LeaderElectionNamespace:    opts.LeaderElection.ResourceNamespace,
	LeaderElectionResourceLock: opts.LeaderElection.ResourceLock,
	HealthProbeBindAddress:     net.JoinHostPort(opts.BindAddress, strconv.Itoa(opts.SecurePort)),
	LivenessEndpointName:       "/healthz",
	Controller: v1alpha1.ControllerConfigurationSpec{
		GroupKindConcurrency: map[string]int{
			workv1alpha2.SchemeGroupVersion.WithKind("ResourceBinding"): rbConcurrentReconciles,
			workv1alpha2.SchemeGroupVersion.WithKind("ClusterResourceBinding"): crbConcurrentReconciles,
			workv1alpha1.SchemeGroupVersion.WithKind("Work"): workConcurrentReconciles,
			......
		},
	},
})

@pigletfly
Copy link
Contributor Author

pigletfly commented Feb 7, 2022

Hi @pigletfly, what about make it easier by using ControllerConfigurationSpec in when NewManager? Like this.

controllerManager, err := controllerruntime.NewManager(config, controllerruntime.Options{
	Scheme:                     gclient.NewSchema(),
	SyncPeriod:                 &opts.ResyncPeriod.Duration,
	LeaderElection:             opts.LeaderElection.LeaderElect,
	LeaderElectionID:           opts.LeaderElection.ResourceName,
	LeaderElectionNamespace:    opts.LeaderElection.ResourceNamespace,
	LeaderElectionResourceLock: opts.LeaderElection.ResourceLock,
	HealthProbeBindAddress:     net.JoinHostPort(opts.BindAddress, strconv.Itoa(opts.SecurePort)),
	LivenessEndpointName:       "/healthz",
	Controller: v1alpha1.ControllerConfigurationSpec{
		GroupKindConcurrency: map[string]int{
			workv1alpha2.SchemeGroupVersion.WithKind("ResourceBinding"): rbConcurrentReconciles,
			workv1alpha2.SchemeGroupVersion.WithKind("ClusterResourceBinding"): crbConcurrentReconciles,
			workv1alpha1.SchemeGroupVersion.WithKind("Work"): workConcurrentReconciles,
			......
		},
	},
})

that's fine with normal controllers (e.g,ResourceBinding) except detector,work,cluster.

@pigletfly pigletfly force-pushed the fix-worker-num branch 2 times, most recently from 1d3cf23 to 037df3b Compare February 7, 2022 08:22
@pigletfly
Copy link
Contributor Author

@Garrybest @RainbowMango PTAL

@Garrybest
Copy link
Member

I think you may forget this.

d.bindingReconcileWorker.Run(1, d.stopCh)

c.worker.Run(c.WorkerNumber, c.StopChan)

@pigletfly
Copy link
Contributor Author

pigletfly commented Feb 8, 2022

I think you may forget this.

d.bindingReconcileWorker.Run(1, d.stopCh)

c.worker.Run(c.WorkerNumber, c.StopChan)

good catch.But resourcebinding shares the same flag,we can't redefined in detector.Or we can hack like this flag.Lookup("resourcebinding-workers").Value.(flag.Getter).Get().(int) in detector.And I will send a separate PR to move resourcebinding reconcile all into resourcebinding_controller,then there will be only one flag.

xref #1309

@pigletfly pigletfly force-pushed the fix-worker-num branch 2 times, most recently from 673936c to 34aed67 Compare February 8, 2022 11:36
Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

I just finished the detector part of the review.

pkg/detector/detector.go Outdated Show resolved Hide resolved
pkg/detector/detector.go Outdated Show resolved Hide resolved
cmd/controller-manager/app/options/options.go Outdated Show resolved Hide resolved
cmd/controller-manager/app/options/options.go Outdated Show resolved Hide resolved
@pigletfly pigletfly force-pushed the fix-worker-num branch 3 times, most recently from 96f65f2 to fb0c3a6 Compare February 24, 2022 07:24
Signed-off-by: pigletfly <wangbing.adam@gmail.com>
@karmada-bot karmada-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 24, 2022
Signed-off-by: RainbowMango <qdurenhongcai@gmail.com>
Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

I guess we are good to go now.
@dddddai @Garrybest
Could you please take a look?

@dddddai
Copy link
Member

dddddai commented Feb 25, 2022

/lgtm

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 25, 2022
@Garrybest
Copy link
Member

Nice work👍
/lgtm

@RainbowMango
Copy link
Member

/retitle introduce concurrent flags to controllers

@karmada-bot karmada-bot changed the title Fix controller reconcile concurrent introduce concurrent flags to controllers Feb 25, 2022
Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

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

/approve

@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 25, 2022
@karmada-bot karmada-bot merged commit d980eea into karmada-io:master Feb 25, 2022
@pigletfly pigletfly deleted the fix-worker-num branch February 25, 2022 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants