diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 5fe2f2a..d97ff1d 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -939,6 +939,13 @@ func applyScoped( } } setRunnerWarnings(warnings) + // **Every new image of a module is on the machine before any of its containers is set aside** (design 51, issue 1966 + // ENG-7). A container replaced after the first one's turn pulled its image only then, and for a module whose first + // container serves the store its second image comes from, that pull met the stopped one. A pull that fails + // replaces nothing of the module: its containers keep running what they ran, the failure names the image, and what + // follows in the module is skipped as it is for any step that did not complete. + in.pulled = map[string]bool{} + pullFirst(ctx, ordered, d, in.pulled, run, &failures, gated, gatedWhy, log) for i, resource := range ordered { if !orphansRemoved && i == guardFirst { // **Only a guard that is up may let the filter go.** Removing the derived filter's @@ -2759,6 +2766,9 @@ type inputs struct { files map[string]bool // handed is every group a record that went this apply handed to a resource still asking for it. handed Handed + // pulled is every image this apply found on the machine, or pulled, before any container was replaced (pullFirst): + // applyContainer does not ask for it again. + pulled map[string]bool } // fileDigest is what a file the container reads holds, by digest. @@ -3165,8 +3175,10 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner, // gone: an image the registry does not hold then left the service down and every later apply // failing. Pulled (or found present) first, an unobtainable image fails this resource loudly, // naming the image, and the old container keeps running. - if err := ensureImage(ctx, cri, r.Image, run); err != nil { - return out, fmt.Errorf("not replacing container %s, which keeps running: %w", r.Name, err) + if !in.pulled[r.Image] { + if err := ensureImage(ctx, cri, r.Image, run); err != nil { + return out, fmt.Errorf("not replacing container %s, which keeps running: %w", r.Name, err) + } } // **The running container is set aside, never removed, until its replacement runs** (issue 1945): stopped and // renamed, so its name and ports are free, and started again under its own name when the replacement does not @@ -3469,16 +3481,16 @@ func ensureImage(ctx context.Context, cri, image string, run Runner) error { // already hold — built here, or handed over. There is no registry that answers for it, so // pulling would fail somewhere that names a network problem instead of a missing image. if strings.HasPrefix(image, "sha256:") { - return fmt.Errorf( + return &PullError{Image: image, Err: fmt.Errorf( "image %s is not on this machine, and an image named by its own digest cannot be "+ "fetched: nothing serves it. Build it here, or load it, before applying this", - image) + image)} } if _, err := run(ctx, cri, "pull", image); err != nil { - return fmt.Errorf("pulling image %s: %w", image, err) + return &PullError{Image: image, Err: fmt.Errorf("pulling image %s: %w", image, err)} } if _, err := run(ctx, cri, "image", "inspect", image); err != nil { - return fmt.Errorf("image %s is not present after pulling it: %w", image, err) + return &PullError{Image: image, Err: fmt.Errorf("image %s is not present after pulling it: %w", image, err)} } return nil } @@ -4340,3 +4352,61 @@ func ownerText(info os.FileInfo) string { } return fmt.Sprintf("%d:%d", uid, gid) } + +// pullFirst finds on the machine, or pulls, the image of each container the declaration runs before any is replaced +// (ENG-7) for a module of two containers or more. A container that is run once is left to its marker, and a runtime that +// does not answer to applyContainer, which says so. The first image that cannot be had fails its container with the image named and gates its module, so none of +// its containers is touched; the images found before it stay pulled. +func pullFirst(ctx context.Context, ordered []declaration.Resource, d *declaration.Declaration, pulled map[string]bool, + run Runner, failures *[]*Error, gated map[string]bool, gatedWhy map[string]string, log func(string)) { + // A module of one container already pulls its image before it sets that container aside (applyContainer), and nothing + // of it comes before: only a module of two or more has an image pulled after one of its containers was replaced. + count := map[string]int{} + for _, resource := range ordered { + if c, isContainer := resource.(*declaration.Container); isContainer && c.Image != "" && !c.RunOnce && c.Schedule == "" { + module, _ := d.ModuleOfID(c.Identity()) + count[module]++ + } + } + var cri string + for _, resource := range ordered { + c, isContainer := resource.(*declaration.Container) + if !isContainer || c.Image == "" || c.RunOnce || c.Schedule != "" || pulled[c.Image] { + continue + } + module, ours := d.ModuleOfID(c.Identity()) + if count[module] < 2 { + continue + } + if ours && gated[module] { + continue + } + if cri == "" { + var err error + if cri, err = containerRuntime(ctx, run); err != nil { + return + } + } + if err := ensureImage(ctx, cri, c.Image, run); err != nil { + err = fmt.Errorf("not replacing container %s, or any other container of its module, which keep running: %w", c.Name, err) + *failures = append(*failures, &Error{Resource: c.Identity(), Err: err}) + log(fmt.Sprintf(" failed %s (%s): %v", c.Identity(), c.Target(), err)) + if ours { + gated[module] = true + gatedWhy[module] = "its image " + c.Image + " could not be had, so none of its containers was replaced" + } + continue + } + pulled[c.Image] = true + } +} + +// PullError is an image that could not be found or pulled, as ensureImage fails: what the node agent reads to tell a failed +// pull from another failed apply (ENG-8). +type PullError struct { + Image string + Err error +} + +func (e *PullError) Error() string { return e.Err.Error() } +func (e *PullError) Unwrap() error { return e.Err } diff --git a/internal/apply/replace_image_test.go b/internal/apply/replace_image_test.go index 447366b..48ff6a9 100644 --- a/internal/apply/replace_image_test.go +++ b/internal/apply/replace_image_test.go @@ -47,6 +47,63 @@ func TestARunningContainerIsKeptWhenItsReplacementImageCannotBeObtained(t *testi if !strings.Contains(err.Error(), pinned) { t.Errorf("the failure does not name the image %s: %v", pinned, err) } + var pull *PullError + if !errors.As(err, &pull) || pull.Image != pinned { + t.Errorf("the failure is not a pull error naming %s: %v", pinned, err) + } + + // design 51, ENG-7: a module of two containers whose second new image cannot be pulled replaces neither, and says the + // image; with both pullable, both are pulled before the first is set aside. + second := strings.Replace(pinned, "postgres", "other", 1) + two := `{"declaration":1,"resources":[ + {"id":"a.first","type":"container","name":"first","image":"` + pinned + `","env":{"X":"1"}}, + {"id":"a.second","type":"container","name":"second","image":"` + second + `","env":{"X":"2"}} + ]}` + for _, c := range []struct { + name string + unpullable string + wantTouched bool + wantPulled string + }{ + {"the second image cannot be pulled", second, false, "pull " + pinned}, + {"both can", "", true, "pull " + pinned + "; pull " + second}, + } { + d := parseTrusted(t, two) + var steps []string + present := map[string]bool{} + run := func(ctx context.Context, name string, args ...string) (string, error) { + args = oldShape(name, args) + switch args[0] { + case "info": + return "27.0\n", nil + case "container": + return "true\tsome-older-spec", nil + case "image": + if present[args[len(args)-1]] { + return "", nil + } + return "", errors.New("no such image") + case "pull": + if args[1] == c.unpullable { + return "", errors.New("manifest unknown") + } + present[args[1]] = true + steps = append(steps, "pull "+args[1]) + case "rm", "rename", "stop", "run": + steps = append(steps, args[0]+" "+strings.Join(args[1:], " ")) + } + return "", nil + } + _, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, run, nil, nil) + got := strings.Join(steps, "; ") + touched := strings.Contains(got, "rename") || strings.Contains(got, "run ") + if touched != c.wantTouched || !strings.HasPrefix(got, c.wantPulled) { + t.Errorf("%s: steps %q (touched %v), want pulls %q first and touched=%v", c.name, got, touched, c.wantPulled, c.wantTouched) + } + if !c.wantTouched && (err == nil || !strings.Contains(err.Error(), second)) { + t.Errorf("%s: the failure does not name %s: %v", c.name, second, err) + } + } } func TestAReplacementImageIsPresentBeforeTheRunningContainerIsRemoved(t *testing.T) {