fix: disabe auto remove if debug is enabled #757

Haegi · 2022-12-21T13:58:21Z

fixes: #748

Changes

This PR changes the behaviour of the WithDebugMode option. If it is specified, the container will not be auto removed.

Tests

Stop container without WithDebugMode -> should be gone
Stop container with WithDebugMode -> should be still visible

orlangure · 2022-12-23T20:00:18Z

This is awesome!

I see one issue with this change: gnomock.Stop() will now not remove the container, if it was created with debug flag, and this isn't the behaviour I would expect in this case. Containers started with debug mode should remain for debugging purposes in case something doesn't work as expected, but if the test completes and calls Stop explicitly, the container should be killed completely. Do you agree?

codecov-commenter · 2022-12-23T20:05:21Z

Codecov Report

Base: 86.32% // Head: 78.52% // Decreases project coverage by -7.80% ⚠️

Coverage data is based on head (64532b3) compared to base (8658ea5).
Patch coverage: 75.00% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #757      +/-   ##
==========================================
- Coverage   86.32%   78.52%   -7.80%     
==========================================
  Files          49       48       -1     
  Lines        2325     2324       -1     
==========================================
- Hits         2007     1825     -182     
- Misses        165      319     +154     
- Partials      153      180      +27

Impacted Files	Coverage Δ
docker.go	`68.44% <73.33%> (-23.77%)`	⬇️
gnomock.go	`69.33% <100.00%> (-15.34%)`	⬇️
parallel.go	`0.00% <0.00%> (-100.00%)`	⬇️
internal/errors/errors.go	`20.00% <0.00%> (-80.00%)`	⬇️
internal/gnomockd/stop.go	`43.75% <0.00%> (-56.25%)`	⬇️
internal/health/health.go	`62.50% <0.00%> (-37.50%)`	⬇️
internal/cleaner/cleaner.go	`66.66% <0.00%> (-33.34%)`	⬇️
options.go	`69.87% <0.00%> (-30.13%)`	⬇️
internal/registry/registry.go	`71.42% <0.00%> (-28.58%)`	⬇️
... and 6 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Haegi · 2022-12-31T12:52:31Z

This is awesome!

I see one issue with this change: gnomock.Stop() will now not remove the container, if it was created with debug flag, and this isn't the behaviour I would expect in this case. Containers started with debug mode should remain for debugging purposes in case something doesn't work as expected, but if the test completes and calls Stop explicitly, the container should be killed completely. Do you agree?

Good point. I will take a look next week.
I guess just the container removal logic when calling gnomock.Stop() is missing, right?

orlangure · 2023-01-03T20:02:54Z

I guess just the container removal logic when calling gnomock.Stop() is missing, right?

@Haegi correct, because if the AutoRemove flag is always set, there is no need to implement container removal; it is enough to just stop it.

Haegi · 2023-01-05T18:31:54Z

I guess just the container removal logic when calling gnomock.Stop() is missing, right?

@Haegi correct, because if the AutoRemove flag is always set, there is no need to implement container removal; it is enough to just stop it.

I implemented it in Haegi@7f3b016.
BUT super rarely I got some message that the deletion is already in progress. But unfortunately, I didn't find a way to reproduce it.

orlangure

@Haegi, thank you for taking care of container removal. There are a couple of small things I think are still missing. Otherwise this is great😻

orlangure · 2023-01-06T17:08:35Z

docker.go

+	d.lock.Lock()
+	defer d.lock.Unlock()
+
+	err := d.client.ContainerRemove(ctx, id, types.ContainerRemoveOptions{})


I think it would be better to use Force: true here, just in case.

I added it here: a2b478c

orlangure · 2023-01-06T17:09:30Z

gnomock.go

@@ -252,7 +252,7 @@ func (g *g) stop(c *Container) error {
 		}
 	}

-	return nil
+	return cli.removeContainer(context.Background(), id)


Please handle the "in progress" error gracefully (return without error), otherwise - fail with error. The error happens most likely due to the AutoRemove flag triggering container removal before the code reaches this line.

I take care of it in a2b478c

gnomock_test.go

Haegi · 2023-01-09T14:44:26Z

@Haegi, thank you for taking care of container removal. There are a couple of small things I think are still missing. Otherwise this is great😻

Thanks :) I also noticed also two other things.

The stopContainer currently doesn't ignore if the container can not be found. Should I also add a line to ignore that?
I noticed that the gnomock-test-image is not stopping without gnomock.Stop() and we therefore don't test the auto remove. My idea would be to add some /shutdown endpoint to the test image and call it before calling gnomock.Stop(). This way wecheck if it still works with the auto remove. What do you think?

orlangure · 2023-01-21T14:18:31Z

Hi @Haegi,

Thanks for the updates and sorry for taking long to reply.

I like both ideas, but I think investing in the second one is a bit of an overkill right now. The first point is much more important. It would be great if you could take care of it.

Waiting for the final fixes to finally release a new version with your fixes 😼

…s-enabled

Haegi · 2023-01-24T17:53:51Z

Hi @Haegi,

Thanks for the updates and sorry for taking long to reply.

I like both ideas, but I think investing in the second one is a bit of an overkill right now. The first point is much more important. It would be great if you could take care of it.

Waiting for the final fixes to finally release a new version with your fixes 😼

No worries :)
I changed the stopContainer logic now to not thrown an error when the container is already gone.
I also had to adapt one test to not use the is already gone error but check it with the client.
But still the TestPreset_duplicateContainerName is failing and I am not sure why the originalContainer is supposed to fail when trying to stop it.

orlangure

Thank you!

The test you are mentioning fails because of the same (or very similar) reason that the other test you fixed was failing: the original test expected an error because it was a "not found" error when trying to stop a container that doesn't exist anymore (it gets replaced with a new container with the same name). Now that we ignore the "not found" error the test doesn't get an expected error, so we basically change the expectations and should not expect an error anymore.

I also left one last comment, and unless I'm missing something, fixing it together with the test will complete that work, and I'll prepare a new release. This is awesome!

orlangure · 2023-01-30T23:05:08Z

docker.go

+	defer d.lock.Unlock()
+
+	err := d.client.ContainerRemove(ctx, id, types.ContainerRemoveOptions{Force: true})
+	if err != nil && !client.IsErrNotFound(err) && isDeletionAlreadyInProgessError(err, id) {


I think isDeletionAlreadyInProgessError should be negated just like "not found" error, we want to ignore it instead of failing container removal.

Thanks for the hint! I had to change the logic of isDeletionAlreadyInProgessError. It seems like it was not really checking for the error. Now I was able to get it right and negate it.

Haegi · 2023-02-21T19:19:48Z

Thank you!

The test you are mentioning fails because of the same (or very similar) reason that the other test you fixed was failing: the original test expected an error because it was a "not found" error when trying to stop a container that doesn't exist anymore (it gets replaced with a new container with the same name). Now that we ignore the "not found" error the test doesn't get an expected error, so we basically change the expectations and should not expect an error anymore.

I also left one last comment, and unless I'm missing something, fixing it together with the test will complete that work, and I'll prepare a new release. This is awesome!

Thanks for the explanation. I removed the error expectation and instead check with the docker client if the container is done.

Unfortunately, I noticed some other side effect. gnomockd doesn't return 500 anymore, when trying to stop an already stopped container. Should I try to change it to 404 instead of 500 or is 200 also fine? But 200 might be a bit misleading.

orlangure · 2023-02-21T22:23:48Z

I think 200 is fine, the goal is to make sure a container doesn't exist in the end after this endpoint is called, and it really won't exist. Since cleaner was added, Stop and /stop are optional anyway most of the time. I think this is the last fix, and your hard work will be merged and released. Thank you!😻

Haegi · 2023-02-22T16:56:26Z

I think 200 is fine, the goal is to make sure a container doesn't exist in the end after this endpoint is called, and it really won't exist. Since cleaner was added, Stop and /stop are optional anyway most of the time. I think this is the last fix, and your hard work will be merged and released. Thank you!😻

I just changed the test and I also extracted the container listing by id into some helper function called ListContainerByID.
It seems like the github action test is a little bit flaky but it is green now.
Looks like we are finally ready to merge this PR :)

orlangure

Amazing job, thank you for the contribution!

Haegi added 2 commits December 15, 2022 18:05

feat: disable auto remove if debug is enabled

efaa752

test: add withDebug tests

c9ffb31

Haegi mentioned this pull request Dec 21, 2022

Bug: Nearly impossible to debug failing containers #748

Closed

orlangure assigned Haegi Dec 23, 2022

fix: gnomock.Stop() should remove the container

7f3b016

orlangure reviewed Jan 6, 2023

View reviewed changes

fix: ignore already in progress error

a2b478c

Haegi and others added 3 commits January 24, 2023 18:04

Merge branch 'orlangure:master' into bh-disabe-auto-remove-if-debug-i…

ec87262

…s-enabled

fix: ignore not found error when stoping container

7e4021b

test: use container list instead of stop error to assert container

be60a44

orlangure reviewed Jan 30, 2023

View reviewed changes

Haegi added 4 commits February 21, 2023 10:34

test: list container to assert that it is not there anymore

3f0ed87

fix: isDeletionAlreadyInProgessError should be negated

552202d

fix: isDeletionAlreadyInProgessError

64532b3

fix: linter

fa7c8c9

Haegi added 3 commits February 22, 2023 11:42

test: gnomockd expect 200 when container can not be found

465b599

test: fix test & extract ListContainerById to helper function

be07fe0

fix: linter

2eb735c

Haegi requested a review from orlangure February 24, 2023 15:13

orlangure approved these changes Feb 24, 2023

View reviewed changes

orlangure merged commit 5356544 into orlangure:master Feb 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: disabe auto remove if debug is enabled #757

fix: disabe auto remove if debug is enabled #757

Haegi commented Dec 21, 2022

orlangure commented Dec 23, 2022

codecov-commenter commented Dec 23, 2022 •

edited

Loading

Haegi commented Dec 31, 2022

orlangure commented Jan 3, 2023

Haegi commented Jan 5, 2023

orlangure left a comment

orlangure Jan 6, 2023

Haegi Jan 9, 2023

orlangure Jan 6, 2023

Haegi Jan 9, 2023

Haegi commented Jan 9, 2023

orlangure commented Jan 21, 2023

Haegi commented Jan 24, 2023

orlangure left a comment

orlangure Jan 30, 2023

Haegi Feb 21, 2023

Haegi commented Feb 21, 2023

orlangure commented Feb 21, 2023

Haegi commented Feb 22, 2023

orlangure left a comment

fix: disabe auto remove if debug is enabled #757

fix: disabe auto remove if debug is enabled #757

Conversation

Haegi commented Dec 21, 2022

Changes

Tests

orlangure commented Dec 23, 2022

codecov-commenter commented Dec 23, 2022 • edited Loading

Codecov Report

Haegi commented Dec 31, 2022

orlangure commented Jan 3, 2023

Haegi commented Jan 5, 2023

orlangure left a comment

Choose a reason for hiding this comment

orlangure Jan 6, 2023

Choose a reason for hiding this comment

Haegi Jan 9, 2023

Choose a reason for hiding this comment

orlangure Jan 6, 2023

Choose a reason for hiding this comment

Haegi Jan 9, 2023

Choose a reason for hiding this comment

Haegi commented Jan 9, 2023

orlangure commented Jan 21, 2023

Haegi commented Jan 24, 2023

orlangure left a comment

Choose a reason for hiding this comment

orlangure Jan 30, 2023

Choose a reason for hiding this comment

Haegi Feb 21, 2023

Choose a reason for hiding this comment

Haegi commented Feb 21, 2023

orlangure commented Feb 21, 2023

Haegi commented Feb 22, 2023

orlangure left a comment

Choose a reason for hiding this comment

codecov-commenter commented Dec 23, 2022 •

edited

Loading