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.

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

Docker-compose expects ports to be exposed while finding containers to wait for them #329

Closed
mdelapenya opened this issue Jun 11, 2021 · 6 comments
Labels
compose Docker Compose.

Comments

@mdelapenya
Copy link
Member

Describe the bug

When using a docker-compose file that has no exposed ports, then it's not possible to apply any wait strategy, because it uses a port to create the filter when retrieving the running containers.

To Reproduce

Can you write a unit test to reproduce your issue?

docker-compose-no-exposed-ports.yml

version: '3'
services:
  nginx:
    image: nginx:stable-alpine
    ports:
     - "80"

Unit Test

func TestDockerComposeWithWaitStrategy_NoExposedPorts(t *testing.T) {
	path := "./testresources/docker-compose-no-exposed-ports.yml"

	identifier := strings.ToLower(uuid.New().String())

	compose := NewLocalDockerCompose([]string{path}, identifier)
	destroyFn := func() {
		err := compose.Down()
		checkIfError(t, err)
	}
	defer destroyFn()

	err := compose.
		WithCommand([]string{"up", "-d"}).
		WithExposedService("nginx_1", 9080, wait.NewHTTPStrategy("/").WithPort("80/tcp").WithStartupTimeout(10*time.Second)).
		Invoke()
	checkIfError(t, err)

	assert.Equal(t, 1, len(compose.Services))
	assert.Contains(t, compose.Services, "nginx")
}

Expected behavior

It's possible to reach the service by service name.

docker info

$ docker info
Client:
 Context:    default
 Debug Mode: false
 Plugins:
  app: Docker App (Docker Inc., v0.9.1-beta3)
  buildx: Build with BuildKit (Docker Inc., v0.5.1-docker)
  compose: Docker Compose (Docker Inc., 2.0.0-beta.1)
  scan: Docker Scan (Docker Inc., v0.8.0)

Server:
 Containers: 1
  Running: 1
  Paused: 0
  Stopped: 0
 Images: 141
 Server Version: 20.10.6
 Storage Driver: overlay2
  Backing Filesystem: extfs
  Supports d_type: true
  Native Overlay Diff: true
  userxattr: false
 Logging Driver: json-file
 Cgroup Driver: cgroupfs
 Cgroup Version: 1
 Plugins:
  Volume: local
  Network: bridge host ipvlan macvlan null overlay
  Log: awslogs fluentd gcplogs gelf journald json-file local logentries splunk syslog
 Swarm: inactive
 Runtimes: io.containerd.runc.v2 io.containerd.runtime.v1.linux runc
 Default Runtime: runc
 Init Binary: docker-init
 containerd version: 05f951a3781f4f2c1911b05e61c160e9c30eaa8e
 runc version: 12644e614e25b05da6fd08a38ffa0cfe1903fdec
 init version: de40ad0
 Security Options:
  seccomp
   Profile: default
 Kernel Version: 5.10.25-linuxkit
 Operating System: Docker Desktop
 OSType: linux
 Architecture: x86_64
 CPUs: 8
 Total Memory: 11.7GiB
 Name: docker-desktop
 ID: YKMI:4Y6G:M2QK:OC7L:RY5H:JONU:TFAN:NGUM:VAV5:XGOM:IIAM:Q7PU
 Docker Root Dir: /var/lib/docker
 Debug Mode: false
 HTTP Proxy: http.docker.internal:3128
 HTTPS Proxy: http.docker.internal:3128
 Registry: https://index.docker.io/v1/
 Labels:
 Experimental: false
 Insecure Registries:
  127.0.0.0/8
 Live Restore Enabled: false
@ajcasagrande
Copy link

What happens if you set the second parameter to 0 in the WithExposedService call? I have used it that way before to wait for non-exposed services and it works, however never in addition to using NewHTTPStrategy, only wait.ForLog.

WithExposedService("nginx_1", 0, wait.NewHTTPStrategy("/").WithPort("80/tcp").WithStartupTimeout(10*time.Second)).

@mdelapenya
Copy link
Member Author

Yes, for sure it's possible to pass zero, but for the sake of a clean API, I consider not forcing the client code to pass the port if not needed. Besides that, looking up the container by container name is more straight-forward than using name and port. I guess it was firstly implemented in that way because of a specific need.

Besides that, a downstream project was suffering this issue: apache/skywalking-infra-e2e#19 (comment)

@mdelapenya
Copy link
Member Author

@ajcasagrande if you agree, we can close this one. OTOH I'd be happy to discuss about any other consideration/implication you see.

Thanks!

@ajcasagrande
Copy link

@ajcasagrande if you agree, we can close this one. OTOH I'd be happy to discuss about any other consideration/implication you see.

Thanks!

I think my preference would be to add a new WithService(name, waitStrategy) method that does not require the port as a parameter. That way there is no breaking change, and it avoids the confusion of passing 0 to WithExposedService.

@mdelapenya
Copy link
Member Author

I have a local branch doing exactly that 😁

@mdelapenya mdelapenya added the compose Docker Compose. label Sep 29, 2022
@mdelapenya
Copy link
Member Author

Given #476 has been merged, I'm closing this issue. In there, it's possible to do what is requested in this issue:

compose, err := NewDockerCompose("./testresources/docker-compose-simple.yml")
assert.NoError(t, err, "NewDockerCompose()")
err = compose.
  // Appending with _1 as given in the Java Test-Containers Example
  WaitForService("mysql-1", wait.NewLogStrategy("started").WithStartupTimeout(10*time.Second).WithOccurrence(1)).
  Up(ctx, Wait(true))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compose Docker Compose.
Projects
None yet
Development

No branches or pull requests

2 participants