Merge pull request #240 from adrianludwin/hrq-drift

Add periodic HRQ resync

Author: k8s-ci-robot

cmd/manager/main.go

@@ -21,6 +21,7 @@ import (
 	"net/http"
 	"os"
 	"strings"
+	"time"
 
 	"contrib.go.opencensus.io/exporter/prometheus"
 	"contrib.go.opencensus.io/exporter/stackdriver"
@@ -75,6 +76,7 @@ var (
 	webhooksOnly            bool
 	enableHRQ               bool
 	hncNamespace            string
+	hrqSyncInterval         time.Duration
 )
 
 // init preloads some global vars before main() starts. Since this is the top-level module, I'm not
@@ -153,6 +155,7 @@ func parseFlags() {
 	flag.BoolVar(&webhooksOnly, "webhooks-only", false, "Disables the controllers so HNC can be run in HA webhook mode")
 	flag.BoolVar(&enableHRQ, "enable-hrq", false, "Enables hierarchical resource quotas")
 	flag.StringVar(&hncNamespace, "namespace", "hnc-system", "Namespace where hnc-manager and hnc resources deployed")
+	flag.DurationVar(&hrqSyncInterval, "hrq-sync-interval", 1*time.Minute, "Frequency to double-check that all HRQ usages are up-to-date (shouldn't be needed)")
 	flag.Parse()
 
 	// Assign the array args to the configuration variables after the args are parsed.
@@ -307,9 +310,10 @@ func startControllers(mgr ctrl.Manager, certsReady chan struct{}) {
 	f := forest.NewForest()
 
 	opts := setup.Options{
-		NoWebhooks:    noWebhooks,
-		MaxReconciles: maxReconciles,
-		HRQ:           enableHRQ,
+		NoWebhooks:      noWebhooks,
+		MaxReconciles:   maxReconciles,
+		HRQ:             enableHRQ,
+		HRQSyncInterval: hrqSyncInterval,
 	}
 	setup.Create(setupLog, mgr, f, opts)
 

internal/forest/namespacehrq.go

@@ -1,9 +1,12 @@
 package forest
 
 import (
+	"errors"
 	"fmt"
 
+	"github.com/go-logr/logr"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/types"
 
 	"sigs.k8s.io/hierarchical-namespaces/internal/hrq/utils"
 )
@@ -246,3 +249,52 @@ func (n *Namespace) UpdateLimits(nm string, l v1.ResourceList) bool {
 	n.quotas.limits[nm] = l
 	return true
 }
+
+// RectifySubtreeUsages ensures that the subtree usages of every namespaces is in sync with all of
+// its descendants. This should be ensured by the logic in UseResources, but bugs happen, so this is
+// an added level of safety. If any discrepancies are found, this function logs an error, updates
+// the corrected usages in-memory, and returns a list of affected HRQ objects so that they can be
+// re-reconciled to show the corrected usages.
+//
+// The forest lock must be held when calling this function.
+func (f *Forest) RectifySubtreeUsages(log logr.Logger) []types.NamespacedName {
+	// Recalculate all usages from scratch
+	usages := map[string]v1.ResourceList{}
+	for _, ns := range f.namespaces {
+		local := ns.quotas.used.local
+		// NB: AncestryNames includes the namespace itself
+		for _, anc := range ns.AncestryNames() {
+			if existing, ok := usages[anc]; ok {
+				usages[anc] = utils.Add(existing, local)
+			} else {
+				usages[anc] = local
+			}
+		}
+	}
+
+	// Look for any out-of-date HRQ usages
+	updated := []types.NamespacedName{}
+	for nm, ns := range f.namespaces {
+		have := ns.quotas.used.subtree
+		actual := utils.FilterUnlimited(usages[nm], ns.Limits())
+		if utils.Equals(have, actual) {
+			continue
+		}
+
+		// Oopsies.
+		err := errors.New("HRQ correctness error")
+		log.Error(err, "incrementally calculated usages are incorrect", "ns", nm, "incremental", have, "actual", actual)
+
+		// Update and return info so the reconciler can write back the corrected usages.
+		ns.quotas.used.subtree = actual
+		for hrq := range ns.quotas.limits {
+			// These are the names of the actual objects
+			updated = append(updated, types.NamespacedName{
+				Namespace: nm,
+				Name:      hrq,
+			})
+		}
+	}
+
+	return updated
+}

internal/hrq/hrqreconciler_test.go

@@ -122,35 +122,33 @@ var _ = Describe("HRQ reconciler tests", func() {
 		Eventually(getRQSpec(ctx, barName)).Should(equalRL("secrets", "100", "cpu", "50"))
 	})
 
-	XIt("should recover if the subtree usages are out of sync in the forest and in reality", func() {
-		setHRQ(ctx, fooHRQName, fooName, "secrets", "6", "pods", "3")
-		setHRQ(ctx, barHRQName, barName, "secrets", "100", "cpu", "50")
-		// Simulate the K8s ResourceQuota controller to update usages.
-		updateRQUsage(ctx, fooName, "secrets", "0", "pods", "0")
-		updateRQUsage(ctx, barName, "secrets", "0", "cpu", "0", "pods", "0")
+	It("should recover if the subtree usages are out of sync in the forest and in reality", func() {
+		setHRQ(ctx, fooHRQName, fooName, "secrets", "100")
+		setHRQ(ctx, barHRQName, barName, "secrets", "100")
 
-		// Consume 10 secrets in bar.
+		// Consume 10 secrets in bar and verified that they're used
 		updateRQUsage(ctx, barName, "secrets", "10")
+		Eventually(getHRQUsed(ctx, fooName, fooHRQName)).Should(equalRL("secrets", "10"))
+		Eventually(getHRQUsed(ctx, barName, barHRQName)).Should(equalRL("secrets", "10"))
 
-		// Get the expected bar local usages and foo subtree usages in the forest.
-		Eventually(forestGetLocalUsages(barName)).Should(equalRL("secrets", "10", "cpu", "0", "pods", "0"))
-		Eventually(forestGetSubtreeUsages(fooName)).Should(equalRL("secrets", "10", "pods", "0"))
+		// If we do the check now, it shouldn't find anything
+		Expect(TestCheckHRQDrift()).Should(Equal(false))
 
-		// Introduce a bug to make the foo subtree usages in the forest out-of-sync.
-		// 10 secrets are consumed in reality while the forest has 9.
-		forestSetSubtreeUsages(fooName, "secrets", "9", "pods", "0")
-		// Verify the subtree secret usages is updated from 10 to 9.
-		Eventually(forestGetSubtreeUsages(fooName)).Should(equalRL("secrets", "9", "pods", "0"))
+		// Introduce a bug to make the foo subtree usages in the forest out-of-sync. Pretend that we
+		// somehow dropped a secret.
+		forestOverrideSubtreeUsages(fooName, "secrets", "9")
 
-		// Now consume 1 more secret (11 compared with previous 10).
+		// Now use an additional secret in the child. The parent should still be out-of-sync by one.
 		updateRQUsage(ctx, barName, "secrets", "11")
+		Eventually(getHRQUsed(ctx, fooName, fooHRQName)).Should(equalRL("secrets", "10"))
+		Eventually(getHRQUsed(ctx, barName, barHRQName)).Should(equalRL("secrets", "11"))
 
-		// We should eventually get 11 secrets subtree usages in the forest.
-		// Note: without the recalculation of subtree usages from scratch in the HRQ
-		// reconciler, we would get an incremental update to 10 according to the
-		// calculation in "UseResources()": 9 + (11 - 10), which is still out-of-sync.
-		Eventually(forestGetSubtreeUsages(fooName), HRQSyncInterval).Should(equalRL("secrets", "11", "pods", "0"))
-		Eventually(getHRQUsed(ctx, fooName, fooHRQName)).Should(equalRL("secrets", "11", "pods", "0"))
+		// Run the checker and make sure it finds something
+		Expect(TestCheckHRQDrift()).Should(Equal(true))
+
+		// And everything should be in sync again
+		Eventually(getHRQUsed(ctx, fooName, fooHRQName)).Should(equalRL("secrets", "11"))
+		Eventually(getHRQUsed(ctx, barName, barHRQName)).Should(equalRL("secrets", "11"))
 	})
 
 	It("should enqueue subtree even if the newly created HRQ has the same `spec.hard` and `status.hard`", func() {
@@ -227,7 +225,7 @@ var _ = Describe("HRQ reconciler tests", func() {
 	})
 })
 
-func forestSetSubtreeUsages(ns string, args ...string) {
+func forestOverrideSubtreeUsages(ns string, args ...string) {
 	TestForest.Lock()
 	defer TestForest.Unlock()
 	TestForest.Get(ns).TestOnlySetSubtreeUsage(argsToResourceList(0, args...))

internal/integtest/setup.go

@@ -51,10 +51,6 @@ var (
 	TestForest         *forest.Forest
 )
 
-const (
-	HRQSyncInterval = 5 * time.Second
-)
-
 func HNCRun(t *testing.T, title string) {
 	RunSpecs(t, title)
 }
@@ -107,11 +103,10 @@ func HNCBeforeSuite() {
 
 	By("creating reconcilers")
 	opts := setup.Options{
-		MaxReconciles:   100,
-		UseFakeClient:   true,
-		HNCCfgRefresh:   1 * time.Second, // so we don't have to wait as long
-		HRQ:             true,
-		HRQSyncInterval: HRQSyncInterval,
+		MaxReconciles: 100,
+		UseFakeClient: true,
+		HNCCfgRefresh: 1 * time.Second, // so we don't have to wait as long
+		HRQ:           true,
 	}
 	TestForest = forest.NewForest()
 	err = setup.CreateReconcilers(k8sManager, TestForest, opts)
@@ -140,3 +135,7 @@ func HNCAfterSuite() {
 	err := testEnv.Stop()
 	Expect(err).ToNot(HaveOccurred())
 }
+
+func TestCheckHRQDrift() bool {
+	return setup.TestOnlyCheckHRQDrift(TestForest)
+}

internal/setup/reconcilers.go

@@ -16,6 +16,11 @@ import (
 	"sigs.k8s.io/hierarchical-namespaces/internal/hrq"
 )
 
+var (
+	// hrqrForTesting is used by TestOnlyCheckHRQDrift
+	hrqrForTesting *hrq.HierarchicalResourceQuotaReconciler
+)
+
 type Options struct {
 	MaxReconciles   int
 	UseFakeClient   bool
@@ -89,6 +94,7 @@ func CreateReconcilers(mgr ctrl.Manager, f *forest.Forest, opts Options) error {
 			RQR:    rqr,
 		}
 		rqr.HRQR = hrqr
+		hrqrForTesting = hrqr
 		f.AddListener(hrqr)
 
 		if err := rqr.SetupWithManager(mgr); err != nil {
@@ -101,7 +107,7 @@ func CreateReconcilers(mgr ctrl.Manager, f *forest.Forest, opts Options) error {
 
 		// Create a periodic checker to make sure the forest is not out-of-sync.
 		if opts.HRQSyncInterval != 0 {
-			go detectIncrementalHRQUsageDrift(f, opts.HRQSyncInterval, hrqr)
+			go watchHRQDrift(f, opts.HRQSyncInterval, hrqr)
 		}
 	}
 
@@ -118,13 +124,27 @@ func CreateReconcilers(mgr ctrl.Manager, f *forest.Forest, opts Options) error {
 	return nil
 }
 
-func detectIncrementalHRQUsageDrift(f *forest.Forest, forestSyncInterval time.Duration, hrqr *hrq.HierarchicalResourceQuotaReconciler) {
+func watchHRQDrift(f *forest.Forest, forestSyncInterval time.Duration, hrqr *hrq.HierarchicalResourceQuotaReconciler) {
 	syncTicker := time.NewTicker(forestSyncInterval)
 	for {
 		<-syncTicker.C
-		// If there's any out-of-sync, enqueue the affected HRQs to update usages.
-		// If not, nothing will be enqueued.
-		//hrqr.Enqueue(hrqr.Log, "recover out-of-sync usages", f.CheckSubtreeUsages())
-		hrqr.Log.Info("TODO: recover out-of-sync usages")
+		checkHRQDrift(f, hrqr)
 	}
 }
+
+func checkHRQDrift(f *forest.Forest, hrqr *hrq.HierarchicalResourceQuotaReconciler) bool {
+	f.Lock()
+	defer f.Unlock()
+	found := false
+	for _, nsnm := range f.RectifySubtreeUsages(hrqr.Log) {
+		found = true
+		hrqr.Enqueue(hrqr.Log, "usage out-of-sync", nsnm.Namespace, nsnm.Name)
+	}
+	return found
+}
+
+// TestOnlyCheckHRQDrift is used in the integ tests to invoke the checker at exactly the right time
+// to verify that it works as expected.
+func TestOnlyCheckHRQDrift(f *forest.Forest) bool {
+	return checkHRQDrift(f, hrqrForTesting)
+}