Merge pull request kubernetes-sigs#192 from kevindelgado/no-atomic-to-granular

Unsupport atomic to granular schema reconciliation

Author: k8s-ci-robot

merge/schema_change_test.go

@@ -204,24 +204,14 @@ func TestAtomicToGranularSchemaChanges(t *testing.T) {
 					},
 				},
 				ChangeParser{Parser: structParser},
+				// No conflict after changing struct to a granular schema
 				Apply{
 					Manager: "two",
 					Object: `
 						struct:
 						  string: "b"
 					`,
 					APIVersion: "v1",
-					Conflicts: merge.Conflicts{
-						merge.Conflict{Manager: "one", Path: _P("struct", "string")},
-					},
-				},
-				ForceApply{
-					Manager: "two",
-					Object: `
-						struct:
-						  string: "b"
-					`,
-					APIVersion: "v1",
 				},
 			},
 			Object: `
@@ -231,9 +221,21 @@ func TestAtomicToGranularSchemaChanges(t *testing.T) {
 			`,
 			APIVersion: "v1",
 			Managed: fieldpath.ManagedFields{
+				// Note that manager one previously owned
+				// the top level _P("struct")
+				// which included all of its subfields
+				// when the struct field was atomic.
+				//
+				// Upon changing the schema of struct from
+				// atomic to granular, manager one continues
+				// to own the same fieldset as before,
+				// but does not retain ownership of any of the subfields.
+				//
+				// This is a known limitation due to the inability
+				// to accurately determine whether an empty field
+				// was previously atomic or not.
 				"one": fieldpath.NewVersionedSet(_NS(
 					_P("struct"),
-					_P("struct", "numeric"),
 				), "v1", true),
 				"two": fieldpath.NewVersionedSet(_NS(
 					_P("struct", "string"),

typed/reconcile_schema.go

@@ -187,19 +187,17 @@ func (v *reconcileWithSchemaWalker) visitListItems(t *schema.List, element *fiel
 }
 
 func (v *reconcileWithSchemaWalker) doList(t *schema.List) (errs ValidationErrors) {
-	// reconcile lists changed from granular to atomic
+	// reconcile lists changed from granular to atomic.
+	// Note that migrations from atomic to granular are not recommended and will
+	// be treated as if they were always granular.
+	//
+	// In this case, the manager that owned the previously atomic field (and all subfields),
+	// will now own just the top-level field and none of the subfields.
 	if !v.isAtomic && t.ElementRelationship == schema.Atomic {
 		v.toRemove = fieldpath.NewSet(v.path) // remove all root and all children fields
 		v.toAdd = fieldpath.NewSet(v.path)    // add the root of the atomic
 		return errs
 	}
-	// reconcile lists changed from atomic to granular
-	if v.isAtomic && t.ElementRelationship == schema.Associative {
-		v.toAdd, errs = buildGranularFieldSet(v.path, v.value)
-		if errs != nil {
-			return errs
-		}
-	}
 	if v.fieldSet != nil {
 		errs = v.visitListItems(t, v.fieldSet)
 	}
@@ -231,42 +229,25 @@ func (v *reconcileWithSchemaWalker) visitMapItems(t *schema.Map, element *fieldp
 }
 
 func (v *reconcileWithSchemaWalker) doMap(t *schema.Map) (errs ValidationErrors) {
-	// reconcile maps and structs changed from granular to atomic
+	// reconcile maps and structs changed from granular to atomic.
+	// Note that migrations from atomic to granular are not recommended and will
+	// be treated as if they were always granular.
+	//
+	// In this case the manager that owned the previously atomic field (and all subfields),
+	// will now own just the top-level field and none of the subfields.
 	if !v.isAtomic && t.ElementRelationship == schema.Atomic {
 		if v.fieldSet != nil && v.fieldSet.Size() > 0 {
 			v.toRemove = fieldpath.NewSet(v.path) // remove all root and all children fields
 			v.toAdd = fieldpath.NewSet(v.path)    // add the root of the atomic
 		}
 		return errs
 	}
-	// reconcile maps changed from atomic to granular
-	if v.isAtomic && (t.ElementRelationship == schema.Separable || t.ElementRelationship == "") {
-		v.toAdd, errs = buildGranularFieldSet(v.path, v.value)
-		if errs != nil {
-			return errs
-		}
-	}
 	if v.fieldSet != nil {
 		errs = v.visitMapItems(t, v.fieldSet)
 	}
 	return errs
 }
 
-func buildGranularFieldSet(path fieldpath.Path, value *TypedValue) (*fieldpath.Set, ValidationErrors) {
-
-	valueFieldSet, err := value.ToFieldSet()
-	if err != nil {
-		return nil, errorf("toFieldSet: %v", err)
-	}
-	if valueFieldSetAtPath, ok := fieldSetAtPath(valueFieldSet, path); ok {
-		result := fieldpath.NewSet(path)
-		resultAtPath := descendToPath(result, path)
-		*resultAtPath = *valueFieldSetAtPath
-		return result, nil
-	}
-	return nil, nil
-}
-
 func fieldSetAtPath(node *fieldpath.Set, path fieldpath.Path) (*fieldpath.Set, bool) {
 	ok := true
 	for _, pe := range path {

typed/reconcile_schema_test.go

@@ -207,39 +207,6 @@ var reconcileCases = []reconcileTestCase{{
 		_P("stringMap"),
 		_P("unchanged", "numeric"),
 	),
-}, {
-	name:         "atomic-to-granular",
-	rootTypeName: "v1",
-	oldSchema:    atomicSchema("v1"),
-	newSchema:    granularSchema("v1"),
-	liveObject:   basicLiveObject,
-	oldFields: _NS(
-		_P("struct"),
-		_P("list"),
-		_P("objectList"),
-		_P("stringMap"),
-		_P("unchanged", "numeric"),
-	),
-	fixedFields: _NS(
-		_P("struct"),
-		_P("struct", "numeric"),
-		_P("struct", "string"),
-		_P("list"),
-		_P("list", _V("one")),
-		_P("list", _V("two")),
-		_P("objectList"),
-		_P("objectList", _KBF("keyA", "a1", "keyB", "b1")),
-		_P("objectList", _KBF("keyA", "a1", "keyB", "b1"), "value"),
-		_P("objectList", _KBF("keyA", "a1", "keyB", "b1"), "keyA"),
-		_P("objectList", _KBF("keyA", "a1", "keyB", "b1"), "keyB"),
-		_P("objectList", _KBF("keyA", "a2", "keyB", "b2")),
-		_P("objectList", _KBF("keyA", "a2", "keyB", "b2"), "value"),
-		_P("objectList", _KBF("keyA", "a2", "keyB", "b2"), "keyA"),
-		_P("objectList", _KBF("keyA", "a2", "keyB", "b2"), "keyB"),
-		_P("stringMap"),
-		_P("stringMap", "key1"),
-		_P("unchanged", "numeric"),
-	),
 }, {
 	name:         "no-change-granular",
 	rootTypeName: "v1",