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.

Sign up for GitHub

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

Jump to bottom

increase controller concurrency and cpu requests #2862

Merged
merged 1 commit into from
Aug 24, 2023
Merged

increase controller concurrency and cpu requests #2862

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion pkg/controller/clone-controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,8 @@ func NewCloneController(mgr manager.Manager,
installerLabels: installerLabels,
}
cloneController, err := controller.New("clone-controller", mgr, controller.Options{
Reconciler: reconciler,
MaxConcurrentReconciles: 3,
Reconciler: reconciler,
})
if err != nil {
return nil, err
Expand Down
3 changes: 2 additions & 1 deletion pkg/controller/config-controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -480,7 +480,8 @@ func NewConfigController(mgr manager.Manager, log logr.Logger, uploadProxyServic
}

configController, err := controller.New("config-controller", mgr, controller.Options{
Reconciler: reconciler,
MaxConcurrentReconciles: 3,
Reconciler: reconciler,
})
if err != nil {
return nil, err
Expand Down
5 changes: 4 additions & 1 deletion pkg/controller/dataimportcron-controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -947,7 +947,10 @@ func NewDataImportCronController(mgr manager.Manager, log logr.Logger, importerI
cdiNamespace: util.GetNamespace(),
installerLabels: installerLabels,
}
dataImportCronController, err := controller.New(dataImportControllerName, mgr, controller.Options{Reconciler: reconciler})
dataImportCronController, err := controller.New(dataImportControllerName, mgr, controller.Options{
MaxConcurrentReconciles: 3,
Reconciler: reconciler,
})
if err != nil {
return nil, err
}
Expand Down
5 changes: 4 additions & 1 deletion pkg/controller/datasource-controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,10 @@ func NewDataSourceController(mgr manager.Manager, log logr.Logger, installerLabe
log: log.WithName(dataSourceControllerName),
installerLabels: installerLabels,
}
DataSourceController, err := controller.New(dataSourceControllerName, mgr, controller.Options{Reconciler: reconciler})
DataSourceController, err := controller.New(dataSourceControllerName, mgr, controller.Options{
MaxConcurrentReconciles: 3,
Reconciler: reconciler,
})
if err != nil {
return nil, err
}
Expand Down
7 changes: 7 additions & 0 deletions pkg/controller/datavolume/controller-base.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -495,10 +495,17 @@ func (r *ReconcilerBase) syncUpdate(log logr.Logger, syncState *dvSyncState) err
return fmt.Errorf("status update is not allowed in sync phase")
}
if !reflect.DeepEqual(syncState.dv.ObjectMeta, syncState.dvMutated.ObjectMeta) {
_, ok := syncState.dv.Annotations[cc.AnnExtendedCloneToken]
_, ok2 := syncState.dvMutated.Annotations[cc.AnnExtendedCloneToken]
if err := r.updateDataVolume(syncState.dvMutated); err != nil {
r.log.Error(err, "Unable to sync update dv meta", "name", syncState.dvMutated.Name)
return err
}
if !ok && ok2 {
delta := time.Since(syncState.dv.ObjectMeta.CreationTimestamp.Time)
log.V(3).Info("Adding extended DataVolume token took", "delta", delta)
}
syncState.dv = syncState.dvMutated.DeepCopy()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Everything looks good but I'm wondering about this section, does this introduce new behavior? Should we test it?

Copy link
Member Author

Choose a reason for hiding this comment

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

In my testing this appeared to reduce updateStatus failures/retries. The point is that once the dv has been updated the "baseline" and mutated DV are the same

Copy link
Collaborator

@akalenyu akalenyu Aug 23, 2023 •
edited

Choose a reason for hiding this comment

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

weird, syncUpdate is usually the last thing we do in the loop, how come something else runs updates after it
EDIT:
nvm I see we updateStatus after it. makes sense.
But updateStatus seems to work on a fresh copy of the DV from the cache, which should have the updates..

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! But the fact is that the "fresh copy" from the cache is often stale (informer didn't receive update yet). Would be better if updatestatus actually uses the copy returned by update in that case. I swear that adding this assignment was reducing errors/retries. Seems unlikely but maybe the additional time to do the assignment gives more time for the informer to update.

Anyway, worst case this assignment has no actual effect. I can remove it if you'd like but it does seem more correct to me that "dv" and "dvMutated" should be equal at that point. The fact that neither value is referenced again is kind of beside the point. That may change in the future

Copy link
Collaborator

@akalenyu akalenyu Aug 23, 2023 •
edited

Choose a reason for hiding this comment

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

That may change in the future

That is my concern; if we ever have to look at dv/dvmutated diff to compute some status transition,
this may throw us off

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we should spend a lot of time wondering about hypotheticals. But what are you suggesting we do here @akalenyu?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I guess that's not very likely, I am fine with keeping it

}
return nil
}
Expand Down
5 changes: 4 additions & 1 deletion pkg/controller/datavolume/external-population-controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,10 @@ func NewPopulatorController(ctx context.Context, mgr manager.Manager, log logr.L
},
}

datavolumeController, err := controller.New(populatorControllerName, mgr, controller.Options{Reconciler: reconciler})
datavolumeController, err := controller.New(populatorControllerName, mgr, controller.Options{
MaxConcurrentReconciles: 3,
Reconciler: reconciler,
})
if err != nil {
return nil, err
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/controller/datavolume/import-controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,8 @@ func NewImportController(
}

datavolumeController, err := controller.New(importControllerName, mgr, controller.Options{
Reconciler: reconciler,
MaxConcurrentReconciles: 3,
Reconciler: reconciler,
})
if err != nil {
return nil, err
Expand Down
3 changes: 2 additions & 1 deletion pkg/controller/datavolume/pvc-clone-controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ func NewPvcCloneController(
}

dataVolumeCloneController, err := controller.New(pvcCloneControllerName, mgr, controller.Options{
Reconciler: reconciler,
MaxConcurrentReconciles: 3,
Reconciler: reconciler,
})
if err != nil {
return nil, err
Expand Down
3 changes: 2 additions & 1 deletion pkg/controller/datavolume/snapshot-clone-controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,8 @@ func NewSnapshotCloneController(
}

dataVolumeCloneController, err := controller.New(snapshotCloneControllerName, mgr, controller.Options{
Reconciler: reconciler,
MaxConcurrentReconciles: 3,
Reconciler: reconciler,
})
if err != nil {
return nil, err
Expand Down
3 changes: 2 additions & 1 deletion pkg/controller/datavolume/upload-controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ func NewUploadController(
}

datavolumeController, err := controller.New(uploadControllerName, mgr, controller.Options{
Reconciler: reconciler,
MaxConcurrentReconciles: 3,
Reconciler: reconciler,
})
if err != nil {
return nil, err
Expand Down
3 changes: 2 additions & 1 deletion pkg/controller/import-controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,8 @@ func NewImportController(mgr manager.Manager, log logr.Logger, importerImage, pu
installerLabels: installerLabels,
}
importController, err := controller.New("import-controller", mgr, controller.Options{
Reconciler: reconciler,
MaxConcurrentReconciles: 3,
Reconciler: reconciler,
})
if err != nil {
return nil, err
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/populators/clone-populator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ func NewClonePopulator(
}

clonePopulator, err := controller.New(clonePopulatorName, mgr, controller.Options{
MaxConcurrentReconciles: 5,
MaxConcurrentReconciles: 3,
Reconciler: reconciler,
})
if err != nil {
Expand Down
3 changes: 2 additions & 1 deletion pkg/controller/populators/import-populator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ func NewImportPopulator(
}

importPopulator, err := controller.New(importPopulatorName, mgr, controller.Options{
Reconciler: reconciler,
MaxConcurrentReconciles: 3,
Reconciler: reconciler,
})
if err != nil {
return nil, err
Expand Down
3 changes: 2 additions & 1 deletion pkg/controller/populators/upload-populator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,8 @@ func NewUploadPopulator(
}

uploadPopulator, err := controller.New(uploadPopulatorName, mgr, controller.Options{
Reconciler: reconciler,
MaxConcurrentReconciles: 3,
Reconciler: reconciler,
})
if err != nil {
return nil, err
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/storageprofile-controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ func NewStorageProfileController(mgr manager.Manager, log logr.Logger, installer
storageProfileController, err := controller.New(
"storageprofile-controller",
mgr,
controller.Options{Reconciler: reconciler})
controller.Options{Reconciler: reconciler, MaxConcurrentReconciles: 3})
if err != nil {
return nil, err
}
Expand Down
3 changes: 2 additions & 1 deletion pkg/controller/transfer/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,8 @@ func NewObjectTransferController(mgr manager.Manager, log logr.Logger, installer
}

ctrl, err := controller.New(name, mgr, controller.Options{
Reconciler: reconciler,
MaxConcurrentReconciles: 3,
Reconciler: reconciler,
})
if err != nil {
return nil, err
Expand Down
3 changes: 2 additions & 1 deletion pkg/controller/upload-controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -662,7 +662,8 @@ func NewUploadController(mgr manager.Manager, log logr.Logger, uploadImage, pull
installerLabels: installerLabels,
}
uploadController, err := controller.New("upload-controller", mgr, controller.Options{
Reconciler: reconciler,
MaxConcurrentReconciles: 3,
Reconciler: reconciler,
})
if err != nil {
return nil, err
Expand Down
5 changes: 4 additions & 1 deletion pkg/operator/controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,10 @@ func (r *ReconcileCDI) Reconcile(_ context.Context, request reconcile.Request) (

func (r *ReconcileCDI) add(mgr manager.Manager) error {
// Create a new controller
c, err := controller.New("cdi-operator-controller", mgr, controller.Options{Reconciler: r})
c, err := controller.New("cdi-operator-controller", mgr, controller.Options{
MaxConcurrentReconciles: 3,
Reconciler: r,
})
if err != nil {
return err
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/operator/resources/namespaced/apiserver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ func createAPIServerDeployment(image, verbosity, pullPolicy string, imagePullSec
}
container.Resources = corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("10m"),
corev1.ResourceCPU: resource.MustParse("100m"),
corev1.ResourceMemory: resource.MustParse("150Mi"),
},
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/operator/resources/namespaced/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ func createControllerDeployment(controllerImage, importerImage, clonerImage, upl
}
container.Resources = corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("10m"),
corev1.ResourceCPU: resource.MustParse("100m"),
corev1.ResourceMemory: resource.MustParse("150Mi"),
},
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/operator/resources/namespaced/uploadproxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ func createUploadProxyDeployment(image, verbosity, pullPolicy string, imagePullS
}
container.Resources = corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("10m"),
corev1.ResourceCPU: resource.MustParse("100m"),
corev1.ResourceMemory: resource.MustParse("150Mi"),
},
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/operator/resources/operator/operator.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ func createOperatorDeployment(operatorVersion, namespace, deployClusterResources
container := utils.CreatePortsContainer("cdi-operator", operatorImage, pullPolicy, createPrometheusPorts())
container.Resources = corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("10m"),
corev1.ResourceCPU: resource.MustParse("100m"),
corev1.ResourceMemory: resource.MustParse("150Mi"),
},
}
Expand Down