pkg/oci: Use a file lock to avoid concurrency issues #3842

mauriciovasquezbernal · 2024-12-27T22:05:56Z

Invoking multiple instances of the ig binary on parallel causes issues
as the oras library doesn't provide any multi process syncronization
guarantees on the index.json file. This commit fixes that by using a
file lock and some logic to detect when this file has been updated by
another process.

Specifically, this commit:

Disables AutoSaveIndex to have control over when the index.json file
is written.
Uses a file lock to be sure only one process is writing to the
index.json file at a time.
Adds some logic to retry an operation when the index.json file has been
updated by another process.

Different alternatives were considered:

big lock: It caused issues as the critical section was too big and
different instances of ig timeout while running gadgets.
merge index: Instead of retry the whole operation when the index.json
file was written by another process, we could merge it with the changes.
However it's difficult to implement as it'll need the oras library to
provide a list of changes on the index to apply.
use a db to store the index: It probably requires a lot of changes to
the oras library.

Testing

All the cases described here work fine with this PR, but fail before it

Multiple ig instances run on parallel

Try executing ig image commands at the same time, like building multiple gadgets and listing all gadgets:

ig build on parallel

terminal 1:

$ while true; do sudo ig image list > /dev/null; done

terminal 2

$ make -C gadgets/ -j 8 build
...

Running multiple gadgets at the same time with the oci store is empty

$ sudo rm -rf /var/lib/ig
$ sudo ig run trace_open & sudo ig run trace_exec
[1] 139772
RUNTIME.CONTAINERNAME          COMM                          PID              TID               FD FNAME                            MODE             ERROR
RUNTIME.CONTAINERNAME                                    COMM                    PID        TID ARGS                               ERRO

ig-k8s/ig daemon and ig run at the same time

Let's start with an empty oci store

$ sudo rm -rf /var/lib/ig
$ sudo ig image list
REPOSITORY                                TAG                                      DIGEST       CREATED

Then, run ig daemon

$ sudo ig daemon
INFO[0000] starting Inspektor Gadget daemon at "unix:///var/run/ig/ig.socket"
WARN[0000] reading config: open /root/.ig/config.yaml: no such file or directory
WARN[0000] no TLS configuration provided, communication between daemon and CLI will not be encrypted

On another terminal, try to run a gadget without pulling the image:

$ sudo gadgetctl run trace_open --pull never
Error: fetching gadget information: getting gadget info: rpc error: code = Unknown desc = getting gadget info: initializing and preparing operators: instantiating operator "oci": ensuring image: resolving image "ghcr.io/inspektor-gadget/gadget/trace_open:latest" on local registry: not found

Pull the image with ig

$ sudo ig image pull trace_open
Pulling trace_open...
Successfully pulled ghcr.io/inspektor-gadget/gadget/trace_open:latest@sha256:b4deed0f11ddc4b365aa492d2b1c73f9e7d3cb3ad2138a83024c6414ea68a120

Running the gadget again works:

$ sudo gadgetctl run trace_open --pull never
RUNTIME.CONTAINERNAME          COMM                    PID        TID  FD FNAME                            MODE             ERROR

It means ig daemon has visibility on the changes make by ig to the store without having to restart it.

Fixes #3554

Ref oras-project/oras-go#472 (comment)
Ref oras-project/oras-go#286
Ref ratify-project/ratify#1110
Ref opencontainers/umoci#216 (comment)
Ref opencontainers/umoci#146

burak-ok

There is another exported function from the oci.go file: GetContentFromDescriptor

I think it would also need the locking, right?

burak-ok · 2025-01-03T13:55:31Z

While thinking about it: Do we want to have a timeout while we wait on the lock?
If the file somehow didn't get unlocked we should notify the user instead of hanging indefinitly and giving no feedback

mauriciovasquezbernal · 2025-01-07T16:02:51Z

While thinking about it: Do we want to have a timeout while we wait on the lock? If the file somehow didn't get unlocked we should notify the user instead of hanging indefinitly and giving no feedback

Good idea. I used the TryLock with a timeout.

alban · 2025-01-07T17:03:51Z

I wonder if the 5-second timeout is too short: pulling an OCI image on a slow network could take more than 5 seconds. Another instance of ig might want to wait for longer before giving up.

Alternatively, is it possible to download a large image in a temporary location before saving it into the OCI store? That might minimize the lock time.

burak-ok · 2025-01-07T18:20:43Z

Alternatively, is it possible to download a large image in a temporary location before saving it into the OCI store? That might minimize the lock time.

We could pull it to a temporary in memory store. And then "copy" that to the filesystem

mauriciovasquezbernal · 2025-01-07T22:09:45Z

Good point about pulling the image. The main issue is to have concurrent writes/reads to the index.json file. I pushed a different version that uses AutoSaveIndex: false and manually saves the index while holding the lock, hence it's held by a very short period of time avoiding timeout issues.

pkg/oci/oci.go

mauriciovasquezbernal · 2025-01-10T16:46:45Z

I had to change this again. I've switched to using different instances of oci.Store in the same process and protecting them with a mutex, it turned to be a bottleneck and the tests were failing with timeouts. Now I'm using a middle ground implementation: for the same process we use the same oci.Store instance and rely on it for the synchronization, for different processes we use a file lock.

I'm aware of the limitations of this implementation: (1) ig can't be run when ig-k8s or ig daemon is running and (2) multiple instances of ig running at the same time will have big contention. Unfortunately, I couldn't find a better solution, and according to oras-project/oras-go#472 (comment) "Multi-process support is also another non-trivial work and it is not on the roadmap of oras-go as it requires an atomicity source such as databases, file systems with O_EXCL support, and etc.", so this PR is a kind of best-effort approach for now.

eiffel-fl · 2025-01-13T11:30:46Z

cmd/ig/daemon.go

+		if err := oci.Lock(context.Background()); err != nil {
+			return fmt.Errorf("locking OCI storage: %w", err)
+		}
+		defer oci.Unlock()


Why do you want to lock the daemon?
If daemon A pull an image, it has the lock, then daemon B should wait for the lock to be released before listing all images.
So, only operations need to be locked.

Please see #3842 (comment) with some reasoning about it. In other words, what you're describing is the ideal case, but it's very difficult to implement (at least, I haven't found a solution for it).

I do not like "big kernel lock"-like solution.
Particularly one problem I see here is we cannot run both ig-k8s and ig at the same time, which is strange because you could use ig daemon for another machine and wanting to run ig on your local one.

We already contributed to oras-go, I would reach them and ask whether they see big blockers toward this.
Having a good solution upstream would be better for everyone.

I do not like "big kernel lock"-like solution.

I agree. However it doesn't seem to be trivial and this PR is just preventing the user from running something racy, like ig-k8s and ig on the same machine.

We already contributed to oras-go, I would reach them and ask whether they see big blockers toward this.
Having a good solution upstream would be better for everyone.

I'll reach to them to see if they have any ideas on how to implement it.

I did some research (ref links on PR description) and updated this PR to use a retry approach. I've been testing it and it seems to be working fine. There is one test failing, but I'm confident it can be fixed.

There is one test failing, but I'm confident it can be fixed.

It should be fixed by the latest push.

gadget-container/gadgettracermanager/main.go

pkg/oci/oci.go

.github/workflows/inspektor-gadget.yml

eiffel-fl

I have some comments on the try approach.
Can you please open a dedicated PR for the first three commits? They are not related to the problem you are trying to solve here and we can merge them as is right now.

pkg/oci/helpers.go

pkg/oci/oci.go

mauriciovasquezbernal · 2025-01-21T12:34:09Z

Can you please open a dedicated PR for the first three commits? They are not related to the problem you are trying to solve here and we can merge them as is right now.

Done in #3919

go.mod

pkg/oci/oci.go

burak-ok

Thanks for fixing this finally

pkg/oci/helpers.go

pkg/oci/oci.go

pkg/oci/helpers.go

pkg/oci/oci.go

pkg/oci/retry.go

alban · 2025-01-29T18:26:00Z

pkg/oci/retry.go

+var errRetry = errors.New("retry")
+
+// ErrRetryLimitExceeded indicates the function was retry too many times.
+var ErrRetryLimitExceeded = errors.New("retry limit exceeded")


Suggested change

var ErrRetryLimitExceeded = errors.New("retry limit exceeded")

var errRetryLimitExceeded = errors.New("retry limit exceeded")

I think it's fine to have this public. Callers could use it.

alban · 2025-01-29T18:31:42Z

pkg/oci/oci.go

+	if err := ociStore.Tag(context.TODO(), targetDescriptor, dst.String()); err != nil {
+		return nil, fmt.Errorf("tagging image: %w", err)


Does Tag fail if it is already tagged by a previous attempt?

It appears to be idempotent:

$ sudo ig image tag trace_open foo $ sudo ig image tag trace_open foo $ sudo ig image tag trace_open foo $ echo $? 0

alban · 2025-01-29T18:38:03Z

pkg/oci/oci.go

+		if err != nil {
+			return
+		}
+		err = ociStore.saveIndexWithLock()


This function calls ociStore.GC(ctx) line 608.

But the documentation of GC says:

// GC removes garbage from Store. Unsaved index will be lost. To prevent unexpected // loss, call SaveIndex() before GC or set AutoSaveIndex to true.

With this PR, SaveIndex() is no longer called before GC().

Good point. I've read this comment but I didn't get the idea before. I updated it to call saveIndexWithLock before GC(). I think GC() can still be racy at this point, but I'll explicitly ignore this for now.

pkg/oci/retry.go

pkg/oci/local_oci_store.go

Invoking multiple instances of the ig binary on parallel causes issues as the oras library doesn't provide any multi process syncronization guarantees on the index.json file. This commit fixes that by using a file lock and some logic to detect when this file has been updated by another process. Specifically, this commit:: - Disables AutoSaveIndex to have control over when the index.json file is written. - Uses a file lock to be sure only one process is writing to the index.json file at a time. - Adds some logic to retry an operation when the index.json file has been updated by another process. Different alternatives were considered: - big lock: It caused issues as the critical section was too big and different instances of ig timeout while running gadgets. - merge index: Instead of retry the whole operation when the index.json file was written by another process, we could merge it with the changes. However it's difficult to implement as it'll need the oras library to provide a list of changes on the index to apply. - use a db to store the index: It probably requires a lot of changes to the oras library. Signed-off-by: Mauricio Vásquez <[email protected]>

Signed-off-by: Francis Laniel <[email protected]>

mauriciovasquezbernal · 2025-01-30T16:13:26Z

I've seen a snapshot process fail twice here, let's merge it and monitor to see if it continues happening.

mauriciovasquezbernal requested a review from alban as a code owner December 27, 2024 22:05

mauriciovasquezbernal force-pushed the mauricio/lock-oci branch from 596036f to be9d61d Compare December 27, 2024 22:09

burak-ok reviewed Dec 30, 2024

View reviewed changes

mauriciovasquezbernal force-pushed the mauricio/lock-oci branch from be9d61d to 616f249 Compare January 7, 2025 16:02

mauriciovasquezbernal force-pushed the mauricio/lock-oci branch from 616f249 to 3eebf1f Compare January 7, 2025 22:03

burak-ok reviewed Jan 8, 2025

View reviewed changes

pkg/oci/oci.go Outdated Show resolved Hide resolved

mauriciovasquezbernal force-pushed the mauricio/lock-oci branch from 3eebf1f to 6742a83 Compare January 8, 2025 16:01

mauriciovasquezbernal requested a review from flyth as a code owner January 8, 2025 16:01

burak-ok reviewed Jan 9, 2025

View reviewed changes

pkg/oci/oci.go Outdated Show resolved Hide resolved

pkg/oci/oci.go Outdated Show resolved Hide resolved

mauriciovasquezbernal force-pushed the mauricio/lock-oci branch 5 times, most recently from 5bd1145 to 75d3f5a Compare January 10, 2025 16:39

mauriciovasquezbernal requested a review from blanquicet as a code owner January 10, 2025 16:39

mauriciovasquezbernal force-pushed the mauricio/lock-oci branch 2 times, most recently from bc32c07 to 0fd8e32 Compare January 10, 2025 19:52

mauriciovasquezbernal requested review from eiffel-fl and mqasimsarfraz as code owners January 10, 2025 19:52

eiffel-fl reviewed Jan 13, 2025

View reviewed changes

.github/workflows/inspektor-gadget.yml Show resolved Hide resolved

mauriciovasquezbernal force-pushed the mauricio/lock-oci branch from 6e6728a to 49f518b Compare January 13, 2025 15:36

mauriciovasquezbernal force-pushed the mauricio/lock-oci branch 2 times, most recently from 6527364 to 71b610a Compare January 20, 2025 21:09

eiffel-fl reviewed Jan 21, 2025

View reviewed changes

pkg/oci/helpers.go Outdated Show resolved Hide resolved

pkg/oci/oci.go Outdated Show resolved Hide resolved

mauriciovasquezbernal mentioned this pull request Jan 21, 2025

pkg/oci: small fixes #3919

Merged

mauriciovasquezbernal force-pushed the mauricio/lock-oci branch 3 times, most recently from 3472437 to 68aebb3 Compare January 27, 2025 19:56

burak-ok reviewed Jan 28, 2025

View reviewed changes

go.mod Outdated Show resolved Hide resolved

pkg/oci/oci.go Outdated Show resolved Hide resolved

pkg/oci/oci.go Outdated Show resolved Hide resolved

mauriciovasquezbernal force-pushed the mauricio/lock-oci branch from 68aebb3 to 4cdc031 Compare January 28, 2025 16:06

burak-ok approved these changes Jan 29, 2025

View reviewed changes

pkg/oci/helpers.go Outdated Show resolved Hide resolved

mauriciovasquezbernal force-pushed the mauricio/lock-oci branch from 4cdc031 to 0b1bfdc Compare January 29, 2025 15:02

eiffel-fl reviewed Jan 29, 2025

View reviewed changes

pkg/oci/oci.go Outdated Show resolved Hide resolved

pkg/oci/oci.go Outdated Show resolved Hide resolved

alban reviewed Jan 29, 2025

View reviewed changes

pkg/oci/helpers.go Outdated Show resolved Hide resolved

pkg/oci/helpers.go Outdated Show resolved Hide resolved

pkg/oci/helpers.go Outdated Show resolved Hide resolved

pkg/oci/helpers.go Outdated Show resolved Hide resolved

pkg/oci/oci.go Outdated Show resolved Hide resolved

mauriciovasquezbernal force-pushed the mauricio/lock-oci branch from 0b1bfdc to 4262ee7 Compare January 29, 2025 17:42

alban reviewed Jan 29, 2025

View reviewed changes

mauriciovasquezbernal force-pushed the mauricio/lock-oci branch from 4262ee7 to 8352606 Compare January 29, 2025 19:57

eiffel-fl approved these changes Jan 30, 2025

View reviewed changes

pkg/oci/retry.go Outdated Show resolved Hide resolved

burak-ok reviewed Jan 30, 2025

View reviewed changes

pkg/oci/local_oci_store.go Outdated Show resolved Hide resolved

mauriciovasquezbernal and others added 3 commits January 30, 2025 08:33

gadgets: Rework the makefile to add better parallelism.

6ea6ca1

Signed-off-by: Francis Laniel <[email protected]>

ci: Use -j$(nproc) with gadgets Makefile.

dbabdd9

Signed-off-by: Francis Laniel <[email protected]>

mauriciovasquezbernal force-pushed the mauricio/lock-oci branch from 8352606 to dbabdd9 Compare January 30, 2025 13:33

mauriciovasquezbernal merged commit 1cb345e into main Jan 30, 2025
77 checks passed

mauriciovasquezbernal deleted the mauricio/lock-oci branch January 30, 2025 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/oci: Use a file lock to avoid concurrency issues #3842

pkg/oci: Use a file lock to avoid concurrency issues #3842

mauriciovasquezbernal commented Dec 27, 2024 •

edited

Loading

burak-ok left a comment

burak-ok commented Jan 3, 2025

mauriciovasquezbernal commented Jan 7, 2025

alban commented Jan 7, 2025

burak-ok commented Jan 7, 2025

mauriciovasquezbernal commented Jan 7, 2025

mauriciovasquezbernal commented Jan 10, 2025

eiffel-fl Jan 13, 2025

mauriciovasquezbernal Jan 13, 2025

eiffel-fl Jan 14, 2025

mauriciovasquezbernal Jan 14, 2025

mauriciovasquezbernal Jan 20, 2025

mauriciovasquezbernal Jan 21, 2025

eiffel-fl left a comment

mauriciovasquezbernal commented Jan 21, 2025

burak-ok left a comment

alban Jan 29, 2025

mauriciovasquezbernal Jan 29, 2025

alban Jan 29, 2025

mauriciovasquezbernal Jan 29, 2025 •

edited

Loading

alban Jan 29, 2025

mauriciovasquezbernal Jan 29, 2025

mauriciovasquezbernal commented Jan 30, 2025

	var ErrRetryLimitExceeded = errors.New("retry limit exceeded")
	var errRetryLimitExceeded = errors.New("retry limit exceeded")

		if err := ociStore.Tag(context.TODO(), targetDescriptor, dst.String()); err != nil {
		return nil, fmt.Errorf("tagging image: %w", err)

pkg/oci: Use a file lock to avoid concurrency issues #3842

pkg/oci: Use a file lock to avoid concurrency issues #3842

Conversation

mauriciovasquezbernal commented Dec 27, 2024 • edited Loading

Testing

Multiple ig instances run on parallel

ig build on parallel

Running multiple gadgets at the same time with the oci store is empty

ig-k8s/ig daemon and ig run at the same time

burak-ok left a comment

Choose a reason for hiding this comment

burak-ok commented Jan 3, 2025

mauriciovasquezbernal commented Jan 7, 2025

alban commented Jan 7, 2025

burak-ok commented Jan 7, 2025

mauriciovasquezbernal commented Jan 7, 2025

mauriciovasquezbernal commented Jan 10, 2025

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

eiffel-fl left a comment

Choose a reason for hiding this comment

mauriciovasquezbernal commented Jan 21, 2025

burak-ok 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

mauriciovasquezbernal Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mauriciovasquezbernal commented Jan 30, 2025

mauriciovasquezbernal commented Dec 27, 2024 •

edited

Loading

mauriciovasquezbernal Jan 29, 2025 •

edited

Loading