diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index 67dabfd733..7f408e8192 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -14,6 +14,8 @@ jobs: name: Upload binaries to release runs-on: ubuntu-latest steps: + - name: Set env + run: echo "RELEASE_TAG=${GITHUB_REF:10}" >> $GITHUB_ENV - name: Check out code uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # tag=v4.2.2 - name: Calculate go version diff --git a/Makefile b/Makefile index 11eaabd431..2361df981d 100644 --- a/Makefile +++ b/Makefile @@ -174,7 +174,7 @@ release-binary: $(RELEASE_DIR) -v "$$(pwd):/workspace$(DOCKER_VOL_OPTS)" \ -w /workspace/tools/setup-envtest \ golang:$(GO_VERSION) \ - go build -a -trimpath -ldflags "-extldflags '-static'" \ + go build -a -trimpath -ldflags "-X 'sigs.k8s.io/controller-runtime/tools/setup-envtest/version.version=$(RELEASE_TAG)' -extldflags '-static'" \ -o ./out/$(RELEASE_BINARY) ./ ## -------------------------------------- diff --git a/pkg/builder/controller.go b/pkg/builder/controller.go index 0760953e02..6d906f6e52 100644 --- a/pkg/builder/controller.go +++ b/pkg/builder/controller.go @@ -163,7 +163,7 @@ func (blder *TypedBuilder[request]) Watches( ) *TypedBuilder[request] { input := WatchesInput[request]{ obj: object, - handler: handler.WithLowPriorityWhenUnchanged(eventHandler), + handler: eventHandler, } for _, opt := range opts { opt.ApplyToWatches(&input) @@ -317,7 +317,7 @@ func (blder *TypedBuilder[request]) doWatch() error { } var hdler handler.TypedEventHandler[client.Object, request] - reflect.ValueOf(&hdler).Elem().Set(reflect.ValueOf(handler.WithLowPriorityWhenUnchanged(&handler.EnqueueRequestForObject{}))) + reflect.ValueOf(&hdler).Elem().Set(reflect.ValueOf(&handler.EnqueueRequestForObject{})) allPredicates := append([]predicate.Predicate(nil), blder.globalPredicates...) allPredicates = append(allPredicates, blder.forInput.predicates...) src := source.TypedKind(blder.mgr.GetCache(), obj, hdler, allPredicates...) @@ -341,11 +341,11 @@ func (blder *TypedBuilder[request]) doWatch() error { } var hdler handler.TypedEventHandler[client.Object, request] - reflect.ValueOf(&hdler).Elem().Set(reflect.ValueOf(handler.WithLowPriorityWhenUnchanged(handler.EnqueueRequestForOwner( + reflect.ValueOf(&hdler).Elem().Set(reflect.ValueOf(handler.EnqueueRequestForOwner( blder.mgr.GetScheme(), blder.mgr.GetRESTMapper(), blder.forInput.object, opts..., - )))) + ))) allPredicates := append([]predicate.Predicate(nil), blder.globalPredicates...) allPredicates = append(allPredicates, own.predicates...) src := source.TypedKind(blder.mgr.GetCache(), obj, hdler, allPredicates...) diff --git a/pkg/client/apiutil/restmapper.go b/pkg/client/apiutil/restmapper.go index ad898617fa..7a7a0d1145 100644 --- a/pkg/client/apiutil/restmapper.go +++ b/pkg/client/apiutil/restmapper.go @@ -246,10 +246,18 @@ func (m *mapper) addGroupVersionResourcesToCacheAndReloadLocked(gvr map[schema.G } if !found { - groupResources.Group.Versions = append(groupResources.Group.Versions, metav1.GroupVersionForDiscovery{ + gv := metav1.GroupVersionForDiscovery{ GroupVersion: metav1.GroupVersion{Group: groupVersion.Group, Version: version}.String(), Version: version, - }) + } + + // Prepend if preferred version, else append. The upstream DiscoveryRestMappper assumes + // the first version is the preferred one: https://github.com/kubernetes/kubernetes/blob/ef54ac803b712137871c1a1f8d635d50e69ffa6c/staging/src/k8s.io/apimachinery/pkg/api/meta/restmapper.go#L458-L461 + if group, ok := m.apiGroups[groupVersion.Group]; ok && group.PreferredVersion.Version == version { + groupResources.Group.Versions = append([]metav1.GroupVersionForDiscovery{gv}, groupResources.Group.Versions...) + } else { + groupResources.Group.Versions = append(groupResources.Group.Versions, gv) + } } // Update data in the cache. @@ -284,14 +292,14 @@ func (m *mapper) findAPIGroupByNameAndMaybeAggregatedDiscoveryLocked(groupName s } m.initialDiscoveryDone = true - if len(maybeResources) > 0 { - didAggregatedDiscovery = true - m.addGroupVersionResourcesToCacheAndReloadLocked(maybeResources) - } for i := range apiGroups.Groups { group := &apiGroups.Groups[i] m.apiGroups[group.Name] = group } + if len(maybeResources) > 0 { + didAggregatedDiscovery = true + m.addGroupVersionResourcesToCacheAndReloadLocked(maybeResources) + } // Looking in the cache again. // Don't return an error here if the API group is not present. diff --git a/pkg/client/apiutil/restmapper_test.go b/pkg/client/apiutil/restmapper_test.go index 00117d00a8..395daccce4 100644 --- a/pkg/client/apiutil/restmapper_test.go +++ b/pkg/client/apiutil/restmapper_test.go @@ -21,6 +21,7 @@ import ( "fmt" "net/http" "strconv" + "sync" "testing" _ "github.com/onsi/ginkgo/v2" @@ -735,6 +736,33 @@ func TestLazyRestMapperProvider(t *testing.T) { g.Expect(err).NotTo(gmg.HaveOccurred()) g.Expect(mapping.Resource.Version).To(gmg.Equal("v1")) }) + + t.Run("Restmapper should consistently return the preferred version", func(t *testing.T) { + g := gmg.NewWithT(t) + + wg := sync.WaitGroup{} + wg.Add(50) + for i := 0; i < 50; i++ { + go func() { + defer wg.Done() + httpClient, err := rest.HTTPClientFor(restCfg) + g.Expect(err).NotTo(gmg.HaveOccurred()) + + mapper, err := apiutil.NewDynamicRESTMapper(restCfg, httpClient) + g.Expect(err).NotTo(gmg.HaveOccurred()) + + mapping, err := mapper.RESTMapping(schema.GroupKind{ + Group: "crew.example.com", + Kind: "Driver", + }) + g.Expect(err).NotTo(gmg.HaveOccurred()) + // APIServer seems to have a heuristic to prefer the higher + // version number. + g.Expect(mapping.GroupVersionKind.Version).To(gmg.Equal("v2")) + }() + } + wg.Wait() + }) }) } } diff --git a/pkg/controller/name.go b/pkg/controller/name.go index 0e71a01c66..00ca655128 100644 --- a/pkg/controller/name.go +++ b/pkg/controller/name.go @@ -34,7 +34,7 @@ func checkName(name string) error { } if usedNames.Has(name) { - return fmt.Errorf("controller with name %s already exists. Controller names must be unique to avoid multiple controllers reporting to the same metric", name) + return fmt.Errorf("controller with name %s already exists. Controller names must be unique to avoid multiple controllers reporting the same metric. This validation can be disabled via the SkipNameValidation option", name) } usedNames.Insert(name) diff --git a/pkg/handler/enqueue.go b/pkg/handler/enqueue.go index 1a1d1ab2f4..64cbe8a4d1 100644 --- a/pkg/handler/enqueue.go +++ b/pkg/handler/enqueue.go @@ -52,25 +52,32 @@ func (e *TypedEnqueueRequestForObject[T]) Create(ctx context.Context, evt event. enqueueLog.Error(nil, "CreateEvent received with no metadata", "event", evt) return } - q.Add(reconcile.Request{NamespacedName: types.NamespacedName{ + + item := reconcile.Request{NamespacedName: types.NamespacedName{ Name: evt.Object.GetName(), Namespace: evt.Object.GetNamespace(), - }}) + }} + + addToQueueCreate(q, evt, item) } // Update implements EventHandler. func (e *TypedEnqueueRequestForObject[T]) Update(ctx context.Context, evt event.TypedUpdateEvent[T], q workqueue.TypedRateLimitingInterface[reconcile.Request]) { switch { case !isNil(evt.ObjectNew): - q.Add(reconcile.Request{NamespacedName: types.NamespacedName{ + item := reconcile.Request{NamespacedName: types.NamespacedName{ Name: evt.ObjectNew.GetName(), Namespace: evt.ObjectNew.GetNamespace(), - }}) + }} + + addToQueueUpdate(q, evt, item) case !isNil(evt.ObjectOld): - q.Add(reconcile.Request{NamespacedName: types.NamespacedName{ + item := reconcile.Request{NamespacedName: types.NamespacedName{ Name: evt.ObjectOld.GetName(), Namespace: evt.ObjectOld.GetNamespace(), - }}) + }} + + addToQueueUpdate(q, evt, item) default: enqueueLog.Error(nil, "UpdateEvent received with no metadata", "event", evt) } diff --git a/pkg/handler/enqueue_mapped.go b/pkg/handler/enqueue_mapped.go index 491bc40c42..be97fa3781 100644 --- a/pkg/handler/enqueue_mapped.go +++ b/pkg/handler/enqueue_mapped.go @@ -21,6 +21,7 @@ import ( "k8s.io/client-go/util/workqueue" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/priorityqueue" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/reconcile" ) @@ -63,7 +64,8 @@ func EnqueueRequestsFromMapFunc(fn MapFunc) EventHandler { // TypedEnqueueRequestsFromMapFunc is experimental and subject to future change. func TypedEnqueueRequestsFromMapFunc[object any, request comparable](fn TypedMapFunc[object, request]) TypedEventHandler[object, request] { return &enqueueRequestsFromMapFunc[object, request]{ - toRequests: fn, + toRequests: fn, + objectImplementsClientObject: implementsClientObject[object](), } } @@ -71,7 +73,8 @@ var _ EventHandler = &enqueueRequestsFromMapFunc[client.Object, reconcile.Reques type enqueueRequestsFromMapFunc[object any, request comparable] struct { // Mapper transforms the argument into a slice of keys to be reconciled - toRequests TypedMapFunc[object, request] + toRequests TypedMapFunc[object, request] + objectImplementsClientObject bool } // Create implements EventHandler. @@ -81,7 +84,15 @@ func (e *enqueueRequestsFromMapFunc[object, request]) Create( q workqueue.TypedRateLimitingInterface[request], ) { reqs := map[request]empty{} - e.mapAndEnqueue(ctx, q, evt.Object, reqs) + + var lowPriority bool + if e.objectImplementsClientObject && isPriorityQueue(q) && !isNil(evt.Object) { + clientObjectEvent := event.CreateEvent{Object: any(evt.Object).(client.Object)} + if isObjectUnchanged(clientObjectEvent) { + lowPriority = true + } + } + e.mapAndEnqueue(ctx, q, evt.Object, reqs, lowPriority) } // Update implements EventHandler. @@ -90,9 +101,13 @@ func (e *enqueueRequestsFromMapFunc[object, request]) Update( evt event.TypedUpdateEvent[object], q workqueue.TypedRateLimitingInterface[request], ) { + var lowPriority bool + if e.objectImplementsClientObject && isPriorityQueue(q) && !isNil(evt.ObjectOld) && !isNil(evt.ObjectNew) { + lowPriority = any(evt.ObjectOld).(client.Object).GetResourceVersion() == any(evt.ObjectNew).(client.Object).GetResourceVersion() + } reqs := map[request]empty{} - e.mapAndEnqueue(ctx, q, evt.ObjectOld, reqs) - e.mapAndEnqueue(ctx, q, evt.ObjectNew, reqs) + e.mapAndEnqueue(ctx, q, evt.ObjectOld, reqs, lowPriority) + e.mapAndEnqueue(ctx, q, evt.ObjectNew, reqs, lowPriority) } // Delete implements EventHandler. @@ -102,7 +117,7 @@ func (e *enqueueRequestsFromMapFunc[object, request]) Delete( q workqueue.TypedRateLimitingInterface[request], ) { reqs := map[request]empty{} - e.mapAndEnqueue(ctx, q, evt.Object, reqs) + e.mapAndEnqueue(ctx, q, evt.Object, reqs, false) } // Generic implements EventHandler. @@ -112,14 +127,26 @@ func (e *enqueueRequestsFromMapFunc[object, request]) Generic( q workqueue.TypedRateLimitingInterface[request], ) { reqs := map[request]empty{} - e.mapAndEnqueue(ctx, q, evt.Object, reqs) + e.mapAndEnqueue(ctx, q, evt.Object, reqs, false) } -func (e *enqueueRequestsFromMapFunc[object, request]) mapAndEnqueue(ctx context.Context, q workqueue.TypedRateLimitingInterface[request], o object, reqs map[request]empty) { +func (e *enqueueRequestsFromMapFunc[object, request]) mapAndEnqueue( + ctx context.Context, + q workqueue.TypedRateLimitingInterface[request], + o object, + reqs map[request]empty, + lowPriority bool, +) { for _, req := range e.toRequests(ctx, o) { _, ok := reqs[req] if !ok { - q.Add(req) + if lowPriority { + q.(priorityqueue.PriorityQueue[request]).AddWithOpts(priorityqueue.AddOpts{ + Priority: LowPriority, + }, req) + } else { + q.Add(req) + } reqs[req] = empty{} } } diff --git a/pkg/handler/enqueue_owner.go b/pkg/handler/enqueue_owner.go index 1680043b46..e8fc8eb46e 100644 --- a/pkg/handler/enqueue_owner.go +++ b/pkg/handler/enqueue_owner.go @@ -72,7 +72,7 @@ func TypedEnqueueRequestForOwner[object client.Object](scheme *runtime.Scheme, m for _, opt := range opts { opt(e) } - return e + return WithLowPriorityWhenUnchanged(e) } // OnlyControllerOwner if provided will only look at the first OwnerReference with Controller: true. diff --git a/pkg/handler/eventhandler.go b/pkg/handler/eventhandler.go index 57107f20e9..84a10ac077 100644 --- a/pkg/handler/eventhandler.go +++ b/pkg/handler/eventhandler.go @@ -18,6 +18,7 @@ package handler import ( "context" + "reflect" "time" "k8s.io/client-go/util/workqueue" @@ -108,10 +109,46 @@ type TypedFuncs[object any, request comparable] struct { GenericFunc func(context.Context, event.TypedGenericEvent[object], workqueue.TypedRateLimitingInterface[request]) } +var typeForClientObject = reflect.TypeFor[client.Object]() + +func implementsClientObject[object any]() bool { + return reflect.TypeFor[object]().Implements(typeForClientObject) +} + +func isPriorityQueue[request comparable](q workqueue.TypedRateLimitingInterface[request]) bool { + _, ok := q.(priorityqueue.PriorityQueue[request]) + return ok +} + // Create implements EventHandler. func (h TypedFuncs[object, request]) Create(ctx context.Context, e event.TypedCreateEvent[object], q workqueue.TypedRateLimitingInterface[request]) { if h.CreateFunc != nil { - h.CreateFunc(ctx, e, q) + if !implementsClientObject[object]() || !isPriorityQueue(q) || isNil(e.Object) { + h.CreateFunc(ctx, e, q) + return + } + wq := workqueueWithCustomAddFunc[request]{ + TypedRateLimitingInterface: q, + // We already know that we have a priority queue, that event.Object implements + // client.Object and that its not nil + addFunc: func(item request, q workqueue.TypedRateLimitingInterface[request]) { + // We construct a new event typed to client.Object because isObjectUnchanged + // is a generic and hence has to know at compile time the type of the event + // it gets. We only figure that out at runtime though, but we know for sure + // that it implements client.Object at this point so we can hardcode the event + // type to that. + evt := event.CreateEvent{Object: any(e.Object).(client.Object)} + var priority int + if isObjectUnchanged(evt) { + priority = LowPriority + } + q.(priorityqueue.PriorityQueue[request]).AddWithOpts( + priorityqueue.AddOpts{Priority: priority}, + item, + ) + }, + } + h.CreateFunc(ctx, e, wq) } } @@ -125,7 +162,27 @@ func (h TypedFuncs[object, request]) Delete(ctx context.Context, e event.TypedDe // Update implements EventHandler. func (h TypedFuncs[object, request]) Update(ctx context.Context, e event.TypedUpdateEvent[object], q workqueue.TypedRateLimitingInterface[request]) { if h.UpdateFunc != nil { - h.UpdateFunc(ctx, e, q) + if !implementsClientObject[object]() || !isPriorityQueue(q) || isNil(e.ObjectOld) || isNil(e.ObjectNew) { + h.UpdateFunc(ctx, e, q) + return + } + + wq := workqueueWithCustomAddFunc[request]{ + TypedRateLimitingInterface: q, + // We already know that we have a priority queue, that event.ObjectOld and ObjectNew implement + // client.Object and that they are not nil + addFunc: func(item request, q workqueue.TypedRateLimitingInterface[request]) { + var priority int + if any(e.ObjectOld).(client.Object).GetResourceVersion() == any(e.ObjectNew).(client.Object).GetResourceVersion() { + priority = LowPriority + } + q.(priorityqueue.PriorityQueue[request]).AddWithOpts( + priorityqueue.AddOpts{Priority: priority}, + item, + ) + }, + } + h.UpdateFunc(ctx, e, wq) } } @@ -142,43 +199,10 @@ const LowPriority = -100 // WithLowPriorityWhenUnchanged reduces the priority of events stemming from the initial listwatch or from a resync if // and only if a priorityqueue.PriorityQueue is used. If not, it does nothing. func WithLowPriorityWhenUnchanged[object client.Object, request comparable](u TypedEventHandler[object, request]) TypedEventHandler[object, request] { + // TypedFuncs already implements this so just wrap return TypedFuncs[object, request]{ - CreateFunc: func(ctx context.Context, tce event.TypedCreateEvent[object], trli workqueue.TypedRateLimitingInterface[request]) { - // Due to how the handlers are factored, we have to wrap the workqueue to be able - // to inject custom behavior. - u.Create(ctx, tce, workqueueWithCustomAddFunc[request]{ - TypedRateLimitingInterface: trli, - addFunc: func(item request, q workqueue.TypedRateLimitingInterface[request]) { - priorityQueue, isPriorityQueue := q.(priorityqueue.PriorityQueue[request]) - if !isPriorityQueue { - q.Add(item) - return - } - var priority int - if isObjectUnchanged(tce) { - priority = LowPriority - } - priorityQueue.AddWithOpts(priorityqueue.AddOpts{Priority: priority}, item) - }, - }) - }, - UpdateFunc: func(ctx context.Context, tue event.TypedUpdateEvent[object], trli workqueue.TypedRateLimitingInterface[request]) { - u.Update(ctx, tue, workqueueWithCustomAddFunc[request]{ - TypedRateLimitingInterface: trli, - addFunc: func(item request, q workqueue.TypedRateLimitingInterface[request]) { - priorityQueue, isPriorityQueue := q.(priorityqueue.PriorityQueue[request]) - if !isPriorityQueue { - q.Add(item) - return - } - var priority int - if tue.ObjectOld.GetResourceVersion() == tue.ObjectNew.GetResourceVersion() { - priority = LowPriority - } - priorityQueue.AddWithOpts(priorityqueue.AddOpts{Priority: priority}, item) - }, - }) - }, + CreateFunc: u.Create, + UpdateFunc: u.Update, DeleteFunc: u.Delete, GenericFunc: u.Generic, } @@ -199,3 +223,35 @@ func (w workqueueWithCustomAddFunc[request]) Add(item request) { func isObjectUnchanged[object client.Object](e event.TypedCreateEvent[object]) bool { return e.Object.GetCreationTimestamp().Time.Before(time.Now().Add(-time.Minute)) } + +// addToQueueCreate adds the reconcile.Request to the priorityqueue in the handler +// for Create requests if and only if the workqueue being used is of type priorityqueue.PriorityQueue[reconcile.Request] +func addToQueueCreate[T client.Object, request comparable](q workqueue.TypedRateLimitingInterface[request], evt event.TypedCreateEvent[T], item request) { + priorityQueue, isPriorityQueue := q.(priorityqueue.PriorityQueue[request]) + if !isPriorityQueue { + q.Add(item) + return + } + + var priority int + if isObjectUnchanged(evt) { + priority = LowPriority + } + priorityQueue.AddWithOpts(priorityqueue.AddOpts{Priority: priority}, item) +} + +// addToQueueUpdate adds the reconcile.Request to the priorityqueue in the handler +// for Update requests if and only if the workqueue being used is of type priorityqueue.PriorityQueue[reconcile.Request] +func addToQueueUpdate[T client.Object, request comparable](q workqueue.TypedRateLimitingInterface[request], evt event.TypedUpdateEvent[T], item request) { + priorityQueue, isPriorityQueue := q.(priorityqueue.PriorityQueue[request]) + if !isPriorityQueue { + q.Add(item) + return + } + + var priority int + if evt.ObjectOld.GetResourceVersion() == evt.ObjectNew.GetResourceVersion() { + priority = LowPriority + } + priorityQueue.AddWithOpts(priorityqueue.AddOpts{Priority: priority}, item) +} diff --git a/pkg/handler/eventhandler_test.go b/pkg/handler/eventhandler_test.go index 6e57c22c3b..748aa3fcc4 100644 --- a/pkg/handler/eventhandler_test.go +++ b/pkg/handler/eventhandler_test.go @@ -659,7 +659,7 @@ var _ = Describe("Eventhandler", func() { }) Describe("Funcs", func() { - failingFuncs := handler.Funcs{ + failingFuncs := handler.TypedFuncs[client.Object, reconcile.Request]{ CreateFunc: func(context.Context, event.CreateEvent, workqueue.TypedRateLimitingInterface[reconcile.Request]) { defer GinkgoRecover() Fail("Did not expect CreateEvent to be called.") @@ -776,128 +776,224 @@ var _ = Describe("Eventhandler", func() { }) Describe("WithLowPriorityWhenUnchanged", func() { - It("should lower the priority of a create request for an object that was created more than one minute in the past", func() { - actualOpts := priorityqueue.AddOpts{} - var actualRequests []reconcile.Request - wq := &fakePriorityQueue{ - addWithOpts: func(o priorityqueue.AddOpts, items ...reconcile.Request) { - actualOpts = o - actualRequests = items + handlerPriorityTests := []struct { + name string + handler func() handler.EventHandler + }{ + { + name: "WithLowPriorityWhenUnchanged wrapper", + handler: func() handler.EventHandler { return handler.WithLowPriorityWhenUnchanged(customHandler{}) }, + }, + { + name: "EnqueueRequestForObject", + handler: func() handler.EventHandler { return &handler.EnqueueRequestForObject{} }, + }, + { + name: "EnqueueRequestForOwner", + handler: func() handler.EventHandler { + return handler.EnqueueRequestForOwner( + scheme.Scheme, + mapper, + &corev1.Pod{}, + ) }, - } - - h := handler.WithLowPriorityWhenUnchanged(&handler.EnqueueRequestForObject{}) - h.Create(ctx, event.CreateEvent{ - Object: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ - Name: "my-pod", - }}, - }, wq) - - Expect(actualOpts).To(Equal(priorityqueue.AddOpts{Priority: handler.LowPriority})) - Expect(actualRequests).To(Equal([]reconcile.Request{{NamespacedName: types.NamespacedName{Name: "my-pod"}}})) - }) - - It("should not lower the priority of a create request for an object that was created less than one minute in the past", func() { - actualOpts := priorityqueue.AddOpts{} - var actualRequests []reconcile.Request - wq := &fakePriorityQueue{ - addWithOpts: func(o priorityqueue.AddOpts, items ...reconcile.Request) { - actualOpts = o - actualRequests = items + }, + { + name: "TypedEnqueueRequestForOwner", + handler: func() handler.EventHandler { + return handler.TypedEnqueueRequestForOwner[client.Object]( + scheme.Scheme, + mapper, + &corev1.Pod{}, + ) }, - } - - h := handler.WithLowPriorityWhenUnchanged(&handler.EnqueueRequestForObject{}) - h.Create(ctx, event.CreateEvent{ - Object: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ - Name: "my-pod", - CreationTimestamp: metav1.Now(), - }}, - }, wq) - - Expect(actualOpts).To(Equal(priorityqueue.AddOpts{})) - Expect(actualRequests).To(Equal([]reconcile.Request{{NamespacedName: types.NamespacedName{Name: "my-pod"}}})) - }) - - It("should lower the priority of an update request with unchanged RV", func() { - actualOpts := priorityqueue.AddOpts{} - var actualRequests []reconcile.Request - wq := &fakePriorityQueue{ - addWithOpts: func(o priorityqueue.AddOpts, items ...reconcile.Request) { - actualOpts = o - actualRequests = items + }, + { + name: "Funcs", + handler: func() handler.EventHandler { + return handler.TypedFuncs[client.Object, reconcile.Request]{ + CreateFunc: func(ctx context.Context, tce event.TypedCreateEvent[client.Object], wq workqueue.TypedRateLimitingInterface[reconcile.Request]) { + wq.Add(reconcile.Request{NamespacedName: types.NamespacedName{ + Namespace: tce.Object.GetNamespace(), + Name: tce.Object.GetName(), + }}) + }, + UpdateFunc: func(ctx context.Context, tue event.TypedUpdateEvent[client.Object], wq workqueue.TypedRateLimitingInterface[reconcile.Request]) { + wq.Add(reconcile.Request{NamespacedName: types.NamespacedName{ + Namespace: tue.ObjectNew.GetNamespace(), + Name: tue.ObjectNew.GetName(), + }}) + }, + } }, - } + }, + { + name: "EnqueueRequestsFromMapFunc", + handler: func() handler.EventHandler { + return handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, obj client.Object) []reconcile.Request { + return []reconcile.Request{{NamespacedName: types.NamespacedName{ + Name: obj.GetName(), + Namespace: obj.GetNamespace(), + }}} + }) + }, + }, + } + for _, test := range handlerPriorityTests { + When("handler is "+test.name, func() { + It("should lower the priority of a create request for an object that was created more than one minute in the past", func() { + actualOpts := priorityqueue.AddOpts{} + var actualRequests []reconcile.Request + wq := &fakePriorityQueue{ + addWithOpts: func(o priorityqueue.AddOpts, items ...reconcile.Request) { + actualOpts = o + actualRequests = items + }, + } - h := handler.WithLowPriorityWhenUnchanged(&handler.EnqueueRequestForObject{}) - h.Update(ctx, event.UpdateEvent{ - ObjectOld: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ - Name: "my-pod", - }}, - ObjectNew: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ - Name: "my-pod", - }}, - }, wq) + test.handler().Create(ctx, event.CreateEvent{ + Object: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ + Name: "my-pod", + OwnerReferences: []metav1.OwnerReference{{ + Kind: "Pod", + Name: "my-pod", + }}, + }}, + }, wq) + + Expect(actualOpts).To(Equal(priorityqueue.AddOpts{Priority: handler.LowPriority})) + Expect(actualRequests).To(Equal([]reconcile.Request{{NamespacedName: types.NamespacedName{Name: "my-pod"}}})) + }) - Expect(actualOpts).To(Equal(priorityqueue.AddOpts{Priority: handler.LowPriority})) - Expect(actualRequests).To(Equal([]reconcile.Request{{NamespacedName: types.NamespacedName{Name: "my-pod"}}})) - }) + It("should not lower the priority of a create request for an object that was created less than one minute in the past", func() { + actualOpts := priorityqueue.AddOpts{} + var actualRequests []reconcile.Request + wq := &fakePriorityQueue{ + addWithOpts: func(o priorityqueue.AddOpts, items ...reconcile.Request) { + actualOpts = o + actualRequests = items + }, + } - It("should not lower the priority of an update request with changed RV", func() { - actualOpts := priorityqueue.AddOpts{} - var actualRequests []reconcile.Request - wq := &fakePriorityQueue{ - addWithOpts: func(o priorityqueue.AddOpts, items ...reconcile.Request) { - actualOpts = o - actualRequests = items - }, - } + test.handler().Create(ctx, event.CreateEvent{ + Object: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ + Name: "my-pod", + CreationTimestamp: metav1.Now(), + OwnerReferences: []metav1.OwnerReference{{ + Kind: "Pod", + Name: "my-pod", + }}, + }}, + }, wq) + + Expect(actualOpts).To(Equal(priorityqueue.AddOpts{})) + Expect(actualRequests).To(Equal([]reconcile.Request{{NamespacedName: types.NamespacedName{Name: "my-pod"}}})) + }) - h := handler.WithLowPriorityWhenUnchanged(&handler.EnqueueRequestForObject{}) - h.Update(ctx, event.UpdateEvent{ - ObjectOld: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ - Name: "my-pod", - }}, - ObjectNew: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ - Name: "my-pod", - ResourceVersion: "1", - }}, - }, wq) + It("should lower the priority of an update request with unchanged RV", func() { + actualOpts := priorityqueue.AddOpts{} + var actualRequests []reconcile.Request + wq := &fakePriorityQueue{ + addWithOpts: func(o priorityqueue.AddOpts, items ...reconcile.Request) { + actualOpts = o + actualRequests = items + }, + } - Expect(actualOpts).To(Equal(priorityqueue.AddOpts{})) - Expect(actualRequests).To(Equal([]reconcile.Request{{NamespacedName: types.NamespacedName{Name: "my-pod"}}})) - }) + test.handler().Update(ctx, event.UpdateEvent{ + ObjectOld: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ + Name: "my-pod", + OwnerReferences: []metav1.OwnerReference{{ + Kind: "Pod", + Name: "my-pod", + }}, + }}, + ObjectNew: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ + Name: "my-pod", + OwnerReferences: []metav1.OwnerReference{{ + Kind: "Pod", + Name: "my-pod", + }}, + }}, + }, wq) + + Expect(actualOpts).To(Equal(priorityqueue.AddOpts{Priority: handler.LowPriority})) + Expect(actualRequests).To(Equal([]reconcile.Request{{NamespacedName: types.NamespacedName{Name: "my-pod"}}})) + }) - It("should have no effect on create if the workqueue is not a priorityqueue", func() { - h := handler.WithLowPriorityWhenUnchanged(&handler.EnqueueRequestForObject{}) - h.Create(ctx, event.CreateEvent{ - Object: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ - Name: "my-pod", - }}, - }, q) + It("should not lower the priority of an update request with changed RV", func() { + actualOpts := priorityqueue.AddOpts{} + var actualRequests []reconcile.Request + wq := &fakePriorityQueue{ + addWithOpts: func(o priorityqueue.AddOpts, items ...reconcile.Request) { + actualOpts = o + actualRequests = items + }, + } - Expect(q.Len()).To(Equal(1)) - item, _ := q.Get() - Expect(item).To(Equal(reconcile.Request{NamespacedName: types.NamespacedName{Name: "my-pod"}})) - }) + test.handler().Update(ctx, event.UpdateEvent{ + ObjectOld: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ + Name: "my-pod", + OwnerReferences: []metav1.OwnerReference{{ + Kind: "Pod", + Name: "my-pod", + }}, + }}, + ObjectNew: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ + Name: "my-pod", + ResourceVersion: "1", + OwnerReferences: []metav1.OwnerReference{{ + Kind: "Pod", + Name: "my-pod", + }}, + }}, + }, wq) + + Expect(actualOpts).To(Equal(priorityqueue.AddOpts{})) + Expect(actualRequests).To(Equal([]reconcile.Request{{NamespacedName: types.NamespacedName{Name: "my-pod"}}})) + }) - It("should have no effect on Update if the workqueue is not a priorityqueue", func() { - h := handler.WithLowPriorityWhenUnchanged(&handler.EnqueueRequestForObject{}) - h.Update(ctx, event.UpdateEvent{ - ObjectOld: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ - Name: "my-pod", - }}, - ObjectNew: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ - Name: "my-pod", - }}, - }, q) + It("should have no effect on create if the workqueue is not a priorityqueue", func() { + test.handler().Create(ctx, event.CreateEvent{ + Object: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ + Name: "my-pod", + OwnerReferences: []metav1.OwnerReference{{ + Kind: "Pod", + Name: "my-pod", + }}, + }}, + }, q) + + Expect(q.Len()).To(Equal(1)) + item, _ := q.Get() + Expect(item).To(Equal(reconcile.Request{NamespacedName: types.NamespacedName{Name: "my-pod"}})) + }) - Expect(q.Len()).To(Equal(1)) - item, _ := q.Get() - Expect(item).To(Equal(reconcile.Request{NamespacedName: types.NamespacedName{Name: "my-pod"}})) - }) + It("should have no effect on Update if the workqueue is not a priorityqueue", func() { + test.handler().Update(ctx, event.UpdateEvent{ + ObjectOld: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ + Name: "my-pod", + OwnerReferences: []metav1.OwnerReference{{ + Kind: "Pod", + Name: "my-pod", + }}, + }}, + ObjectNew: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{ + Name: "my-pod", + OwnerReferences: []metav1.OwnerReference{{ + Kind: "Pod", + Name: "my-pod", + }}, + }}, + }, q) + + Expect(q.Len()).To(Equal(1)) + item, _ := q.Get() + Expect(item).To(Equal(reconcile.Request{NamespacedName: types.NamespacedName{Name: "my-pod"}})) + }) + }) + } }) - }) type fakePriorityQueue struct { @@ -905,9 +1001,42 @@ type fakePriorityQueue struct { addWithOpts func(o priorityqueue.AddOpts, items ...reconcile.Request) } +func (f *fakePriorityQueue) Add(item reconcile.Request) { + f.AddWithOpts(priorityqueue.AddOpts{}, item) +} + func (f *fakePriorityQueue) AddWithOpts(o priorityqueue.AddOpts, items ...reconcile.Request) { f.addWithOpts(o, items...) } func (f *fakePriorityQueue) GetWithPriority() (item reconcile.Request, priority int, shutdown bool) { panic("GetWithPriority is not expected to be called") } + +// customHandler re-implements the basic enqueueRequestForObject logic +// to be able to test the WithLowPriorityWhenUnchanged wrapper +type customHandler struct{} + +func (ch customHandler) Create(ctx context.Context, evt event.CreateEvent, q workqueue.TypedRateLimitingInterface[reconcile.Request]) { + q.Add(reconcile.Request{NamespacedName: types.NamespacedName{ + Namespace: evt.Object.GetNamespace(), + Name: evt.Object.GetName(), + }}) +} +func (ch customHandler) Update(ctx context.Context, evt event.UpdateEvent, q workqueue.TypedRateLimitingInterface[reconcile.Request]) { + q.Add(reconcile.Request{NamespacedName: types.NamespacedName{ + Namespace: evt.ObjectNew.GetNamespace(), + Name: evt.ObjectNew.GetName(), + }}) +} +func (ch customHandler) Delete(ctx context.Context, evt event.DeleteEvent, q workqueue.TypedRateLimitingInterface[reconcile.Request]) { + q.Add(reconcile.Request{NamespacedName: types.NamespacedName{ + Namespace: evt.Object.GetNamespace(), + Name: evt.Object.GetName(), + }}) +} +func (ch customHandler) Generic(ctx context.Context, evt event.GenericEvent, q workqueue.TypedRateLimitingInterface[reconcile.Request]) { + q.Add(reconcile.Request{NamespacedName: types.NamespacedName{ + Namespace: evt.Object.GetNamespace(), + Name: evt.Object.GetName(), + }}) +} diff --git a/pkg/internal/controller/controller.go b/pkg/internal/controller/controller.go index cc734dfb61..3f8cfdaa09 100644 --- a/pkg/internal/controller/controller.go +++ b/pkg/internal/controller/controller.go @@ -31,6 +31,7 @@ import ( "k8s.io/apimachinery/pkg/util/uuid" "k8s.io/client-go/util/workqueue" + "sigs.k8s.io/controller-runtime/pkg/controller/priorityqueue" ctrlmetrics "sigs.k8s.io/controller-runtime/pkg/internal/controller/metrics" logf "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" @@ -60,7 +61,7 @@ type Controller[request comparable] struct { // Queue is an listeningQueue that listens for events from Informers and adds object keys to // the Queue for processing - Queue workqueue.TypedRateLimitingInterface[request] + Queue priorityqueue.PriorityQueue[request] // mu is used to synchronize Controller setup mu sync.Mutex @@ -157,7 +158,12 @@ func (c *Controller[request]) Start(ctx context.Context) error { // Set the internal context. c.ctx = ctx - c.Queue = c.NewQueue(c.Name, c.RateLimiter) + queue := c.NewQueue(c.Name, c.RateLimiter) + if priorityQueue, isPriorityQueue := queue.(priorityqueue.PriorityQueue[request]); isPriorityQueue { + c.Queue = priorityQueue + } else { + c.Queue = &priorityQueueWrapper[request]{TypedRateLimitingInterface: queue} + } go func() { <-ctx.Done() c.Queue.ShutDown() @@ -268,7 +274,7 @@ func (c *Controller[request]) Start(ctx context.Context) error { // processNextWorkItem will read a single work item off the workqueue and // attempt to process it, by calling the reconcileHandler. func (c *Controller[request]) processNextWorkItem(ctx context.Context) bool { - obj, shutdown := c.Queue.Get() + obj, priority, shutdown := c.Queue.GetWithPriority() if shutdown { // Stop working return false @@ -285,7 +291,7 @@ func (c *Controller[request]) processNextWorkItem(ctx context.Context) bool { ctrlmetrics.ActiveWorkers.WithLabelValues(c.Name).Add(1) defer ctrlmetrics.ActiveWorkers.WithLabelValues(c.Name).Add(-1) - c.reconcileHandler(ctx, obj) + c.reconcileHandler(ctx, obj, priority) return true } @@ -308,7 +314,7 @@ func (c *Controller[request]) initMetrics() { ctrlmetrics.ActiveWorkers.WithLabelValues(c.Name).Set(0) } -func (c *Controller[request]) reconcileHandler(ctx context.Context, req request) { +func (c *Controller[request]) reconcileHandler(ctx context.Context, req request, priority int) { // Update metrics after processing each item reconcileStartTS := time.Now() defer func() { @@ -331,7 +337,7 @@ func (c *Controller[request]) reconcileHandler(ctx context.Context, req request) if errors.Is(err, reconcile.TerminalError(nil)) { ctrlmetrics.TerminalReconcileErrors.WithLabelValues(c.Name).Inc() } else { - c.Queue.AddRateLimited(req) + c.Queue.AddWithOpts(priorityqueue.AddOpts{RateLimited: true, Priority: priority}, req) } ctrlmetrics.ReconcileErrors.WithLabelValues(c.Name).Inc() ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, labelError).Inc() @@ -346,11 +352,11 @@ func (c *Controller[request]) reconcileHandler(ctx context.Context, req request) // We need to drive to stable reconcile loops before queuing due // to result.RequestAfter c.Queue.Forget(req) - c.Queue.AddAfter(req, result.RequeueAfter) + c.Queue.AddWithOpts(priorityqueue.AddOpts{After: result.RequeueAfter, Priority: priority}, req) ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, labelRequeueAfter).Inc() case result.Requeue: log.V(5).Info("Reconcile done, requeueing") - c.Queue.AddRateLimited(req) + c.Queue.AddWithOpts(priorityqueue.AddOpts{RateLimited: true, Priority: priority}, req) ctrlmetrics.ReconcileTotal.WithLabelValues(c.Name, labelRequeue).Inc() default: log.V(5).Info("Reconcile successful") @@ -388,3 +394,25 @@ type reconcileIDKey struct{} func addReconcileID(ctx context.Context, reconcileID types.UID) context.Context { return context.WithValue(ctx, reconcileIDKey{}, reconcileID) } + +type priorityQueueWrapper[request comparable] struct { + workqueue.TypedRateLimitingInterface[request] +} + +func (p *priorityQueueWrapper[request]) AddWithOpts(opts priorityqueue.AddOpts, items ...request) { + for _, item := range items { + switch { + case opts.RateLimited: + p.TypedRateLimitingInterface.AddRateLimited(item) + case opts.After > 0: + p.TypedRateLimitingInterface.AddAfter(item, opts.After) + default: + p.TypedRateLimitingInterface.Add(item) + } + } +} + +func (p *priorityQueueWrapper[request]) GetWithPriority() (request, int, bool) { + item, shutdown := p.TypedRateLimitingInterface.Get() + return item, 0, shutdown +} diff --git a/pkg/internal/controller/controller_test.go b/pkg/internal/controller/controller_test.go index 3a23156a9c..bf334d22e8 100644 --- a/pkg/internal/controller/controller_test.go +++ b/pkg/internal/controller/controller_test.go @@ -38,6 +38,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/cache/informertest" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllertest" + "sigs.k8s.io/controller-runtime/pkg/controller/priorityqueue" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" ctrlmetrics "sigs.k8s.io/controller-runtime/pkg/internal/controller/metrics" @@ -345,9 +346,10 @@ var _ = Describe("controller", func() { }) It("should check for correct TypedSyncingSource if custom types are used", func() { - queue := &controllertest.TypedQueue[TestRequest]{ - TypedInterface: workqueue.NewTyped[TestRequest](), - } + queue := &priorityQueueWrapper[TestRequest]{ + TypedRateLimitingInterface: &controllertest.TypedQueue[TestRequest]{ + TypedInterface: workqueue.NewTyped[TestRequest](), + }} ctrl := &Controller[TestRequest]{ NewQueue: func(string, workqueue.TypedRateLimiter[TestRequest]) workqueue.TypedRateLimitingInterface[TestRequest] { return queue @@ -400,10 +402,6 @@ var _ = Describe("controller", func() { Eventually(func() int { return queue.NumRequeues(request) }).Should(Equal(0)) }) - PIt("should forget an item if it is not a Request and continue processing items", func() { - // TODO(community): write this test - }) - It("should requeue a Request if there is an error and continue processing items", func() { ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -523,6 +521,37 @@ var _ = Describe("controller", func() { Eventually(func() int { return dq.NumRequeues(request) }).Should(Equal(0)) }) + It("should retain the priority when the reconciler requests a requeue", func() { + q := &fakePriorityQueue{PriorityQueue: priorityqueue.New[reconcile.Request]("controller1")} + ctrl.NewQueue = func(string, workqueue.TypedRateLimiter[reconcile.Request]) workqueue.TypedRateLimitingInterface[reconcile.Request] { + return q + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go func() { + defer GinkgoRecover() + Expect(ctrl.Start(ctx)).NotTo(HaveOccurred()) + }() + + q.PriorityQueue.AddWithOpts(priorityqueue.AddOpts{Priority: 10}, request) + + By("Invoking Reconciler which will request a requeue") + fakeReconcile.AddResult(reconcile.Result{Requeue: true}, nil) + Expect(<-reconciled).To(Equal(request)) + Eventually(func() []priorityQueueAddition { + q.lock.Lock() + defer q.lock.Unlock() + return q.added + }).Should(Equal([]priorityQueueAddition{{ + AddOpts: priorityqueue.AddOpts{ + RateLimited: true, + Priority: 10, + }, + items: []reconcile.Request{request}, + }})) + }) + It("should requeue a Request after a duration (but not rate-limitted) if the Result sets RequeueAfter (regardless of Requeue)", func() { dq := &DelegatingQueue{TypedRateLimitingInterface: ctrl.NewQueue("controller1", nil)} ctrl.NewQueue = func(string, workqueue.TypedRateLimiter[reconcile.Request]) workqueue.TypedRateLimitingInterface[reconcile.Request] { @@ -555,6 +584,37 @@ var _ = Describe("controller", func() { Eventually(func() int { return dq.NumRequeues(request) }).Should(Equal(0)) }) + It("should retain the priority with RequeAfter", func() { + q := &fakePriorityQueue{PriorityQueue: priorityqueue.New[reconcile.Request]("controller1")} + ctrl.NewQueue = func(string, workqueue.TypedRateLimiter[reconcile.Request]) workqueue.TypedRateLimitingInterface[reconcile.Request] { + return q + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go func() { + defer GinkgoRecover() + Expect(ctrl.Start(ctx)).NotTo(HaveOccurred()) + }() + + q.PriorityQueue.AddWithOpts(priorityqueue.AddOpts{Priority: 10}, request) + + By("Invoking Reconciler which will ask for RequeueAfter") + fakeReconcile.AddResult(reconcile.Result{RequeueAfter: time.Millisecond * 100}, nil) + Expect(<-reconciled).To(Equal(request)) + Eventually(func() []priorityQueueAddition { + q.lock.Lock() + defer q.lock.Unlock() + return q.added + }).Should(Equal([]priorityQueueAddition{{ + AddOpts: priorityqueue.AddOpts{ + After: time.Millisecond * 100, + Priority: 10, + }, + items: []reconcile.Request{request}, + }})) + }) + It("should perform error behavior if error is not nil, regardless of RequeueAfter", func() { dq := &DelegatingQueue{TypedRateLimitingInterface: ctrl.NewQueue("controller1", nil)} ctrl.NewQueue = func(string, workqueue.TypedRateLimiter[reconcile.Request]) workqueue.TypedRateLimitingInterface[reconcile.Request] { @@ -586,6 +646,37 @@ var _ = Describe("controller", func() { Eventually(func() int { return dq.NumRequeues(request) }).Should(Equal(0)) }) + It("should retain the priority when there was an error", func() { + q := &fakePriorityQueue{PriorityQueue: priorityqueue.New[reconcile.Request]("controller1")} + ctrl.NewQueue = func(string, workqueue.TypedRateLimiter[reconcile.Request]) workqueue.TypedRateLimitingInterface[reconcile.Request] { + return q + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + go func() { + defer GinkgoRecover() + Expect(ctrl.Start(ctx)).NotTo(HaveOccurred()) + }() + + q.PriorityQueue.AddWithOpts(priorityqueue.AddOpts{Priority: 10}, request) + + By("Invoking Reconciler which will return an error") + fakeReconcile.AddResult(reconcile.Result{}, errors.New("oups, I did it again")) + Expect(<-reconciled).To(Equal(request)) + Eventually(func() []priorityQueueAddition { + q.lock.Lock() + defer q.lock.Unlock() + return q.added + }).Should(Equal([]priorityQueueAddition{{ + AddOpts: priorityqueue.AddOpts{ + RateLimited: true, + Priority: 10, + }, + items: []reconcile.Request{request}, + }})) + }) + PIt("should return if the queue is shutdown", func() { // TODO(community): write this test }) @@ -977,3 +1068,21 @@ func (t *bisignallingSource[T]) WaitForSync(ctx context.Context) error { return ctx.Err() } } + +type priorityQueueAddition struct { + priorityqueue.AddOpts + items []reconcile.Request +} + +type fakePriorityQueue struct { + priorityqueue.PriorityQueue[reconcile.Request] + + lock sync.Mutex + added []priorityQueueAddition +} + +func (f *fakePriorityQueue) AddWithOpts(o priorityqueue.AddOpts, items ...reconcile.Request) { + f.lock.Lock() + defer f.lock.Unlock() + f.added = append(f.added, priorityQueueAddition{AddOpts: o, items: items}) +} diff --git a/tools/setup-envtest/main.go b/tools/setup-envtest/main.go index 3121e206fd..7eb5ec43d3 100644 --- a/tools/setup-envtest/main.go +++ b/tools/setup-envtest/main.go @@ -184,6 +184,9 @@ Commands: reads a .tar.gz file from stdin and expand it into the store. must have a concrete version and platform. + version: + list the installed version of setup-envtest. + Versions: Versions take the form of a small subset of semver selectors. @@ -256,7 +259,6 @@ Environment Variables: version = flag.Arg(1) } env := setupEnv(globalLog, version) - // perform our main set of actions switch action := flag.Arg(0); action { case "use": @@ -274,6 +276,8 @@ Environment Variables: Input: os.Stdin, PrintFormat: printFormat, }.Do(env) + case "version": + workflows.Version{}.Do(env) default: flag.Usage() envp.Exit(2, "unknown action %q", action) diff --git a/tools/setup-envtest/version/version.go b/tools/setup-envtest/version/version.go new file mode 100644 index 0000000000..58e7e309a4 --- /dev/null +++ b/tools/setup-envtest/version/version.go @@ -0,0 +1,21 @@ +package version + +import "runtime/debug" + +// Version to be set using ldflags: +// -ldflags "-X sigs.k8s.io/controller-tools/pkg/version.version=v1.0.0" +// falls back to module information is unse +var version = "" + +// Version returns the version of the main module +func Version() string { + if version != "" { + return version + } + info, ok := debug.ReadBuildInfo() + if !ok || info == nil || info.Main.Version == "" { + // binary has not been built with module support or doesn't contain a version. + return "(unknown)" + } + return info.Main.Version +} diff --git a/tools/setup-envtest/version/version_suite_test.go b/tools/setup-envtest/version/version_suite_test.go new file mode 100644 index 0000000000..99c623e8d4 --- /dev/null +++ b/tools/setup-envtest/version/version_suite_test.go @@ -0,0 +1,27 @@ +/* +Copyright 2024 The Kubernetes Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package version + +import ( + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +func TestVersioning(t *testing.T) { + RegisterFailHandler(Fail) + RunSpecs(t, "Test Version Suite") +} diff --git a/tools/setup-envtest/version/version_test.go b/tools/setup-envtest/version/version_test.go new file mode 100644 index 0000000000..4178cac870 --- /dev/null +++ b/tools/setup-envtest/version/version_test.go @@ -0,0 +1,54 @@ +/* +Copyright 2024 The Kubernetes Authors. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + +See the License for the specific language governing permissions and + +limitations under the License. +*/ + +package version + +import ( + "runtime/debug" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("TestVersion", func() { + + info, ok := debug.ReadBuildInfo() + Expect(ok).To(BeTrue()) + tests := map[string]struct { + version string + expected string + }{ + "empty returns build info": { + version: "", + expected: info.Main.Version, + }, + "set to a value returns it": { + version: "1.2.3", + expected: "1.2.3", + }, + } + for name, tc := range tests { + It("Version set to "+name, func() { + versionBackup := version + defer func() { + version = versionBackup + }() + version = tc.version + result := Version() + Expect(result).To(Equal(tc.expected)) + }) + } +}) diff --git a/tools/setup-envtest/workflows/workflows.go b/tools/setup-envtest/workflows/workflows.go index fdabd995ae..fb9123d269 100644 --- a/tools/setup-envtest/workflows/workflows.go +++ b/tools/setup-envtest/workflows/workflows.go @@ -5,11 +5,13 @@ package workflows import ( "context" + "fmt" "io" "github.com/go-logr/logr" envp "sigs.k8s.io/controller-runtime/tools/setup-envtest/env" + "sigs.k8s.io/controller-runtime/tools/setup-envtest/version" ) // Use is a workflow that prints out information about stored @@ -85,3 +87,12 @@ func (f Sideload) Do(env *envp.Env) { env.Sideload(ctx, f.Input) env.PrintInfo(f.PrintFormat) } + +// Version is the workflow that shows the current binary version +// of setup-envtest. +type Version struct{} + +// Do executes the workflow. +func (v Version) Do(env *envp.Env) { + fmt.Fprintf(env.Out, "setup-envtest version: %s\n", version.Version()) +} diff --git a/tools/setup-envtest/workflows/workflows_test.go b/tools/setup-envtest/workflows/workflows_test.go index 27d4ec6770..435ae24285 100644 --- a/tools/setup-envtest/workflows/workflows_test.go +++ b/tools/setup-envtest/workflows/workflows_test.go @@ -8,6 +8,7 @@ import ( "fmt" "io/fs" "path/filepath" + "runtime/debug" "sort" "strings" @@ -443,4 +444,16 @@ var _ = Describe("Workflows", func() { Expect(string(outContents)).To(HavePrefix(expectedPrefix), "should have the debugging prefix") }) }) + + Describe("version", func() { + It("should print out the version if the RELEASE_TAG is empty", func() { + v := wf.Version{} + v.Do(env) + info, ok := debug.ReadBuildInfo() + Expect(ok).To(BeTrue()) + Expect(out.String()).ToNot(BeEmpty()) + Expect(out.String()).To(Equal(fmt.Sprintf("setup-envtest version: %s\n", info.Main.Version))) + }) + }) + })