Enable audit to write cache to disk to reduce memory #1634

ritazh · 2021-11-02T01:36:21Z

Signed-off-by: Rita Zhang rita.z.zhang@gmail.com

What this PR does / why we need it:
Improve audit memory footprint by writing audit cache to disk

Which issue(s) this PR fixes (optional, using fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when the PR gets merged):
Fixes #163
Fixes #1088
Fixes #1279
Partial #1405

Special notes for your reviewer:

codecov-commenter · 2021-11-02T01:40:00Z

Codecov Report

Merging #1634 (40a7480) into master (87cb662) will decrease coverage by 0.66%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1634      +/-   ##
==========================================
- Coverage   52.81%   52.14%   -0.67%     
==========================================
  Files          98       98              
  Lines        8591     8693     +102     
==========================================
- Hits         4537     4533       -4     
- Misses       3695     3800     +105     
- Partials      359      360       +1

Flag	Coverage Δ
unittests	`52.14% <0.00%> (-0.67%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/audit/manager.go	`0.00% <0.00%> (ø)`
...onstrainttemplate/constrainttemplate_controller.go	`58.76% <0.00%> (-0.95%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87cb662...40a7480. Read the comment docs.

config/manager/manager.yaml

pkg/audit/manager.go

maxsmythe · 2021-11-02T03:33:07Z

pkg/audit/manager.go

-					}
-					resp, err := am.opa.Review(ctx, augmentedObj)
+					// for each item, write object to a file along with the namespace
+					fileName := fmt.Sprintf("%s_%d", objNamespace, index)


What purpose does embedding the namespace in the filename serve?

From few lines above, since we are already calling the api server to get the namespace of the object to see if we can skip it to avoid having to create the file if namespace is excluded, we should persist the namespace as part of the filename so we do not have to make another api call later when loading the obj from disk.

objNamespace := objList.Items[index].GetNamespace() isExcludedNamespace, err := am.skipExcludedNamespace(&objList.Items[index])

Two problems with this benefit:

We need to get the full namespace object so that we can evaluate namespace selectors, not the name

We are already caching the namespace in nsCache

From few lines below, the namespace saved as part of the file name is used to lookup the full namespace object from nsCache:

objNs := strings.Split(fileName, "_")[0] ns := corev1.Namespace{} if objNs != "" { ns, err = nsCache.Get(ctx, am.client, objNs)

nit: Can we encapsulate the encoding and decoding logic for these filenames into helper functions, making this naming convention more explicit?

I'm removing the namespace from the filename but prefixing with kind to avoid delays in os.RemoveAll.

maxsmythe · 2021-11-02T03:35:35Z

pkg/audit/manager.go

@@ -403,13 +414,71 @@ func (am *Manager) auditResources(
 			}
 		}
 	}
+	// loop thru each subDir in output dir to get files
+	for i := 0; i < folderCount; i++ {


Actually, it looks like we are auditing after we've looped through all GVKs, we'd definitely need to write out objects to separate directories by GVK in order to avoid clobbering.

One note about how we could be gentler on the API server...

If we perform the audit for a given kind before grabbing the next kind from the API server, we even out the load on the API server somewhat.

Per above comment, I dont think we are clobbering it.

If we perform the audit for a given kind before grabbing the next kind from the API server, we even out the load on the API server somewhat.

Can this optimization be a follow up PR given that we would need to do few rounds of load tests?

Yeah, you're not clobbering it.

One other benefit of interleaving audits and lists... you can wipe the disk after each audit, lowering the maximum amount of disk space used.

Why would we need load tests?

In any case, we can defer, but the reduction in disk space seems valuable (akin to how much memory we got back when we switched to auditing on a per-kind basis)

latest commit audits right after cache to disk for a given kind

maxsmythe · 2021-11-02T03:37:08Z

pkg/audit/manager.go

+		}
+		for _, f := range files {
+			fileName := f.Name()
+			objNs := strings.Split(fileName, "_")[0]


If this is why we are embedding the namespace in the filename, could we retrieve the namespace after we've called am.readUnstructured() and get it from the object itself?

Per above comment, we are already making the api call to get namespace to test ns exclusion prior to saving the file. It makes more sense to me to save it as part of the file at that point instead.

Except we are calling nsCache.Get() here anyway? Per above, we'd need to get the whole namespace object so we can evaluate the namespace selector.

maxsmythe

mostly nits

pkg/audit/manager.go

maxsmythe · 2021-11-03T01:22:43Z

pkg/audit/manager.go

-					}
-					resp, err := am.opa.Review(ctx, augmentedObj)
+					// for each item, write object to a file along with the namespace
+					fileName := fmt.Sprintf("%s_%d", objNamespace, index)


Two problems with this benefit:

We need to get the full namespace object so that we can evaluate namespace selectors, not the name

We are already caching the namespace in nsCache

maxsmythe · 2021-11-03T01:27:19Z

pkg/audit/manager.go

@@ -403,13 +414,71 @@ func (am *Manager) auditResources(
 			}
 		}
 	}
+	// loop thru each subDir in output dir to get files
+	for i := 0; i < folderCount; i++ {


Yeah, you're not clobbering it.

One other benefit of interleaving audits and lists... you can wipe the disk after each audit, lowering the maximum amount of disk space used.

Why would we need load tests?

In any case, we can defer, but the reduction in disk space seems valuable (akin to how much memory we got back when we switched to auditing on a per-kind basis)

maxsmythe · 2021-11-03T01:28:36Z

pkg/audit/manager.go

+		}
+		for _, f := range files {
+			fileName := f.Name()
+			objNs := strings.Split(fileName, "_")[0]


Except we are calling nsCache.Get() here anyway? Per above, we'd need to get the whole namespace object so we can evaluate the namespace selector.

shomron

@ritazh overall looks good! I had a few questions and nits, nothing blocking. LMKWYT.
I think a cool future enhancement might be to process the reviews concurrently to filling the cache - this might improve overall audit time for larger clusters.

pkg/audit/manager.go

shomron · 2021-11-05T01:40:39Z

pkg/audit/manager.go

@@ -239,6 +243,15 @@ func (am *Manager) auditResources(
 	totalViolationsPerConstraint map[util.KindVersionResource]int64,
 	totalViolationsPerEnforcementAction map[util.EnforcementAction]int64,
 	timestamp string) error {
+	// delete all from cache dir before starting audit
+	dir, err := os.ReadDir(*outputDir)


Do we know how many files will be in this directory? Is it better to page through the items using File.Readdirnames(n) instead of making assumptions, or do we know this will be manageable?

Is there any validation we want to do to make sure it's actually our cache directory, and not / or some other unfortunate mistake?

Do we know how many files will be in this directory?

latest commit adds paging to get files

Is there any validation we want to do to make sure it's actually our cache directory

we pass in apiCacheDir to these functions.

apiCacheDir = flag.String("api-cache-dir", defaultApiCacheDir, "The directory where

pkg/audit/manager.go

shomron · 2021-11-05T01:49:14Z

pkg/audit/manager.go

-					}
-					resp, err := am.opa.Review(ctx, augmentedObj)
+					// for each item, write object to a file along with the namespace
+					fileName := fmt.Sprintf("%s_%d", objNamespace, index)


nit: Can we encapsulate the encoding and decoding logic for these filenames into helper functions, making this naming convention more explicit?

pkg/audit/manager.go

Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>

ritazh · 2021-11-08T05:36:56Z

cmd/build/helmify/static/values.yaml

@@ -12,7 +12,7 @@ enableDeleteOperations: false
 enableExternalData: false
 mutatingWebhookFailurePolicy: Ignore
 mutatingWebhookTimeoutSeconds: 3
-auditChunkSize: 0
+auditChunkSize: 500


NOTE: updating default auditChunkSize value

ritazh · 2021-11-08T05:41:49Z

pkg/audit/manager.go

@@ -43,16 +46,18 @@ const (
 	msgSize                          = 256
 	defaultAuditInterval             = 60
 	defaultConstraintViolationsLimit = 20
-	defaultListLimit                 = 0
+	defaultListLimit                 = 500


NOTE: updated default to 500 and flag usage. Will open a separate PR to update doc after the next release is out.

Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>

ritazh · 2021-11-08T15:56:29Z

@maxsmythe @shomron I have addressed you comments, PTAL.

pkg/audit/manager.go

shomron · 2021-11-08T17:20:12Z

pkg/audit/manager.go

+	defer dir.Close()
+	for {
+		names, err := dir.Readdirnames(batchSize)
+		if err == io.EOF || len(names) == 0 {


I think we ignore non-EOF errors here. Should we bubble those back up to the caller?

shomron · 2021-11-08T17:34:28Z

pkg/audit/manager.go

-						Namespace: &ns,
-					}
-					resp, err := am.opa.Review(ctx, augmentedObj)
+					// for each item, write object to a file along with the namespace


Is this comment still accurate? Does it just mean that the namespace is stored within the file payload?

pkg/audit/manager.go

shomron · 2021-11-08T19:20:35Z

pkg/audit/manager.go

@@ -335,6 +347,14 @@ func (am *Manager) auditResources(
 	for gv, gvKinds := range clusterAPIResources {
 	kindsLoop:
 		for kind := range gvKinds {
+			// delete all existing folders from cache dir before starting next kind
+			err := am.removeAllFromDir(*apiCacheDir, int(*auditChunkSize))


Why did this move into the loop? Are we trying to reduce peak disk usage?

I think so?

Could be useful if there are a lot of large lists of kinds.

shomron · 2021-11-08T19:22:21Z

pkg/audit/manager.go


+func (am *Manager) reviewObjects(ctx context.Context, kind string, folderCount int, nsCache *nsCache,


Can we add a comment somewhere with the cache directory structure? Otherwise we need to infer this from the code.

this was commented earlier when we create the sub folders.

// for each batch, create a parent folder // prefix kind to avoid delays in removeall subPath = fmt.Sprintf("%s_%d", kind, folderCount) parentDir := path.Join(*apiCacheDir, subPath)

I will add the same comment in this func as well.

shomron · 2021-11-08T19:23:57Z

pkg/audit/manager.go

+		subDir := fmt.Sprintf("%s_%d", kind, i)
+		pDir := path.Join(*apiCacheDir, subDir)
+
+		files, err := am.getFilesFromDir(pDir, int(*auditChunkSize))


While this pages files from the OS, it still aggregates the list in memory before processing. This should be fine unless we expect the list to be very large. Otherwise we could pass in a processing function to be called per page.

Or maybe walk the directory and call audit on every file we walk into?

walk() is probably preferable, but I don't have an issue with this as-is, since the list of file names is probably much smaller than the list of actual objects.

I think the list of file names is pretty small compare to everything else (actual resources) we are caching. We can further optimize this if we see a dramatic improvement later.

maxsmythe · 2021-11-09T03:41:53Z

pkg/audit/manager.go

@@ -335,6 +347,14 @@ func (am *Manager) auditResources(
 	for gv, gvKinds := range clusterAPIResources {
 	kindsLoop:
 		for kind := range gvKinds {
+			// delete all existing folders from cache dir before starting next kind
+			err := am.removeAllFromDir(*apiCacheDir, int(*auditChunkSize))


I think so?

Could be useful if there are a lot of large lists of kinds.

maxsmythe · 2021-11-09T03:47:04Z

pkg/audit/manager.go

+		subDir := fmt.Sprintf("%s_%d", kind, i)
+		pDir := path.Join(*apiCacheDir, subDir)
+
+		files, err := am.getFilesFromDir(pDir, int(*auditChunkSize))


Or maybe walk the directory and call audit on every file we walk into?

walk() is probably preferable, but I don't have an issue with this as-is, since the list of file names is probably much smaller than the list of actual objects.

Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>

sozercan

LGTM

anlandu · 2023-06-17T00:15:38Z

QQ, I might be misreading but it looks like this only excludes from writing to disk objects in namespaces that are excluded by ProcessExcluder, which would just be the GK config-level excludedNamespaces? Could the map of matchedKinds also include a field for matchedNamespaces so as to also exclude those that don't match the namespaces defined in the constraint? And potentially use that info to winnow down the List api call?

maxsmythe · 2023-06-21T00:06:32Z

In theory, but that would presume that:

It's common to ignore namespaces
The namespaces that are ignored contain a significant chunk of the resources of the cluster

A second point: performance "optimizations" that lower resource coverage are less optimizations and more an acknowledgement of the limits of how far a system can scale. If I can only handle 1000 resources, and remove some from scope, the system can still only handle 1000 resources and I will still have a problem if I scale beyond that size WRT resources that are in scope.

As such, performance improvements, graceful degradation, and removal of upper bounds (e.g. via chunking) should be the main focus, with culling more useful for triage and avoiding unnecessary processing time (as opposed to being an operational necessity).

anlandu · 2023-06-21T00:17:21Z

Thanks for the reply, that makes a lot of sense! I was looking at a case where a cluster had hundreds of thousands of a certain resource type, but none in the default namespace, and were applying the "shouldn't use the default namespace" template with "default" as the only included namespace on the constraint. Obviously a very niche case but it just spurred me to think about possible triages and their feasibility. Totally understand if it's not as worthwhile as actual optimizations!

maxsmythe reviewed Nov 2, 2021

View reviewed changes

maxsmythe requested changes Nov 3, 2021

View reviewed changes

shomron approved these changes Nov 5, 2021

View reviewed changes

ritazh added 3 commits November 7, 2021 21:20

Enable audit to write cache to disk to reduce memory

1208281

Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>

audit right after cache to disk for a kind

e378b74

Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>

add paging to get files

4e90a7d

Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>

ritazh force-pushed the fix-audit-memory branch from 7a8cb30 to 0b074c4 Compare November 8, 2021 05:36

ritazh commented Nov 8, 2021

View reviewed changes

ritazh force-pushed the fix-audit-memory branch from 0b074c4 to 77acf31 Compare November 8, 2021 05:46

update manifests

8fd8a51

Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>

ritazh force-pushed the fix-audit-memory branch from 77acf31 to 8fd8a51 Compare November 8, 2021 05:47

sozercan reviewed Nov 8, 2021

View reviewed changes

pkg/audit/manager.go Show resolved Hide resolved

shomron approved these changes Nov 8, 2021

View reviewed changes

maxsmythe approved these changes Nov 9, 2021

View reviewed changes

add comment and ret non EOF err

40a7480

Signed-off-by: Rita Zhang <rita.z.zhang@gmail.com>

sozercan approved these changes Nov 9, 2021

View reviewed changes

Merge branch 'master' into fix-audit-memory

8f98027

ritazh merged commit 3442bc9 into open-policy-agent:master Nov 10, 2021

ritazh deleted the fix-audit-memory branch November 10, 2021 06:04

peteroneilljr mentioned this pull request May 13, 2022

OPA is writing some stuff to disk what is it? open-policy-agent/community#193

Closed


		func (am Manager) reviewObjects(ctx context.Context, kind string, folderCount int, nsCache nsCache,

Enable audit to write cache to disk to reduce memory #1634

Enable audit to write cache to disk to reduce memory #1634

Conversation

ritazh commented Nov 2, 2021 • edited Loading

codecov-commenter commented Nov 2, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritazh Nov 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxsmythe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shomron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ritazh Nov 8, 2021 • edited Loading

Choose a reason for hiding this comment

ritazh commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sozercan left a comment

Choose a reason for hiding this comment

anlandu commented Jun 17, 2023 • edited Loading

maxsmythe commented Jun 21, 2023

anlandu commented Jun 21, 2023 • edited Loading

ritazh commented Nov 2, 2021 •

edited

Loading

codecov-commenter commented Nov 2, 2021 •

edited

Loading

ritazh Nov 5, 2021 •

edited

Loading

ritazh Nov 8, 2021 •

edited

Loading

anlandu commented Jun 17, 2023 •

edited

Loading

anlandu commented Jun 21, 2023 •

edited

Loading