Skip to content

Commit 07acb31

Browse files
psviderskipsviderski
committed
fix: do not include outdated containers from the removed machine in DNS and Caddy configs
1 parent a26af3b commit 07acb31

4 files changed

Lines changed: 110 additions & 22 deletions

File tree

‎cmd/uncloud/machine/rm.go‎

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -151,20 +151,22 @@ func remove(ctx context.Context, uncli *cli.CLI, nameOrID string, opts removeOpt
151151
fmt.Println()
152152
}
153153

154-
if _, err = client.RemoveMachine(ctx, &pb.RemoveMachineRequest{Id: m.Id}); err != nil {
155-
return fmt.Errorf("remove machine from cluster: %w", err)
156-
}
157-
fmt.Printf("Machine '%s' removed from the cluster.\n", m.Name)
158-
154+
// Initiate reset before removing the machine from the cluster to stop it from updating the cluster store.
155+
// This is still optimistic as Reset only triggers the reset process that runs asynchronously.
159156
if reset && reachable {
160157
_, err = client.MachineClient.Reset(rmCtx, &pb.ResetRequest{})
161158
if err != nil {
162-
fmt.Printf("WARNING: Failed to reset machine: %v\n", err)
159+
tui.PrintWarning(fmt.Sprintf("Failed to reset machine: %v\n", err))
163160
} else {
164161
fmt.Println("Machine reset initiated and will complete in the background.")
165162
}
166163
}
167164

165+
if _, err = client.RemoveMachine(ctx, &pb.RemoveMachineRequest{Id: m.Id}); err != nil {
166+
return fmt.Errorf("remove machine from cluster: %w", err)
167+
}
168+
fmt.Printf("Machine '%s' removed from the cluster.\n", m.Name)
169+
168170
// Remove the connection to the machine from the uncloud config if it exists.
169171
if uncli.Config != nil {
170172
contextName := uncli.ContextOverrideOrCurrent()

‎internal/machine/cluster/cluster.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -334,6 +334,13 @@ func (c *Cluster) RemoveMachine(ctx context.Context, req *pb.RemoveMachineReques
334334
return nil, status.Error(codes.InvalidArgument, "machine ID not set")
335335
}
336336

337+
// Cleanup machine containers from the store that could be left if the machine is unavailable
338+
// removed with --no-reset, or didn't have time to finish propagating changes before resetting.
339+
if err := c.store.DeleteContainers(ctx, store.DeleteOptions{MachineIDs: []string{req.Id}}); err != nil {
340+
slog.Error("Failed to delete container records from the cluster store for the machine being removed.",
341+
"id", req.Id, "err", err)
342+
}
343+
337344
if err := c.store.DeleteMachine(ctx, req.Id); err != nil {
338345
if errors.Is(err, store.ErrMachineNotFound) {
339346
return nil, status.Errorf(codes.NotFound, "machine not found: %s", req.Id)

‎internal/machine/store/container.go‎

Lines changed: 29 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,10 @@ type ServiceIDOrNameOptions struct {
4646
}
4747

4848
type DeleteOptions struct {
49+
// IDs filters containers by their container IDs.
4950
IDs []string
51+
// MachineIDs filters containers by the machine IDs they are running on.
52+
MachineIDs []string
5053
}
5154

5255
// CreateOrUpdateContainer creates a new container record or updates an existing one in the store database.
@@ -100,21 +103,24 @@ func normaliseContainerForStore(ctr *api.ServiceContainer) {
100103
}
101104

102105
// ListContainers returns a list of container records from the store database that match the given options.
106+
// The result excludes orphan containers whose machine is no longer in the cluster.
103107
func (s *Store) ListContainers(ctx context.Context, opts ListOptions) ([]ContainerRecord, error) {
104-
q := sq.Select("id", "container", "machine_id", "sync_status", "updated_at").From("containers").
105-
Where(sq.Eq{"sync_status": SyncStatusSynced})
108+
q := sq.Select("c.id", "c.container", "c.machine_id", "c.sync_status", "c.updated_at").
109+
From("containers c").
110+
Join("machines m ON m.id = c.machine_id").
111+
Where(sq.Eq{"c.sync_status": SyncStatusSynced})
106112

107113
if len(opts.MachineIDs) > 0 {
108-
q = q.Where(sq.Eq{"machine_id": opts.MachineIDs})
114+
q = q.Where(sq.Eq{"c.machine_id": opts.MachineIDs})
109115
}
110116

111117
if opts.ServiceIDOrName.ID != "" || opts.ServiceIDOrName.Name != "" {
112118
var conditions []sq.Sqlizer
113119
if opts.ServiceIDOrName.ID != "" {
114-
conditions = append(conditions, sq.Eq{"service_id": opts.ServiceIDOrName.ID})
120+
conditions = append(conditions, sq.Eq{"c.service_id": opts.ServiceIDOrName.ID})
115121
}
116122
if opts.ServiceIDOrName.Name != "" {
117-
conditions = append(conditions, sq.Eq{"service_name": opts.ServiceIDOrName.Name})
123+
conditions = append(conditions, sq.Eq{"c.service_name": opts.ServiceIDOrName.Name})
118124
}
119125
q = q.Where(sq.Or(conditions))
120126
}
@@ -172,35 +178,42 @@ func (s *Store) ListContainers(ctx context.Context, opts ListOptions) ([]Contain
172178
}
173179

174180
// DeleteContainers deletes container records from the store database that match the given options.
181+
// If no filter is set, all container records are deleted. Filters are combined with AND.
175182
func (s *Store) DeleteContainers(ctx context.Context, opts DeleteOptions) error {
176-
query := "DELETE FROM containers"
177-
var args []any
178-
183+
q := sq.Delete("containers")
179184
if len(opts.IDs) > 0 {
180-
query += " WHERE id IN (?" + strings.Repeat(", ?", len(opts.IDs)-1) + ")"
181-
args = make([]any, len(opts.IDs))
182-
for i, id := range opts.IDs {
183-
args[i] = id
184-
}
185+
q = q.Where(sq.Eq{"id": opts.IDs})
186+
}
187+
if len(opts.MachineIDs) > 0 {
188+
q = q.Where(sq.Eq{"machine_id": opts.MachineIDs})
189+
}
190+
191+
query, args, err := q.ToSql()
192+
if err != nil {
193+
return fmt.Errorf("build query: %w", err)
185194
}
186195

187196
res, err := s.corro.ExecContext(ctx, query, args...)
188197
if err != nil {
189198
return fmt.Errorf("delete query: %w", err)
190199
}
191200
if res.RowsAffected > 0 {
192-
slog.Debug("Container records deleted from store DB.", "ids", opts.IDs, "count", res.RowsAffected)
201+
slog.Debug("Container records deleted from store DB.",
202+
"ids", opts.IDs, "machine_ids", opts.MachineIDs, "count", res.RowsAffected)
193203
}
194204

195205
return nil
196206
}
197207

198208
// SubscribeContainers returns a list of containers and a channel that signals changes to the list. The channel doesn't
199209
// receive any values, it just signals when a container(s) has been added, updated, or deleted in the database.
210+
// The result excludes orphan containers whose machine is no longer in the cluster.
200211
func (s *Store) SubscribeContainers(ctx context.Context) ([]ContainerRecord, <-chan struct{}, error) {
201212
// TODO: figure out whether we need sync_status at all (not used at the moment).
202-
q := sq.Select("id", "container", "machine_id", "sync_status", "updated_at").From("containers").
203-
Where(sq.Eq{"sync_status": SyncStatusSynced})
213+
q := sq.Select("c.id", "c.container", "c.machine_id", "c.sync_status", "c.updated_at").
214+
From("containers c").
215+
Join("machines m ON m.id = c.machine_id").
216+
Where(sq.Eq{"c.sync_status": SyncStatusSynced})
204217
query, args, err := q.ToSql()
205218
if err != nil {
206219
return nil, nil, fmt.Errorf("build query: %w", err)

‎test/e2e/machine_test.go‎

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,9 @@ package e2e
33
import (
44
"context"
55
"errors"
6+
"strings"
67
"testing"
8+
"time"
79

810
"github.com/psviderski/uncloud/internal/machine/api/pb"
911
"github.com/psviderski/uncloud/internal/ucind"
@@ -434,4 +436,68 @@ func TestMachineOperations(t *testing.T) {
434436
}
435437
assert.Equal(t, len(targetMachine.Machine.Network.Endpoints), len(updatedMachine.Network.Endpoints))
436438
})
439+
440+
t.Run("remove machine clears container records from cluster store", func(t *testing.T) {
441+
// Deploy a global service with an HTTP ingress port so the auto-generated Caddyfile lists
442+
// each container's IP as an upstream.
443+
serviceName := "test-machine-rm-cleanup"
444+
spec := api.ServiceSpec{
445+
Name: serviceName,
446+
Mode: api.ServiceModeGlobal,
447+
Container: api.ContainerSpec{
448+
Image: "portainer/pause:latest",
449+
},
450+
Ports: []api.PortSpec{
451+
{
452+
Hostname: "test-machine-rm-cleanup.example.com",
453+
ContainerPort: 8000,
454+
Protocol: api.ProtocolHTTP,
455+
Mode: api.PortModeIngress,
456+
},
457+
},
458+
}
459+
460+
t.Cleanup(func() {
461+
err := cli.RemoveService(ctx, serviceName)
462+
if err != nil && !errors.Is(err, api.ErrNotFound) {
463+
assert.NoError(t, err)
464+
}
465+
})
466+
_, err = cli.NewDeployment(spec, nil).Run(ctx)
467+
require.NoError(t, err)
468+
469+
svc, err := cli.InspectService(ctx, serviceName)
470+
require.NoError(t, err)
471+
472+
containers := serviceContainersByMachine(svc)
473+
474+
removedMachine := c.Machines[2]
475+
removedIP := containers[removedMachine.ID][0].Container.UncloudNetworkIP().String()
476+
keptIP := containers[c.Machines[0].ID][0].Container.UncloudNetworkIP().String()
477+
478+
// The Caddyfile contains both upstream IPs before the machine removal.
479+
require.Eventually(t, func() bool {
480+
cfg, err := cli.Caddy.GetConfig(ctx, nil)
481+
if err != nil {
482+
return false
483+
}
484+
return strings.Contains(cfg.Caddyfile, removedIP) && strings.Contains(cfg.Caddyfile, keptIP)
485+
}, 15*time.Second, 100*time.Millisecond,
486+
"expected Caddyfile to include both the to-be-removed and kept container IPs")
487+
488+
_, err = cli.RemoveMachine(ctx, &pb.RemoveMachineRequest{Id: removedMachine.ID})
489+
require.NoError(t, err)
490+
491+
// After removal, the removed machine's container record should be gone from the cluster store,
492+
// so the Caddy controller regenerates a Caddyfile without that upstream while keeping the
493+
// upstreams for the still-running containers.
494+
require.Eventually(t, func() bool {
495+
cfg, err := cli.Caddy.GetConfig(ctx, nil)
496+
if err != nil {
497+
return false
498+
}
499+
return !strings.Contains(cfg.Caddyfile, removedIP) && strings.Contains(cfg.Caddyfile, keptIP)
500+
}, 15*time.Second, 100*time.Millisecond,
501+
"expected Caddyfile to drop the removed machine's container IP and keep the rest")
502+
})
437503
}

0 commit comments

Comments
 (0)