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

fix(kubernetes): Make Executor TailContainer before RunContainer #277

Closed
wants to merge 8 commits into from

Conversation

cognifloyd
Copy link
Member

@cognifloyd cognifloyd commented Feb 24, 2022

This should make Kubernetes log capture more robust without changing the behavior for the Docker runtime.

The Kubernetes runtime needs to start asking for logs before the container is created, or logs from short-lived steps might be gone by the time TailContainer starts.

The Docker runtime, however, needs to RunContainer first or the docker engine will complain that the container does not exist. So, we use a special channel to let the Docker Runtime's TailContainer know when RunContainer has finished to allow it to delay pulling logs until the container exists.

The Kubernetes runtime needs to start asking for logs before the container is created,
or logs from short-lived steps might be gone by the time TailContainer starts.

The Docker runtime, however, needs to RunContainer first or the docker engine will
complain that the container does not exist. So, we use a special `runCtx` to let the
Docker Runtime's TailContainer know when RunContainer has finished to allow it
to delay pulling logs until the container exists.
@cognifloyd cognifloyd requested a review from a team as a code owner February 24, 2022 21:25
@codecov
Copy link

codecov bot commented Feb 24, 2022

Codecov Report

Merging #277 (943e015) into master (c1d4563) will decrease coverage by 0.98%.
The diff coverage is 96.05%.

❗ Current head 943e015 differs from pull request most recent head 9877d71. Consider uploading reports for the commit 9877d71 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #277      +/-   ##
==========================================
- Coverage   78.91%   77.92%   -0.99%     
==========================================
  Files          67       67              
  Lines        4870     4907      +37     
==========================================
- Hits         3843     3824      -19     
- Misses        884      943      +59     
+ Partials      143      140       -3     
Impacted Files Coverage Δ
runtime/kubernetes/container.go 56.01% <0.00%> (-21.60%) ⬇️
runtime/docker/container.go 84.03% <50.00%> (-0.85%) ⬇️
executor/linux/secret.go 72.76% <100.00%> (+2.30%) ⬆️
executor/linux/service.go 63.66% <100.00%> (+1.12%) ⬆️
executor/linux/step.go 66.00% <100.00%> (+0.88%) ⬆️
executor/local/service.go 91.34% <100.00%> (+0.52%) ⬆️
executor/local/step.go 87.69% <100.00%> (+0.59%) ⬆️
... and 4 more

@cognifloyd
Copy link
Member Author

This makes use of a new CancelContext to inform TailContainer once RunContainer has finished.

Is there a better signaling primitive to synchronize the stream+TailContainer go routine with when the outer scope finishes RunContainer?

@cognifloyd cognifloyd closed this Feb 24, 2022
@cognifloyd cognifloyd reopened this Feb 24, 2022
@cognifloyd
Copy link
Member Author

Oops. I hit the wrong button and closed it. :(

@cognifloyd cognifloyd changed the title Make Executor TailContainer before RunContainer [kubernetes] Make Executor TailContainer before RunContainer Feb 24, 2022
@cognifloyd
Copy link
Member Author

I switched from a Context to a plain channel. That feels a bit cleaner.

executor/linux/stage.go Outdated Show resolved Hide resolved
The purpose is a bit clearer this way
@cognifloyd cognifloyd marked this pull request as draft March 12, 2022 00:33
@cognifloyd
Copy link
Member Author

I'm not confident this is a good fix. I'm looking at alternatives

@cognifloyd
Copy link
Member Author

Alternate version in #286

@cognifloyd cognifloyd changed the title [kubernetes] Make Executor TailContainer before RunContainer fix(kubernetes): Make Executor TailContainer before RunContainer Mar 15, 2022
@cognifloyd
Copy link
Member Author

Closing in favor of #303

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

4 participants