react to feedback, make the interface tighter on the gate

Signed-off-by: Scott Nichols <[email protected]>

Author: n3wscott

pkg/controller/gate.go

@@ -9,8 +9,7 @@ type Gate interface {
 	// Register to call a callback function when all given GVKs are marked true. If the callback is unblocked, the
 	// registration is removed.
 	Register(callback func(), gvks ...schema.GroupVersionKind)
-	// True marks a given gvk true, and then looks for unlocked callbacks. Returns if there was an update or not.
-	True(gvk schema.GroupVersionKind) bool
-	// False marks a given gvk false. Returns if there was an update or not.
-	False(gkv schema.GroupVersionKind) bool
+	// Set marks the associated condition to the given value. If the condition is already set as
+	// that value, then this is a no-op. Returns true if there was an update detected.
+	Set(gvk schema.GroupVersionKind, ready bool) bool
 }

pkg/gate/gate.go

@@ -29,9 +29,13 @@ type Gate[T comparable] struct {
 	fns        []gated[T]
 }
 
+// gated is an internal tracking resource.
 type gated[T comparable] struct {
-	fn       func()
-	depends  []T
+	// fn is the function callback we will invoke when all the dependent conditions are true.
+	fn func()
+	// depends is the list of conditions this gated function is waiting on. This is an AND.
+	depends []T
+	// released means the gated function has been invoked and we can garbage collect this gated function.
 	released bool
 }
 
@@ -46,19 +50,9 @@ func (g *Gate[T]) Register(fn func(), depends ...T) {
 	g.process()
 }
 
-// True marks the associated condition as true. If the condition is already true, then this is a no-op.
-// Returns if there was an update detected. Thread safe.
-func (g *Gate[T]) True(condition T) bool {
-	return g.markCondition(condition, true)
-}
-
-// False marks the associated condition as false. If the condition is already false, then this is a no-op.
-// Returns if there was an update detected. Thread safe.
-func (g *Gate[T]) False(condition T) bool {
-	return g.markCondition(condition, false)
-}
-
-func (g *Gate[T]) markCondition(condition T, ready bool) bool {
+// Set marks the associated condition to the given value. If the condition is already set as that value, then this is a
+// no-op. Returns true if there was an update detected. Thread safe.
+func (g *Gate[T]) Set(condition T, value bool) bool {
 	g.mux.Lock()
 
 	if g.conditions == nil {
@@ -68,9 +62,9 @@ func (g *Gate[T]) markCondition(condition T, ready bool) bool {
 	old, found := g.conditions[condition]
 
 	updated := false
-	if !found || old != ready {
+	if !found || old != value {
 		updated = true
-		g.conditions[condition] = ready
+		g.conditions[condition] = value
 	}
 	// process() would also like to lock the mux, so we must unlock here directly and not use defer.
 	g.mux.Unlock()
@@ -87,22 +81,25 @@ func (g *Gate[T]) process() {
 	defer g.mux.Unlock()
 
 	for i := range g.fns {
+		// release controls if we should release the function.
 		release := true
 
 		for _, dep := range g.fns[i].depends {
-			if ready := g.conditions[dep]; !ready {
+			if !g.conditions[dep] {
 				release = false
 			}
 		}
 
 		if release {
 			fn := g.fns[i].fn
+			// mark the function released so we can garbage collect after we are done with the loop.
 			g.fns[i].released = true
 			// Need to capture a copy of fn or else we would be accessing a deleted member when the go routine runs.
 			go fn()
 		}
 	}
 
+	// garbage collect released functions.
 	g.fns = slices.DeleteFunc(g.fns, func(a gated[T]) bool {
 		return a.released
 	})

pkg/gate/gate_test.go

@@ -108,7 +108,7 @@ func TestGate_Integration(t *testing.T) {
 				}, "condition1")
 
 				// Set condition to true (will be initialized as false first)
-				g.True("condition1")
+				g.Set("condition1", true)
 				return called
 			},
 			want: want{
@@ -124,8 +124,8 @@ func TestGate_Integration(t *testing.T) {
 				}, "condition1", "condition2")
 
 				// Set both conditions to true
-				g.True("condition1")
-				g.True("condition2")
+				g.Set("condition1", true)
+				g.Set("condition2", true)
 				return called
 			},
 			want: want{
@@ -141,7 +141,7 @@ func TestGate_Integration(t *testing.T) {
 				}, "condition1", "condition2")
 
 				// Set only one condition to true
-				g.True("condition1")
+				g.Set("condition1", true)
 				return called
 			},
 			want: want{
@@ -153,8 +153,8 @@ func TestGate_Integration(t *testing.T) {
 			setup: func(g *gate.Gate[string]) chan bool {
 				called := make(chan bool, 1)
 
-				g.True("condition1")
-				g.True("condition2")
+				g.Set("condition1", true)
+				g.Set("condition2", true)
 
 				g.Register(func() {
 					called <- true
@@ -175,8 +175,8 @@ func TestGate_Integration(t *testing.T) {
 				}, "condition1")
 
 				// Set condition to true then false (function already called when true)
-				g.True("condition1")
-				g.False("condition1")
+				g.Set("condition1", true)
+				g.Set("condition1", false)
 				return called
 			},
 			want: want{
@@ -192,9 +192,9 @@ func TestGate_Integration(t *testing.T) {
 				}, "condition1")
 
 				// Set condition multiple times
-				g.True("condition1")
-				g.False("condition1")
-				g.True("condition1")
+				g.Set("condition1", true)
+				g.Set("condition1", false)
+				g.Set("condition1", true)
 				return called
 			},
 			want: want{
@@ -259,7 +259,7 @@ func TestGate_Concurrency(t *testing.T) {
 	wg.Wait()
 
 	// Set condition to true once
-	g.True("shared-condition")
+	g.Set("shared-condition", true)
 
 	// Give some time for goroutines to execute
 	time.Sleep(100 * time.Millisecond)
@@ -286,9 +286,9 @@ func TestGate_TypeSafety(t *testing.T) {
 		called = true
 	}, 1, 2, 3)
 
-	intGate.True(1)
-	intGate.True(2)
-	intGate.True(3)
+	intGate.Set(1, true)
+	intGate.Set(2, true)
+	intGate.Set(3, true)
 
 	// Give some time for goroutine to execute
 	time.Sleep(10 * time.Millisecond)

pkg/reconciler/customresourcesgate/reconciler.go

@@ -37,8 +37,6 @@ type Reconciler struct {
 	gate controller.Gate
 }
 
-// +kubebuilder:rbac:groups=apiextensions.k8s.io,resources=customresourcedefinitions,verbs=get;list;watch
-
 // Reconcile reconciles CustomResourceDefinitions and reports ready and unready GVKs to the gate.
 func (r *Reconciler) Reconcile(_ context.Context, crd *apiextensionsv1.CustomResourceDefinition) (ctrl.Result, error) {
 	// If there is no gate, then we don't need to do work.
@@ -55,7 +53,7 @@ func (r *Reconciler) Reconcile(_ context.Context, crd *apiextensionsv1.CustomRes
 	case !established || !crd.GetDeletionTimestamp().IsZero():
 		for gvk := range gkvs {
 			r.log.Debug("gvk is not ready", "gvk", gvk)
-			r.gate.False(gvk)
+			r.gate.Set(gvk, false)
 		}
 
 		return ctrl.Result{}, nil
@@ -65,7 +63,7 @@ func (r *Reconciler) Reconcile(_ context.Context, crd *apiextensionsv1.CustomRes
 		for gvk, served := range gkvs {
 			if served {
 				r.log.Debug("gvk is ready", "gvk", gvk)
-				r.gate.True(gvk)
+				r.gate.Set(gvk, true)
 			}
 		}
 	}

pkg/reconciler/customresourcesgate/reconciler_test.go

@@ -18,6 +18,8 @@ package customresourcesgate
 
 import (
 	"context"
+	"slices"
+	"strings"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -271,23 +273,20 @@ func NewMockGate() *MockGate {
 	}
 }
 
-func (m *MockGate) True(gvk schema.GroupVersionKind) bool {
-	if m.TrueCalls == nil {
-		m.TrueCalls = make([]schema.GroupVersionKind, 0)
-	}
-
-	m.TrueCalls = append(m.TrueCalls, gvk)
+func (m *MockGate) Set(gvk schema.GroupVersionKind, value bool) bool {
+	if value {
+		if m.TrueCalls == nil {
+			m.TrueCalls = make([]schema.GroupVersionKind, 0)
+		}
 
-	return true
-}
+		m.TrueCalls = append(m.TrueCalls, gvk)
+	} else {
+		if m.FalseCalls == nil {
+			m.FalseCalls = make([]schema.GroupVersionKind, 0)
+		}
 
-func (m *MockGate) False(gvk schema.GroupVersionKind) bool {
-	if m.FalseCalls == nil {
-		m.FalseCalls = make([]schema.GroupVersionKind, 0)
+		m.FalseCalls = append(m.FalseCalls, gvk)
 	}
-
-	m.FalseCalls = append(m.FalseCalls, gvk)
-
 	return true
 }
 
@@ -570,10 +569,22 @@ func TestReconcile(t *testing.T) {
 
 			// Only check gate calls if gate is not nil
 			if tc.fields.gate != nil {
+				slices.SortFunc(tc.want.trueCalls, func(a, b schema.GroupVersionKind) int {
+					return strings.Compare(a.Kind, b.Kind)
+				})
+				slices.SortFunc(tc.fields.gate.TrueCalls, func(a, b schema.GroupVersionKind) int {
+					return strings.Compare(a.Kind, b.Kind)
+				})
 				if diff := cmp.Diff(tc.want.trueCalls, tc.fields.gate.TrueCalls); diff != "" {
 					t.Errorf("\n%s\ngate.True calls: -want, +got:\n%s", tc.reason, diff)
 				}
 
+				slices.SortFunc(tc.want.falseCalls, func(a, b schema.GroupVersionKind) int {
+					return strings.Compare(a.Kind, b.Kind)
+				})
+				slices.SortFunc(tc.fields.gate.FalseCalls, func(a, b schema.GroupVersionKind) int {
+					return strings.Compare(a.Kind, b.Kind)
+				})
 				if diff := cmp.Diff(tc.want.falseCalls, tc.fields.gate.FalseCalls); diff != "" {
 					t.Errorf("\n%s\ngate.False calls: -want, +got:\n%s", tc.reason, diff)
 				}