diff --git a/pkg/reconciler/builder/builder.go b/pkg/reconciler/builder/builder.go index 720260813..ac739b11b 100644 --- a/pkg/reconciler/builder/builder.go +++ b/pkg/reconciler/builder/builder.go @@ -5,6 +5,8 @@ import ( "go.uber.org/zap" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" "knative.dev/pkg/logging/logkey" "github.com/google/go-containerregistry/pkg/authn" @@ -110,22 +112,40 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { } func (c *Reconciler) reconcileBuilder(ctx context.Context, builder *buildapi.Builder) (buildapi.BuilderRecord, error) { - clusterStore, err := c.ClusterStoreLister.Get(builder.Spec.Store.Name) + err := c.Tracker.Track(reconciler.Key{ + NamespacedName: types.NamespacedName{ + Name: builder.Spec.Store.Name, + Namespace: v1.NamespaceAll, + }, + GroupKind: schema.GroupKind{ + Group: "kpack.io", + Kind: buildapi.ClusterStoreKind, + }, + }, builder.NamespacedName()) if err != nil { return buildapi.BuilderRecord{}, err } - err = c.Tracker.Track(clusterStore, builder.NamespacedName()) + err = c.Tracker.Track(reconciler.Key{ + NamespacedName: types.NamespacedName{ + Name: builder.Spec.Stack.Name, + Namespace: v1.NamespaceAll, + }, + GroupKind: schema.GroupKind{ + Group: "kpack.io", + Kind: buildapi.ClusterStackKind, + }, + }, builder.NamespacedName()) if err != nil { return buildapi.BuilderRecord{}, err } - clusterStack, err := c.ClusterStackLister.Get(builder.Spec.Stack.Name) + clusterStore, err := c.ClusterStoreLister.Get(builder.Spec.Store.Name) if err != nil { return buildapi.BuilderRecord{}, err } - err = c.Tracker.Track(clusterStack, builder.NamespacedName()) + clusterStack, err := c.ClusterStackLister.Get(builder.Spec.Stack.Name) if err != nil { return buildapi.BuilderRecord{}, err } diff --git a/pkg/reconciler/builder/builder_test.go b/pkg/reconciler/builder/builder_test.go index 7a03a6006..3f9cee9cb 100644 --- a/pkg/reconciler/builder/builder_test.go +++ b/pkg/reconciler/builder/builder_test.go @@ -8,7 +8,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" clientgotesting "k8s.io/client-go/testing" "k8s.io/client-go/tools/record" @@ -62,17 +62,25 @@ func testBuilderReconciler(t *testing.T, when spec.G, it spec.S) { }) clusterStore := &buildapi.ClusterStore{ - ObjectMeta: v1.ObjectMeta{ + ObjectMeta: metav1.ObjectMeta{ Name: "some-store", }, + TypeMeta: metav1.TypeMeta{ + Kind: "ClusterStore", + APIVersion: "kpack.io/v1alpha2", + }, Spec: buildapi.ClusterStoreSpec{}, Status: buildapi.ClusterStoreStatus{}, } clusterStack := &buildapi.ClusterStack{ - ObjectMeta: v1.ObjectMeta{ + ObjectMeta: metav1.ObjectMeta{ Name: "some-stack", }, + TypeMeta: metav1.TypeMeta{ + Kind: "ClusterStack", + APIVersion: "kpack.io/v1alpha2", + }, Status: buildapi.ClusterStackStatus{ Status: corev1alpha1.Status{ ObservedGeneration: 0, @@ -87,7 +95,7 @@ func testBuilderReconciler(t *testing.T, when spec.G, it spec.S) { } builder := &buildapi.Builder{ - ObjectMeta: v1.ObjectMeta{ + ObjectMeta: metav1.ObjectMeta{ Name: builderName, Generation: initialGeneration, Namespace: testNamespace, @@ -257,8 +265,12 @@ func testBuilderReconciler(t *testing.T, when spec.G, it spec.S) { WantErr: false, }) - require.True(t, fakeTracker.IsTracking(clusterStore, builder.NamespacedName())) - require.True(t, fakeTracker.IsTracking(clusterStack, builder.NamespacedName())) + require.True(t, fakeTracker.IsTracking( + kreconciler.KeyForObject(clusterStore), + builder.NamespacedName())) + require.True(t, fakeTracker.IsTracking( + kreconciler.KeyForObject(clusterStack), + builder.NamespacedName())) }) it("does not update the status with no status change", func() { @@ -342,14 +354,17 @@ func testBuilderReconciler(t *testing.T, when spec.G, it spec.S) { }, }, }) - }) it("updates status and doesn't build builder when stack not ready", func() { notReadyClusterStack := &buildapi.ClusterStack{ - ObjectMeta: v1.ObjectMeta{ + ObjectMeta: metav1.ObjectMeta{ Name: "some-stack", }, + TypeMeta: metav1.TypeMeta{ + Kind: "ClusterStack", + APIVersion: "kpack.io/v1alpha2", + }, Status: buildapi.ClusterStackStatus{ Status: corev1alpha1.Status{ ObservedGeneration: 0, @@ -362,6 +377,7 @@ func testBuilderReconciler(t *testing.T, when spec.G, it spec.S) { }, }, } + rt.Test(rtesting.TableRow{ Key: builderKey, Objects: []runtime.Object{ @@ -392,11 +408,82 @@ func testBuilderReconciler(t *testing.T, when spec.G, it spec.S) { }, }) - //still track resources - require.True(t, fakeTracker.IsTracking(clusterStore, builder.NamespacedName())) - require.True(t, fakeTracker.IsTracking(notReadyClusterStack, builder.NamespacedName())) + // still track resources + require.True(t, fakeTracker.IsTracking(kreconciler.KeyForObject(clusterStore), builder.NamespacedName())) + require.True(t, fakeTracker.IsTracking(kreconciler.KeyForObject(notReadyClusterStack), builder.NamespacedName())) + require.Len(t, builderCreator.CreateBuilderCalls, 0) + }) + + it("updates status and doesn't build builder when the store doesn't exist", func() { + rt.Test(rtesting.TableRow{ + Key: builderKey, + Objects: []runtime.Object{ + clusterStack, + builder, + }, + WantErr: true, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{ + { + Object: &buildapi.Builder{ + ObjectMeta: builder.ObjectMeta, + Spec: builder.Spec, + Status: buildapi.BuilderStatus{ + Status: corev1alpha1.Status{ + ObservedGeneration: 1, + Conditions: corev1alpha1.Conditions{ + { + Type: corev1alpha1.ConditionReady, + Status: corev1.ConditionFalse, + Message: `clusterstore.kpack.io "some-store" not found`, + }, + }, + }, + }, + }, + }, + }, + }) + + // still track resources + require.True(t, fakeTracker.IsTracking(kreconciler.KeyForObject(clusterStore), builder.NamespacedName())) + require.True(t, fakeTracker.IsTracking(kreconciler.KeyForObject(clusterStack), builder.NamespacedName())) require.Len(t, builderCreator.CreateBuilderCalls, 0) }) + it("updates status and doesn't build builder when the stack doesn't exist", func() { + rt.Test(rtesting.TableRow{ + Key: builderKey, + Objects: []runtime.Object{ + clusterStore, + builder, + }, + WantErr: true, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{ + { + Object: &buildapi.Builder{ + ObjectMeta: builder.ObjectMeta, + Spec: builder.Spec, + Status: buildapi.BuilderStatus{ + Status: corev1alpha1.Status{ + ObservedGeneration: 1, + Conditions: corev1alpha1.Conditions{ + { + Type: corev1alpha1.ConditionReady, + Status: corev1.ConditionFalse, + Message: `clusterstack.kpack.io "some-stack" not found`, + }, + }, + }, + }, + }, + }, + }, + }) + + // still track resources + require.True(t, fakeTracker.IsTracking(kreconciler.KeyForObject(clusterStore), builder.NamespacedName())) + require.True(t, fakeTracker.IsTracking(kreconciler.KeyForObject(clusterStack), builder.NamespacedName())) + require.Len(t, builderCreator.CreateBuilderCalls, 0) + }) }) } diff --git a/pkg/reconciler/clusterbuilder/clusterbuilder.go b/pkg/reconciler/clusterbuilder/clusterbuilder.go index b5a610531..d569a556e 100644 --- a/pkg/reconciler/clusterbuilder/clusterbuilder.go +++ b/pkg/reconciler/clusterbuilder/clusterbuilder.go @@ -4,7 +4,10 @@ import ( "context" "go.uber.org/zap" + v1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" "knative.dev/pkg/logging/logkey" "github.com/google/go-containerregistry/pkg/authn" @@ -108,22 +111,40 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { } func (c *Reconciler) reconcileBuilder(ctx context.Context, builder *buildapi.ClusterBuilder) (buildapi.BuilderRecord, error) { - clusterStore, err := c.ClusterStoreLister.Get(builder.Spec.Store.Name) + err := c.Tracker.Track(reconciler.Key{ + NamespacedName: types.NamespacedName{ + Name: builder.Spec.Store.Name, + Namespace: v1.NamespaceAll, + }, + GroupKind: schema.GroupKind{ + Group: "kpack.io", + Kind: buildapi.ClusterStoreKind, + }, + }, builder.NamespacedName()) if err != nil { return buildapi.BuilderRecord{}, err } - err = c.Tracker.Track(clusterStore, builder.NamespacedName()) + err = c.Tracker.Track(reconciler.Key{ + NamespacedName: types.NamespacedName{ + Name: builder.Spec.Stack.Name, + Namespace: v1.NamespaceAll, + }, + GroupKind: schema.GroupKind{ + Group: "kpack.io", + Kind: buildapi.ClusterStackKind, + }, + }, builder.NamespacedName()) if err != nil { return buildapi.BuilderRecord{}, err } - clusterStack, err := c.ClusterStackLister.Get(builder.Spec.Stack.Name) + clusterStore, err := c.ClusterStoreLister.Get(builder.Spec.Store.Name) if err != nil { return buildapi.BuilderRecord{}, err } - err = c.Tracker.Track(clusterStack, builder.NamespacedName()) + clusterStack, err := c.ClusterStackLister.Get(builder.Spec.Stack.Name) if err != nil { return buildapi.BuilderRecord{}, err } diff --git a/pkg/reconciler/clusterbuilder/clusterbuilder_test.go b/pkg/reconciler/clusterbuilder/clusterbuilder_test.go index aec483c89..fd0004a6b 100644 --- a/pkg/reconciler/clusterbuilder/clusterbuilder_test.go +++ b/pkg/reconciler/clusterbuilder/clusterbuilder_test.go @@ -64,6 +64,10 @@ func testClusterBuilderReconciler(t *testing.T, when spec.G, it spec.S) { ObjectMeta: metav1.ObjectMeta{ Name: "some-store", }, + TypeMeta: metav1.TypeMeta{ + Kind: "ClusterStore", + APIVersion: "kpack.io/v1alpha2", + }, Spec: buildapi.ClusterStoreSpec{}, Status: buildapi.ClusterStoreStatus{}, } @@ -72,6 +76,10 @@ func testClusterBuilderReconciler(t *testing.T, when spec.G, it spec.S) { ObjectMeta: metav1.ObjectMeta{ Name: "some-stack", }, + TypeMeta: metav1.TypeMeta{ + Kind: "ClusterStack", + APIVersion: "kpack.io/v1alpha2", + }, Status: buildapi.ClusterStackStatus{ Status: corev1alpha1.Status{ ObservedGeneration: 0, @@ -90,6 +98,10 @@ func testClusterBuilderReconciler(t *testing.T, when spec.G, it spec.S) { Name: builderName, Generation: initialGeneration, }, + TypeMeta: metav1.TypeMeta{ + Kind: "ClusterBuilder", + APIVersion: "kpack.io/v1alpha2", + }, Spec: buildapi.ClusterBuilderSpec{ BuilderSpec: buildapi.BuilderSpec{ Tag: builderTag, @@ -162,6 +174,7 @@ func testClusterBuilderReconciler(t *testing.T, when spec.G, it spec.S) { expectedBuilder := &buildapi.ClusterBuilder{ ObjectMeta: builder.ObjectMeta, + TypeMeta: builder.TypeMeta, Spec: builder.Spec, Status: buildapi.BuilderStatus{ Status: corev1alpha1.Status{ @@ -228,6 +241,7 @@ func testClusterBuilderReconciler(t *testing.T, when spec.G, it spec.S) { expectedBuilder := &buildapi.ClusterBuilder{ ObjectMeta: builder.ObjectMeta, + TypeMeta: builder.TypeMeta, Spec: builder.Spec, Status: buildapi.BuilderStatus{ Status: corev1alpha1.Status{ @@ -258,8 +272,10 @@ func testClusterBuilderReconciler(t *testing.T, when spec.G, it spec.S) { WantErr: false, }) - require.True(t, fakeTracker.IsTracking(clusterStore, expectedBuilder.NamespacedName())) - require.True(t, fakeTracker.IsTracking(clusterStack, builder.NamespacedName())) + require.True(t, fakeTracker.IsTracking( + kreconciler.KeyForObject(clusterStore), expectedBuilder.NamespacedName())) + require.True(t, fakeTracker.IsTracking( + kreconciler.KeyForObject(clusterStack), builder.NamespacedName())) }) it("does not update the status with no status change", func() { @@ -316,6 +332,7 @@ func testClusterBuilderReconciler(t *testing.T, when spec.G, it spec.S) { expectedBuilder := &buildapi.ClusterBuilder{ ObjectMeta: builder.ObjectMeta, + TypeMeta: builder.TypeMeta, Spec: builder.Spec, Status: buildapi.BuilderStatus{ Status: corev1alpha1.Status{ @@ -352,6 +369,10 @@ func testClusterBuilderReconciler(t *testing.T, when spec.G, it spec.S) { ObjectMeta: metav1.ObjectMeta{ Name: "some-stack", }, + TypeMeta: metav1.TypeMeta{ + Kind: "ClusterStack", + APIVersion: "kpack.io/v1alpha2", + }, Status: buildapi.ClusterStackStatus{ Status: corev1alpha1.Status{ ObservedGeneration: 0, @@ -376,6 +397,7 @@ func testClusterBuilderReconciler(t *testing.T, when spec.G, it spec.S) { { Object: &buildapi.ClusterBuilder{ ObjectMeta: builder.ObjectMeta, + TypeMeta: builder.TypeMeta, Spec: builder.Spec, Status: buildapi.BuilderStatus{ Status: corev1alpha1.Status{ @@ -394,11 +416,96 @@ func testClusterBuilderReconciler(t *testing.T, when spec.G, it spec.S) { }, }) - //still track resources - require.True(t, fakeTracker.IsTracking(clusterStore, builder.NamespacedName())) - require.True(t, fakeTracker.IsTracking(notReadyClusterStack, builder.NamespacedName())) + // still track resources + require.True(t, fakeTracker.IsTracking( + kreconciler.KeyForObject(clusterStore), + builder.NamespacedName())) + require.True(t, fakeTracker.IsTracking( + kreconciler.KeyForObject(notReadyClusterStack), + builder.NamespacedName())) require.Len(t, builderCreator.CreateBuilderCalls, 0) }) + it("updates status and doesn't build builder when the store does not exist", func() { + rt.Test(rtesting.TableRow{ + Key: builderKey, + Objects: []runtime.Object{ + clusterStack, + builder, + }, + WantErr: true, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{ + { + Object: &buildapi.ClusterBuilder{ + ObjectMeta: builder.ObjectMeta, + TypeMeta: builder.TypeMeta, + Spec: builder.Spec, + Status: buildapi.BuilderStatus{ + Status: corev1alpha1.Status{ + ObservedGeneration: 1, + Conditions: corev1alpha1.Conditions{ + { + Type: corev1alpha1.ConditionReady, + Status: corev1.ConditionFalse, + Message: `clusterstore.kpack.io "some-store" not found`, + }, + }, + }, + }, + }, + }, + }, + }) + + // still track resources + require.True(t, fakeTracker.IsTracking( + kreconciler.KeyForObject(clusterStore), + builder.NamespacedName())) + require.True(t, fakeTracker.IsTracking( + kreconciler.KeyForObject(clusterStack), + builder.NamespacedName())) + require.Len(t, builderCreator.CreateBuilderCalls, 0) + }) + + it("updates status and doesn't build builder when the stack does not exist", func() { + rt.Test(rtesting.TableRow{ + Key: builderKey, + Objects: []runtime.Object{ + clusterStore, + builder, + }, + WantErr: true, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{ + { + Object: &buildapi.ClusterBuilder{ + ObjectMeta: builder.ObjectMeta, + TypeMeta: builder.TypeMeta, + Spec: builder.Spec, + Status: buildapi.BuilderStatus{ + Status: corev1alpha1.Status{ + ObservedGeneration: 1, + Conditions: corev1alpha1.Conditions{ + { + Type: corev1alpha1.ConditionReady, + Status: corev1.ConditionFalse, + Message: `clusterstack.kpack.io "some-stack" not found`, + }, + }, + }, + }, + }, + }, + }, + }) + + // still track resources + require.True(t, fakeTracker.IsTracking( + kreconciler.KeyForObject(clusterStore), + builder.NamespacedName())) + require.True(t, fakeTracker.IsTracking( + kreconciler.KeyForObject(clusterStack), + builder.NamespacedName())) + require.Len(t, builderCreator.CreateBuilderCalls, 0) + }) }) } diff --git a/pkg/reconciler/image/image.go b/pkg/reconciler/image/image.go index a41e3ff45..659bb61ee 100644 --- a/pkg/reconciler/image/image.go +++ b/pkg/reconciler/image/image.go @@ -11,7 +11,9 @@ import ( k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/selection" + "k8s.io/apimachinery/pkg/types" coreinformers "k8s.io/client-go/informers/core/v1" k8sclient "k8s.io/client-go/kubernetes" corelisters "k8s.io/client-go/listers/core/v1" @@ -122,6 +124,20 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error { } func (c *Reconciler) reconcileImage(ctx context.Context, image *buildapi.Image) (*buildapi.Image, error) { + err := c.Tracker.Track(reconciler.Key{ + NamespacedName: types.NamespacedName{ + Name: image.Spec.Builder.Name, + Namespace: image.Namespace, + }, + GroupKind: schema.GroupKind{ + Group: "kpack.io", + Kind: image.Spec.Builder.Kind, + }, + }, image.NamespacedName()) + if err != nil { + return nil, err + } + builder, err := c.DuckBuilderLister.Namespace(image.Namespace).Get(image.Spec.Builder) if err != nil && !k8serrors.IsNotFound(err) { return nil, err @@ -131,11 +147,6 @@ func (c *Reconciler) reconcileImage(ctx context.Context, image *buildapi.Image) return image, nil } - err = c.Tracker.Track(builder, image.NamespacedName()) - if err != nil { - return nil, err - } - lastBuild, err := c.fetchLastBuild(image) if err != nil { return nil, err diff --git a/pkg/reconciler/image/image_test.go b/pkg/reconciler/image/image_test.go index 7f32cc8a2..e452429a6 100644 --- a/pkg/reconciler/image/image_test.go +++ b/pkg/reconciler/image/image_test.go @@ -24,6 +24,7 @@ import ( buildapi "github.com/pivotal/kpack/pkg/apis/build/v1alpha2" corev1alpha1 "github.com/pivotal/kpack/pkg/apis/core/v1alpha1" "github.com/pivotal/kpack/pkg/client/clientset/versioned/fake" + "github.com/pivotal/kpack/pkg/reconciler" "github.com/pivotal/kpack/pkg/reconciler/image" "github.com/pivotal/kpack/pkg/reconciler/testhelpers" ) @@ -33,7 +34,6 @@ func TestImageReconciler(t *testing.T) { } func testImageReconciler(t *testing.T, when spec.G, it spec.S) { - const ( imageName = "image-name" builderName = "builder-name" @@ -45,9 +45,7 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) { someValueToPassThrough = "to-pass-through" originalGeneration int64 = 1 ) - var ( - fakeTracker = testhelpers.FakeTracker{} - ) + fakeTracker := testhelpers.FakeTracker{} rt := testhelpers.ReconcilerTester(t, func(t *testing.T, row *rtesting.TableRow) (reconciler controller.Reconciler, lists rtesting.ActionRecorderList, list rtesting.EventList) { @@ -117,7 +115,8 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) { Namespace: namespace, }, TypeMeta: metav1.TypeMeta{ - Kind: buildapi.BuilderKind, + Kind: buildapi.BuilderKind, + APIVersion: "kpack.io/v1alpha2", }, Status: buildapi.BuilderStatus{ LatestImage: "some/builder@sha256:acf123", @@ -151,7 +150,8 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) { Name: clusterBuilderName, }, TypeMeta: metav1.TypeMeta{ - Kind: buildapi.ClusterBuilderKind, + Kind: buildapi.ClusterBuilderKind, + APIVersion: "kpack.io/v1alpha2", }, Status: buildapi.BuilderStatus{ LatestImage: "some/clusterbuilder@sha256:acf123", @@ -233,7 +233,9 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) { WantErr: false, }) - require.True(t, fakeTracker.IsTracking(builder, image.NamespacedName())) + require.True(t, fakeTracker.IsTracking( + reconciler.KeyForObject(builder).WithNamespace(namespace), + image.NamespacedName())) }) it("sets condition not ready for non-existent builder", func() { @@ -265,6 +267,11 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) { }, }, }) + + // still track resource + require.True(t, fakeTracker.IsTracking( + reconciler.KeyForObject(builder).WithNamespace(namespace), + image.NamespacedName())) }) when("reconciling source resolvers", func() { @@ -494,7 +501,7 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) { }) it("updates build cache if size changes", func() { - var imageCacheName = image.CacheName() + imageCacheName := image.CacheName() image.Status.BuildCacheName = imageCacheName newCacheSize := resource.MustParse("2.5") @@ -553,7 +560,7 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) { }) it("updates build cache if desired labels change", func() { - var imageCacheName = image.CacheName() + imageCacheName := image.CacheName() image.Spec.Cache.Volume.Size = &cacheSize image.Status.BuildCacheName = imageCacheName cache := image.BuildCache() @@ -2175,7 +2182,6 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) { }, }, }) - }) it("reports unknown when last build was successful and builder is not ready", func() { @@ -2224,11 +2230,9 @@ func testImageReconciler(t *testing.T, when spec.G, it spec.S) { }, }, }) - }) when("reconciling old builds", func() { - it("deletes a failed build if more than the limit", func() { image.Spec.FailedBuildHistoryLimit = limit(4) image.Status.LatestBuildRef = "image-name-build-5" diff --git a/pkg/reconciler/testhelpers/fake_tracker.go b/pkg/reconciler/testhelpers/fake_tracker.go index 58e275aa6..5d9a88fab 100644 --- a/pkg/reconciler/testhelpers/fake_tracker.go +++ b/pkg/reconciler/testhelpers/fake_tracker.go @@ -3,19 +3,19 @@ package testhelpers import ( "fmt" - v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/pivotal/kpack/pkg/reconciler" "k8s.io/apimachinery/pkg/types" ) type FakeTracker map[string]map[types.NamespacedName]struct{} -func (f FakeTracker) Track(ref v1.ObjectMetaAccessor, obj types.NamespacedName) error { - _, ok := f[key(ref)] +func (f FakeTracker) Track(ref reconciler.Key, obj types.NamespacedName) error { + _, ok := f[ref.String()] if !ok { - f[key(ref)] = map[types.NamespacedName]struct{}{} + f[ref.String()] = map[types.NamespacedName]struct{}{} } - f[key(ref)][obj] = struct{}{} + f[ref.String()][obj] = struct{}{} return nil } @@ -23,8 +23,8 @@ func (FakeTracker) OnChanged(obj interface{}) { panic("I should not be called in tests") } -func (f FakeTracker) IsTracking(ref v1.ObjectMetaAccessor, obj types.NamespacedName) bool { - trackingObs, ok := f[key(ref)] +func (f FakeTracker) IsTracking(ref reconciler.Key, obj types.NamespacedName) bool { + trackingObs, ok := f[ref.String()] if !ok { return false } @@ -33,6 +33,6 @@ func (f FakeTracker) IsTracking(ref v1.ObjectMetaAccessor, obj types.NamespacedN return ok } -func key(ref v1.ObjectMetaAccessor) string { - return fmt.Sprintf("%s/%s", ref.GetObjectMeta().GetNamespace(), ref.GetObjectMeta().GetName()) +func (f FakeTracker) String() string { + return fmt.Sprintf("%#v", f) } diff --git a/pkg/reconciler/tracker.go b/pkg/reconciler/tracker.go index 4a63d1145..480a7b391 100644 --- a/pkg/reconciler/tracker.go +++ b/pkg/reconciler/tracker.go @@ -1,11 +1,48 @@ package reconciler import ( - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "fmt" + + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" ) +type Key struct { + GroupKind schema.GroupKind + NamespacedName types.NamespacedName +} + +func (k Key) String() string { + return fmt.Sprintf("%s/%s", k.GroupKind, k.NamespacedName) +} + +func (k Key) WithNamespace(namespace string) Key { + return Key{ + GroupKind: k.GroupKind, + NamespacedName: types.NamespacedName{ + Namespace: namespace, + Name: k.NamespacedName.Name, + }, + } +} + +type Object interface { + GetName() string + GetNamespace() string + GetObjectKind() schema.ObjectKind +} + +func KeyForObject(obj Object) Key { + return Key{ + NamespacedName: types.NamespacedName{ + Name: obj.GetName(), + Namespace: obj.GetNamespace(), + }, + GroupKind: obj.GetObjectKind().GroupVersionKind().GroupKind(), + } +} + type Tracker interface { - Track(ref metav1.ObjectMetaAccessor, obj types.NamespacedName) error + Track(ref Key, obj types.NamespacedName) error OnChanged(obj interface{}) } diff --git a/pkg/tracker/tracker.go b/pkg/tracker/tracker.go index 1a8e0f59a..385786231 100644 --- a/pkg/tracker/tracker.go +++ b/pkg/tracker/tracker.go @@ -25,7 +25,7 @@ import ( "sync" "time" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/pivotal/kpack/pkg/reconciler" "k8s.io/apimachinery/pkg/types" ) @@ -40,7 +40,7 @@ type Tracker struct { m sync.Mutex // mapping maps from an object reference to the set of // keys for objects watching it. - mapping map[types.UID]set + mapping map[string]set // The amount of time that an object may watch another // before having to renew the lease. @@ -53,21 +53,21 @@ type Tracker struct { type set map[types.NamespacedName]time.Time // Track implements Interface. -func (i *Tracker) Track(ref metav1.ObjectMetaAccessor, obj types.NamespacedName) error { +func (i *Tracker) Track(ref reconciler.Key, obj types.NamespacedName) error { i.m.Lock() defer i.m.Unlock() if i.mapping == nil { - i.mapping = make(map[types.UID]set) + i.mapping = make(map[string]set) } - l, ok := i.mapping[ref.GetObjectMeta().GetUID()] + l, ok := i.mapping[ref.String()] if !ok { l = set{} } // Overwrite the key with a new expiration. l[obj] = time.Now().Add(i.leaseDuration) - i.mapping[ref.GetObjectMeta().GetUID()] = l + i.mapping[ref.String()] = l return nil } @@ -77,16 +77,18 @@ func isExpired(expiry time.Time) bool { // OnChanged implements Interface. func (i *Tracker) OnChanged(obj interface{}) { - item, ok := obj.(metav1.Object) + reconcilerObj, ok := obj.(reconciler.Object) if !ok { return } + key := reconciler.KeyForObject(reconcilerObj) + // TODO(mattmoor): Consider locking the mapping (global) for a // smaller scope and leveraging a per-set lock to guard its access. i.m.Lock() defer i.m.Unlock() - s, ok := i.mapping[item.GetUID()] + s, ok := i.mapping[key.String()] if !ok { // TODO(mattmoor): We should consider logging here. return @@ -102,6 +104,6 @@ func (i *Tracker) OnChanged(obj interface{}) { } if len(s) == 0 { - delete(i.mapping, item.GetUID()) + delete(i.mapping, key.String()) } } diff --git a/pkg/tracker/tracker_test.go b/pkg/tracker/tracker_test.go index 0dab6f64f..a07cec1f1 100644 --- a/pkg/tracker/tracker_test.go +++ b/pkg/tracker/tracker_test.go @@ -10,6 +10,7 @@ import ( "k8s.io/apimachinery/pkg/types" buildapi "github.com/pivotal/kpack/pkg/apis/build/v1alpha2" + "github.com/pivotal/kpack/pkg/reconciler" "github.com/pivotal/kpack/pkg/tracker" ) @@ -32,7 +33,7 @@ func testTracker(t *testing.T, when spec.G, it spec.S) { Namespace: "some-namespace", }, } - err := track.Track(&builder.ObjectMeta, types.NamespacedName{ + err := track.Track(reconciler.KeyForObject(builder), types.NamespacedName{ Namespace: "some-other-namespace", Name: "call-me-when-builder-changes", }) @@ -58,8 +59,12 @@ func testTracker(t *testing.T, when spec.G, it spec.S) { ObjectMeta: v1.ObjectMeta{ Name: "some-name", }, + TypeMeta: v1.TypeMeta{ + Kind: "ClusterBuilder", + APIVersion: "kpack.io/v1alpha2", + }, } - err := track.Track(&clusterBuilder.ObjectMeta, types.NamespacedName{ + err := track.Track(reconciler.KeyForObject(clusterBuilder), types.NamespacedName{ Namespace: "some-other-namespace", Name: "call-me-when-builder-changes", })