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: resolve issue where skaffold logger could hang indefinitely if k8s job pod wasn't created #8717

Merged
merged 1 commit into from
Apr 28, 2023

Conversation

aaron-prindle
Copy link
Contributor

fixes #8682

@codecov
Copy link

codecov bot commented Apr 27, 2023

Codecov Report

Merging #8717 (2209e8d) into main (290280e) will decrease coverage by 6.07%.
The diff coverage is 49.59%.

@@            Coverage Diff             @@
##             main    #8717      +/-   ##
==========================================
- Coverage   70.48%   64.42%   -6.07%     
==========================================
  Files         515      617     +102     
  Lines       23150    31177    +8027     
==========================================
+ Hits        16317    20085    +3768     
- Misses       5776     9586    +3810     
- Partials     1057     1506     +449     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/completion.go 13.04% <0.00%> (-1.25%) ⬇️
cmd/skaffold/app/cmd/config/list.go 65.21% <ø> (ø)
cmd/skaffold/app/cmd/config/set.go 88.72% <ø> (ø)
cmd/skaffold/app/cmd/config/util.go 54.28% <ø> (ø)
cmd/skaffold/app/cmd/credits.go 100.00% <ø> (ø)
cmd/skaffold/app/cmd/credits/export.go 0.00% <0.00%> (ø)
cmd/skaffold/app/cmd/deploy.go 40.90% <0.00%> (-12.94%) ⬇️
cmd/skaffold/app/cmd/generate_pipeline.go 60.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_modules.go 65.00% <ø> (ø)
cmd/skaffold/app/cmd/inspect_profiles.go 66.66% <ø> (ø)
... and 40 more

... and 406 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pkg/skaffold/k8sjob/logger/log.go Outdated Show resolved Hide resolved
pkg/skaffold/k8sjob/logger/log.go Show resolved Hide resolved
@renzodavid9
Copy link
Contributor

I was trying the following example:

apiVersion: skaffold/v4beta5
kind: Config
verify:
  - name: alpine-1
    container:
      name: alpine-1
      image: alpine:3.15.4
      command: ["/bin/sh"]
      args: ["-c", "echo $FOO; sleep 10; echo bye"]
    executionMode:
      kubernetesCluster:
        overrides: '{ "metadata": {"namespace": "not-created-ns"}}'

not-created-ns is a namespace that is not created in my minikube cluster. Running skaffold verify with the previous configuration makes the program to never end. Not sure if this is another case not related with the one mentioned in the related issue (#8682) or if I'm missing something 🤔 .

@aaron-prindle
Copy link
Contributor Author

aaron-prindle commented Apr 27, 2023

I just attempted the repro you suggested with the same skaffold.yaml + minikube and the call does timeout after the ~ 1 minute window added in the code here (working as intended). Did you wait ~1 min in your testing? The test that you did there is identical to the test I used for manual testing:

aprindle@aprindle-ssd ~/repro-8682 time skaffold verify
Tags used in verification:
WARN[0032] <nil>                                         subtask=-1 task=DevLoop
WARN[0062] <nil>                                         subtask=-1 task=DevLoop
WARN[0062] verifier cleanup:cleaning up deployed job: jobs.batch "alpine-1" not found  subtask=-1 task=DevLoop
1 error(s) occurred:
* creating verify job in cluster: namespaces "not-created-ns" not found

real	1m3.037s
user	0m0.611s
sys	0m0.467s

@renzodavid9
Copy link
Contributor

I just attempted the repro you suggested with the same skaffold.yaml + minikube and the call does timeout after the ~ 1 minute window added in the code here (working as intended). Did you wait ~1 min in your testing? The test that you did there is identical to the test I used for manual testing:

aprindle@aprindle-ssd ~/repro-8682 time skaffold verify
Tags used in verification:
WARN[0032] <nil>                                         subtask=-1 task=DevLoop
WARN[0062] <nil>                                         subtask=-1 task=DevLoop
WARN[0062] verifier cleanup:cleaning up deployed job: jobs.batch "alpine-1" not found  subtask=-1 task=DevLoop
1 error(s) occurred:
* creating verify job in cluster: namespaces "not-created-ns" not found

real	1m3.037s
user	0m0.611s
sys	0m0.467s

Yes! I just tried again, I was not waiting long enough for the timeout. Thanks for checking!

@aaron-prindle aaron-prindle merged commit fa3298d into GoogleContainerTools:main Apr 28, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants