Skip to content

Conversation

taylorsilva
Copy link
Member

@taylorsilva taylorsilva commented Jul 6, 2024

Changes proposed by this PR

Unblocks #8893

We needed to call runtime.WithRequestTimeout() in order to correctly configure the createContainer lock with a non-zero timeout. It's actually amazing that all the other tests in here also weren't failing in a similar manner.

I figured this out by putting fmt.Println statements everywhere and printing out the configured timeout. In the lock, I was seeing that it was configured with a 0s timeout. I eventually realized that we were setting the timeout on the containerd client, but not on the Garden client (which contains the containerd client), and we create the lock with the timeout from the Garden client, not the containerd client. oops!

type GardebBackend struct {
  client containerdClient // <--configured timeout in this client
  ...
  requestTimeout time.Duration // <--didn't configure timeout here using runtime.WithRequestTimeout()
}

We configure the garden client correctly in the actual code, which is why no user has ever had an issue with creating containers like we were seeing in this test suite.

Also took the time to ensure all the tests that made custom backends were setup in the same way.

Notes to reviewer

N/A

Release Note

We needed to call runtime.WithRequestTimeout() in order to correctly
configure the createContainer lock with a non-zero timeout. It's
actually amazing that all the other tests in here also weren't failing
in a similar manner.

I figured this out by putting `fmt.Println` statements everywhere and
printing out the configured timeout. In the lock, I was seeing that it
was configured with a `0s` timeout. I eventually realized that we were
setting the timeout on the containerd client, but not on the Garden
client (which contains the containerd client), and we create the lock
with the timeout from the Garden client, not the containerd client.
oops!

We configure the garden client correctly in the actual code, which is
why no user has ever had an issue with creating containers like we were
seeing in this test suite.

Signed-off-by: Taylor Silva <dev@taydev.net>
@taylorsilva taylorsilva added release/undocumented This didn't warrant being documented or put in release notes. misc labels Jul 6, 2024
@taylorsilva taylorsilva requested a review from a team as a code owner July 6, 2024 20:13
@taylorsilva taylorsilva added this to the v7.12.0 milestone Jul 6, 2024
@taylorsilva taylorsilva merged commit d172d47 into master Jul 7, 2024
@taylorsilva taylorsilva deleted the fix-runtime-integration-tests branch July 7, 2024 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
misc release/undocumented This didn't warrant being documented or put in release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant