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

fix: automatically cleanup ZeebeVolumes #660

Merged
merged 3 commits into from
Mar 11, 2024
Merged

fix: automatically cleanup ZeebeVolumes #660

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
21d75bd
fix: mark volumes to be cleaned up automatically
lenaschoenburg Mar 8, 2024
ddddae3
test: verify that volumes are cleanup up automatically
lenaschoenburg Mar 8, 2024
2814d63
style: fix formatting of pom
lenaschoenburg Mar 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions core/src/main/java/io/zeebe/containers/ZeebeVolume.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,15 @@
import io.zeebe.containers.util.TinyContainer;
import java.io.IOException;
import java.nio.file.Path;
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
import java.util.function.UnaryOperator;
import org.apiguardian.api.API;
import org.apiguardian.api.API.Status;
import org.testcontainers.DockerClientFactory;
import org.testcontainers.containers.GenericContainer;
import org.testcontainers.utility.ResourceReaper;

/**
* A simple wrapper to create Docker volumes which are managed by Testcontainers. The created object
Expand Down Expand Up @@ -160,9 +163,13 @@
*/
public static ZeebeVolume newVolume(final UnaryOperator<CreateVolumeCmd> configurator) {
final DockerClient client = DockerClientFactory.instance().client();
try (final CreateVolumeCmd command = client.createVolumeCmd()) {
final CreateVolumeResponse response =
configurator.apply(command.withLabels(DockerClientFactory.DEFAULT_LABELS)).exec();
final Map<String, String> labels = new HashMap<>();
labels.putAll(DockerClientFactory.DEFAULT_LABELS);
//noinspection deprecation
labels.putAll(ResourceReaper.instance().getLabels());

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note

Invoking
ResourceReaper.getLabels
Loading
should be avoided because it has been deprecated.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this means it's at risk of being removed, or at least hidden? 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of relying on an internal API, what about adding our own marker label? So we keep adding the default labels, but then add our own label (e.g. io.camunda.zeebe.sessionId), and register it via

public final class ZeebeVolume {
  private static final String SESSION_LABEL = "io.zeebe.containers.sessionId";
  private static final Map<String, String> MARKER_LABELS = Collections.singletonMap(SESSION_LABEL, DockerClientFactory.SESSION_ID);

  static {
    ResourceReaper.instance().registerLabelsFilterForCleanup(MARKER_LABELS);
  }

  public static ZeebeVolume newVolume(final UnaryOperator<CreateVolumeCmd> configurator) {
    final DockerClient client = DockerClientFactory.instance().client();
    try (final CreateVolumeCmd command = client.createVolumeCmd()) {
      final Map<String, String> labels = new HashMap<String, String>(DockerClientFactory.DEFAULT_LABELS);
      labels.putAll(MARKER_LABELS);

      final CreateVolumeResponse response = configurator.apply(command.withLabels(labels).exec();
      return new ZeebeVolume(response.getName(), client);
    }
  }

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also means we won't need reflection in the test to check if everything is working as expected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right, that's how this is supposed to work 🤦

Although I don't see how this removes the need for reflection in the tests. We'd still want to test that the container is actually removed, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's exactly what getCleanupRunnable does, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought maybe that part is not necessary?

        final Method performCleanup = reaperClass.getDeclaredMethod("performCleanup");
        performCleanup.setAccessible(true);
        performCleanup.invoke(reaper);

🤷 It's really minor anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah that works 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Registering the new labels with Ryuk does work btw:

[testcontainers-ryuk] DEBUG org.testcontainers.utility.ResourceReaper - Sending 'label=io.zeebe.containers.sessionId%3D85662216-fce6-4aab-a7c1-fe9afd3e3419' to Ryuk

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, we can make use of that when they remove the deprecated API 😄


try (final CreateVolumeCmd command = client.createVolumeCmd().withLabels(labels)) {
final CreateVolumeResponse response = configurator.apply(command).exec();
return new ZeebeVolume(response.getName(), client);
}
}
Expand Down
35 changes: 35 additions & 0 deletions core/src/test/java/io/zeebe/containers/ZeebeVolumeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
import com.github.dockerjava.api.command.InspectVolumeResponse;
import com.github.dockerjava.api.model.AccessMode;
import com.github.dockerjava.api.model.Bind;
import java.lang.reflect.Constructor;
import java.lang.reflect.Method;
import java.util.Map;
import org.junit.jupiter.api.Test;
import org.testcontainers.DockerClientFactory;
Expand Down Expand Up @@ -97,6 +99,39 @@ void shouldAttachToZeebeBroker() {
}
}

@Test
void shouldCleanupVolume() {
// given
final DockerClient client = DockerClientFactory.lazyClient();
final ZeebeVolume volume = ZeebeVolume.newVolume();
final Runnable performCleanup = getCleanupRunnable();

// when
performCleanup.run();

// then
assertThat(client.listVolumesCmd().exec().getVolumes())
.as("the volume should be removed")
.noneMatch(v -> v.getName().equals(volume.getName()));
}

private Runnable getCleanupRunnable() {
return () -> {
try {
final Class<?> reaperClass =
Class.forName("org.testcontainers.utility.JVMHookResourceReaper");
final Constructor<?> constructor = reaperClass.getDeclaredConstructor();
constructor.setAccessible(true);
final Object reaper = constructor.newInstance();
final Method performCleanup = reaperClass.getDeclaredMethod("performCleanup");
performCleanup.setAccessible(true);
performCleanup.invoke(reaper);
} catch (Exception e) {
throw new RuntimeException(e);
}
};
}

private void assertVolumeIsCorrectlyMounted(
final DockerClient client, final ZeebeVolume volume, final String containerId) {
final InspectContainerResponse containerResponse =
Expand Down
8 changes: 4 additions & 4 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
<artifactId>community-hub-release-parent</artifactId>
<version>1.4.4</version>
<!-- do not remove empty tag - http://jira.codehaus.org/browse/MNG-4687 -->
<relativePath />
<relativePath/>
</parent>

<groupId>io.zeebe</groupId>
Expand Down Expand Up @@ -483,7 +483,7 @@
<excludes>
<exclude>**/target/**/*.md</exclude>
</excludes>
<flexmark />
<flexmark/>
</markdown>
</configuration>
<executions>
Expand Down Expand Up @@ -531,7 +531,7 @@
<goal>check</goal>
</goals>
<phase>validate</phase>
<configuration />
<configuration/>
</execution>
</executions>
</plugin>
Expand Down Expand Up @@ -666,7 +666,7 @@
<configuration>
<!-- see more https://maven.apache.org/enforcer/enforcer-rules/index.html -->
<rules>
<banDuplicatePomDependencyVersions />
<banDuplicatePomDependencyVersions/>
</rules>
</configuration>
</execution>
Expand Down
Loading