Skip to content

Commit

Permalink
Rename ns.RequiredChildOf to ns.Owner in forest
Browse files Browse the repository at this point in the history 
This commit fixes kubernetes-retired#469.

Tested by 'make test' and 'make test HNS=1'
  • Loading branch information
@yiqigao217
yiqigao217 committed Mar 18, 2020
1 parent c026647 commit 68acb5e
Show file tree
Hide file tree
Showing 5 changed files with 47 additions and 48 deletions.
7 changes: 3 additions & 4 deletions incubator/hnc/pkg/forest/forest.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -154,10 +154,9 @@ type Namespace struct {
// on this namespace.
conditions conditions

// TODO rename it to Owner. See issue - https://github.com/kubernetes-sigs/multi-tenancy/issues/469
// RequiredChildOf indicates that this namespace is being or was created solely to live as a
// Owner indicates that this namespace is being or was created solely to live as a
// subnamespace of the specified parent.
RequiredChildOf string
Owner string
}

type condition struct {
Expand Down Expand Up @@ -271,7 +270,7 @@ func (ns *Namespace) OwnedNames() []string {
}
nms := []string{}
for nm, ns := range ns.forest.namespaces {
if ns.RequiredChildOf == ns.name {
if ns.Owner == ns.name {
nms = append(nms, nm)
}
}
Expand Down
6 changes: 2 additions & 4 deletions incubator/hnc/pkg/reconcilers/hierarchical_namespace.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,7 @@ func (r *HierarchicalNamespaceReconciler) syncMissing(log logr.Logger, inst *api

// Set the "Owner" in the forest of the hierarchical namespace to the current namespace.
log.Info("Setting the subnamespace's owner in the forest", "owner", pnm, "namespace", nm)
// TODO rename RequiredChildOf to Owner in the forest. See issue:
// https://github.com/kubernetes-sigs/multi-tenancy/issues/469
ns.RequiredChildOf = pnm
ns.Owner = pnm

// Enqueue the not-yet existent self-serve subnamespace. The HierarchyConfig reconciler will
// create the namespace and the HierarchyConfig instances on apiserver accordingly.
Expand All @@ -158,7 +156,7 @@ func (r *HierarchicalNamespaceReconciler) syncExisting(log logr.Logger, inst *ap
// or a human manually created the namespace with the right annotation but no HC.
// Both cases meant to create this namespace as HNS.
log.Info("Setting the subnamespace's owner in the forest", "owner", pnm, "namespace", nm)
ns.RequiredChildOf = pnm
ns.Owner = pnm
r.hcr.enqueueAffected(log, "updated subnamespace", nm)

// We will set it as "Conflict" though it's just a short transient state. Once the hc is
Expand Down
66 changes: 34 additions & 32 deletions incubator/hnc/pkg/reconcilers/hierarchy_config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,13 +185,13 @@ func (r *HierarchyConfigReconciler) onMissingNamespace(log logr.Logger, ns *fore
}

if !ns.Exists() {
// The namespace doesn't exist on the server, but it's been requested for a parent. Initialize
// The namespace doesn't exist on the server, but it's been self-served by a parent. Initialize
// it so it gets created; once it is, it will be reconciled again.
if ns.RequiredChildOf != "" {
log.Info("Will create missing namespace", "forParent", ns.RequiredChildOf)
if ns.Owner != "" {
log.Info("Will create missing namespace", "forOwner", ns.Owner)
nsInst.Name = ns.Name()
// Set "api.AnnotationOwner" annotation to the non-existent yet namespace.
metadata.SetAnnotation(nsInst, api.AnnotationOwner, ns.RequiredChildOf)
metadata.SetAnnotation(nsInst, api.AnnotationOwner, ns.Owner)
}
return true
}
Expand All @@ -200,8 +200,8 @@ func (r *HierarchyConfigReconciler) onMissingNamespace(log logr.Logger, ns *fore
r.enqueueAffected(log, "relative of deleted namespace", ns.RelativesNames()...)
// Enqueue the HNS if the self-serve subnamespace is deleted.
if r.HNSReconcilerEnabled {
if ns.RequiredChildOf != "" {
r.hnsr.enqueue(log, ns.Name(), ns.RequiredChildOf, "hns for the deleted self-serve subnamespace")
if ns.Owner != "" {
r.hnsr.enqueue(log, ns.Name(), ns.Owner, "hns for the deleted self-serve subnamespace")
}
}
ns.UnsetExists()
Expand All @@ -215,42 +215,44 @@ func (r *HierarchyConfigReconciler) markExisting(log logr.Logger, ns *forest.Nam
if ns.SetExists() {
log.Info("Reconciling new namespace")
r.enqueueAffected(log, "relative of newly synced/created namespace", ns.RelativesNames()...)
if ns.RequiredChildOf != "" {
r.enqueueAffected(log, "parent of newly synced/created required subnamespace", ns.RequiredChildOf)
if ns.Owner != "" {
r.enqueueAffected(log, "owner of newly synced/created self-serve subnamespace", ns.Owner)
}
}
}

// TODO update the func name to "syncOwner" and update the comment. See issue -
// https://github.com/kubernetes-sigs/multi-tenancy/issues/469
// syncRequiredChildOf propagates the required child value from the forest to the spec if possible
// (the spec itself will be synced next), or removes the requiredChildOf value if there's a problem
// and notifies the would-be parent namespace.
func (r *HierarchyConfigReconciler) syncRequiredChildOf(log logr.Logger, inst *api.HierarchyConfiguration, ns *forest.Namespace) {
if ns.RequiredChildOf == "" {
if ns.Owner == "" {
return
}

switch inst.Spec.Parent {
case "":
log.Info("Required subnamespace: initializing", "parent", ns.RequiredChildOf)
inst.Spec.Parent = ns.RequiredChildOf
case ns.RequiredChildOf:
log.Info("Self-serve subnamespace: initializing", "owner", ns.Owner)
inst.Spec.Parent = ns.Owner
case ns.Owner:
// ok
if r.HNSReconcilerEnabled {
// Enqueue HNS to update the state to "Ok".
r.hnsr.enqueue(log, ns.Name(), ns.RequiredChildOf, "the HNS state should be updated to ok")
r.hnsr.enqueue(log, ns.Name(), ns.Owner, "the HNS state should be updated to ok")
}
default:
if r.HNSReconcilerEnabled {
// Enqueue the HNS to report the conflict in hierarchicalnamespace.Status.State and enqueue the
// owner namespace to report the "SubnamespaceConflict" condition.
log.Info("Self-serve subnamespace: conflict with parent", "owner", ns.RequiredChildOf, "parent", inst.Spec.Parent)
r.hnsr.enqueue(log, ns.Name(), ns.RequiredChildOf, "self-serve subnamespace has a parent but it's not the owner")
r.enqueueAffected(log, "required child already has a parent", ns.RequiredChildOf)
log.Info("Self-serve subnamespace: conflict with parent", "owner", ns.Owner, "parent", inst.Spec.Parent)
r.hnsr.enqueue(log, ns.Name(), ns.Owner, "self-serve subnamespace has a parent but it's not the owner")
r.enqueueAffected(log, "self-serve subnamespace already has a parent", ns.Owner)
} else {
// This should never happen unless there's some crazy race condition.
log.Info("Required subnamespace: conflict with existing parent", "requiredChildOf", ns.RequiredChildOf, "actual", inst.Spec.Parent)
r.enqueueAffected(log, "required child already has a parent", ns.RequiredChildOf)
ns.RequiredChildOf = "" // get back in sync with apiserver
log.Info("Self-serve subnamespace: conflict with existing parent", "owner", ns.Owner, "actualParent", inst.Spec.Parent)
r.enqueueAffected(log, "self-serve subnamespace already has a parent", ns.Owner)
ns.Owner = "" // get back in sync with apiserver
}
}

Expand Down Expand Up @@ -355,17 +357,17 @@ func (r *HierarchyConfigReconciler) syncChildren(log logr.Logger, inst *api.Hier
for _, cn := range inst.Status.Children {
cns := r.Forest.Get(cn)

if cns.RequiredChildOf != "" && cns.RequiredChildOf != ns.Name() {
if cns.Owner != "" && cns.Owner != ns.Name() {
// Since the list of children of this namespace came from the forest, this implies that the
// in-memory forest is out of sync with itself: requiredChildOf != parent. Obviously, this
// in-memory forest is out of sync with itself: owner != parent. Obviously, this
// should never happen.
//
// Let's just log an error and enqueue the other namespace so it can report the condition.
// The forest will be reset to the correct value, below.
log.Error(forest.OutOfSync, "While syncing children", "child", cn, "requiredChildOf", cns.RequiredChildOf)
r.enqueueAffected(log, "forest out-of-sync: requiredChildOf != parent", cns.RequiredChildOf)
log.Error(forest.OutOfSync, "While syncing children", "child", cn, "owner", cns.Owner)
r.enqueueAffected(log, "forest out-of-sync: owner != parent", cns.Owner)
if r.HNSReconcilerEnabled {
r.hnsr.enqueue(log, cn, cns.RequiredChildOf, "forest out-of-sync: owner != parent")
r.hnsr.enqueue(log, cn, cns.Owner, "forest out-of-sync: owner != parent")
}
}

Expand All @@ -375,16 +377,16 @@ func (r *HierarchyConfigReconciler) syncChildren(log logr.Logger, inst *api.Hier
}
} else {
if isRequired[cn] {
// This child is actually required. Remove it from the set so we know we found it. Also, the
// This child is actually owned. Remove it from the set so we know we found it. Also, the
// forest is almost certainly already in sync, but just set it again in case something went
// wrong (eg, the error shown above).
delete(isRequired, cn)
cns.RequiredChildOf = ns.Name()
cns.Owner = ns.Name()
} else {
// This isn't a required child, but it looks like it *used* to be a required child of this
// namespace. Clear the RequiredChildOf field from the forest to bring our state in line with
// This isn't an owned child, but it looks like it *used* to be an owned child of this
// namespace. Clear the Owner field from the forest to bring our state in line with
// what's on the apiserver.
cns.RequiredChildOf = ""
cns.Owner = ""
}
}
}
Expand All @@ -410,15 +412,15 @@ func (r *HierarchyConfigReconciler) syncChildren(log logr.Logger, inst *api.Hier

// If this child isn't claimed by another parent, claim it and make sure it gets reconciled
// (which is when it will be created).
if cns.Parent() == nil && (cns.RequiredChildOf == "" || cns.RequiredChildOf == ns.Name()) {
cns.RequiredChildOf = ns.Name()
if cns.Parent() == nil && (cns.Owner == "" || cns.Owner == ns.Name()) {
cns.Owner = ns.Name()
r.enqueueAffected(log, "required child is missing", cn)
if !cns.Exists() {
msg = fmt.Sprintf("required subnamespace %s does not exist", cn)
}
} else {
// Someone else got it first. This should never happen if the validator is working correctly.
other := cns.RequiredChildOf
other := cns.Owner
if other == "" {
other = cns.Parent().Name()
}
Expand Down
8 changes: 4 additions & 4 deletions incubator/hnc/pkg/validators/hierarchy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,9 @@ func (v *Hierarchy) checkParent(ns, curParent, newParent *forest.Namespace) admi
return deny(metav1.StatusReasonConflict, "Illegal parent: "+reason)
}

// Prevent changing parent of a required child
if ns.RequiredChildOf != "" && ns.RequiredChildOf != newParent.Name() {
reason := fmt.Sprintf("Cannot set the parent of %q to %q because it's a required child of %q", ns.Name(), newParent.Name(), ns.RequiredChildOf)
// Prevent changing parent of an owned child
if ns.Owner != "" && ns.Owner != newParent.Name() {
reason := fmt.Sprintf("Cannot set the parent of %q to %q because it's a self-serve subnamespace of %q", ns.Name(), newParent.Name(), ns.Owner)
return deny(metav1.StatusReasonConflict, "Illegal parent: "+reason)
}

Expand All @@ -186,7 +186,7 @@ func (v *Hierarchy) checkRequiredChildren(ns *forest.Namespace, requiredChildren
continue
}
// If this is already a child, or is about to be, no problem.
if cns.Parent() == ns || (cns.Parent() == nil && cns.RequiredChildOf == ns.Name()) {
if cns.Parent() == ns || (cns.Parent() == nil && cns.Owner == ns.Name()) {
continue
}
reason := fmt.Sprintf("Cannot set %q as the required child of %q because it already exists and is not a child of %q", cns.Name(), ns.Name(), ns.Name())
Expand Down
8 changes: 4 additions & 4 deletions incubator/hnc/pkg/validators/hierarchy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ func TestStructure(t *testing.T) {
// -> fooRC (RequiredChild of foo)
foo := createNS(f, "foo", nil)
bar := createNS(f, "bar", nil)
createRCNS(f, "fooRC", foo)
createOwnedNS(f, "fooRC", foo)
createNS(f, "baz", nil)
bar.SetParent(foo)
h := &Hierarchy{Forest: f}
Expand Down Expand Up @@ -162,12 +162,12 @@ func createNS(f *forest.Forest, nnm string, ue []string) *forest.Namespace {
return ns
}

// createRCNS creates rc (requiredChild) and sets its parent and requiredChildOf to nm and sets it
// createOwnedNS creates a namespace and sets its parent and owner to nm and sets it
// to existing.
func createRCNS(f *forest.Forest, rc string, p *forest.Namespace) *forest.Namespace {
func createOwnedNS(f *forest.Forest, rc string, p *forest.Namespace) *forest.Namespace {
ns := f.Get(rc)
ns.SetParent(p)
ns.RequiredChildOf = p.Name()
ns.Owner = p.Name()
ns.SetExists()
return ns
}
Expand Down

0 comments on commit 68acb5e

Please sign in to comment.