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

Make docker sandbox compose up wait time configurable #1270

Conversation

tw-aisi
Copy link
Contributor

@tw-aisi tw-aisi commented Feb 7, 2025

This PR contains:

  • New features
  • Changes to dev-tools e.g. CI config / github tooling
  • Docs
  • Bug fixes
  • Code refactor

What is the current behavior? (You can also link to an open issue here)

Currently Inspect will only wait 2 minutes for sandbox containers to start.

What is the new behavior?

Maintains the default 2 minute wait, but allows this to be overridden using the INSPECT_EVAL_COMPOSE_UP_WAIT flag.

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

No.

Other information:

None

@jjallaire
Copy link
Collaborator

We have access to the YAML for the service definition, which in turn can define a timeout, e.g.

services:
  default:
    image: my-image
    healthcheck:
      test: ["CMD", "curl", "-f", "http://localhost:80/"]
      interval: 10s
      timeout: 5s
      retries: 3
      start_period: 30s

In the case of a timeout being specified in the YAML we could:

  1. Not pass the --wait-timeout CLI flag; and
  2. Match our exec timeout to the one in the YAML (we still need this outer timeout so we can do retries of docker compose commands, which are sometimes required due to unreliability / race conditions in the docker daemon)

@andrew-aisi
Copy link
Contributor

@tw-aisi originally created these changes for me because I thought I was seeing cases where inspect wouldn't give my containers a chance to run if they had very long startup times (even with properly configured healthchecks). I just tried creating a minimal example to demonstrate this behavior and couldn't do it - inspect properly waited for the healthcheck and everything seemed to work correctly. I must have done something wrong in my prior tests.

So I don't think this PR is necessary, sorry for pulling you down this path @tw-aisi!

@jjallaire
Copy link
Collaborator

Okay, thanks for the update! I do think there is one piece of work we'll want to do here: we'll still want to read the healthcheck timeout and make sure that the internal timeout used by Inspect exceeds it. Will take care of this ASAP.

@jjallaire-aisi
Copy link
Collaborator

PR for this here: #1305

@jjallaire
Copy link
Collaborator

Superseded by #1305

@jjallaire jjallaire closed this Feb 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants