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

Use GOMAXPROCS for MaxConcurrentReconciles #142

Merged
merged 1 commit into from
Nov 28, 2023
Merged

Use GOMAXPROCS for MaxConcurrentReconciles #142

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions internal/controller/datamovement_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"path/filepath"
"reflect"
"regexp"
"runtime"
"strconv"
"strings"
"sync"
Expand All @@ -39,7 +40,7 @@ import (
corev1 "k8s.io/api/core/v1"
apierrors "k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
kruntime "k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/util/retry"

Expand Down Expand Up @@ -76,7 +77,7 @@ var progressRe = regexp.MustCompile(`Copied.+\(([[:digit:]]{1,3})%\)`)
// DataMovementReconciler reconciles a DataMovement object
type DataMovementReconciler struct {
client.Client
Scheme *runtime.Scheme
Scheme *kruntime.Scheme

// We maintain a map of active operations which allows us to process cancel requests
// This is a thread safe map since multiple data movement reconcilers and go routines will be executing at the same time.
Expand Down Expand Up @@ -736,9 +737,10 @@ func filterByNamespace(namespace string) predicate.Predicate {

// SetupWithManager sets up the controller with the Manager.
func (r *DataMovementReconciler) SetupWithManager(mgr ctrl.Manager) error {
maxReconciles := runtime.GOMAXPROCS(0)
return ctrl.NewControllerManagedBy(mgr).
For(&nnfv1alpha1.NnfDataMovement{}).
WithOptions(controller.Options{MaxConcurrentReconciles: 128}).
WithOptions(controller.Options{MaxConcurrentReconciles: maxReconciles}).
Copy link
Contributor

Choose a reason for hiding this comment

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

GOMAXPROCS defaults to the number of CPUs. How many are on a rabbit?
The cmd.wait() likely allows Go to pick up another go proc, if one is available. So this controller can probably handle many more data movements than GOMAXPROCS would allow.

I wonder if this controller should even have a max setting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean that it should default to 1?

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, no. I hadn't realized that was the default. It should be a high number. Let's stay with 128 until we see a reason to change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This does equal 128 on the rabbit nodes:

root@nnf-dm-worker-pctgf:~# ./maxprocs
maxReconciles: 128
root@nnf-dm-worker-pctgf:~# nproc
128

Here's the code for ./maxprocs:

package main

import (
	"fmt"
	"runtime"
)

func main() {
	maxReconciles := runtime.GOMAXPROCS(0)
	fmt.Printf("maxReconciles: %d\n", maxReconciles)
}

WithEventFilter(filterByNamespace(r.WatchNamespace)).
Complete(r)
}
Loading