diff --git a/internals/overlord/planstate/manager.go b/internals/overlord/planstate/manager.go index 3767e95a3..dc6ca21d4 100644 --- a/internals/overlord/planstate/manager.go +++ b/internals/overlord/planstate/manager.go @@ -116,7 +116,8 @@ func (m *PlanManager) Plan() *plan.Plan { // layer.Order field to the new order. If a layer with layer.Label already // exists, return an error of type *LabelExists. Inner must be set to true // if the append operation may be demoted to an insert due to the layer -// configuration being located in a sub-directory. +// configuration being located in a sub-directory. On any error the plan is +// left exactly as it was, including its layer list. func (m *PlanManager) AppendLayer(layer *plan.Layer, inner bool) error { var newPlan *plan.Plan defer func() { m.callChangeListeners(newPlan) }() @@ -240,7 +241,9 @@ func (m *PlanManager) appendLayer(newLayer *plan.Layer, inner bool) (*plan.Plan, return nil, fmt.Errorf("cannot insert sub-directory layer without 'inner' attribute set") } - newLayers := slices.Insert(m.plan.Layers, newIndex, newLayer) + // Insert into a copy: slices.Insert shifts in place when the slice has + // spare capacity, which would change the live plan before validation. + newLayers := slices.Insert(slices.Clone(m.plan.Layers), newIndex, newLayer) newPlan, err := m.updatePlanLayers(newLayers) if err != nil { return nil, err diff --git a/internals/overlord/planstate/manager_test.go b/internals/overlord/planstate/manager_test.go index a8bc901a1..3e73e1c53 100644 --- a/internals/overlord/planstate/manager_test.go +++ b/internals/overlord/planstate/manager_test.go @@ -15,6 +15,9 @@ package planstate_test import ( + "fmt" + "sort" + "strings" "sync/atomic" "time" @@ -674,3 +677,77 @@ workloads: err = ps.planMgr.CombineLayer(layer, false) c.Assert(err, IsNil) } + +// TestAppendLayerRejectedLeavesPlanUnchanged pins the rollback contract of +// AppendLayer: a layer that fails plan validation must leave the plan's layer +// list exactly as it was. The layout matters - the sub-directory layer is +// first so the insert index is not the end of the list, and there are enough +// layers for the slice to carry spare capacity, which is what once let +// slices.Insert shift the live list in place before validation ran. +func (ps *planSuite) TestAppendLayerRejectedLeavesPlanUnchanged(c *C) { + var err error + ps.planMgr, err = planstate.NewManager(ps.layersDir) + c.Assert(err, IsNil) + + serviceLayer := string(reindent(` + services: + %s: + override: replace + command: sleep 1000 + `)) + + appendLabels := []string{"foo/one", "bbb", "ccc", "ddd", "eee"} + for _, label := range appendLabels { + name := "svc-" + strings.ReplaceAll(label, "/", "-") + layer := ps.parseLayer(c, 0, label, fmt.Sprintf(serviceLayer, name)) + err = ps.planMgr.AppendLayer(layer, true) + c.Assert(err, IsNil) + } + + before := ps.planMgr.Plan() + labelsBefore := layerLabels(before) + servicesBefore := serviceNames(before) + + // This layer parses, but fails plan validation because svc-missing is + // not defined anywhere. + rejected := ps.parseLayer(c, 0, "foo/two", string(reindent(` + services: + svc-injected: + override: replace + command: sleep 1000 + requires: [svc-missing] + `))) + err = ps.planMgr.AppendLayer(rejected, true) + c.Assert(err, ErrorMatches, `service "svc-missing" does not exist`) + + after := ps.planMgr.Plan() + c.Assert(layerLabels(after), DeepEquals, labelsBefore) + c.Assert(serviceNames(after), DeepEquals, servicesBefore) + + // A later valid append must not resurrect the rejected layer, and must + // not have lost a layer either. + accepted := ps.parseLayer(c, 0, "later", fmt.Sprintf(serviceLayer, "svc-later")) + err = ps.planMgr.AppendLayer(accepted, false) + c.Assert(err, IsNil) + + final := ps.planMgr.Plan() + c.Assert(layerLabels(final), DeepEquals, append(append([]string(nil), labelsBefore...), "later")) + c.Assert(serviceNames(final), DeepEquals, append(append([]string(nil), servicesBefore...), "svc-later")) +} + +func layerLabels(p *plan.Plan) []string { + labels := make([]string, 0, len(p.Layers)) + for _, layer := range p.Layers { + labels = append(labels, layer.Label) + } + return labels +} + +func serviceNames(p *plan.Plan) []string { + names := make([]string, 0, len(p.Services)) + for name := range p.Services { + names = append(names, name) + } + sort.Strings(names) + return names +}