HRQ: Resources are not removed from the quotas' usages when a namespace is moved out of a subtree

[mzeevi]

Hi,

The HRQ documentation notes the following:

When a namespace is moved out of a subtree with hierarchical quotas in its ancestors, it is no longer subject to these quotas, and its resources are removed from the quotas' usages.

However, this does not seem to be the case here.

Steps to reproduce:

1. Clone HNC repo and install kubectl-hns
2. Set up Kind cluster with cert-manager:

$ kind create cluster
$ kubectl apply -f https://github.com/cert-manager/cert-manager/releases/download/v1.8.0/cert-manager.yaml

3. Deploy on Kind cluster:

$ make deploy-hrq HNC_IMG="localhost/hnc-manager:v0.0.1" CONFIG=kind

4. Set up hierarchy using kubectl-hns:

$ kubectl create ns root-ns
$ kubectl-hns create subns-1 -n root-ns
$ kubectl-hns create subns-2 -n root-ns
$ kubectl-hns create subns-3 -n subns-1

5. The root-ns tree now looks like this:

root-ns
├── [s] subns-1
│   └── [s] subns-3
└── [s] subns-2

[s] indicates subnamespaces

6. Create HierarchicalResourceQuota object with limit of 10 on configmaps on root-ns.

$ cat <<EOF | kubectl apply -f -
apiVersion: hnc.x-k8s.io/v1alpha2
kind: HierarchicalResourceQuota
metadata:
  name: example-hrq
  namespace: root-ns
spec:
  hard:
    configmaps: "10"
EOF

7. The status.used property of HierarchicalResourceQuota/example-hrq shows Configmaps: 4, corresponding to the 4 kube-root-ca.crt configmaps - one for each namespace/subns:

$ kubectl describe HierarchicalResourceQuota example-hrq -n root-ns

Name:         example-hrq
Namespace:    root-ns
Labels:       <none>
Annotations:  <none>
API Version:  hnc.x-k8s.io/v1alpha2
Kind:         HierarchicalResourceQuota
Metadata:
  Creation Timestamp:  2022-06-28T11:14:11Z
  Generation:          4
  Managed Fields:
    API Version:  hnc.x-k8s.io/v1alpha2
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:annotations:
          .:
          f:kubectl.kubernetes.io/last-applied-configuration:
      f:spec:
        .:
        f:hard:
          .:
          f:configmaps:
    Manager:      kubectl-client-side-apply
    Operation:    Update
    Time:         2022-06-28T11:14:11Z
    API Version:  hnc.x-k8s.io/v1alpha2
    Fields Type:  FieldsV1
    fieldsV1:
      f:status:
        .:
        f:hard:
          .:
          f:configmaps:
        f:used:
          .:
          f:configmaps:
    Manager:         manager
    Operation:       Update
    Time:            2022-06-28T11:14:12Z
  Resource Version:  4434
  UID:               a99f5217-f78a-45ab-8856-9547df97fb9b
Spec:
  Hard:
    Configmaps:  10
Status:
  Hard:
    Configmaps:  10
  Used:
    Configmaps:  4
Events:          <none>

8. Create a new full namespace and a config map in this namespace:

$ kubectl create ns test-ns
$ cat <<EOF | kubectl apply -f -
apiVersion: v1
kind: ConfigMap
metadata:
  name: test-cm  
  namespace: test-ns    
data:
 number: "3"
EOF

9. Move test-ns into root-ns:

$ kubectl-hns set test-ns --parent root-ns

10. The status.used property of HierarchicalResourceQuota/example-hrq shows Configmaps: 6, which makes sense and satisfies the following from the HRQ [documentation](https://cloud.google.com/anthos-config-management/docs/how-to/using-hierarchical-resource-quotas:

When a namespace is added to a subtree with hierarchical quotas, it is subject to the hierarchical quotas, and its resource usages are added to the quotas' usages.

$ kubectl describe HierarchicalResourceQuota example-hrq -n root-ns

Name:         example-hrq
Namespace:    root-ns
Labels:       <none>
Annotations:  <none>
API Version:  hnc.x-k8s.io/v1alpha2
Kind:         HierarchicalResourceQuota
Metadata:
  Creation Timestamp:  2022-06-28T11:14:11Z
  Generation:          5
  Managed Fields:
    API Version:  hnc.x-k8s.io/v1alpha2
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:annotations:
          .:
          f:kubectl.kubernetes.io/last-applied-configuration:
      f:spec:
        .:
        f:hard:
          .:
          f:configmaps:
    Manager:      kubectl-client-side-apply
    Operation:    Update
    Time:         2022-06-28T11:14:11Z
    API Version:  hnc.x-k8s.io/v1alpha2
    Fields Type:  FieldsV1
    fieldsV1:
      f:status:
        .:
        f:hard:
          .:
          f:configmaps:
        f:used:
          .:
          f:configmaps:
    Manager:         manager
    Operation:       Update
    Time:            2022-06-28T11:14:12Z
  Resource Version:  4594
  UID:               a99f5217-f78a-45ab-8856-9547df97fb9b
Spec:
  Hard:
    Configmaps:  10
Status:
  Hard:
    Configmaps:  10
  Used:
    Configmaps:  6
Events:          <none>

10. Now move test-ns out of the root-ns subtree by setting it as root:

$ kubectl-hns set --root test-ns

11. The status.used property of HierarchicalResourceQuota/example-hrq still shows Configmaps: 6, but should show Configmaps: 4.

$ kubectl describe HierarchicalResourceQuota/example-hrq -n root-ns
Name:         example-hrq
Namespace:    root-ns
Labels:       <none>
Annotations:  <none>
API Version:  hnc.x-k8s.io/v1alpha2
Kind:         HierarchicalResourceQuota
Metadata:
  Creation Timestamp:  2022-06-28T11:14:11Z
  Generation:          5
  Managed Fields:
    API Version:  hnc.x-k8s.io/v1alpha2
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:annotations:
          .:
          f:kubectl.kubernetes.io/last-applied-configuration:
      f:spec:
        .:
        f:hard:
          .:
          f:configmaps:
    Manager:      kubectl-client-side-apply
    Operation:    Update
    Time:         2022-06-28T11:14:11Z
    API Version:  hnc.x-k8s.io/v1alpha2
    Fields Type:  FieldsV1
    fieldsV1:
      f:status:
        .:
        f:hard:
          .:
          f:configmaps:
        f:used:
          .:
          f:configmaps:
    Manager:         manager
    Operation:       Update
    Time:            2022-06-28T11:14:12Z
  Resource Version:  4594
  UID:               a99f5217-f78a-45ab-8856-9547df97fb9b
Spec:
  Hard:
    Configmaps:  10
Status:
  Hard:
    Configmaps:  10
  Used:
    Configmaps:  6
Events:          <none>

I'm aware that this feature is not yet production-ready, but is it a bug or something that hasn't been implemented yet or perhaps my usage here is wrong?

[mzeevi]

I tried to reproduce it today and noticed that after the controller is restarted (I deleted the pod and let the deployment create a new one), the status is shown correctly - 4 config maps in the status.

The update does not seem to be triggered without a restart for some reason

Name:         example-hrq
Namespace:    root-ns
Labels:       <none>
Annotations:  <none>
API Version:  hnc.x-k8s.io/v1alpha2
Kind:         HierarchicalResourceQuota
Metadata:
  Creation Timestamp:  2022-07-25T14:22:08Z
  Generation:          7
  Managed Fields:
    API Version:  hnc.x-k8s.io/v1alpha2
    Fields Type:  FieldsV1
    fieldsV1:
      f:metadata:
        f:annotations:
          .:
          f:kubectl.kubernetes.io/last-applied-configuration:
      f:spec:
        .:
        f:hard:
          .:
          f:configmaps:
    Manager:      kubectl-client-side-apply
    Operation:    Update
    Time:         2022-07-25T14:22:08Z
    API Version:  hnc.x-k8s.io/v1alpha2
    Fields Type:  FieldsV1
    fieldsV1:
      f:status:
        .:
        f:hard:
          .:
          f:configmaps:
        f:used:
          .:
          f:configmaps:
    Manager:         manager
    Operation:       Update
    Time:            2022-07-25T14:22:08Z
  Resource Version:  1405
  UID:               3d5432fe-4b0c-4cd2-9356-d335328bf750
Spec:
  Hard:
    Configmaps:  10
Status:
  Hard:
    Configmaps:  10
  Used:
    Configmaps:  4
Events:          <none>

[adrianludwin]

Thanks for this! I'll try to look into it later this month but I'm focused on a bunch of other stuff right now. Is this urgent for you? Are you able to contribute patches yourself (e.g. write Go code, get Github PRs reviewed, etc)? That could accelerate this process.

On Mon, Jul 25, 2022 at 10:31 AM mzeevi @.***> wrote:

I tried to reproduce it today and noticed that after the controller is restarted (I deleted the pod and let the deployment create a new one), the status is shown correctly - 4 config maps in the status.

The update does not seem to be triggered without a restart for some reason

Name: example-hrq Namespace: root-ns Labels: Annotations: API Version: hnc.x-k8s.io/v1alpha2 Kind: HierarchicalResourceQuota Metadata: Creation Timestamp: 2022-07-25T14:22:08Z Generation: 7 Managed Fields: API Version: hnc.x-k8s.io/v1alpha2 Fields Type: FieldsV1 fieldsV1: f:metadata: f:annotations: .: f:kubectl.kubernetes.io/last-applied-configuration: f:spec: .: f:hard: .: f:configmaps: Manager: kubectl-client-side-apply Operation: Update Time: 2022-07-25T14:22:08Z API Version: hnc.x-k8s.io/v1alpha2 Fields Type: FieldsV1 fieldsV1: f:status: .: f:hard: .: f:configmaps: f:used: .: f:configmaps: Manager: manager Operation: Update Time: 2022-07-25T14:22:08Z Resource Version: 1405 UID: 3d5432fe-4b0c-4cd2-9356-d335328bf750 Spec: Hard: Configmaps: 10 Status: Hard: Configmaps: 10 Used: Configmaps: 4 Events:

— Reply to this email directly, view it on GitHub https://github.com/kubernetes-sigs/hierarchical-namespaces/issues/214#issuecomment-1194128475, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE43PZDLJRVSTGQUATBI2VTVV2QNHANCNFSM52B4KRSA . You are receiving this because you are subscribed to this thread.Message ID: @.*** com>

[mzeevi]

It's not urgent, no.

I'll gladly take a look into the code and see if I can get it fixed.

Any suggestions on where you think the problem might be?

[mzeevi]

I have a suspicion of where the problem lies. From what I can tell, the way that the HRQ status is being updated is the following:

• In the HRQ Reconciler, we call syncWithForest.
• In that function we[call syncUsages which updates the HRQ usages if they are changed.
• The usages are got through GetSubtreeUsages, which is implemented in the the following way:

// GetSubtreeUsages returns a copy of subtree resource usages.
func (n *Namespace) GetSubtreeUsages() v1.ResourceList {
   u := n.quotas.used.subtree.DeepCopy()
   return u
}

• n.quotas.used.subtree is set by UpdateSubtreeUsages. This function is only called by UseResources in the following way:

func (n *Namespace) UseResources(newUsage v1.ResourceList) {
   oldUsage := n.quotas.used.local
   // We only consider limited usages.
   newUsage = utils.CleanupUnneeded(newUsage, n.Limits())
   // Early exit if there's no usages change. It's safe because the forest would
   // remain unchanged and the caller would always enqueue all ancestor HRQs.
   if utils.Equals(oldUsage, newUsage) {
       return
   }
   n.quotas.used.local = newUsage

   // Determine the delta in resource usage as this now needs to be applied to each ancestor.
   delta := utils.Subtract(newUsage, oldUsage)

   // Update subtree usages in the ancestors (including itself). The incremental
   // change here is safe because there's a goroutine periodically calculating
   // subtree usages from-scratch to make sure the forest is not out-of-sync. If
   // all goes well, the periodic sync isn't needed - it's *purely* there in case
   // there's a bug.
   for _, nsnm := range n.AncestryNames() {
       ns := n.forest.Get(nsnm)

       // Get the new subtree usage and remove no longer limited usages.
       newSubUsg := utils.Add(delta, ns.quotas.used.subtree)
       ns.UpdateSubtreeUsages(newSubUsg)
   }
}

When moving a namespace out of the subtree and making it root namespace, the delta that is calculated is -{quantityOfResources} and the now-root namespace is updated. However, since the namespace no longer has a parent, the previous parent of the namespace (which has the HRQ object in its namespace) is not updated.

We need to somehow be able to know from which namespace (the previous parent) to decrease resources. This problem may be solved by the period sync, which I think is not yet implemented but probably could be solved in a different way as well.

Because forest is in-memory data and the sync is with the forest then when the controller is restarted everything is calculated from scratch and this is why the correct value is shown after restarting the controller.

[mzeevi]

I believe this should be fixed by #219

[adrianludwin]

Thanks, I've left comments in the PR. We shouldn't rely on the periodic sync, that's basically just to recover from bugs and shouldn't be relied on in normal operation.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[adrianludwin]

/remove-lifecycle stale

I don't work on a ton of HNC stuff these days, but for this I'll make an exception :)

On Mon, Nov 7, 2022 at 11:09 AM Kubernetes Triage Robot < @.***> wrote:

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage https://www.kubernetes.dev/docs/guide/issue-triage/

Please send feedback to sig-contributor-experience at kubernetes/community https://github.com/kubernetes/community.

/lifecycle stale

— Reply to this email directly, view it on GitHub https://github.com/kubernetes-sigs/hierarchical-namespaces/issues/214#issuecomment-1305840329, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE43PZBU65BY6NKKGTCOU3DWHESULANCNFSM52B4KRSA . You are receiving this because you commented.Message ID: @.***>

[adrianludwin]

Oh wait, this was fixed by #219 . Thanks! /close

[k8s-ci-robot]

@adrianludwin: Closing this issue.

In response to this:

Oh wait, this was fixed by #219 . Thanks! /close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.