Schema Reconciler inaccurately determines whether the node is atomic or not #191
Description
Currently the reconcileWithSchemaWalker will believe that a list/map node has changed from atomic to granular if the node was previously atomic (and the current element relationship is not atomic).
The way it determines whether a node is atomic is by naively assuming that if the node’s fieldSet contains an path element that is a member, but that path element has no children, than the node must be atomic.
This is incorrect because there are situations where a node might be an empty list/map, but not be atomic. For example, if a manager wants to own just the spec.metadata field to ensure it doesn't get removed, but does not want to own any of its contents.
The guiding example that exposes this issue is the following proposed test to be added to the merge package
// BROKEN
"no_change_list": {
Ops: []Operation{
// Apply an empty list with the first manager
// Meaning we want the manager to own the list itself
// but none of its elements
Apply{
Manager: "one",
Object: `
list:
`,
APIVersion: "v1",
},
// Apply specific elements to the list created by manager one.
// Manager two should own these individual elements in the list.
// Manager one should continue to own the list itself but none of the elements.
Apply{
Manager: "two",
Object: `
list:
- a
- b
`,
APIVersion: "v1",
},
// Prior to this apply, manager one SHOULD own the list but none of the individual
// elements, but when we use manager two to remove element "a" from the list,
// we see that actually manager one becomes an owner of all the elements in the list.
//
// This occurs because `reconcileWithSchemaWalker` thinks the list's schema has
// changed from atomic to granular.
//
// It was never atomic in the first place, but the reconciler determines node
// atomicity by checking if a node (in this case the list itself) exists as a member in the
// fieldSet but this node has no children (because the list is empty).
Apply{
Manager: "two",
Object: `
list:
- b
`,
APIVersion: "v1",
},
},
// BROKEN: expected:
//Object: `
// list:
// - b
//`,
// but actually got:
Object: `
list:
- a
- b
`,
APIVersion: "v1",
Managed: fieldpath.ManagedFields{
"apply-one": fieldpath.NewVersionedSet(
// BROKEN expected:
//_NS(
// _P("list"),
//),
// but actually got:
_NS(
_P("list"),
_P("list", _V("a")),
_P("list", _V("b")),
),
"v1",
false,
),
"apply-two": fieldpath.NewVersionedSet(
_NS(
_P("list", _V("b")),
),
"v1",
false,
),
},
},
A perfectly correct solution may not be possible because there is no way of determining what the previous schema was.
One proposal is to maintain a holistic view of things we know not to be atomic (because some other applier was able to apply individual elements) and then for our atomicity check we look at that holistic view to see whether or not any node was previously not atomic.
While not perfect, this is more correct than the current situation.