Skip to content

Commit 9c24b89

Browse files
committed
more tests
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
1 parent 137c312 commit 9c24b89

File tree

6 files changed

+2283
-32
lines changed

6 files changed

+2283
-32
lines changed

pkg/compose/create.go

Lines changed: 46 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -109,10 +109,13 @@ func (s *composeService) create(ctx context.Context, project *types.Project, opt
109109
}
110110
}
111111

112-
// Validate external networks exist before reconciling
112+
// Validate external resources exist before reconciling
113113
if err := s.validateExternalNetworks(ctx, project, options.Services); err != nil {
114114
return err
115115
}
116+
if err := s.validateExternalVolumes(ctx, project, options.Services); err != nil {
117+
return err
118+
}
116119

117120
// Phase 2: Reconcile desired vs observed state (pure function)
118121
plan, err := Reconcile(project, observed, ReconcileOptions{
@@ -127,14 +130,14 @@ func (s *composeService) create(ctx context.Context, project *types.Project, opt
127130
return err
128131
}
129132

133+
s.emitUntouchedContainerEvents(project, observed, plan)
134+
130135
if plan.IsEmpty() {
131136
return nil
132137
}
133138

134-
s.emitUntouchedContainerEvents(project, observed, plan)
135-
136139
// Phase 3: Execute the plan
137-
return s.ExecutePlan(ctx, project, plan)
140+
return s.ExecutePlan(ctx, project, plan, observed)
138141
}
139142

140143
// emitUntouchedContainerEvents emits progress events for containers that are
@@ -143,13 +146,7 @@ func (s *composeService) emitUntouchedContainerEvents(project *types.Project, ob
143146
for _, service := range project.Services {
144147
for _, ctr := range observed.Containers[service.Name] {
145148
ctrName := getCanonicalContainerName(ctr)
146-
if _, touched := plan.Operations["stop-container:"+ctrName]; touched {
147-
continue
148-
}
149-
if _, touched := plan.Operations["start-container:"+ctrName]; touched {
150-
continue
151-
}
152-
if _, touched := plan.Operations["create-container:"+ctrName]; touched {
149+
if plan.ContainerTouched(ctrName) {
153150
continue
154151
}
155152
if ctr.State == container.StateRunning {
@@ -189,6 +186,44 @@ func (s *composeService) validateExternalNetworks(ctx context.Context, project *
189186
return nil
190187
}
191188

189+
// validateExternalVolumes checks that external volumes exist for services
190+
// that are part of the current operation. Returns an error if a required
191+
// external volume is not found.
192+
func (s *composeService) validateExternalVolumes(ctx context.Context, project *types.Project, services []string) error {
193+
for key, vol := range project.Volumes {
194+
if !vol.External {
195+
continue
196+
}
197+
// Check if any targeted service uses this volume
198+
usedByTargetedService := false
199+
for _, service := range project.Services {
200+
if len(services) > 0 && !slices.Contains(services, service.Name) {
201+
continue
202+
}
203+
for _, v := range service.Volumes {
204+
if v.Type == string(mount.TypeVolume) && v.Source == key {
205+
usedByTargetedService = true
206+
break
207+
}
208+
}
209+
if usedByTargetedService {
210+
break
211+
}
212+
}
213+
if !usedByTargetedService {
214+
continue
215+
}
216+
_, err := s.apiClient().VolumeInspect(ctx, vol.Name, client.VolumeInspectOptions{})
217+
if err != nil {
218+
if errdefs.IsNotFound(err) {
219+
return fmt.Errorf("external volume %q not found", vol.Name)
220+
}
221+
return err
222+
}
223+
}
224+
return nil
225+
}
226+
192227
func prepareNetworks(project *types.Project) {
193228
for k, nw := range project.Networks {
194229
nw.CustomLabels = nw.CustomLabels.

pkg/compose/observed_state_test.go

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -155,3 +155,62 @@ func TestReconciliationPlanString(t *testing.T) {
155155
`
156156
assert.Equal(t, plan.String(), expected)
157157
}
158+
159+
func TestContainerTouchedMatchesContainerOps(t *testing.T) {
160+
plan := &ReconciliationPlan{
161+
Operations: map[string]*Operation{
162+
"stop-container:web-1": {
163+
ID: "stop-container:web-1",
164+
Type: OpStopContainer,
165+
ContainerOp: &ContainerOperation{
166+
ContainerName: "web-1",
167+
},
168+
},
169+
"create-container:tmpname_web-2": {
170+
ID: "create-container:tmpname_web-2",
171+
Type: OpCreateContainer,
172+
ContainerOp: &ContainerOperation{
173+
ContainerName: "tmpname_web-2",
174+
},
175+
},
176+
"rename-container:web-2": {
177+
ID: "rename-container:web-2",
178+
Type: OpRenameContainer,
179+
RenameOp: &RenameOperation{
180+
CurrentName: "tmpname_web-2",
181+
NewName: "web-2",
182+
},
183+
// No ContainerOp — rename ops use RenameOp
184+
},
185+
"remove-container:db-1": {
186+
ID: "remove-container:db-1",
187+
Type: OpRemoveContainer,
188+
ContainerOp: &ContainerOperation{
189+
ContainerName: "db-1",
190+
},
191+
},
192+
"create-network:mynet": {
193+
ID: "create-network:mynet",
194+
Type: OpCreateNetwork,
195+
NetworkOp: &NetworkOperation{
196+
NetworkKey: "default",
197+
},
198+
},
199+
},
200+
Dependents: map[string][]string{},
201+
}
202+
203+
// Containers with ContainerOp are touched
204+
assert.Assert(t, plan.ContainerTouched("web-1"), "stop op should mark web-1 as touched")
205+
assert.Assert(t, plan.ContainerTouched("tmpname_web-2"), "create op should mark tmpname_web-2 as touched")
206+
assert.Assert(t, plan.ContainerTouched("db-1"), "remove op should mark db-1 as touched")
207+
208+
// Rename ops have RenameOp, not ContainerOp — rename target is NOT touched via ContainerOp
209+
assert.Assert(t, !plan.ContainerTouched("web-2"), "rename op (RenameOp only) should not mark web-2 as touched")
210+
211+
// Network ops are not container ops
212+
assert.Assert(t, !plan.ContainerTouched("mynet"), "network op should not match container name")
213+
214+
// Non-existent container
215+
assert.Assert(t, !plan.ContainerTouched("nonexistent"), "unknown container should not be touched")
216+
}

pkg/compose/plan_executor.go

Lines changed: 48 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,12 @@ import (
2929
"github.com/compose-spec/compose-go/v2/types"
3030
containerType "github.com/moby/moby/api/types/container"
3131
"github.com/moby/moby/client"
32+
"go.opentelemetry.io/otel"
33+
"go.opentelemetry.io/otel/attribute"
34+
"go.opentelemetry.io/otel/codes"
3235
"golang.org/x/sync/errgroup"
3336

37+
"github.com/docker/compose/v5/internal/tracing"
3438
"github.com/docker/compose/v5/pkg/api"
3539
)
3640

@@ -148,19 +152,15 @@ func (es *executionState) resolveSharedNamespaces(service *types.ServiceConfig)
148152
// ExecutePlan executes a reconciliation plan using DAG traversal similar to
149153
// graphTraversal.visit() in dependencies.go. Operations are executed
150154
// concurrently, respecting dependency ordering.
151-
func (s *composeService) ExecutePlan(ctx context.Context, project *types.Project, plan *ReconciliationPlan) error {
155+
func (s *composeService) ExecutePlan(ctx context.Context, project *types.Project, plan *ReconciliationPlan, observed *ObservedState) error {
152156
if plan.IsEmpty() {
153157
return nil
154158
}
155159

156160
// Pre-populate execution state with existing containers so that
157161
// resolveServiceReferences can find containers for services not
158162
// included in the plan (e.g. --no-deps scenarios).
159-
allContainers, err := s.getContainers(ctx, project.Name, oneOffExclude, true)
160-
if err != nil {
161-
return err
162-
}
163-
state := newExecutionStateFrom(allContainers)
163+
state := newExecutionStateFrom(observed.allContainers())
164164

165165
// Build dependency count map: number of unsatisfied deps per operation.
166166
// The consumer goroutine is single-threaded, so no mutex is needed for depCount.
@@ -219,6 +219,30 @@ func (s *composeService) ExecutePlan(ctx context.Context, project *types.Project
219219
}
220220

221221
func (s *composeService) executeOperation(ctx context.Context, project *types.Project, op *Operation, state *executionState) error {
222+
spanName := op.Type.String()
223+
opts := tracing.SpanOptions{}
224+
if op.ContainerOp != nil {
225+
opts = tracing.ServiceOptions(op.ContainerOp.Service)
226+
}
227+
ctx, span := otel.Tracer("").Start(ctx, spanName, opts.SpanStartOptions()...)
228+
defer span.End()
229+
span.SetAttributes(
230+
attribute.String("operation.id", op.ID),
231+
attribute.String("operation.resource", op.Resource),
232+
attribute.String("operation.reason", op.Reason),
233+
)
234+
235+
err := s.dispatchOperation(ctx, project, op, state)
236+
if err != nil {
237+
span.SetStatus(codes.Error, err.Error())
238+
span.RecordError(err)
239+
} else {
240+
span.SetStatus(codes.Ok, "")
241+
}
242+
return err
243+
}
244+
245+
func (s *composeService) dispatchOperation(ctx context.Context, project *types.Project, op *Operation, state *executionState) error {
222246
switch op.Type {
223247
case OpCreateNetwork:
224248
return s.executePlanCreateNetwork(ctx, project, op, state)
@@ -397,7 +421,10 @@ func (s *composeService) executePlanRunPlugin(ctx context.Context, project *type
397421
// DisplayPlan performs a topological sort of operations and displays them
398422
// grouped by resource type.
399423
func DisplayPlan(plan *ReconciliationPlan, w io.Writer) error {
400-
ops := topologicalSort(plan)
424+
ops, err := topologicalSort(plan)
425+
if err != nil {
426+
return err
427+
}
401428

402429
// Group operations by category
403430
var networkOps, volumeOps []*Operation
@@ -497,7 +524,8 @@ func opVerb(t OperationType) string {
497524
}
498525

499526
// topologicalSort returns operations in dependency order using Kahn's algorithm.
500-
func topologicalSort(plan *ReconciliationPlan) []*Operation {
527+
// Returns an error if the dependency graph contains a cycle.
528+
func topologicalSort(plan *ReconciliationPlan) ([]*Operation, error) {
501529
inDegree := make(map[string]int, len(plan.Operations))
502530
for _, op := range plan.Operations {
503531
inDegree[op.ID] = len(op.DependsOn)
@@ -529,5 +557,16 @@ func topologicalSort(plan *ReconciliationPlan) []*Operation {
529557
queue = append(queue, next...)
530558
}
531559

532-
return sorted
560+
if len(sorted) != len(plan.Operations) {
561+
var cycled []string
562+
for id, degree := range inDegree {
563+
if degree > 0 {
564+
cycled = append(cycled, id)
565+
}
566+
}
567+
sort.Strings(cycled)
568+
return nil, fmt.Errorf("dependency cycle detected involving operations: %v", cycled)
569+
}
570+
571+
return sorted, nil
533572
}

0 commit comments

Comments
 (0)