From c77ad3116f4eb66a4aa18927ad96ac13b1d3815f Mon Sep 17 00:00:00 2001 From: Alexandru Matei Date: Wed, 5 Apr 2023 13:05:27 +0300 Subject: [PATCH] kubelet: fix setup of emptyDir with sizeLimit (fsquota) When size limit is specified subsequent invocations will fail because ibytes is changed to -1 and stored internally in quotaSizeMap during the first call. Later invocation will see that the requested size doesn't match the actual stored value and it will fail. Signed-off-by: Alexandru Matei --- pkg/volume/util/fsquota/quota_linux.go | 9 ++-- pkg/volume/util/fsquota/quota_linux_test.go | 56 +++++++++++++-------- 2 files changed, 41 insertions(+), 24 deletions(-) diff --git a/pkg/volume/util/fsquota/quota_linux.go b/pkg/volume/util/fsquota/quota_linux.go index 240cc356ee8..33b74ab3b5f 100644 --- a/pkg/volume/util/fsquota/quota_linux.go +++ b/pkg/volume/util/fsquota/quota_linux.go @@ -353,10 +353,11 @@ func AssignQuota(m mount.Interface, path string, poduid types.UID, bytes *resour } // When enforcing quotas are enabled, we'll condition this // on their being disabled also. - if ibytes > 0 { - ibytes = -1 + fsbytes := ibytes + if fsbytes > 0 { + fsbytes = -1 } - if err = setQuotaOnDir(path, id, ibytes); err == nil { + if err = setQuotaOnDir(path, id, fsbytes); err == nil { quotaPodMap[id] = internalPodUid quotaSizeMap[id] = ibytes podQuotaMap[internalPodUid] = id @@ -364,7 +365,7 @@ func AssignQuota(m mount.Interface, path string, poduid types.UID, bytes *resour dirPodMap[path] = internalPodUid podUidMap[internalPodUid] = externalPodUid podDirCountMap[internalPodUid]++ - klog.V(4).Infof("Assigning quota ID %d (%d) to %s", id, ibytes, path) + klog.V(4).Infof("Assigning quota ID %d (request limit %d, actual limit %d) to %s", id, ibytes, fsbytes, path) return nil } removeProjectID(path, id) diff --git a/pkg/volume/util/fsquota/quota_linux_test.go b/pkg/volume/util/fsquota/quota_linux_test.go index 7cf51ca362d..13bdd786839 100644 --- a/pkg/volume/util/fsquota/quota_linux_test.go +++ b/pkg/volume/util/fsquota/quota_linux_test.go @@ -383,6 +383,7 @@ func fakeClearQuota(path string) error { } type quotaTestCase struct { + name string path string poduid types.UID bytes int64 @@ -444,42 +445,57 @@ volume1048581:1048581 var quotaTestCases = []quotaTestCase{ { + "SupportsQuotaOnQuotaVolume", "/quota1/a", "", 1024, "Supports", "", "", true, true, 0, 0, 0, 0, 1, 1, 0, 0, 1, 1, 1, }, { + "AssignQuotaFirstTime", "/quota1/a", "", 1024, "Set", projects1, projid1, true, true, 1, 1, 1, 1, 0, 0, 1, 1, 0, 0, 0, }, { + "AssignQuotaFirstTime", "/quota1/b", "x", 1024, "Set", projects2, projid2, true, true, 1, 1, 1, 1, 0, 1, 1, 1, 1, 1, 1, }, { + "AssignQuotaFirstTime", "/quota2/b", "x", 1024, "Set", projects3, projid3, true, true, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, }, { + "AssignQuotaSecondTimeWithSameSize", "/quota1/b", "x", 1024, "Set", projects3, projid3, + true, true, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, + }, + { + "AssignQuotaSecondTimeWithDifferentSize", + "/quota2/b", "x", 2048, "Set", projects3, projid3, true, false, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, }, { + "ClearQuotaFirstTime", "/quota1/b", "", 1024, "Clear", projects4, projid4, true, true, -1, -1, -1, -1, 0, -1, -1, -1, -1, -1, -1, }, { + "SupportsQuotaOnNonQuotaVolume", "/noquota/a", "", 1024, "Supports", projects4, projid4, false, false, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, }, { + "ClearQuotaFirstTime", "/quota1/a", "", 1024, "Clear", projects5, projid5, true, true, -1, -1, -1, -1, 0, -1, -1, -1, -1, -1, -1, }, { + "ClearQuotaSecondTime", "/quota1/a", "", 1024, "Clear", projects5, projid5, true, false, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, }, { + "ClearQuotaFirstTime", "/quota2/b", "", 1024, "Clear", "", "", true, true, -1, -1, -1, -1, 0, -1, -1, -1, -1, -1, -1, }, @@ -522,11 +538,11 @@ func runCaseEnabled(t *testing.T, testcase quotaTestCase, seq int) bool { supports, err := fakeSupportsQuotas(testcase.path) if err != nil { fail = true - t.Errorf("Case %v (%s, %v) Got error in fakeSupportsQuotas: %v", seq, testcase.path, true, err) + t.Errorf("Case %v (%s, %s, %v) Got error in fakeSupportsQuotas: %v", seq, testcase.name, testcase.path, true, err) } if supports != testcase.supportsQuota { fail = true - t.Errorf("Case %v (%s, %v) fakeSupportsQuotas got %v, expect %v", seq, testcase.path, true, supports, testcase.supportsQuota) + t.Errorf("Case %v (%s, %s, %v) fakeSupportsQuotas got %v, expect %v", seq, testcase.name, testcase.path, true, supports, testcase.supportsQuota) } return fail case "Set": @@ -538,15 +554,15 @@ func runCaseEnabled(t *testing.T, testcase quotaTestCase, seq int) bool { case "GetInodes": _, err = GetInodes(testcase.path) default: - t.Errorf("Case %v (%s, %v) unknown operation %s", seq, testcase.path, true, testcase.op) + t.Errorf("Case %v (%s, %s, %v) unknown operation %s", seq, testcase.name, testcase.path, true, testcase.op) return true } if err != nil && testcase.expectsSetQuota { fail = true - t.Errorf("Case %v (%s, %v) %s expected to clear quota but failed %v", seq, testcase.path, true, testcase.op, err) + t.Errorf("Case %v (%s, %s, %v) %s expected to clear quota but failed %v", seq, testcase.name, testcase.path, true, testcase.op, err) } else if err == nil && !testcase.expectsSetQuota { fail = true - t.Errorf("Case %v (%s, %v) %s expected not to clear quota but succeeded", seq, testcase.path, true, testcase.op) + t.Errorf("Case %v (%s, %s, %v) %s expected not to clear quota but succeeded", seq, testcase.name, testcase.path, true, testcase.op) } return fail } @@ -557,7 +573,7 @@ func runCaseDisabled(t *testing.T, testcase quotaTestCase, seq int) bool { switch testcase.op { case "Supports": if supports, _ = fakeSupportsQuotas(testcase.path); supports { - t.Errorf("Case %v (%s, %v) supports quotas but shouldn't", seq, testcase.path, false) + t.Errorf("Case %v (%s, %s, %v) supports quotas but shouldn't", seq, testcase.name, testcase.path, false) return true } return false @@ -570,11 +586,11 @@ func runCaseDisabled(t *testing.T, testcase quotaTestCase, seq int) bool { case "GetInodes": _, err = GetInodes(testcase.path) default: - t.Errorf("Case %v (%s, %v) unknown operation %s", seq, testcase.path, false, testcase.op) + t.Errorf("Case %v (%s, %s, %v) unknown operation %s", seq, testcase.name, testcase.path, false, testcase.op) return true } if err == nil { - t.Errorf("Case %v (%s, %v) %s: supports quotas but shouldn't", seq, testcase.path, false, testcase.op) + t.Errorf("Case %v (%s, %s, %v) %s: supports quotas but shouldn't", seq, testcase.name, testcase.path, false, testcase.op) return true } return false @@ -678,50 +694,50 @@ func testAddRemoveQuotas(t *testing.T, enabled bool) { compareProjectsFiles(t, testcase, projectsFile, projidFile, enabled) if len(podQuotaMap) != expectedPodQuotaCount { fail = true - t.Errorf("Case %v (%s, %v) podQuotaCount mismatch: got %v, expect %v", seq, testcase.path, enabled, len(podQuotaMap), expectedPodQuotaCount) + t.Errorf("Case %v (%s, %s, %v) podQuotaCount mismatch: got %v, expect %v", seq, testcase.name, testcase.path, enabled, len(podQuotaMap), expectedPodQuotaCount) } if len(dirQuotaMap) != expectedDirQuotaCount { fail = true - t.Errorf("Case %v (%s, %v) dirQuotaCount mismatch: got %v, expect %v", seq, testcase.path, enabled, len(dirQuotaMap), expectedDirQuotaCount) + t.Errorf("Case %v (%s, %s, %v) dirQuotaCount mismatch: got %v, expect %v", seq, testcase.name, testcase.path, enabled, len(dirQuotaMap), expectedDirQuotaCount) } if len(quotaPodMap) != expectedQuotaPodCount { fail = true - t.Errorf("Case %v (%s, %v) quotaPodCount mismatch: got %v, expect %v", seq, testcase.path, enabled, len(quotaPodMap), expectedQuotaPodCount) + t.Errorf("Case %v (%s, %s, %v) quotaPodCount mismatch: got %v, expect %v", seq, testcase.name, testcase.path, enabled, len(quotaPodMap), expectedQuotaPodCount) } if len(dirPodMap) != expectedDirPodCount { fail = true - t.Errorf("Case %v (%s, %v) dirPodCount mismatch: got %v, expect %v", seq, testcase.path, enabled, len(dirPodMap), expectedDirPodCount) + t.Errorf("Case %v (%s, %s, %v) dirPodCount mismatch: got %v, expect %v", seq, testcase.name, testcase.path, enabled, len(dirPodMap), expectedDirPodCount) } if len(devApplierMap) != expectedDevApplierCount { fail = true - t.Errorf("Case %v (%s, %v) devApplierCount mismatch: got %v, expect %v", seq, testcase.path, enabled, len(devApplierMap), expectedDevApplierCount) + t.Errorf("Case %v (%s, %s, %v) devApplierCount mismatch: got %v, expect %v", seq, testcase.name, testcase.path, enabled, len(devApplierMap), expectedDevApplierCount) } if len(dirApplierMap) != expectedDirApplierCount { fail = true - t.Errorf("Case %v (%s, %v) dirApplierCount mismatch: got %v, expect %v", seq, testcase.path, enabled, len(dirApplierMap), expectedDirApplierCount) + t.Errorf("Case %v (%s, %s, %v) dirApplierCount mismatch: got %v, expect %v", seq, testcase.name, testcase.path, enabled, len(dirApplierMap), expectedDirApplierCount) } if len(podDirCountMap) != expectedPodDirCountCount { fail = true - t.Errorf("Case %v (%s, %v) podDirCountCount mismatch: got %v, expect %v", seq, testcase.path, enabled, len(podDirCountMap), expectedPodDirCountCount) + t.Errorf("Case %v (%s, %s, %v) podDirCountCount mismatch: got %v, expect %v", seq, testcase.name, testcase.path, enabled, len(podDirCountMap), expectedPodDirCountCount) } if len(quotaSizeMap) != expectedQuotaSizeCount { fail = true - t.Errorf("Case %v (%s, %v) quotaSizeCount mismatch: got %v, expect %v", seq, testcase.path, enabled, len(quotaSizeMap), expectedQuotaSizeCount) + t.Errorf("Case %v (%s, %s, %v) quotaSizeCount mismatch: got %v, expect %v", seq, testcase.name, testcase.path, enabled, len(quotaSizeMap), expectedQuotaSizeCount) } if len(supportsQuotasMap) != expectedSupportsQuotasCount { fail = true - t.Errorf("Case %v (%s, %v) supportsQuotasCount mismatch: got %v, expect %v", seq, testcase.path, enabled, len(supportsQuotasMap), expectedSupportsQuotasCount) + t.Errorf("Case %v (%s, %s, %v) supportsQuotasCount mismatch: got %v, expect %v", seq, testcase.name, testcase.path, enabled, len(supportsQuotasMap), expectedSupportsQuotasCount) } if len(backingDevMap) != expectedBackingDevCount { fail = true - t.Errorf("Case %v (%s, %v) BackingDevCount mismatch: got %v, expect %v", seq, testcase.path, enabled, len(backingDevMap), expectedBackingDevCount) + t.Errorf("Case %v (%s, %s, %v) BackingDevCount mismatch: got %v, expect %v", seq, testcase.name, testcase.path, enabled, len(backingDevMap), expectedBackingDevCount) } if len(mountpointMap) != expectedMountpointCount { fail = true - t.Errorf("Case %v (%s, %v) MountpointCount mismatch: got %v, expect %v", seq, testcase.path, enabled, len(mountpointMap), expectedMountpointCount) + t.Errorf("Case %v (%s, %s, %v) MountpointCount mismatch: got %v, expect %v", seq, testcase.name, testcase.path, enabled, len(mountpointMap), expectedMountpointCount) } if fail { - logAllMaps(fmt.Sprintf("%v %s", seq, testcase.path)) + logAllMaps(fmt.Sprintf("%v %s %s", seq, testcase.name, testcase.path)) } } os.Remove(projectsFile)