bmuschko / gradle-docker-plugin

Gradle plugin for managing Docker images and containers.
https://bmuschko.github.io/gradle-docker-plugin/current/user-guide/
Apache License 2.0
1.23k stars 361 forks source link

Starting/Stopping docker daemon invalidates Gradle configuration cache #1192

Closed Jocce-Nilsson closed 1 year ago

Jocce-Nilsson commented 1 year ago

Current Behavior

The file /var/run/docker.sock is saved as a file system entry to the configuration cache. When starting or stopping the Docker daemon, this file changes and causes the configuration cache being invalidated.

Expected Behavior

Configuration cache state should not be affected of file changes outside of the project

Steps to Reproduce (for bugs)

Attached zip file contain minimal test project and a README.md file reproduce-config-cache-docker-sock.zip

Your Environment

Linux

bmuschko commented 1 year ago

AFAIKT, the Unix socket the Docker daemon listens on by default has only been defined as a String input here: https://github.com/bmuschko/gradle-docker-plugin/blob/master/src/main/java/com/bmuschko/gradle/docker/DockerExtension.java#L26-L33. I am not sure that's the issue you are seeing. Am I missing something?

Under what condition do you need to restart the Docker daemon?

Jocce-Nilsson commented 1 year ago

Hi Ben, thanks for a quick reply. It is defined as String only, but... it is evaluated during configuration time so it will be marked as input. https://blog.gradle.org/improvements-in-the-build-configuration-input-tracking

Examples of the build configuration inputs are:

  • Files read by the build logic or plugins at configuration time.
  • Information about the file system (existence of files, directory structure) obtained at configuration time.

And no, there it normally no need to start or stop the Docker daemon unless there is a software update, so in practice perhaps once every couple of weeks. Still, it is something that invalidates the configuration cache and can be avoided.

bmuschko commented 1 year ago

I think we are talking about a tradeoff here.

The daemon endpoint has been declared as an input. Any change to the assigned value, e.g. you are pointing to an endpoint on a different machine rather than your local machine, should cause the plugin's task to be re-run. Does it happen often? Probably not but it if it does happen, it could lead to extremely confusing behavior to end users.

A daemon restart doesn't happen very often either. I am wondering what exactly changes for the file upon a restart. I suspect that a version or identifier changes. Generally speaking, endpoint signatures can change after an upgrade and therefore it would be correct to re-run tasks. There's a chance that a breaking change was introduced or the internal behavior of Docker Engine changed.

From your perspective, how can we fulfill the correctness of both use cases?

Jocce-Nilsson commented 1 year ago

According to the Gradle documentation regarding configuration cache, reading or accessing files during configuration time is not recommended.

I was thinking about replacing the default setting of the extension url with a ValueProvider as the logic is a bit more complex than just using one file. A valueProvider is still possible to override. The ValueProvider would be created by Gradle and be available regardless if configuration cache is calculated or taken from cache.

public abstract class DefaultDockerUrlValueSource implements ValueSource<String, ValueSourceParameters.None> {

    private static final Logger logger = Logging.getLogger(DefaultDockerUrlValueSource.class);

    @Override
    public String obtain() {
        String dockerUrl = getEnv("DOCKER_HOST");
        if (dockerUrl == null) {
            if (OsUtils.isWindows()) {
                if (isFileExists("\\\\.\\pipe\\docker_engine")) {
                    dockerUrl = "npipe:////./pipe/docker_engine";
                }
            } else {
                // macOS or Linux
                if (isFileExists("/var/run/docker.sock")) {
                    dockerUrl = "unix:///var/run/docker.sock";
                } else if (isFileExists(System.getProperty("user.home") + "/.docker/run/docker.sock")) {
                    dockerUrl = "unix://" + System.getProperty("user.home") + "/.docker/run/docker.sock";
                }
            }

            if (dockerUrl == null) {
                dockerUrl = "tcp://127.0.0.1:2375";
            }
        }

        logger.info("Default docker.url set to " + dockerUrl);
        return dockerUrl;
    }

    @Nullable
    String getEnv(String name) {
        return System.getenv(name);
    }

    boolean isFileExists(String path) {
        return new File(path).exists();
    }
}

Then add it to the extension setup.

    public DockerExtension(ObjectFactory objectFactory, ProviderFactory providerFactory) {
        DockerConfigResolver dockerConfigResolver = new DefaultDockerConfigResolver();

        url = objectFactory.property(String.class);
        url.convention(providerFactory.of(DefaultDockerUrlValueSource.class, noneValueSourceSpec -> {}));

All task inputs remain the same, and functionality and task input or up-to-date and cache is same. But configuration cache status will remain over a plain Docker daemon restart.

Jocce-Nilsson commented 1 year ago

Speaking of the devil 😄 image

bmuschko commented 1 year ago

Gotcha. Can you provide a pull request? I don't have the time right now to look deeper into this.

Jocce-Nilsson commented 1 year ago

I started but did not have time to adjust the tests or add a new one, I will see if I can find the time this week.

Jocce-Nilsson commented 1 year ago

@bmuschko the PR is up. About the flag I added: com.bmuschko.gradle.docker.internal.DefaultDockerUrlValueSource.skipCheckOfVarRun It is adding a feature that was not needed for the change, only to be able to test it. On the other hand, perhaps someone has a real use case for skipping /var/run/ in some scenario. It is not needed and can be removed if you want to remove the test case and only keep the change