Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Partial match in volume discovery can lead to endless volume removal loop #2207

Open
thomaswiese0 opened this issue Oct 10, 2023 · 2 comments · May be fixed by #2218
Open

Partial match in volume discovery can lead to endless volume removal loop #2207

thomaswiese0 opened this issue Oct 10, 2023 · 2 comments · May be fixed by #2218

Comments

@thomaswiese0
Copy link

If non balena-managed volumes are present that partially match the <appId>_<volumeName> expression in deconstructDockerName, e.g., a2_b, the supervisor will go into an endless loop trying to remove a volume with name 2_b (the partial match). The device becomes unusable.
@thomaswiese0
Copy link
Author

This should fix it:

From fd2af070b5e455bc001bd7531d24ec39fd6c6de8 Mon Sep 17 00:00:00 2001
From: Thomas Wiese <[email protected]>
Date: Tue, 10 Oct 2023 12:35:51 +0200
Subject: [PATCH] Match full string when discovering docker volumes

When non-balena managed docker volumes are present that partially match
the appId_volumeName regular expression, the partial name will be
used incorrectly as the full name by the supervisor.

Change-type: patch

Signed-off-by: Thomas Wiese <[email protected]>
---
 src/compose/volume.ts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/compose/volume.ts b/src/compose/volume.ts
index 976eb507..da1392f3 100644
--- a/src/compose/volume.ts
+++ b/src/compose/volume.ts
@@ -126,7 +126,7 @@ export class Volume {
 		name: string;
 		appId: number;
 	} {
-		const match = name.match(/(\d+)_(\S+)/);
+		const match = name.match(/^(\d+)_(\S+)$/);
 		if (match == null) {
 			throw new InternalInconsistencyError(
 				`Could not detect volume data from docker name: ${name}`,
-- 
2.41.0

Better yet to use labels somehow:
if [ "$(balena volume inspect $volume | jq '.[0].Labels."io.balena.supervised"')" = '"true"' ]; then ...

pipex added a commit that referenced this issue Oct 23, 2023
@pipex
Fix volume regex to match full volume name
86a5075 
Previous regex would recognize non balena volumes with partial matches.

Closes: #2207
@pipex pipex linked a pull request Oct 23, 2023 that will close this issue
Open
pipex added a commit that referenced this issue Oct 23, 2023
@pipex
Match full string when discovering docker volumes
311fb61 
When non-balena managed docker volumes are present that partially match
the appId_volumeName regular expression, the partial name will be
used incorrectly as the full name by the supervisor.

Contributed-by: @thomaswiese0
Change-type: patch
Closes: #2207
@pipex
Copy link
Contributor

pipex commented Oct 23, 2023

Hi @thomaswiese0 thank you for the issue and the correction. You are correct that using labels is the right approach, unfortunately the only way to modify volumes is to recreate them which we cannot do without destroying data. We are considering a volume migration procedure at which point we should be able to sanitize volume metadata. In the meantime, I've created PR #2218 with your patch

Thanks again

pipex added a commit that referenced this issue Oct 23, 2023
@pipex
Match full string when discovering docker volumes
cedffa2 
When non-balena managed docker volumes are present that partially match
the appId_volumeName regular expression, the partial name will be
used incorrectly as the full name by the supervisor.

Change-type: patch
Contributed-by: @thomaswiese0
Closes: #2207
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix volume regex to match full volume name balena-os/balena-supervisor
2 participants
@pipex @thomaswiese0