Change PV naming and fix pointer bug (#3986)

* Change PV naming and fix pointer bug

* Must use existing for Update

* Remove debug

porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/fake_client.go

@@ -26,24 +26,29 @@ import (
 
 type fakeClient struct {
 	objects []client.Object
+	created []client.Object
+	updated []client.Object
+	deleted []client.Object
 	client.Client
 }
 
 var _ client.Client = &fakeClient{}
 
 func (f *fakeClient) Create(_ context.Context, obj client.Object, _ ...client.CreateOption) error {
-	f.objects = append(f.objects, obj)
+	fmt.Println("Creating", obj.GetName())
+	f.created = append(f.created, obj)
 	return nil
 }
 
 func (f *fakeClient) Delete(_ context.Context, obj client.Object, _ ...client.DeleteOption) error {
-	var newObjects []client.Object
-	for _, old := range f.objects {
-		if obj.GetName() != old.GetName() {
-			newObjects = append(newObjects, old)
-		}
-	}
-	f.objects = newObjects
+	fmt.Println("Deleting", obj.GetName())
+	f.deleted = append(f.deleted, obj)
+	return nil
+}
+
+func (f *fakeClient) Update(_ context.Context, obj client.Object, _ ...client.UpdateOption) error {
+	fmt.Println("Updating", obj.GetName())
+	f.updated = append(f.updated, obj)
 	return nil
 }
 
@@ -76,27 +81,27 @@ items:
 - apiVersion: config.porch.kpt.dev
   kind: PackageVariant
   metadata:
-    name: my-pv-1
+    name: my-pvs-dnrepo1-dnpkg1
   spec:
     upstream:
       repo: up
       package: up
       revision: up
     downstream:
-      repo: dn-1
-      package: dn-1
+      repo: dnrepo1
+      package: dnpkg1
 - apiVersion: config.porch.kpt.dev
   kind: PackageVariant
   metadata:
-    name: my-pv-2
+    name: my-pvs-dnrepo2-dnpkg2
   spec:
     upstream:
       repo: up
       package: up
       revision: up
     downstream:
-      repo: dn-2
-      package: dn-2`
+      repo: dnrepo2
+      package: dnpkg2`
 
 	var err error
 	switch v := obj.(type) {

porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller.go

@@ -63,6 +63,9 @@ const (
 
 	ConditionTypeStalled = "Stalled" // whether or not the resource reconciliation is making progress or not
 	ConditionTypeReady   = "Ready"   // whether or not the reconciliation succeeded
+
+	PackageVariantNameMaxLength  = 63
+	PackageVariantNameHashLength = 8
 )
 
 //go:generate go run sigs.k8s.io/controller-tools/cmd/controller-gen@v0.8.0 rbac:roleName=porch-controllers-packagevariantsets webhook paths="." output:rbac:artifacts:config=../../../config/rbac
@@ -242,7 +245,7 @@ func (r *PackageVariantSetReconciler) unrollDownstreamTargets(ctx context.Contex
 				template:       target.Template,
 				repoDefault:    u.GetName(),
 				packageDefault: upstreamPackageName,
-				object:         &u,
+				object:         u.DeepCopy(),
 			})
 
 		}
@@ -277,11 +280,8 @@ func (r *PackageVariantSetReconciler) ensurePackageVariants(ctx context.Context,
 	desiredPackageVariantMap := make(map[string]*pkgvarapi.PackageVariant, len(downstreams))
 
 	for _, pv := range pvList.Items {
-		hash, err := hashFromPackageVariantSpec(&pv.Spec)
-		if err != nil {
-			return err
-		}
-		existingPackageVariantMap[hash] = &pv
+		pvId := packageVariantIdentifier(pvs.Name, &pv.Spec)
+		existingPackageVariantMap[pvId] = pv.DeepCopy()
 	}
 
 	tr := true
@@ -290,17 +290,14 @@ func (r *PackageVariantSetReconciler) ensurePackageVariants(ctx context.Context,
 		if err != nil {
 			return err
 		}
-		hash, err := hashFromPackageVariantSpec(pvSpec)
-		if err != nil {
-			return err
-		}
+		pvId := packageVariantIdentifier(pvs.Name, pvSpec)
 		pv := pkgvarapi.PackageVariant{
 			TypeMeta: metav1.TypeMeta{
 				Kind:       "PackageVariant",
 				APIVersion: "config.porch.kpt.dev",
 			},
 			ObjectMeta: metav1.ObjectMeta{
-				Name:       fmt.Sprintf("%s-%s", pvs.Name, hash),
+				Name:       packageVariantName(pvId),
 				Namespace:  pvs.Namespace,
 				Finalizers: []string{pkgvarapi.Finalizer},
 				Labels:     map[string]string{PackageVariantSetOwnerLabel: string(pvs.UID)},
@@ -315,11 +312,11 @@ func (r *PackageVariantSetReconciler) ensurePackageVariants(ctx context.Context,
 			},
 			Spec: *pvSpec,
 		}
-		desiredPackageVariantMap[hash] = &pv
+		desiredPackageVariantMap[pvId] = &pv
 	}
 
-	for existingPvHash, existingPV := range existingPackageVariantMap {
-		if _, found := desiredPackageVariantMap[existingPvHash]; found {
+	for existingPvId, existingPV := range existingPackageVariantMap {
+		if _, found := desiredPackageVariantMap[existingPvId]; found {
 			// this PackageVariant exists in both the desired PackageVariant set and the
 			// existing PackageVariant set, so we don't need to do anything.
 		} else {
@@ -332,10 +329,16 @@ func (r *PackageVariantSetReconciler) ensurePackageVariants(ctx context.Context,
 		}
 	}
 
-	for desiredPvHash, desiredPv := range desiredPackageVariantMap {
-		if _, found := existingPackageVariantMap[desiredPvHash]; found {
+	for desiredPvId, desiredPv := range desiredPackageVariantMap {
+		if existingPv, found := existingPackageVariantMap[desiredPvId]; found {
 			// this PackageVariant exists in both the desired PackageVariant set and the
-			// existing PackageVariant set, so we don't need to do anything.
+			// existing PackageVariant set, so we update it
+			// we only change the spec
+			existingPv.Spec = desiredPv.Spec
+			err := r.Client.Update(ctx, existingPv)
+			if err != nil {
+				return err
+			}
 		} else {
 			// this PackageVariant exists in the desired PackageVariant set, but not
 			// the existing PackageVariant set, so we need to create it.
@@ -349,13 +352,18 @@ func (r *PackageVariantSetReconciler) ensurePackageVariants(ctx context.Context,
 	return nil
 }
 
-func hashFromPackageVariantSpec(spec *pkgvarapi.PackageVariantSpec) (string, error) {
-	b, err := yaml.Marshal(spec)
-	if err != nil {
-		return "", err
+func packageVariantIdentifier(pvsName string, spec *pkgvarapi.PackageVariantSpec) string {
+	return pvsName + "-" + spec.Downstream.Repo + "-" + spec.Downstream.Package
+}
+
+func packageVariantName(pvId string) string {
+	if len(pvId) <= PackageVariantNameMaxLength {
+		return pvId
 	}
-	hash := sha1.Sum(b)
-	return hex.EncodeToString(hash[:]), nil
+
+	hash := sha1.Sum([]byte(pvId))
+	stubIdx := PackageVariantNameMaxLength - PackageVariantNameHashLength - 1
+	return fmt.Sprintf("%s-%s", pvId[:stubIdx], hex.EncodeToString(hash[:])[:PackageVariantNameHashLength])
 }
 
 // SetupWithManager sets up the controller with the Manager.

porch/controllers/packagevariantsets/pkg/controllers/packagevariantset/packagevariantset_controller_test.go

@@ -67,10 +67,56 @@ status: {}
 	})
 }
 
+func TestUnrollDownstreamTargets(t *testing.T) {
+	pvs := &api.PackageVariantSet{
+		ObjectMeta: metav1.ObjectMeta{Name: "my-pvs"},
+		Spec: api.PackageVariantSetSpec{
+			Upstream: &pkgvarapi.Upstream{Repo: "up", Package: "up", Revision: "up"},
+			Targets: []api.Target{
+				{
+					Repositories: []api.RepositoryTarget{
+						{Name: "r1", PackageNames: []string{"p1", "p2", "p3"}},
+						{Name: "r2"},
+					},
+				},
+				{
+					RepositorySelector: &metav1.LabelSelector{},
+				},
+			},
+		},
+	}
+
+	fc := &fakeClient{}
+	reconciler := &PackageVariantSetReconciler{Client: fc}
+	downstreams, err := reconciler.unrollDownstreamTargets(context.Background(), pvs)
+	require.NoError(t, err)
+	require.Equal(t, 6, len(downstreams))
+	require.Equal(t, downstreams[0].repoDefault, "r1")
+	require.Equal(t, downstreams[0].packageDefault, "p1")
+	require.Equal(t, downstreams[1].repoDefault, "r1")
+	require.Equal(t, downstreams[1].packageDefault, "p2")
+	require.Equal(t, downstreams[2].repoDefault, "r1")
+	require.Equal(t, downstreams[2].packageDefault, "p3")
+
+	require.Equal(t, downstreams[3].repoDefault, "r2")
+	require.Equal(t, downstreams[3].packageDefault, "up")
+
+	// from the RepositorySelector, but fake client returns pods anyway
+	require.Equal(t, downstreams[4].repoDefault, "my-pod-1")
+	require.Equal(t, downstreams[4].packageDefault, "up")
+	require.Equal(t, downstreams[4].object.GetName(), "my-pod-1")
+
+	require.Equal(t, downstreams[5].repoDefault, "my-pod-2")
+	require.Equal(t, downstreams[5].packageDefault, "up")
+	require.Equal(t, downstreams[5].object.GetName(), "my-pod-2")
+
+}
+
 func TestEnsurePackageVariants(t *testing.T) {
 	downstreams := []pvContext{
-		{repoDefault: "dn-1", packageDefault: "dn-1"},
-		{repoDefault: "dn-3", packageDefault: "dn-3"},
+		{repoDefault: "dnrepo2", packageDefault: "dnpkg2"},
+		{repoDefault: "dnrepo3", packageDefault: "dnpkg3"},
+		{repoDefault: "dnrepo4", packageDefault: "supersupersuperloooooooooooooooooooooooooooooooooooooooooooooooooongpkgname"},
 	}
 	fc := &fakeClient{}
 	reconciler := &PackageVariantSetReconciler{Client: fc}
@@ -84,7 +130,11 @@ func TestEnsurePackageVariants(t *testing.T) {
 		&configapi.RepositoryList{},
 		&porchapi.PackageRevision{},
 		downstreams))
-	require.Equal(t, 2, len(fc.objects))
-	require.Equal(t, "my-pv-1", fc.objects[0].GetName())
-	require.Equal(t, "my-pvs-9c07e0818b755a2067903d10aecf91e19dcb9a82", fc.objects[1].GetName())
+	require.Equal(t, 1, len(fc.deleted))
+	require.Equal(t, "my-pvs-dnrepo1-dnpkg1", fc.deleted[0].GetName())
+	require.Equal(t, 1, len(fc.updated))
+	require.Equal(t, "my-pvs-dnrepo2-dnpkg2", fc.updated[0].GetName())
+	require.Equal(t, 2, len(fc.created))
+	require.Equal(t, "my-pvs-dnrepo3-dnpkg3", fc.created[0].GetName())
+	require.Equal(t, "my-pvs-dnrepo4-supersupersuperlooooooooooooooooooooooo-bec36506", fc.created[1].GetName())
 }