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

Make concurrent reconciliation configurable #153

Merged
merged 2 commits into from
Jun 24, 2021
Merged

Make concurrent reconciliation configurable #153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f78b993
Make concurrent reconciliation configurable
Jun 23, 2021
03043e5
chore: add options type for both controllers
Jun 24, 2021
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
10 changes: 9 additions & 1 deletion controllers/imagepolicy_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"k8s.io/client-go/tools/reference"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller"
"sigs.k8s.io/controller-runtime/pkg/handler"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
"sigs.k8s.io/controller-runtime/pkg/source"
Expand Down Expand Up @@ -57,6 +58,10 @@ type ImagePolicyReconciler struct {
Database DatabaseReader
}

type ImagePolicyReconcilerOptions struct {
MaxConcurrentReconciles int
}

// +kubebuilder:rbac:groups=image.toolkit.fluxcd.io,resources=imagepolicies,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=image.toolkit.fluxcd.io,resources=imagepolicies/status,verbs=get;update;patch
// +kubebuilder:rbac:groups=image.toolkit.fluxcd.io,resources=imagerepositories,verbs=get;list;watch
Expand Down Expand Up @@ -191,7 +196,7 @@ func (r *ImagePolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request)
return ctrl.Result{}, err
}

func (r *ImagePolicyReconciler) SetupWithManager(mgr ctrl.Manager) error {
func (r *ImagePolicyReconciler) SetupWithManager(mgr ctrl.Manager, opts ImagePolicyReconcilerOptions) error {
// index the policies by which image repo they point at, so that
// it's easy to list those out when an image repo changes.
if err := mgr.GetFieldIndexer().IndexField(context.Background(), &imagev1.ImagePolicy{}, imageRepoKey, func(obj client.Object) []string {
Expand All @@ -207,6 +212,9 @@ func (r *ImagePolicyReconciler) SetupWithManager(mgr ctrl.Manager) error {
&source.Kind{Type: &imagev1.ImageRepository{}},
handler.EnqueueRequestsFromMapFunc(r.imagePoliciesForRepository),
).
WithOptions(controller.Options{
MaxConcurrentReconciles: opts.MaxConcurrentReconciles,
}).
Complete(r)
}

Expand Down
10 changes: 9 additions & 1 deletion controllers/imagerepository_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import (
"k8s.io/client-go/tools/reference"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller"
"sigs.k8s.io/controller-runtime/pkg/predicate"

"github.com/fluxcd/pkg/apis/meta"
Expand Down Expand Up @@ -75,6 +76,10 @@ type ImageRepositoryReconciler struct {
}
}

type ImageRepositoryReconcilerOptions struct {
MaxConcurrentReconciles int
}

type dockerConfig struct {
Auths map[string]authn.AuthConfig
}
Expand Down Expand Up @@ -352,10 +357,13 @@ func (r *ImageRepositoryReconciler) shouldScan(repo imagev1.ImageRepository, now
return false, when, nil
}

func (r *ImageRepositoryReconciler) SetupWithManager(mgr ctrl.Manager) error {
func (r *ImageRepositoryReconciler) SetupWithManager(mgr ctrl.Manager, opts ImageRepositoryReconcilerOptions) error {
return ctrl.NewControllerManagedBy(mgr).
For(&imagev1.ImageRepository{}).
WithEventFilter(predicate.Or(predicate.GenerationChangedPredicate{}, predicates.ReconcileRequestedPredicate{})).
WithOptions(controller.Options{
MaxConcurrentReconciles: opts.MaxConcurrentReconciles,
}).
Complete(r)
}

Expand Down
4 changes: 2 additions & 2 deletions controllers/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,14 +105,14 @@ var _ = BeforeSuite(func(done Done) {
Scheme: scheme.Scheme,
Database: database.NewBadgerDatabase(badgerDB),
}
Expect(imageRepoReconciler.SetupWithManager(k8sMgr)).To(Succeed())
Expect(imageRepoReconciler.SetupWithManager(k8sMgr, ImageRepositoryReconcilerOptions{})).To(Succeed())

imagePolicyReconciler = &ImagePolicyReconciler{
Client: k8sMgr.GetClient(),
Scheme: scheme.Scheme,
Database: database.NewBadgerDatabase(badgerDB),
}
Expect(imagePolicyReconciler.SetupWithManager(k8sMgr)).To(Succeed())
Expect(imagePolicyReconciler.SetupWithManager(k8sMgr, ImagePolicyReconcilerOptions{})).To(Succeed())

// this must be started for the caches to be running, and thereby
// for the client to be usable.
Expand Down
10 changes: 8 additions & 2 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ func main() {
watchAllNamespaces bool
storagePath string
storageValueLogFileSize int64
concurrent int
)

flag.StringVar(&metricsAddr, "metrics-addr", ":8080", "The address the metric endpoint binds to.")
Expand All @@ -77,6 +78,7 @@ func main() {
"Watch for custom resources in all namespaces, if set to false it will only watch the runtime namespace.")
flag.StringVar(&storagePath, "storage-path", "/data", "Where to store the persistent database of image metadata")
flag.Int64Var(&storageValueLogFileSize, "storage-value-log-file-size", 1<<28, "Set the database's memory mapped value log file size in bytes. Effective memory usage is about two times this size.")
flag.IntVar(&concurrent, "concurrent", 4, "The number of concurrent resource reconciles.")
Copy link
Member

Choose a reason for hiding this comment

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

I think this flag definition comes from elsewhere; but either way, it deserves a better description, on two counts:

  • it's the maximum number of workers
  • "reconciles" doesn't tell the user what it affects.

Something like "The maximum number of scans that can run concurrently" would be an improvement, I reckon. WDYT?

Copy link
Member

Choose a reason for hiding this comment

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

Oh it was merged already. OK never mind.

Copy link
Member Author

Choose a reason for hiding this comment

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

@squaremo I agree. I'll come up with a follow-up PR.

Copy link
Member

@stefanprodan stefanprodan Jun 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We could extract this into fluxcd/pkg/runtime like we did with the log level and other common flags. The description used here is the same for all the others controllers, better to change it in one place.

clientOptions.BindFlags(flag.CommandLine)
logOptions.BindFlags(flag.CommandLine)
leaderElectionOptions.BindFlags(flag.CommandLine)
Expand Down Expand Up @@ -142,7 +144,9 @@ func main() {
ExternalEventRecorder: eventRecorder,
MetricsRecorder: metricsRecorder,
Database: db,
}).SetupWithManager(mgr); err != nil {
}).SetupWithManager(mgr, controllers.ImageRepositoryReconcilerOptions{
MaxConcurrentReconciles: concurrent,
}); err != nil {
setupLog.Error(err, "unable to create controller", "controller", imagev1.ImageRepositoryKind)
os.Exit(1)
}
Expand All @@ -153,7 +157,9 @@ func main() {
ExternalEventRecorder: eventRecorder,
MetricsRecorder: metricsRecorder,
Database: db,
}).SetupWithManager(mgr); err != nil {
}).SetupWithManager(mgr, controllers.ImagePolicyReconcilerOptions{
MaxConcurrentReconciles: concurrent,
}); err != nil {
setupLog.Error(err, "unable to create controller", "controller", imagev1.ImagePolicyKind)
os.Exit(1)
}
Expand Down