HNC: Update namespace labels when a namespace has a critical condition

[sophieliu15]

Currently namespace labels of a namespace are not updated if the namespace has a critical condition (see here).

Ideally, when a namespace has a critical condition, we should update the labels to reflect the namespace hierarchy that we are certain about. We propose the following namespace label updating strategies for each critical condition:

• CritParentMissing
  

CritParentMissing means the parent of a namespace is missing (e.g., the parent is deleted). In this case, we propose to view the namespace that has the condition as a root of a namespace tree and it should only contain depth=0 label. Namespace labels of the descendants of the namespace should include the path from a descendant up till the namespace. Following is an example:

Suppose we have a tree as below where r is the parent of a; a is the parent of b; b is the parent of c.

r
|
a
|
b
|
c

If we delete r, each namespace will have following conditions and proposed labels:

a: Condition: CritParentMissing; Proposed Label: a/depth=0
b: Condition: CritAncestor; Proposed Labels: b/depth=0; a/depth=1
c: Condition: CritAncestor; Proposed Labels: c/depth=0; b/depth=1; a/depth=2

The hierarchy based on proposed labels becomes:

a
|
b
|
c

• CritParentInvalid
  

CritParentInvalid means the parent of a namespace is invalid and causes a cycle. In this case, we propose to view a namespace that has CritParentInvalid as a root and it should only contain the depth=0 label. Namespace labels of the descendants of the namespace should include the path from a descendant up till the namespace. Following is an example:

Suppose we have a tree as below where r is the parent of a; a is the parent of b and c; b is the parent of d.

r
|
a - b - d
|
c

If we set c to be parent of a, each namespace will have following conditions and proposed labels:

r: Condition: nil; Proposed Label: r/depth=0
a: Condition: CritParentInvalid; Proposed Label: a/depth=0
b: Condition: CritAncestor; Proposed Labels: b/depth=0; a/depth=1
d: Condition: CritAncestor; Proposed Labels: d/depth=0; b/depth=1; a/depth=2
c: Condition: CritAncestor; Proposed Labels: c/depth=0; a/depth=1

The hierarchy based on proposed labels becomes:

r

a - b - d
|
c

• CritAncestor

CritAncestor means a critical error (i.e., CritParentMissing and/or CritParentInvalid) exists in an ancestor namespace. In this case, the namespace labels include the path from the current namespace till the root as described above.

[sophieliu15]

cc:
@adrianludwin
@adrianchung
@rjbez17
@yiqigao217

[adrianludwin]

This all lgtm.
/cc @rjbez17

What do you think of all this?

[adrianludwin]

Oh, except for one thing: if a is the parent of c, and c is the parent of a, then both a and c should have the CritParentInvalid condition, not just a. That is, all namespaces involved in a cycle should have this condition set. Did you try it and see?

[sophieliu15]

Yes. If you set a to be parent of c first then tried to set c as parent of a, only a will have CritParentInvalid condition. c will have CritAncestor condition.

Here is an example command and outputs.

$ k create ns a-1
namespace/a-1 created

$ k create ns b-1
namespace/b-1 created

$ k hns set b-1 --parent a-1
Setting the parent of b-1 to a-1
Succesfully updated 1 property of the hierarchical configuration of b-1

$ k hns tree a-1
a-1
└── b-1

$ k hns set a-1 --parent b-1
Setting the parent of a-1 to b-1
Succesfully updated 1 property of the hierarchical configuration of a-1

$ k hns tree a-1
a-1 (1)
└── b-1 (2)

Conditions:
1) CritParentInvalid: cycle when making "b-1" the parent of "a-1": current ancestry is "a-1 <- b-1"
2) CritAncestor: Propagation paused in the subtree of a-1 due to a critical condition

$ k describe hierarchyconfigurations hierarchy -n a-1
Name:         hierarchy
Namespace:    a-1
Labels:       <none>
Annotations:  <none>
API Version:  hnc.x-k8s.io/v1alpha1
Kind:         HierarchyConfiguration
Metadata:
  Creation Timestamp:  2020-04-22T15:20:29Z
  Generation:          3
  Resource Version:    36141694
  Self Link:           /apis/hnc.x-k8s.io/v1alpha1/namespaces/a-1/hierarchyconfigurations/hierarchy
  UID:                 ccf77469-84ac-11ea-ae7e-42010a8a0192
Spec:
  Parent:  b-1
Status:
  Children:
    b-1
  Conditions:
    Affects:
    Code:  CritParentInvalid
    Msg:   cycle when making "b-1" the parent of "a-1": current ancestry is "a-1 <- b-1"
Events:    <none>

$ k describe hierarchyconfigurations hierarchy -n b-1
Name:         hierarchy
Namespace:    b-1
Labels:       <none>
Annotations:  <none>
API Version:  hnc.x-k8s.io/v1alpha1
Kind:         HierarchyConfiguration
Metadata:
  Creation Timestamp:  2020-04-22T15:20:29Z
  Generation:          2
  Resource Version:    36141695
  Self Link:           /apis/hnc.x-k8s.io/v1alpha1/namespaces/b-1/hierarchyconfigurations/hierarchy
  UID:                 ccf47a83-84ac-11ea-ae7e-42010a8a0192
Spec:
  Parent:  a-1
Status:
  Conditions:
    Affects:
      Namespace:  a-1
    Code:         CritAncestor
    Msg:          Propagation paused in the subtree of a-1 due to a critical condition
Events:           <none>

IIUC, current behavior makes sense. When we first set a to be parent of c, it is valid and HNC accepts that. When setting c to be parent of a, we view c as an invalid parent because c is the child of a. Therefore we put the CritInvalidParent on a. I guess the assumption here is that a is still a valid parent of c since it is set first.

Consider another example: set a to be parent of b; b to be parent of c, so far so good. Now if we set c to be parent of a, a cycle is created since c is a grandchild of a. CritInvalidParent will be put on a to indicate that c should not be a parent of a. However, it might be confusing if we also put the CritInvalidParent condition on c because b is the parent of c and there is nothing wrong with this relationship (i.e., b is a valid parent).

WDYT?

[adrianludwin]

Thanks for investigating that. K8s objects should be "stateless" - it shouldn't matter what order a series of changes are made in, they should always result in the same outcome. For example, imagine we restart HNC - the order in which we sync a and c will affect which conditions we end up displaying. This seems unfortunate.

I think we should file a separate bug to ensure that both a and c get the same condition. It shouldn't matter who was "first" - they both now have the same problem. Make sense?

[adrianludwin]

Maybe we should change CritInvalidParent to CritCycle or something like that?

[sophieliu15]

Thanks for the reply. I think that makes sense. Right now it is nondeterministic which namespace will get CritParentInvalid condition when restarting HNC (I restarted HNC and this time b-1 got the condition instead of a-1 in my example above).

Changing the name SGTM.

This issue is address in a separate bug: #666