antivirus seems to keep files in memory

[butonic]

this causes the process to be OOMKilled quite frequently in loadtests. maybe because it does not release the memory early enough? maybe because it does not stream the file content? Needs to be investigated.

600Mi memory gets killed, 700Mi seems to work

[wkloucek]

[kobergj]

It keeps them in memory while scanning. Currently there is no streaming possible from the dataserver. It never stores files on disc.

We would need to implement:
A) Streaming from dataserver to antivirus
B) Streaming from antivirus to icap
C) A save way of writing files to disc and deleting them afterwards

Just for the records: This is a feature request, not a bug 😄

[wkloucek]

I had a look at @butonic 's configuration and it seems like he did not limit the maximum scansize. But from what I know, his tests never use files larger than 100MB.

So even if the antivirus service has this file in memory, it should not need > 600MB of memory.

[kobergj]

Mmh. Codewise it looks fine. Compare antivirus scanning.

It holds the response body in memory until the file is scanned, but closes it correctly, even in error cases.

Could it be that there are multiple virus scans active at the same time?

[wkloucek]

Could it be that there are multiple virus scans active at the same time?
From a code perspective this should currenlty not be possible:

ocis/services/antivirus/pkg/service/service.go

Lines 99 to 100 in c9651de

	for e := range ch {
	ev := e.Event.(events.StartPostprocessingStep)

It holds the response body in memory until the file is scanned, but closes it correctly, even in error cases.

But we don't know what happens inside the icap lib?
Eg. https://github.com/egirna/icap-client/blob/master/request.go#L74C3-L74C7 looks like it duplicates the body and that could happen multiple times!?

[kobergj]

Yes, icap lib could be the baddy. We have already a ticket to replace it: #6764

[dragotin]

This also sounds as if it could cause severe trouble on big installations. Increasing prio.

[dragotin]

Handled in #6764, thus removing this from the prio list. Sorry for the noise.