fix(executor): register credentials before login, pass cancel to health checks, keep failed removals pending, clean by job label (GO-007-R, GO-001-R, GO-002-R, GO-003-R)
- commit
- c7248448576439d8ff3e12489d0309613ede6f3b
- author
- brice · · 2026-10-14 06:43 UTC
- parent
- ff447d8
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
5 files · +254 -42
M internal/executor/containers_cleanup_test.go +142 -9
| @@ -23,11 +23,11 @@ func names(l [][]string) string { | |||
| 23 | 23 | } | |
| 24 | 24 | ||
| 25 | 25 | // GO-001: a docker command that stalls is stopped by its own limit, the job | |
| 26 | - | // does not wait for its timeout, and the cleanup still finds the container by | |
| 27 | - | // the name chosen before the create. | |
| 28 | - | func TestStalledCreateIsBoundedAndCleanedByName(t *testing.T) { | |
| 26 | + | // does not wait for its timeout, and the cleanup still finds the container | |
| 27 | + | // that the daemon did create, by the label of the job. | |
| 28 | + | func TestStalledCreateIsBoundedAndCleanedByLabel(t *testing.T) { | |
| 29 | 29 | f := newFakeCtr(t) | |
| 30 | - | f.set(t, "hang_create", "1") | |
| 30 | + | f.set(t, "hang_create_after", "1") // created in the daemon, the CLI never returns | |
| 31 | 31 | start := time.Now() | |
| 32 | 32 | rec, res := ctrRun(t, f, ctrJob, " - run: echo never\n", fastTimeouts) | |
| 33 | 33 | if res.Conclusion != ConclusionFailure || time.Since(start) > 20*time.Second { | |
| @@ -37,18 +37,39 @@ func TestStalledCreateIsBoundedAndCleanedByName(t *testing.T) { | |||
| 37 | 37 | if !strings.Contains(log, "##[error]The docker command timed out after 400ms: create --name ") { | |
| 38 | 38 | t.Errorf("init:\n%s", log) | |
| 39 | 39 | } | |
| 40 | - | var rm []string | |
| 40 | + | if b, _ := os.ReadFile(filepath.Join(f.dir, "created")); strings.TrimSpace(string(b)) != "" { | |
| 41 | + | t.Errorf("container left in the daemon: %q", b) | |
| 42 | + | } | |
| 43 | + | if got := logOf(rec.step(t, "Stop containers")); !strings.Contains(got, "Remove container network: github_network_") { | |
| 44 | + | t.Errorf("stop:\n%s", got) | |
| 45 | + | } | |
| 46 | + | } | |
| 47 | + | ||
| 48 | + | // GO-003-R: options: may carry another --name (the runner passes it on). The | |
| 49 | + | // container is then not named as recorded; it is found by the label. | |
| 50 | + | func TestOverridingNameAndTimedOutCreateAreCleanedByLabel(t *testing.T) { | |
| 51 | + | f := newFakeCtr(t) | |
| 52 | + | f.set(t, "hang_create_after", "1") | |
| 53 | + | rec, _ := ctrRun(t, f, " container:\n image: ubuntu:24.04\n options: --name my-own-name\n", " - run: echo never\n", fastTimeouts) | |
| 54 | + | var ps, rm []string | |
| 41 | 55 | for _, c := range f.calls(t) { | |
| 56 | + | if c[0] == "ps" && strings.Contains(strings.Join(c, " "), "--quiet") { | |
| 57 | + | ps = c | |
| 58 | + | } | |
| 42 | 59 | if c[0] == "rm" { | |
| 43 | 60 | rm = c | |
| 44 | 61 | } | |
| 45 | 62 | } | |
| 46 | - | if len(rm) < 3 || !strings.Contains(rm[2], "_ubuntu2404_") || strings.Contains(rm[2], strings.Repeat("0", 20)) { | |
| 47 | - | t.Errorf("rm by name expected: %v", rm) | |
| 63 | + | if ps == nil || !strings.Contains(strings.Join(ps, "\n"), "label=") { | |
| 64 | + | t.Errorf("no listing by label: %v", ps) | |
| 48 | 65 | } | |
| 49 | - | if got := logOf(rec.step(t, "Stop containers")); !strings.Contains(got, "Remove container network: github_network_") { | |
| 50 | - | t.Errorf("stop:\n%s", got) | |
| 66 | + | if len(rm) < 3 || !containerIDRe.MatchString(rm[2]) { | |
| 67 | + | t.Errorf("rm by id expected: %v", rm) | |
| 68 | + | } | |
| 69 | + | if b, _ := os.ReadFile(filepath.Join(f.dir, "created")); strings.TrimSpace(string(b)) != "" { | |
| 70 | + | t.Errorf("container left: %q", b) | |
| 51 | 71 | } | |
| 72 | + | _ = rec | |
| 52 | 73 | } | |
| 53 | 74 | ||
| 54 | 75 | // GO-001: the cancellation of the job reaches every docker call of the init. | |
| @@ -201,3 +222,115 @@ func TestRegistryConfigRemovedAfterFailedPullAndRemovalFailureIsReported(t *test | |||
| 201 | 222 | t.Errorf("init:\n%s", got) | |
| 202 | 223 | } | |
| 203 | 224 | } | |
| 225 | + | ||
| 226 | + | // GO-001-R: the health inspection takes the cancellation too. | |
| 227 | + | func TestCancelDuringHealthInspection(t *testing.T) { | |
| 228 | + | f := newFakeCtr(t) | |
| 229 | + | f.set(t, "hang_inspect", "1") | |
| 230 | + | cancel := NewCanceller() | |
| 231 | + | go func() { time.Sleep(500 * time.Millisecond); cancel.Cancel(CancelUser) }() | |
| 232 | + | start := time.Now() | |
| 233 | + | rec, res := ctrRunC(t, f, cancel, " services:\n s:\n image: redis:7\n", " - run: echo never\n", func(c *Config) { | |
| 234 | + | c.Actions.Containers.Command = time.Minute | |
| 235 | + | }) | |
| 236 | + | if res.Conclusion != ConclusionCancelled || time.Since(start) > 15*time.Second { | |
| 237 | + | t.Fatalf("%+v after %s %v", res, time.Since(start), rec.names()) | |
| 238 | + | } | |
| 239 | + | var health bool | |
| 240 | + | for _, c := range f.calls(t) { | |
| 241 | + | if c[0] == "inspect" && strings.Contains(strings.Join(c, " "), "Healthcheck") { | |
| 242 | + | health = true | |
| 243 | + | } | |
| 244 | + | } | |
| 245 | + | if !health { | |
| 246 | + | t.Errorf("the health inspection was not reached: %s", names(f.calls(t))) | |
| 247 | + | } | |
| 248 | + | } | |
| 249 | + | ||
| 250 | + | // GO-002-R: a failed removal is not a removal. It stays pending and the job's | |
| 251 | + | // fallback tries again; "No such container" is a removal. | |
| 252 | + | func TestFailedRemovalStaysPendingAndAbsenceCounts(t *testing.T) { | |
| 253 | + | count := func(f *fakeCtr, verb string) int { | |
| 254 | + | n := 0 | |
| 255 | + | for _, c := range f.calls(t) { | |
| 256 | + | if c[0] == verb || (verb == "network rm" && c[0] == "network" && len(c) > 1 && c[1] == "rm") { | |
| 257 | + | n++ | |
| 258 | + | } | |
| 259 | + | } | |
| 260 | + | return n | |
| 261 | + | } | |
| 262 | + | f := newFakeCtr(t) | |
| 263 | + | f.set(t, "rm_fail", "1") | |
| 264 | + | f.set(t, "netrm_fail", "1") | |
| 265 | + | rec, _ := ctrRun(t, f, ctrJob, " - run: echo ok\n", fastTimeouts) | |
| 266 | + | if count(f, "rm") != 2 || count(f, "network rm") != 2 { | |
| 267 | + | t.Errorf("rm=%d network rm=%d, want the fallback to retry both once", count(f, "rm"), count(f, "network rm")) | |
| 268 | + | } | |
| 269 | + | if got := logOf(rec.step(t, "Stop containers")); !strings.Contains(got, "Docker rm fail with exit code 1") || !strings.Contains(got, "Docker network rm fail with exit code 1") { | |
| 270 | + | t.Errorf("warnings:\n%s", got) | |
| 271 | + | } | |
| 272 | + | ||
| 273 | + | f2 := newFakeCtr(t) | |
| 274 | + | f2.set(t, "rm_absent", "1") | |
| 275 | + | f2.set(t, "netrm_absent", "1") | |
| 276 | + | rec2, res2 := ctrRun(t, f2, ctrJob, " - run: echo ok\n", fastTimeouts) | |
| 277 | + | if res2.Conclusion != ConclusionSuccess || count(f2, "rm") != 1 || count(f2, "network rm") != 1 { | |
| 278 | + | t.Errorf("absent: %+v rm=%d network rm=%d\n%s", res2, count(f2, "rm"), count(f2, "network rm"), logOf(rec2.step(t, "Stop containers"))) | |
| 279 | + | } | |
| 280 | + | } | |
| 281 | + | ||
| 282 | + | // GO-007-R: the credentials directory is registered before the login; a panic | |
| 283 | + | // in the login or in the pull leaves nothing behind. | |
| 284 | + | func TestPanicDuringLoginOrPullLeavesNoCredentials(t *testing.T) { | |
| 285 | + | for _, during := range []string{" login ", " pull "} { | |
| 286 | + | f := newFakeCtr(t) | |
| 287 | + | var temp string | |
| 288 | + | func() { | |
| 289 | + | defer func() { _ = recover() }() | |
| 290 | + | ctrRun(t, f, " container:\n image: ghcr.io/o/i:1\n credentials:\n username: u\n password: pw-secret\n", " - run: echo\n", func(c *Config) { | |
| 291 | + | temp = c.Layout.Temp | |
| 292 | + | c.Reporter = &panicOnLog{Reporter: c.Reporter, contains: during} | |
| 293 | + | }) | |
| 294 | + | }() | |
| 295 | + | if m, _ := filepath.Glob(filepath.Join(temp, ".docker_*")); len(m) != 0 { | |
| 296 | + | t.Errorf("panic during%s: credentials left: %v", during, m) | |
| 297 | + | } | |
| 298 | + | } | |
| 299 | + | } | |
| 300 | + | ||
| 301 | + | type panicOnLog struct { | |
| 302 | + | Reporter | |
| 303 | + | contains string | |
| 304 | + | } | |
| 305 | + | ||
| 306 | + | func (p *panicOnLog) Log(n int, at time.Time, line string) { | |
| 307 | + | if strings.HasPrefix(line, "##[command]") && strings.Contains(line, p.contains) { | |
| 308 | + | panic("boom") | |
| 309 | + | } | |
| 310 | + | p.Reporter.Log(n, at, line) | |
| 311 | + | } | |
| 312 | + | ||
| 313 | + | // GO-004-R3: a variable name that holds "=" or NUL, or is empty, is refused | |
| 314 | + | // for create and exec, before docker is called. | |
| 315 | + | func TestInvalidEnvNamesAreRefused(t *testing.T) { | |
| 316 | + | if checkEnvNames([]string{"A", "b_c", "A.B", "A-B"}) != nil { | |
| 317 | + | t.Error("valid names refused") | |
| 318 | + | } | |
| 319 | + | for _, k := range []string{"", "A=B", "HTTP_PROXY=http://x/?y", "A\x00B", "="} { | |
| 320 | + | if err := checkEnvNames([]string{"OK", k}); err == nil || !strings.Contains(err.Error(), "Invalid environment variable name") { | |
| 321 | + | t.Errorf("%q: %v", k, err) | |
| 322 | + | } | |
| 323 | + | } | |
| 324 | + | f := newFakeCtr(t) | |
| 325 | + | rec, res := ctrRun(t, f, " container:\n image: ubuntu:24.04\n env:\n \"HTTP_PROXY=http://x/?y\": v\n", " - run: echo never\n", nil) | |
| 326 | + | if res.Conclusion != ConclusionFailure || !strings.Contains(logOf(rec.step(t, "Set up job")), "##[error]Invalid environment variable name 'HTTP_PROXY=http://x/?y'") { | |
| 327 | + | t.Errorf("container env: %+v", res) | |
| 328 | + | } | |
| 329 | + | rec, res = ctrRun(t, f, ctrJob, " - name: s\n env:\n \"HTTP_PROXY=http://x/?y\": v\n run: echo never\n", nil) | |
| 330 | + | if res.Conclusion != ConclusionFailure || !strings.Contains(logOf(rec.step(t, "s")), "##[error]Invalid environment variable name 'HTTP_PROXY=http://x/?y'") { | |
| 331 | + | t.Errorf("step env: %+v\n%s", res, logOf(rec.step(t, "s"))) | |
| 332 | + | } | |
| 333 | + | if n := len(f.execCalls(t)); n != 0 { | |
| 334 | + | t.Errorf("docker exec called %d times", n) | |
| 335 | + | } | |
| 336 | + | } | |
| @@ -23,11 +23,11 @@ func names(l [][]string) string { | |||||
| 23 | } | 23 | } | ||
| 24 | 24 | ||||
| 25 | // GO-001: a docker command that stalls is stopped by its own limit, the job | 25 | // GO-001: a docker command that stalls is stopped by its own limit, the job | ||
| 26 | - | // does not wait for its timeout, and the cleanup still finds the container by | 26 | + | // does not wait for its timeout, and the cleanup still finds the container |
| 27 | - | // the name chosen before the create. | 27 | + | // that the daemon did create, by the label of the job. |
| 28 | - | func TestStalledCreateIsBoundedAndCleanedByName(t *testing.T) { | 28 | + | func TestStalledCreateIsBoundedAndCleanedByLabel(t *testing.T) { |
| 29 | f := newFakeCtr(t) | 29 | f := newFakeCtr(t) | ||
| 30 | - | f.set(t, "hang_create", "1") | 30 | + | f.set(t, "hang_create_after", "1") // created in the daemon, the CLI never returns |
| 31 | start := time.Now() | 31 | start := time.Now() | ||
| 32 | rec, res := ctrRun(t, f, ctrJob, " - run: echo never\n", fastTimeouts) | 32 | rec, res := ctrRun(t, f, ctrJob, " - run: echo never\n", fastTimeouts) | ||
| 33 | if res.Conclusion != ConclusionFailure || time.Since(start) > 20*time.Second { | 33 | if res.Conclusion != ConclusionFailure || time.Since(start) > 20*time.Second { | ||
| @@ -37,18 +37,39 @@ func TestStalledCreateIsBoundedAndCleanedByName(t *testing.T) { | |||||
| 37 | if !strings.Contains(log, "##[error]The docker command timed out after 400ms: create --name ") { | 37 | if !strings.Contains(log, "##[error]The docker command timed out after 400ms: create --name ") { | ||
| 38 | t.Errorf("init:\n%s", log) | 38 | t.Errorf("init:\n%s", log) | ||
| 39 | } | 39 | } | ||
| 40 | - | var rm []string | 40 | + | if b, _ := os.ReadFile(filepath.Join(f.dir, "created")); strings.TrimSpace(string(b)) != "" { |
| 41 | + | t.Errorf("container left in the daemon: %q", b) | |||
| 42 | + | } | |||
| 43 | + | if got := logOf(rec.step(t, "Stop containers")); !strings.Contains(got, "Remove container network: github_network_") { | |||
| 44 | + | t.Errorf("stop:\n%s", got) | |||
| 45 | + | } | |||
| 46 | + | } | |||
| 47 | + | ||||
| 48 | + | // GO-003-R: options: may carry another --name (the runner passes it on). The | |||
| 49 | + | // container is then not named as recorded; it is found by the label. | |||
| 50 | + | func TestOverridingNameAndTimedOutCreateAreCleanedByLabel(t *testing.T) { | |||
| 51 | + | f := newFakeCtr(t) | |||
| 52 | + | f.set(t, "hang_create_after", "1") | |||
| 53 | + | rec, _ := ctrRun(t, f, " container:\n image: ubuntu:24.04\n options: --name my-own-name\n", " - run: echo never\n", fastTimeouts) | |||
| 54 | + | var ps, rm []string | |||
| 41 | for _, c := range f.calls(t) { | 55 | for _, c := range f.calls(t) { | ||
| 56 | + | if c[0] == "ps" && strings.Contains(strings.Join(c, " "), "--quiet") { | |||
| 57 | + | ps = c | |||
| 58 | + | } | |||
| 42 | if c[0] == "rm" { | 59 | if c[0] == "rm" { | ||
| 43 | rm = c | 60 | rm = c | ||
| 44 | } | 61 | } | ||
| 45 | } | 62 | } | ||
| 46 | - | if len(rm) < 3 || !strings.Contains(rm[2], "_ubuntu2404_") || strings.Contains(rm[2], strings.Repeat("0", 20)) { | 63 | + | if ps == nil || !strings.Contains(strings.Join(ps, "\n"), "label=") { |
| 47 | - | t.Errorf("rm by name expected: %v", rm) | 64 | + | t.Errorf("no listing by label: %v", ps) |
| 48 | } | 65 | } | ||
| 49 | - | if got := logOf(rec.step(t, "Stop containers")); !strings.Contains(got, "Remove container network: github_network_") { | 66 | + | if len(rm) < 3 || !containerIDRe.MatchString(rm[2]) { |
| 50 | - | t.Errorf("stop:\n%s", got) | 67 | + | t.Errorf("rm by id expected: %v", rm) |
| 68 | + | } | |||
| 69 | + | if b, _ := os.ReadFile(filepath.Join(f.dir, "created")); strings.TrimSpace(string(b)) != "" { | |||
| 70 | + | t.Errorf("container left: %q", b) | |||
| 51 | } | 71 | } | ||
| 72 | + | _ = rec | |||
| 52 | } | 73 | } | ||
| 53 | 74 | ||||
| 54 | // GO-001: the cancellation of the job reaches every docker call of the init. | 75 | // GO-001: the cancellation of the job reaches every docker call of the init. | ||
| @@ -201,3 +222,115 @@ func TestRegistryConfigRemovedAfterFailedPullAndRemovalFailureIsReported(t *test | |||||
| 201 | t.Errorf("init:\n%s", got) | 222 | t.Errorf("init:\n%s", got) | ||
| 202 | } | 223 | } | ||
| 203 | } | 224 | } | ||
| 225 | + | ||||
| 226 | + | // GO-001-R: the health inspection takes the cancellation too. | |||
| 227 | + | func TestCancelDuringHealthInspection(t *testing.T) { | |||
| 228 | + | f := newFakeCtr(t) | |||
| 229 | + | f.set(t, "hang_inspect", "1") | |||
| 230 | + | cancel := NewCanceller() | |||
| 231 | + | go func() { time.Sleep(500 * time.Millisecond); cancel.Cancel(CancelUser) }() | |||
| 232 | + | start := time.Now() | |||
| 233 | + | rec, res := ctrRunC(t, f, cancel, " services:\n s:\n image: redis:7\n", " - run: echo never\n", func(c *Config) { | |||
| 234 | + | c.Actions.Containers.Command = time.Minute | |||
| 235 | + | }) | |||
| 236 | + | if res.Conclusion != ConclusionCancelled || time.Since(start) > 15*time.Second { | |||
| 237 | + | t.Fatalf("%+v after %s %v", res, time.Since(start), rec.names()) | |||
| 238 | + | } | |||
| 239 | + | var health bool | |||
| 240 | + | for _, c := range f.calls(t) { | |||
| 241 | + | if c[0] == "inspect" && strings.Contains(strings.Join(c, " "), "Healthcheck") { | |||
| 242 | + | health = true | |||
| 243 | + | } | |||
| 244 | + | } | |||
| 245 | + | if !health { | |||
| 246 | + | t.Errorf("the health inspection was not reached: %s", names(f.calls(t))) | |||
| 247 | + | } | |||
| 248 | + | } | |||
| 249 | + | ||||
| 250 | + | // GO-002-R: a failed removal is not a removal. It stays pending and the job's | |||
| 251 | + | // fallback tries again; "No such container" is a removal. | |||
| 252 | + | func TestFailedRemovalStaysPendingAndAbsenceCounts(t *testing.T) { | |||
| 253 | + | count := func(f *fakeCtr, verb string) int { | |||
| 254 | + | n := 0 | |||
| 255 | + | for _, c := range f.calls(t) { | |||
| 256 | + | if c[0] == verb || (verb == "network rm" && c[0] == "network" && len(c) > 1 && c[1] == "rm") { | |||
| 257 | + | n++ | |||
| 258 | + | } | |||
| 259 | + | } | |||
| 260 | + | return n | |||
| 261 | + | } | |||
| 262 | + | f := newFakeCtr(t) | |||
| 263 | + | f.set(t, "rm_fail", "1") | |||
| 264 | + | f.set(t, "netrm_fail", "1") | |||
| 265 | + | rec, _ := ctrRun(t, f, ctrJob, " - run: echo ok\n", fastTimeouts) | |||
| 266 | + | if count(f, "rm") != 2 || count(f, "network rm") != 2 { | |||
| 267 | + | t.Errorf("rm=%d network rm=%d, want the fallback to retry both once", count(f, "rm"), count(f, "network rm")) | |||
| 268 | + | } | |||
| 269 | + | if got := logOf(rec.step(t, "Stop containers")); !strings.Contains(got, "Docker rm fail with exit code 1") || !strings.Contains(got, "Docker network rm fail with exit code 1") { | |||
| 270 | + | t.Errorf("warnings:\n%s", got) | |||
| 271 | + | } | |||
| 272 | + | ||||
| 273 | + | f2 := newFakeCtr(t) | |||
| 274 | + | f2.set(t, "rm_absent", "1") | |||
| 275 | + | f2.set(t, "netrm_absent", "1") | |||
| 276 | + | rec2, res2 := ctrRun(t, f2, ctrJob, " - run: echo ok\n", fastTimeouts) | |||
| 277 | + | if res2.Conclusion != ConclusionSuccess || count(f2, "rm") != 1 || count(f2, "network rm") != 1 { | |||
| 278 | + | t.Errorf("absent: %+v rm=%d network rm=%d\n%s", res2, count(f2, "rm"), count(f2, "network rm"), logOf(rec2.step(t, "Stop containers"))) | |||
| 279 | + | } | |||
| 280 | + | } | |||
| 281 | + | ||||
| 282 | + | // GO-007-R: the credentials directory is registered before the login; a panic | |||
| 283 | + | // in the login or in the pull leaves nothing behind. | |||
| 284 | + | func TestPanicDuringLoginOrPullLeavesNoCredentials(t *testing.T) { | |||
| 285 | + | for _, during := range []string{" login ", " pull "} { | |||
| 286 | + | f := newFakeCtr(t) | |||
| 287 | + | var temp string | |||
| 288 | + | func() { | |||
| 289 | + | defer func() { _ = recover() }() | |||
| 290 | + | ctrRun(t, f, " container:\n image: ghcr.io/o/i:1\n credentials:\n username: u\n password: pw-secret\n", " - run: echo\n", func(c *Config) { | |||
| 291 | + | temp = c.Layout.Temp | |||
| 292 | + | c.Reporter = &panicOnLog{Reporter: c.Reporter, contains: during} | |||
| 293 | + | }) | |||
| 294 | + | }() | |||
| 295 | + | if m, _ := filepath.Glob(filepath.Join(temp, ".docker_*")); len(m) != 0 { | |||
| 296 | + | t.Errorf("panic during%s: credentials left: %v", during, m) | |||
| 297 | + | } | |||
| 298 | + | } | |||
| 299 | + | } | |||
| 300 | + | ||||
| 301 | + | type panicOnLog struct { | |||
| 302 | + | Reporter | |||
| 303 | + | contains string | |||
| 304 | + | } | |||
| 305 | + | ||||
| 306 | + | func (p *panicOnLog) Log(n int, at time.Time, line string) { | |||
| 307 | + | if strings.HasPrefix(line, "##[command]") && strings.Contains(line, p.contains) { | |||
| 308 | + | panic("boom") | |||
| 309 | + | } | |||
| 310 | + | p.Reporter.Log(n, at, line) | |||
| 311 | + | } | |||
| 312 | + | ||||
| 313 | + | // GO-004-R3: a variable name that holds "=" or NUL, or is empty, is refused | |||
| 314 | + | // for create and exec, before docker is called. | |||
| 315 | + | func TestInvalidEnvNamesAreRefused(t *testing.T) { | |||
| 316 | + | if checkEnvNames([]string{"A", "b_c", "A.B", "A-B"}) != nil { | |||
| 317 | + | t.Error("valid names refused") | |||
| 318 | + | } | |||
| 319 | + | for _, k := range []string{"", "A=B", "HTTP_PROXY=http://x/?y", "A\x00B", "="} { | |||
| 320 | + | if err := checkEnvNames([]string{"OK", k}); err == nil || !strings.Contains(err.Error(), "Invalid environment variable name") { | |||
| 321 | + | t.Errorf("%q: %v", k, err) | |||
| 322 | + | } | |||
| 323 | + | } | |||
| 324 | + | f := newFakeCtr(t) | |||
| 325 | + | rec, res := ctrRun(t, f, " container:\n image: ubuntu:24.04\n env:\n \"HTTP_PROXY=http://x/?y\": v\n", " - run: echo never\n", nil) | |||
| 326 | + | if res.Conclusion != ConclusionFailure || !strings.Contains(logOf(rec.step(t, "Set up job")), "##[error]Invalid environment variable name 'HTTP_PROXY=http://x/?y'") { | |||
| 327 | + | t.Errorf("container env: %+v", res) | |||
| 328 | + | } | |||
| 329 | + | rec, res = ctrRun(t, f, ctrJob, " - name: s\n env:\n \"HTTP_PROXY=http://x/?y\": v\n run: echo never\n", nil) | |||
| 330 | + | if res.Conclusion != ConclusionFailure || !strings.Contains(logOf(rec.step(t, "s")), "##[error]Invalid environment variable name 'HTTP_PROXY=http://x/?y'") { | |||
| 331 | + | t.Errorf("step env: %+v\n%s", res, logOf(rec.step(t, "s"))) | |||
| 332 | + | } | |||
| 333 | + | if n := len(f.execCalls(t)); n != 0 { | |||
| 334 | + | t.Errorf("docker exec called %d times", n) | |||
| 335 | + | } | |||
| 336 | + | } | |||
M internal/executor/containers_fake_test.go +12 -4
| @@ -35,14 +35,19 @@ hang() { trap 'exit 130' INT TERM; while :; do sleep 0.05; done; } | |||
| 35 | 35 | if [ -f $D/hang_for ]; then case "$*" in *"$(cat $D/hang_for)"*) hang ;; esac; fi | |
| 36 | 36 | case "$1" in | |
| 37 | 37 | version) echo "'1.48'" ;; | |
| 38 | - | ps) case "$*" in *--quiet*) ;; *status=running*) id=$(echo "$*" | sed 's/.*id=\([^ ]*\).*/\1/'); echo "$id Up Less than a second" ;; esac ;; | |
| 39 | - | network) ;; | |
| 38 | + | ps) case "$*" in *--quiet*) [ -f $D/created ] && cat $D/created ;; *status=running*) id=$(echo "$*" | sed 's/.*id=\([^ ]*\).*/\1/'); echo "$id Up Less than a second" ;; esac ;; | |
| 39 | + | network) if [ "$2" = rm ]; then | |
| 40 | + | [ -f $D/netrm_fail ] && { echo "Error response from daemon: network is busy"; exit 1; } | |
| 41 | + | [ -f $D/netrm_absent ] && { echo "Error response from daemon: network $3 not found"; exit 1; } | |
| 42 | + | fi ;; | |
| 40 | 43 | login) cat > $D/login_stdin | |
| 41 | 44 | if [ -f $D/lock_cfg ]; then mkdir -p $CFG/sub; : > $CFG/sub/f; chmod 500 $CFG/sub; fi ;; | |
| 42 | 45 | pull) if [ -f $D/pull_fail ]; then n=$(cat $D/pull_fail); if [ "$n" -gt 0 ]; then echo $((n-1)) > $D/pull_fail; echo "pull denied"; exit 1; fi; fi; echo "pulled $2" ;; | |
| 43 | 46 | create) [ -f $D/create_fail ] && { echo "unknown flag"; exit 125; } | |
| 44 | 47 | [ -f $D/create_out ] && { cat $D/create_out; exit 0; } | |
| 45 | - | n=$(cat $D/seq 2>/dev/null || echo 0); n=$((n+1)); echo $n > $D/seq; printf "%064d\n" $n ;; | |
| 48 | + | n=$(cat $D/seq 2>/dev/null || echo 0); n=$((n+1)); echo $n > $D/seq; id=$(printf "%064d" $n); echo $id >> $D/created | |
| 49 | + | [ -f $D/hang_create_after ] && hang | |
| 50 | + | echo $id ;; | |
| 46 | 51 | start) echo "$2" ;; | |
| 47 | 52 | inspect) case "$*" in | |
| 48 | 53 | *Healthcheck*) if [ -s $D/health ]; then w=$(head -1 $D/health); sed -i 1d $D/health; echo "$w"; fi ;; | |
| @@ -50,7 +55,10 @@ inspect) case "$*" in | |||
| 50 | 55 | esac ;; | |
| 51 | 56 | port) [ -f $D/ports ] && cat $D/ports ;; | |
| 52 | 57 | logs) echo "service log of $3" ;; | |
| 53 | - | rm) echo "$3" ;; | |
| 58 | + | rm) [ -f $D/rm_fail ] && { echo "Error response from daemon: cannot remove container: boom"; exit 1; } | |
| 59 | + | [ -f $D/rm_absent ] && { echo "Error response from daemon: No such container: $3"; exit 1; } | |
| 60 | + | [ -f $D/created ] && { grep -v "^$3$" $D/created > $D/created.n; mv $D/created.n $D/created; } | |
| 61 | + | echo "$3" ;; | |
| 54 | 62 | exec) if [ -f $D/add_path ]; then f=$(echo "$GITHUB_PATH" | sed "s#^/__w#$(cat $D/add_path)#"); echo /added >> "$f"; fi | |
| 55 | 63 | [ -f $D/exec_env ] && echo "ENV GITHUB_WORKSPACE=$GITHUB_WORKSPACE RUNNER_TEMP=$RUNNER_TEMP HOME=${HOME:-<unset>} GITHUB_OUTPUT=$GITHUB_OUTPUT GITHUB_EVENT_PATH=$GITHUB_EVENT_PATH" | |
| 56 | 64 | [ -f $D/hang_exec ] && { trap 'exit 130' INT TERM; while :; do sleep 0.05; done; } | |
| @@ -35,14 +35,19 @@ hang() { trap 'exit 130' INT TERM; while :; do sleep 0.05; done; } | |||||
| 35 | if [ -f $D/hang_for ]; then case "$*" in *"$(cat $D/hang_for)"*) hang ;; esac; fi | 35 | if [ -f $D/hang_for ]; then case "$*" in *"$(cat $D/hang_for)"*) hang ;; esac; fi | ||
| 36 | case "$1" in | 36 | case "$1" in | ||
| 37 | version) echo "'1.48'" ;; | 37 | version) echo "'1.48'" ;; | ||
| 38 | - | ps) case "$*" in *--quiet*) ;; *status=running*) id=$(echo "$*" | sed 's/.*id=\([^ ]*\).*/\1/'); echo "$id Up Less than a second" ;; esac ;; | 38 | + | ps) case "$*" in *--quiet*) [ -f $D/created ] && cat $D/created ;; *status=running*) id=$(echo "$*" | sed 's/.*id=\([^ ]*\).*/\1/'); echo "$id Up Less than a second" ;; esac ;; |
| 39 | - | network) ;; | 39 | + | network) if [ "$2" = rm ]; then |
| 40 | + | [ -f $D/netrm_fail ] && { echo "Error response from daemon: network is busy"; exit 1; } | |||
| 41 | + | [ -f $D/netrm_absent ] && { echo "Error response from daemon: network $3 not found"; exit 1; } | |||
| 42 | + | fi ;; | |||
| 40 | login) cat > $D/login_stdin | 43 | login) cat > $D/login_stdin | ||
| 41 | if [ -f $D/lock_cfg ]; then mkdir -p $CFG/sub; : > $CFG/sub/f; chmod 500 $CFG/sub; fi ;; | 44 | if [ -f $D/lock_cfg ]; then mkdir -p $CFG/sub; : > $CFG/sub/f; chmod 500 $CFG/sub; fi ;; | ||
| 42 | pull) if [ -f $D/pull_fail ]; then n=$(cat $D/pull_fail); if [ "$n" -gt 0 ]; then echo $((n-1)) > $D/pull_fail; echo "pull denied"; exit 1; fi; fi; echo "pulled $2" ;; | 45 | pull) if [ -f $D/pull_fail ]; then n=$(cat $D/pull_fail); if [ "$n" -gt 0 ]; then echo $((n-1)) > $D/pull_fail; echo "pull denied"; exit 1; fi; fi; echo "pulled $2" ;; | ||
| 43 | create) [ -f $D/create_fail ] && { echo "unknown flag"; exit 125; } | 46 | create) [ -f $D/create_fail ] && { echo "unknown flag"; exit 125; } | ||
| 44 | [ -f $D/create_out ] && { cat $D/create_out; exit 0; } | 47 | [ -f $D/create_out ] && { cat $D/create_out; exit 0; } | ||
| 45 | - | n=$(cat $D/seq 2>/dev/null || echo 0); n=$((n+1)); echo $n > $D/seq; printf "%064d\n" $n ;; | 48 | + | n=$(cat $D/seq 2>/dev/null || echo 0); n=$((n+1)); echo $n > $D/seq; id=$(printf "%064d" $n); echo $id >> $D/created |
| 49 | + | [ -f $D/hang_create_after ] && hang | |||
| 50 | + | echo $id ;; | |||
| 46 | start) echo "$2" ;; | 51 | start) echo "$2" ;; | ||
| 47 | inspect) case "$*" in | 52 | inspect) case "$*" in | ||
| 48 | *Healthcheck*) if [ -s $D/health ]; then w=$(head -1 $D/health); sed -i 1d $D/health; echo "$w"; fi ;; | 53 | *Healthcheck*) if [ -s $D/health ]; then w=$(head -1 $D/health); sed -i 1d $D/health; echo "$w"; fi ;; | ||
| @@ -50,7 +55,10 @@ inspect) case "$*" in | |||||
| 50 | esac ;; | 55 | esac ;; | ||
| 51 | port) [ -f $D/ports ] && cat $D/ports ;; | 56 | port) [ -f $D/ports ] && cat $D/ports ;; | ||
| 52 | logs) echo "service log of $3" ;; | 57 | logs) echo "service log of $3" ;; | ||
| 53 | - | rm) echo "$3" ;; | 58 | + | rm) [ -f $D/rm_fail ] && { echo "Error response from daemon: cannot remove container: boom"; exit 1; } |
| 59 | + | [ -f $D/rm_absent ] && { echo "Error response from daemon: No such container: $3"; exit 1; } | |||
| 60 | + | [ -f $D/created ] && { grep -v "^$3$" $D/created > $D/created.n; mv $D/created.n $D/created; } | |||
| 61 | + | echo "$3" ;; | |||
| 54 | exec) if [ -f $D/add_path ]; then f=$(echo "$GITHUB_PATH" | sed "s#^/__w#$(cat $D/add_path)#"); echo /added >> "$f"; fi | 62 | exec) if [ -f $D/add_path ]; then f=$(echo "$GITHUB_PATH" | sed "s#^/__w#$(cat $D/add_path)#"); echo /added >> "$f"; fi | ||
| 55 | [ -f $D/exec_env ] && echo "ENV GITHUB_WORKSPACE=$GITHUB_WORKSPACE RUNNER_TEMP=$RUNNER_TEMP HOME=${HOME:-<unset>} GITHUB_OUTPUT=$GITHUB_OUTPUT GITHUB_EVENT_PATH=$GITHUB_EVENT_PATH" | 63 | [ -f $D/exec_env ] && echo "ENV GITHUB_WORKSPACE=$GITHUB_WORKSPACE RUNNER_TEMP=$RUNNER_TEMP HOME=${HOME:-<unset>} GITHUB_OUTPUT=$GITHUB_OUTPUT GITHUB_EVENT_PATH=$GITHUB_EVENT_PATH" | ||
| 56 | [ -f $D/hang_exec ] && { trap 'exit 130' INT TERM; while :; do sleep 0.05; done; } | 64 | [ -f $D/hang_exec ] && { trap 'exit 130' INT TERM; while :; do sleep 0.05; done; } | ||
M internal/executor/containers_init.go +19 -1
| @@ -8,6 +8,7 @@ import ( | |||
| 8 | 8 | "os" | |
| 9 | 9 | "path/filepath" | |
| 10 | 10 | "regexp" | |
| 11 | + | "slices" | |
| 11 | 12 | "strconv" | |
| 12 | 13 | "strings" | |
| 13 | 14 | "sync" | |
| @@ -35,7 +36,20 @@ type containerSet struct { | |||
| 35 | 36 | ||
| 36 | 37 | func (cs *containerSet) tryNetwork(name string) { cs.mu.Lock(); cs.networkTry = name; cs.mu.Unlock() } | |
| 37 | 38 | ||
| 38 | - | func (cs *containerSet) addDir(d string) { cs.mu.Lock(); cs.dirs = append(cs.dirs, d); cs.mu.Unlock() } | |
| 39 | + | func (cs *containerSet) addDir(d string) { | |
| 40 | + | cs.mu.Lock() | |
| 41 | + | defer cs.mu.Unlock() | |
| 42 | + | if !slices.Contains(cs.dirs, d) { | |
| 43 | + | cs.dirs = append(cs.dirs, d) | |
| 44 | + | } | |
| 45 | + | } | |
| 46 | + | ||
| 47 | + | // delDir forgets a directory that is gone. | |
| 48 | + | func (cs *containerSet) delDir(d string) { | |
| 49 | + | cs.mu.Lock() | |
| 50 | + | cs.dirs = slices.DeleteFunc(cs.dirs, func(x string) bool { return x == d }) | |
| 51 | + | cs.mu.Unlock() | |
| 52 | + | } | |
| 39 | 53 | ||
| 40 | 54 | func (cs *containerSet) all() []*container { | |
| 41 | 55 | var l []*container | |
| @@ -227,6 +241,7 @@ func (j *job) removeConfigDir(dir string) error { | |||
| 227 | 241 | j.cs.addDir(dir) | |
| 228 | 242 | return ghErrorf("Failed to remove directory containing Docker client credentials: %v", err) | |
| 229 | 243 | } | |
| 244 | + | j.cs.delDir(dir) // unregistered only once it is gone | |
| 230 | 245 | return nil | |
| 231 | 246 | } | |
| 232 | 247 | ||
| @@ -244,6 +259,9 @@ func (j *job) registryLogin(s *stepRun, c *container, cancel <-chan struct{}) (s | |||
| 244 | 259 | if err := os.MkdirAll(dir, 0o700); err != nil { | |
| 245 | 260 | return "", ghErrorf("Failed to create directory to store registry client credentials: %v", err) | |
| 246 | 261 | } | |
| 262 | + | // Registered with the cleanup of the job before anything can fail or panic: | |
| 263 | + | // from here on the fallback removes it whatever happens. | |
| 264 | + | j.cs.addDir(dir) | |
| 247 | 265 | j.own(dir) | |
| 248 | 266 | args := []string{j.dockerPath(), "--config", dir, "login"} | |
| 249 | 267 | line := "--config " + dir + " login" | |
| @@ -8,6 +8,7 @@ import ( | |||||
| 8 | "os" | 8 | "os" | ||
| 9 | "path/filepath" | 9 | "path/filepath" | ||
| 10 | "regexp" | 10 | "regexp" | ||
| 11 | + | "slices" | |||
| 11 | "strconv" | 12 | "strconv" | ||
| 12 | "strings" | 13 | "strings" | ||
| 13 | "sync" | 14 | "sync" | ||
| @@ -35,7 +36,20 @@ type containerSet struct { | |||||
| 35 | 36 | ||||
| 36 | func (cs *containerSet) tryNetwork(name string) { cs.mu.Lock(); cs.networkTry = name; cs.mu.Unlock() } | 37 | func (cs *containerSet) tryNetwork(name string) { cs.mu.Lock(); cs.networkTry = name; cs.mu.Unlock() } | ||
| 37 | 38 | ||||
| 38 | - | func (cs *containerSet) addDir(d string) { cs.mu.Lock(); cs.dirs = append(cs.dirs, d); cs.mu.Unlock() } | 39 | + | func (cs *containerSet) addDir(d string) { |
| 40 | + | cs.mu.Lock() | |||
| 41 | + | defer cs.mu.Unlock() | |||
| 42 | + | if !slices.Contains(cs.dirs, d) { | |||
| 43 | + | cs.dirs = append(cs.dirs, d) | |||
| 44 | + | } | |||
| 45 | + | } | |||
| 46 | + | ||||
| 47 | + | // delDir forgets a directory that is gone. | |||
| 48 | + | func (cs *containerSet) delDir(d string) { | |||
| 49 | + | cs.mu.Lock() | |||
| 50 | + | cs.dirs = slices.DeleteFunc(cs.dirs, func(x string) bool { return x == d }) | |||
| 51 | + | cs.mu.Unlock() | |||
| 52 | + | } | |||
| 39 | 53 | ||||
| 40 | func (cs *containerSet) all() []*container { | 54 | func (cs *containerSet) all() []*container { | ||
| 41 | var l []*container | 55 | var l []*container | ||
| @@ -227,6 +241,7 @@ func (j *job) removeConfigDir(dir string) error { | |||||
| 227 | j.cs.addDir(dir) | 241 | j.cs.addDir(dir) | ||
| 228 | return ghErrorf("Failed to remove directory containing Docker client credentials: %v", err) | 242 | return ghErrorf("Failed to remove directory containing Docker client credentials: %v", err) | ||
| 229 | } | 243 | } | ||
| 244 | + | j.cs.delDir(dir) // unregistered only once it is gone | |||
| 230 | return nil | 245 | return nil | ||
| 231 | } | 246 | } | ||
| 232 | 247 | ||||
| @@ -244,6 +259,9 @@ func (j *job) registryLogin(s *stepRun, c *container, cancel <-chan struct{}) (s | |||||
| 244 | if err := os.MkdirAll(dir, 0o700); err != nil { | 259 | if err := os.MkdirAll(dir, 0o700); err != nil { | ||
| 245 | return "", ghErrorf("Failed to create directory to store registry client credentials: %v", err) | 260 | return "", ghErrorf("Failed to create directory to store registry client credentials: %v", err) | ||
| 246 | } | 261 | } | ||
| 262 | + | // Registered with the cleanup of the job before anything can fail or panic: | |||
| 263 | + | // from here on the fallback removes it whatever happens. | |||
| 264 | + | j.cs.addDir(dir) | |||
| 247 | j.own(dir) | 265 | j.own(dir) | ||
| 248 | args := []string{j.dockerPath(), "--config", dir, "login"} | 266 | args := []string{j.dockerPath(), "--config", dir, "login"} | ||
| 249 | line := "--config " + dir + " login" | 267 | line := "--config " + dir + " login" | ||
M internal/executor/containers_start.go +4 -0
| @@ -100,6 +100,7 @@ func (j *job) startContainer(s *stepRun, c *container, cancel <-chan struct{}) e | |||
| 100 | 100 | } | |
| 101 | 101 | return err | |
| 102 | 102 | } | |
| 103 | + | defer func() { _ = j.removeConfigDir(cfg) }() // also on a panic; idempotent | |
| 103 | 104 | code, err := j.pullWithRetry(s, c.image, cfg, cancel) | |
| 104 | 105 | // Credentials are removed after the pull, whatever it did (:171-173). | |
| 105 | 106 | if rerr := j.removeConfigDir(cfg); rerr != nil { | |
| @@ -120,6 +121,9 @@ func (j *job) startContainer(s *stepRun, c *container, cancel <-chan struct{}) e | |||
| 120 | 121 | return err | |
| 121 | 122 | } | |
| 122 | 123 | } | |
| 124 | + | if err := checkEnvNames(c.env.Keys()); err != nil { | |
| 125 | + | return err | |
| 126 | + | } | |
| 123 | 127 | c.createTried = true // the container may exist from here on, named c.name | |
| 124 | 128 | out, err := j.dockerList(s, j.createCmd(c), cancel) | |
| 125 | 129 | if err != nil { | |
| @@ -100,6 +100,7 @@ func (j *job) startContainer(s *stepRun, c *container, cancel <-chan struct{}) e | |||||
| 100 | } | 100 | } | ||
| 101 | return err | 101 | return err | ||
| 102 | } | 102 | } | ||
| 103 | + | defer func() { _ = j.removeConfigDir(cfg) }() // also on a panic; idempotent | |||
| 103 | code, err := j.pullWithRetry(s, c.image, cfg, cancel) | 104 | code, err := j.pullWithRetry(s, c.image, cfg, cancel) | ||
| 104 | // Credentials are removed after the pull, whatever it did (:171-173). | 105 | // Credentials are removed after the pull, whatever it did (:171-173). | ||
| 105 | if rerr := j.removeConfigDir(cfg); rerr != nil { | 106 | if rerr := j.removeConfigDir(cfg); rerr != nil { | ||
| @@ -120,6 +121,9 @@ func (j *job) startContainer(s *stepRun, c *container, cancel <-chan struct{}) e | |||||
| 120 | return err | 121 | return err | ||
| 121 | } | 122 | } | ||
| 122 | } | 123 | } | ||
| 124 | + | if err := checkEnvNames(c.env.Keys()); err != nil { | |||
| 125 | + | return err | |||
| 126 | + | } | |||
| 123 | c.createTried = true // the container may exist from here on, named c.name | 127 | c.createTried = true // the container may exist from here on, named c.name | ||
| 124 | out, err := j.dockerList(s, j.createCmd(c), cancel) | 128 | out, err := j.dockerList(s, j.createCmd(c), cancel) | ||
| 125 | if err != nil { | 129 | if err != nil { | ||
M internal/executor/containers_stop.go +77 -28
| @@ -27,7 +27,7 @@ func (j *job) containerHealth(s *stepRun, c *container, cancel <-chan struct{}) | |||
| 27 | 27 | inspect := func() (string, error) { | |
| 28 | 28 | cmd := j.newDockerCmd("inspect", | |
| 29 | 29 | eqOpt("--format", "{{if .Config.Healthcheck}}{{print .State.Health.Status}}{{end}}"), raw(c.id)) | |
| 30 | - | l, err := j.dockerList(s, cmd, nil) | |
| 30 | + | l, err := j.dockerList(s, cmd, cancel) | |
| 31 | 31 | if err != nil || len(l) == 0 { | |
| 32 | 32 | return "", err | |
| 33 | 33 | } | |
| @@ -169,16 +169,64 @@ func (j *job) cleanupContainers(s *stepRun) (errs []error) { | |||
| 169 | 169 | _, code, err := j.dockerDo(c, dockerOpts{s: s, cancel: budget, all: true, timeout: t.CleanupCommand}) | |
| 170 | 170 | return code, err | |
| 171 | 171 | } | |
| 172 | + | // rm runs `docker rm --force target`. It reports whether the target is | |
| 173 | + | // gone: removed, or docker says it does not exist. Any other failure leaves | |
| 174 | + | // it pending, for the fallback of the job to try again. | |
| 175 | + | rm := func(kind string, c dockerCmd) (gone bool) { | |
| 176 | + | lines, code, err := j.dockerDoCleanup(c, s, budget, t.CleanupCommand) | |
| 177 | + | switch { | |
| 178 | + | case err != nil: | |
| 179 | + | errs = append(errs, err) | |
| 180 | + | return false | |
| 181 | + | case code == 0: | |
| 182 | + | return true | |
| 183 | + | case absent(lines): | |
| 184 | + | return true | |
| 185 | + | } | |
| 186 | + | j.warnTo(s, fmt.Sprintf("Docker %s fail with exit code %d", kind, code)) | |
| 187 | + | return false | |
| 188 | + | } | |
| 189 | + | // A container whose create did not finish is not known by its id, and its | |
| 190 | + | // name may not be ours (options: can set another --name): it is found by | |
| 191 | + | // the label every container of the job carries (DockerCreate adds it). | |
| 192 | + | var unresolved []*container | |
| 172 | 193 | for _, c := range cs.all() { | |
| 173 | - | if !c.createTried || c.removed { | |
| 174 | - | continue | |
| 194 | + | if c.createTried && !c.removed && c.id == "" { | |
| 195 | + | unresolved = append(unresolved, c) | |
| 196 | + | } | |
| 197 | + | } | |
| 198 | + | if len(unresolved) > 0 { | |
| 199 | + | ok := true | |
| 200 | + | ids, code, err := j.dockerDoCleanup(j.newDockerCmd("ps", raw("--all", "--quiet", "--no-trunc"), quotedOpt("--filter", "label="+j.dockerLabel())), nil, budget, t.CleanupCommand) | |
| 201 | + | switch { | |
| 202 | + | case err != nil: | |
| 203 | + | errs, ok = append(errs, err), false | |
| 204 | + | case code != 0: | |
| 205 | + | j.warnTo(s, fmt.Sprintf("Docker ps fail with exit code %d", code)) | |
| 206 | + | ok = false | |
| 207 | + | } | |
| 208 | + | known := map[string]bool{} | |
| 209 | + | for _, c := range cs.all() { | |
| 210 | + | if c.id != "" { | |
| 211 | + | known[c.id] = true // those are removed below, after their logs | |
| 212 | + | } | |
| 213 | + | } | |
| 214 | + | for _, id := range ids { | |
| 215 | + | if containerIDRe.MatchString(id) && !known[id] && !rm("rm", j.newDockerCmd("rm", raw("--force", id))) { | |
| 216 | + | ok = false | |
| 217 | + | } | |
| 175 | 218 | } | |
| 176 | - | target := c.id | |
| 177 | - | if target == "" { | |
| 178 | - | // The create is not known to have finished: the name was ours. | |
| 179 | - | target = c.name | |
| 219 | + | if ok { // listed, and everything listed is gone: nothing of the job is left | |
| 220 | + | for _, c := range unresolved { | |
| 221 | + | c.removed = true | |
| 222 | + | } | |
| 223 | + | } | |
| 224 | + | } | |
| 225 | + | for _, c := range cs.all() { | |
| 226 | + | if !c.createTried || c.removed || c.id == "" { | |
| 227 | + | continue | |
| 180 | 228 | } | |
| 181 | - | if c.id != "" && !c.isJob && !c.failedInit { | |
| 229 | + | if !c.isJob && !c.failedInit { | |
| 182 | 230 | out("Print service container logs: " + c.name) | |
| 183 | 231 | code, err := run(j.newDockerCmd("logs", raw("--details", c.id))) | |
| 184 | 232 | switch { | |
| @@ -191,16 +239,7 @@ func (j *job) cleanupContainers(s *stepRun) (errs []error) { | |||
| 191 | 239 | } | |
| 192 | 240 | } | |
| 193 | 241 | out("Stop and remove container: " + c.name) | |
| 194 | - | code, err := run(j.newDockerCmd("rm", raw("--force", target))) | |
| 195 | - | switch { | |
| 196 | - | case err != nil: | |
| 197 | - | errs = append(errs, err) | |
| 198 | - | case code != 0: | |
| 199 | - | j.warnTo(s, fmt.Sprintf("Docker rm fail with exit code %d", code)) | |
| 200 | - | if c.id == "" { | |
| 201 | - | c.removed = true // a name that docker does not know is nothing to remove | |
| 202 | - | } | |
| 203 | - | default: | |
| 242 | + | if rm("rm", j.newDockerCmd("rm", raw("--force", c.id))) { | |
| 204 | 243 | c.removed = true | |
| 205 | 244 | } | |
| 206 | 245 | } | |
| @@ -209,16 +248,7 @@ func (j *job) cleanupContainers(s *stepRun) (errs []error) { | |||
| 209 | 248 | cs.mu.Unlock() | |
| 210 | 249 | if network != "" && !gone { | |
| 211 | 250 | out("Remove container network: " + network) | |
| 212 | - | code, err := run(j.newDockerCmd("network", raw("rm", network))) | |
| 213 | - | switch { | |
| 214 | - | case err != nil: | |
| 215 | - | errs = append(errs, err) | |
| 216 | - | case code != 0: | |
| 217 | - | j.warnTo(s, fmt.Sprintf("Docker network rm failed with exit code %d", code)) | |
| 218 | - | cs.mu.Lock() | |
| 219 | - | cs.netRemoved = true // the runner warns and moves on; docker knows no such network (or refuses) | |
| 220 | - | cs.mu.Unlock() | |
| 221 | - | default: | |
| 251 | + | if rm("network rm", j.newDockerCmd("network", raw("rm", network))) { | |
| 222 | 252 | cs.mu.Lock() | |
| 223 | 253 | cs.netRemoved = true | |
| 224 | 254 | cs.mu.Unlock() | |
| @@ -245,3 +275,22 @@ func (j *job) warnTo(s *stepRun, msg string) { | |||
| 245 | 275 | } | |
| 246 | 276 | j.cfg.Log.Warn().Msg(msg) | |
| 247 | 277 | } | |
| 278 | + | ||
| 279 | + | // absent reports whether docker's answer to a removal says the object does not | |
| 280 | + | // exist: that is the state the cleanup wants. | |
| 281 | + | func absent(lines []string) bool { | |
| 282 | + | for _, l := range lines { | |
| 283 | + | l = strings.ToLower(l) | |
| 284 | + | if strings.Contains(l, "no such container") || strings.Contains(l, "no such network") || | |
| 285 | + | strings.Contains(l, "no such object") || strings.Contains(l, "network") && strings.Contains(l, "not found") { | |
| 286 | + | return true | |
| 287 | + | } | |
| 288 | + | } | |
| 289 | + | return false | |
| 290 | + | } | |
| 291 | + | ||
| 292 | + | // dockerDoCleanup runs a command of the cleanup: bounded by its own limit and | |
| 293 | + | // by the budget, its output (stdout and stderr) returned, logged to s if any. | |
| 294 | + | func (j *job) dockerDoCleanup(c dockerCmd, s *stepRun, budget <-chan struct{}, limit time.Duration) ([]string, int, error) { | |
| 295 | + | return j.dockerDo(c, dockerOpts{s: s, cancel: budget, all: true, timeout: limit, errToOut: true}) | |
| 296 | + | } | |
| @@ -27,7 +27,7 @@ func (j *job) containerHealth(s *stepRun, c *container, cancel <-chan struct{}) | |||||
| 27 | inspect := func() (string, error) { | 27 | inspect := func() (string, error) { | ||
| 28 | cmd := j.newDockerCmd("inspect", | 28 | cmd := j.newDockerCmd("inspect", | ||
| 29 | eqOpt("--format", "{{if .Config.Healthcheck}}{{print .State.Health.Status}}{{end}}"), raw(c.id)) | 29 | eqOpt("--format", "{{if .Config.Healthcheck}}{{print .State.Health.Status}}{{end}}"), raw(c.id)) | ||
| 30 | - | l, err := j.dockerList(s, cmd, nil) | 30 | + | l, err := j.dockerList(s, cmd, cancel) |
| 31 | if err != nil || len(l) == 0 { | 31 | if err != nil || len(l) == 0 { | ||
| 32 | return "", err | 32 | return "", err | ||
| 33 | } | 33 | } | ||
| @@ -169,16 +169,64 @@ func (j *job) cleanupContainers(s *stepRun) (errs []error) { | |||||
| 169 | _, code, err := j.dockerDo(c, dockerOpts{s: s, cancel: budget, all: true, timeout: t.CleanupCommand}) | 169 | _, code, err := j.dockerDo(c, dockerOpts{s: s, cancel: budget, all: true, timeout: t.CleanupCommand}) | ||
| 170 | return code, err | 170 | return code, err | ||
| 171 | } | 171 | } | ||
| 172 | + | // rm runs `docker rm --force target`. It reports whether the target is | |||
| 173 | + | // gone: removed, or docker says it does not exist. Any other failure leaves | |||
| 174 | + | // it pending, for the fallback of the job to try again. | |||
| 175 | + | rm := func(kind string, c dockerCmd) (gone bool) { | |||
| 176 | + | lines, code, err := j.dockerDoCleanup(c, s, budget, t.CleanupCommand) | |||
| 177 | + | switch { | |||
| 178 | + | case err != nil: | |||
| 179 | + | errs = append(errs, err) | |||
| 180 | + | return false | |||
| 181 | + | case code == 0: | |||
| 182 | + | return true | |||
| 183 | + | case absent(lines): | |||
| 184 | + | return true | |||
| 185 | + | } | |||
| 186 | + | j.warnTo(s, fmt.Sprintf("Docker %s fail with exit code %d", kind, code)) | |||
| 187 | + | return false | |||
| 188 | + | } | |||
| 189 | + | // A container whose create did not finish is not known by its id, and its | |||
| 190 | + | // name may not be ours (options: can set another --name): it is found by | |||
| 191 | + | // the label every container of the job carries (DockerCreate adds it). | |||
| 192 | + | var unresolved []*container | |||
| 172 | for _, c := range cs.all() { | 193 | for _, c := range cs.all() { | ||
| 173 | - | if !c.createTried || c.removed { | 194 | + | if c.createTried && !c.removed && c.id == "" { |
| 174 | - | continue | 195 | + | unresolved = append(unresolved, c) |
| 196 | + | } | |||
| 197 | + | } | |||
| 198 | + | if len(unresolved) > 0 { | |||
| 199 | + | ok := true | |||
| 200 | + | ids, code, err := j.dockerDoCleanup(j.newDockerCmd("ps", raw("--all", "--quiet", "--no-trunc"), quotedOpt("--filter", "label="+j.dockerLabel())), nil, budget, t.CleanupCommand) | |||
| 201 | + | switch { | |||
| 202 | + | case err != nil: | |||
| 203 | + | errs, ok = append(errs, err), false | |||
| 204 | + | case code != 0: | |||
| 205 | + | j.warnTo(s, fmt.Sprintf("Docker ps fail with exit code %d", code)) | |||
| 206 | + | ok = false | |||
| 207 | + | } | |||
| 208 | + | known := map[string]bool{} | |||
| 209 | + | for _, c := range cs.all() { | |||
| 210 | + | if c.id != "" { | |||
| 211 | + | known[c.id] = true // those are removed below, after their logs | |||
| 212 | + | } | |||
| 213 | + | } | |||
| 214 | + | for _, id := range ids { | |||
| 215 | + | if containerIDRe.MatchString(id) && !known[id] && !rm("rm", j.newDockerCmd("rm", raw("--force", id))) { | |||
| 216 | + | ok = false | |||
| 217 | + | } | |||
| 175 | } | 218 | } | ||
| 176 | - | target := c.id | 219 | + | if ok { // listed, and everything listed is gone: nothing of the job is left |
| 177 | - | if target == "" { | 220 | + | for _, c := range unresolved { |
| 178 | - | // The create is not known to have finished: the name was ours. | 221 | + | c.removed = true |
| 179 | - | target = c.name | 222 | + | } |
| 223 | + | } | |||
| 224 | + | } | |||
| 225 | + | for _, c := range cs.all() { | |||
| 226 | + | if !c.createTried || c.removed || c.id == "" { | |||
| 227 | + | continue | |||
| 180 | } | 228 | } | ||
| 181 | - | if c.id != "" && !c.isJob && !c.failedInit { | 229 | + | if !c.isJob && !c.failedInit { |
| 182 | out("Print service container logs: " + c.name) | 230 | out("Print service container logs: " + c.name) | ||
| 183 | code, err := run(j.newDockerCmd("logs", raw("--details", c.id))) | 231 | code, err := run(j.newDockerCmd("logs", raw("--details", c.id))) | ||
| 184 | switch { | 232 | switch { | ||
| @@ -191,16 +239,7 @@ func (j *job) cleanupContainers(s *stepRun) (errs []error) { | |||||
| 191 | } | 239 | } | ||
| 192 | } | 240 | } | ||
| 193 | out("Stop and remove container: " + c.name) | 241 | out("Stop and remove container: " + c.name) | ||
| 194 | - | code, err := run(j.newDockerCmd("rm", raw("--force", target))) | 242 | + | if rm("rm", j.newDockerCmd("rm", raw("--force", c.id))) { |
| 195 | - | switch { | |||
| 196 | - | case err != nil: | |||
| 197 | - | errs = append(errs, err) | |||
| 198 | - | case code != 0: | |||
| 199 | - | j.warnTo(s, fmt.Sprintf("Docker rm fail with exit code %d", code)) | |||
| 200 | - | if c.id == "" { | |||
| 201 | - | c.removed = true // a name that docker does not know is nothing to remove | |||
| 202 | - | } | |||
| 203 | - | default: | |||
| 204 | c.removed = true | 243 | c.removed = true | ||
| 205 | } | 244 | } | ||
| 206 | } | 245 | } | ||
| @@ -209,16 +248,7 @@ func (j *job) cleanupContainers(s *stepRun) (errs []error) { | |||||
| 209 | cs.mu.Unlock() | 248 | cs.mu.Unlock() | ||
| 210 | if network != "" && !gone { | 249 | if network != "" && !gone { | ||
| 211 | out("Remove container network: " + network) | 250 | out("Remove container network: " + network) | ||
| 212 | - | code, err := run(j.newDockerCmd("network", raw("rm", network))) | 251 | + | if rm("network rm", j.newDockerCmd("network", raw("rm", network))) { |
| 213 | - | switch { | |||
| 214 | - | case err != nil: | |||
| 215 | - | errs = append(errs, err) | |||
| 216 | - | case code != 0: | |||
| 217 | - | j.warnTo(s, fmt.Sprintf("Docker network rm failed with exit code %d", code)) | |||
| 218 | - | cs.mu.Lock() | |||
| 219 | - | cs.netRemoved = true // the runner warns and moves on; docker knows no such network (or refuses) | |||
| 220 | - | cs.mu.Unlock() | |||
| 221 | - | default: | |||
| 222 | cs.mu.Lock() | 252 | cs.mu.Lock() | ||
| 223 | cs.netRemoved = true | 253 | cs.netRemoved = true | ||
| 224 | cs.mu.Unlock() | 254 | cs.mu.Unlock() | ||
| @@ -245,3 +275,22 @@ func (j *job) warnTo(s *stepRun, msg string) { | |||||
| 245 | } | 275 | } | ||
| 246 | j.cfg.Log.Warn().Msg(msg) | 276 | j.cfg.Log.Warn().Msg(msg) | ||
| 247 | } | 277 | } | ||
| 278 | + | ||||
| 279 | + | // absent reports whether docker's answer to a removal says the object does not | |||
| 280 | + | // exist: that is the state the cleanup wants. | |||
| 281 | + | func absent(lines []string) bool { | |||
| 282 | + | for _, l := range lines { | |||
| 283 | + | l = strings.ToLower(l) | |||
| 284 | + | if strings.Contains(l, "no such container") || strings.Contains(l, "no such network") || | |||
| 285 | + | strings.Contains(l, "no such object") || strings.Contains(l, "network") && strings.Contains(l, "not found") { | |||
| 286 | + | return true | |||
| 287 | + | } | |||
| 288 | + | } | |||
| 289 | + | return false | |||
| 290 | + | } | |||
| 291 | + | ||||
| 292 | + | // dockerDoCleanup runs a command of the cleanup: bounded by its own limit and | |||
| 293 | + | // by the budget, its output (stdout and stderr) returned, logged to s if any. | |||
| 294 | + | func (j *job) dockerDoCleanup(c dockerCmd, s *stepRun, budget <-chan struct{}, limit time.Duration) ([]string, int, error) { | |||
| 295 | + | return j.dockerDo(c, dockerOpts{s: s, cancel: budget, all: true, timeout: limit, errToOut: true}) | |||
| 296 | + | } | |||