Merge pull request #1169 from monopole/improveNameTransformComments

Improve performance in name transform code.

pkg/resmap/resmap.go

@@ -134,15 +134,13 @@ type ResMap interface {
 	// Clear removes all resources and Ids.
 	Clear()
 
-	// ResourcesThatCouldReference returns a new ResMap with
-	// resources that _might_ reference the resource represented
-	// by the argument Id, excluding resources that should
-	// _never_ reference the Id.  E.g., if the Id
-	// refers to a ConfigMap, the returned set may include a
-	// Deployment from the same namespace and exclude Deployments
-	// from other namespaces.  Cluster wide objects are
-	// never excluded.
-	ResourcesThatCouldReference(resid.ResId) ResMap
+	// SubsetThatCouldBeReferencedBy returns a ResMap subset
+	// of self with resources that could be referenced by the
+	// resource represented by the argument Id.
+	// This is a filter; it excludes things that cannot be
+	// referenced by the Id's resource, e.g. objects in other
+	// namespaces. Cluster wide objects are never excluded.
+	SubsetThatCouldBeReferencedBy(resid.ResId) ResMap
 
 	// DeepCopy copies the ResMap and underlying resources.
 	DeepCopy() ResMap
@@ -456,8 +454,8 @@ func (m *resWrangler) makeCopy(copier resCopier) ResMap {
 	return result
 }
 
-// ResourcesThatCouldReference implements ResMap.
-func (m *resWrangler) ResourcesThatCouldReference(inputId resid.ResId) ResMap {
+// SubsetThatCouldBeReferencedBy implements ResMap.
+func (m *resWrangler) SubsetThatCouldBeReferencedBy(inputId resid.ResId) ResMap {
 	if inputId.Gvk().IsClusterKind() {
 		return m
 	}

pkg/resmap/resmap_test.go

@@ -381,7 +381,7 @@ func TestFilterBy(t *testing.T) {
 	for name, test := range tests {
 		test := test
 		t.Run(name, func(t *testing.T) {
-			got := test.resMap.ResourcesThatCouldReference(test.filter)
+			got := test.resMap.SubsetThatCouldBeReferencedBy(test.filter)
 			err := test.expected.ErrorIfNotEqualSets(got)
 			if err != nil {
 				t.Fatalf("Expected %v but got back %v", test.expected, got)

pkg/transformers/namereference.go

@@ -41,38 +41,68 @@ func NewNameReferenceTransformer(br []config.NameBackReferences) Transformer {
 	return &nameReferenceTransformer{backRefs: br}
 }
 
-// Transform updates name references in resource A that refer to resource B,
-// given that B's name may have changed.
+// Transform updates name references in resource A that
+// refer to resource B, given that B's name may have
+// changed.
 //
-// For example, a HorizontalPodAutoscaler (HPA) necessarily refers to a
-// Deployment (the thing that the HPA scales). The Deployment name might change
-// (e.g. prefix added), and the reference in the HPA has to be fixed.
+// For example, a HorizontalPodAutoscaler (HPA)
+// necessarily refers to a Deployment, the thing that
+// the HPA scales. The Deployment name might change
+// (e.g. prefix added), and the reference in the HPA
+// has to be fixed.
 //
-// In the outer loop below, we encounter an HPA.  In scanning backrefs, we
-// find that HPA refers to a Deployment.  So we find all resources in the same
-// namespace as the HPA (and with the same prefix and suffix), and look through
-// them to find all the Deployments with a resId that has a Name matching the
-// field in HPA.  For each match, we overwrite the HPA name field with the value
-// found in the Deployment's name field (the name in the raw object - the
-// modified name - not the unmodified name in the resId).
+// In the outer loop over the ResMap below, say we
+// encounter a specific HPA. Then, in scanning backrefs,
+// we encounter an entry like
 //
-// This assumes that the name stored in a ResId (the ResMap key) isn't modified
-// by name transformers.  Name transformers should only modify the name in the
+//   - kind: Deployment
+//     fieldSpecs:
+//     - path: spec/scaleTargetRef/name
+//       kind: HorizontalPodAutoscaler
+//
+// saying that an HPA, via its 'spec/scaleTargetRef/name'
+// field, may refer to a Deployment.  This match to HPA
+// means we may need to modify the value in its
+// 'spec/scaleTargetRef/name' field, by searching for
+// the thing it refers to, and getting its new name.
+//
+// As a filter, and search optimization, we compute a
+// subset of all resources that the HPA could refer to,
+// by excluding objects from other namespaces, and
+// excluding objects that don't have the same prefix-
+// suffix mods as the HPA.
+//
+// We look in this subset for all Deployment objects
+// with a resId that has a Name matching the field value
+// present in the HPA.  If no match do nothing; if more
+// than one match, it's an error.
+//
+// We overwrite the HPA name field with the value found
+// in the Deployment's name field (the name in the raw
+// object - the modified name - not the unmodified name
+// in the Deployment's resId).
+//
+// This process assumes that the name stored in a ResId
+// (the ResMap key) isn't modified by name transformers.
+// Name transformers should only modify the name in the
 // body of the resource object (the value in the ResMap).
+//
 func (o *nameReferenceTransformer) Transform(m resmap.ResMap) error {
-	// TODO: Too much looping.
-	// Even more hidden loops in ResourcesThatCouldReference,
-	// updateNameReference and FindByGVKN.
-	for id, r := range m.AsMap() {
-		for _, backRef := range o.backRefs {
-			for _, fSpec := range backRef.FieldSpecs {
-				if id.Gvk().IsSelected(&fSpec.Gvk) {
+	// TODO: Too much looping, here and in transitive calls.
+	for referrer, res := range m.AsMap() {
+		var candidates resmap.ResMap
+		for _, target := range o.backRefs {
+			for _, fSpec := range target.FieldSpecs {
+				if referrer.Gvk().IsSelected(&fSpec.Gvk) {
+					if candidates == nil {
+						candidates = m.SubsetThatCouldBeReferencedBy(referrer)
+					}
 					err := mutateField(
-						r.Map(),
+						res.Map(),
 						fSpec.PathSlice(),
 						fSpec.CreateIfNotPresent,
-						o.updateNameReference(
-							id, backRef.Gvk, m.ResourcesThatCouldReference(id)))
+						o.getNewName(
+							referrer, target.Gvk, candidates))
 					if err != nil {
 						return err
 					}
@@ -83,24 +113,28 @@ func (o *nameReferenceTransformer) Transform(m resmap.ResMap) error {
 	return nil
 }
 
-func (o *nameReferenceTransformer) updateNameReference(
-	rid resid.ResId, backRef gvk.Gvk, m resmap.ResMap) func(in interface{}) (interface{}, error) {
+func (o *nameReferenceTransformer) getNewName(
+	referrer resid.ResId,
+	target gvk.Gvk,
+	referralCandidates resmap.ResMap) func(in interface{}) (interface{}, error) {
 	return func(in interface{}) (interface{}, error) {
 		switch in.(type) {
 		case string:
-			s, _ := in.(string)
-			for id, res := range m.AsMap() {
-				if id.Gvk().IsSelected(&backRef) && id.Name() == s {
-					matchedIds := m.GetMatchingIds(id.GvknEquals)
+			oldName, _ := in.(string)
+			for id, res := range referralCandidates.AsMap() {
+				if id.Gvk().IsSelected(&target) && id.Name() == oldName {
+					matchedIds := referralCandidates.GetMatchingIds(id.GvknEquals)
 					// If there's more than one match, there's no way
 					// to know which one to pick, so emit error.
 					if len(matchedIds) > 1 {
 						return nil, fmt.Errorf(
 							"Multiple matches for name %s:\n  %v", id, matchedIds)
 					}
+					// In the resource, note that it is referenced
+					// by the referrer.
+					res.AppendRefBy(referrer)
 					// Return transformed name of the object,
 					// complete with prefixes, hashes, etc.
-					res.AppendRefBy(rid)
 					return res.GetName(), nil
 				}
 			}
@@ -111,28 +145,30 @@ func (o *nameReferenceTransformer) updateNameReference(
 			for _, item := range l {
 				name, ok := item.(string)
 				if !ok {
-					return nil, fmt.Errorf("%#v is expected to be %T", item, name)
+					return nil, fmt.Errorf(
+						"%#v is expected to be %T", item, name)
 				}
 				names = append(names, name)
 			}
-			for id, res := range m.AsMap() {
+			for id, res := range referralCandidates.AsMap() {
 				indexes := indexOf(id.Name(), names)
-				if id.Gvk().IsSelected(&backRef) && len(indexes) > 0 {
-					matchedIds := m.GetMatchingIds(id.GvknEquals)
+				if id.Gvk().IsSelected(&target) && len(indexes) > 0 {
+					matchedIds := referralCandidates.GetMatchingIds(id.GvknEquals)
 					if len(matchedIds) > 1 {
 						return nil, fmt.Errorf(
 							"Multiple matches for name %s:\n %v", id, matchedIds)
 					}
 					for _, index := range indexes {
 						l[index] = res.GetName()
 					}
-					res.AppendRefBy(rid)
+					res.AppendRefBy(referrer)
 					return l, nil
 				}
 			}
 			return in, nil
 		default:
-			return nil, fmt.Errorf("%#v is expected to be either a string or a []interface{}", in)
+			return nil, fmt.Errorf(
+				"%#v is expected to be either a string or a []interface{}", in)
 		}
 	}
 }