From 143e68cc9563fcebae15de3a84bf0c67f5cc7c76 Mon Sep 17 00:00:00 2001 From: Pavel Sviderski Date: Mon, 21 Apr 2025 16:53:43 +1000 Subject: [PATCH] chore: do not set default volume driver to local, distinguish behaviour when it's set or not --- cmd/uncloud/service/run.go | 3 +- internal/machine/docker/server.go | 28 +++++------ pkg/api/volume.go | 26 +++++----- pkg/client/deploy/container_test.go | 57 ++++++++++++++++++++-- pkg/client/deploy/scheduler/volume.go | 1 + pkg/client/deploy/scheduler/volume_test.go | 28 +++++++++-- pkg/client/service.go | 18 ++++--- test/e2e/assert.go | 11 ++++- test/e2e/service_test.go | 11 +---- 9 files changed, 128 insertions(+), 55 deletions(-) diff --git a/cmd/uncloud/service/run.go b/cmd/uncloud/service/run.go index 7bab51da..9b13e7b2 100644 --- a/cmd/uncloud/service/run.go +++ b/cmd/uncloud/service/run.go @@ -11,7 +11,6 @@ import ( "github.com/psviderski/uncloud/internal/cli" "github.com/psviderski/uncloud/internal/secret" "github.com/psviderski/uncloud/pkg/api" - "github.com/psviderski/uncloud/pkg/client" "github.com/psviderski/uncloud/pkg/client/deploy" "github.com/spf13/cobra" ) @@ -111,7 +110,7 @@ func run(ctx context.Context, uncli *cli.CLI, opts runOptions) error { } defer clusterClient.Close() - var resp client.RunServiceResponse + var resp api.RunServiceResponse err = progress.RunWithTitle(ctx, func(ctx context.Context) error { resp, err = clusterClient.RunService(ctx, spec) if err != nil { diff --git a/internal/machine/docker/server.go b/internal/machine/docker/server.go index ca3df723..0060ea22 100644 --- a/internal/machine/docker/server.go +++ b/internal/machine/docker/server.go @@ -538,9 +538,14 @@ func (s *Server) CreateServiceContainer( } func ToDockerMounts(volumes []api.VolumeSpec, mounts []api.VolumeMount) ([]mount.Mount, error) { + normalisedVolumes := make([]api.VolumeSpec, len(volumes)) + for i, v := range volumes { + normalisedVolumes[i] = v.SetDefaults() + } + dockerMounts := make([]mount.Mount, 0, len(mounts)) for _, m := range mounts { - idx := slices.IndexFunc(volumes, func(v api.VolumeSpec) bool { + idx := slices.IndexFunc(normalisedVolumes, func(v api.VolumeSpec) bool { return v.Name == m.VolumeName }) if idx == -1 { @@ -548,7 +553,7 @@ func ToDockerMounts(volumes []api.VolumeSpec, mounts []api.VolumeMount) ([]mount m.VolumeName) } - vol := volumes[idx] + vol := normalisedVolumes[idx] if err := vol.Validate(); err != nil { return nil, fmt.Errorf("invalid volume: %w", err) } @@ -564,19 +569,12 @@ func ToDockerMounts(volumes []api.VolumeSpec, mounts []api.VolumeMount) ([]mount dm.Source = vol.BindOptions.HostPath dm.BindOptions = toDockerBindOptions(vol.BindOptions) case api.VolumeTypeVolume: - dm.Source = vol.Name - - if vol.VolumeOptions != nil { - dm.VolumeOptions = &mount.VolumeOptions{ - NoCopy: vol.VolumeOptions.NoCopy, - Labels: vol.VolumeOptions.Labels, - Subpath: vol.VolumeOptions.SubPath, - DriverConfig: vol.VolumeOptions.Driver, - } - - if vol.VolumeOptions.Name != "" { - dm.Source = vol.VolumeOptions.Name - } + dm.Source = vol.DockerVolumeName() + dm.VolumeOptions = &mount.VolumeOptions{ + NoCopy: vol.VolumeOptions.NoCopy, + Labels: vol.VolumeOptions.Labels, + Subpath: vol.VolumeOptions.SubPath, + DriverConfig: vol.VolumeOptions.Driver, } case api.VolumeTypeTmpfs: dm.TmpfsOptions = vol.TmpfsOptions diff --git a/pkg/api/volume.go b/pkg/api/volume.go index 76ba0714..4697bafc 100644 --- a/pkg/api/volume.go +++ b/pkg/api/volume.go @@ -78,9 +78,6 @@ func (v *VolumeSpec) SetDefaults() VolumeSpec { if spec.VolumeOptions == nil { spec.VolumeOptions = &VolumeOptions{} } - if spec.VolumeOptions.Driver == nil { - spec.VolumeOptions.Driver = &mount.Driver{Name: VolumeDriverLocal} - } if spec.VolumeOptions.Name == "" { spec.VolumeOptions.Name = spec.Name } @@ -117,7 +114,7 @@ func (v *VolumeSpec) Equals(other VolumeSpec) bool { } // MatchesDockerVolume checks if this VolumeSpec is compatible with the given named Docker volume. -// In other words, it checks if the spec could be used to create the volume. +// In other words, it checks if the spec could be used to mount the volume. func (v *VolumeSpec) MatchesDockerVolume(vol volume.Volume) bool { if v.Type != VolumeTypeVolume { return false @@ -128,16 +125,19 @@ func (v *VolumeSpec) MatchesDockerVolume(vol volume.Volume) bool { return false } - volDriver := vol.Driver - if volDriver == "" { - volDriver = VolumeDriverLocal - } - if spec.VolumeOptions.Driver.Name != volDriver { - return false - } + if spec.VolumeOptions.Driver != nil { + volDriver := vol.Driver + if volDriver == "" { + volDriver = VolumeDriverLocal + } - if !reflect.DeepEqual(spec.VolumeOptions.Driver.Options, vol.Options) { - return false + if spec.VolumeOptions.Driver.Name != volDriver { + return false + } + + if !reflect.DeepEqual(spec.VolumeOptions.Driver.Options, vol.Options) { + return false + } } return true diff --git a/pkg/client/deploy/container_test.go b/pkg/client/deploy/container_test.go index c56a42d0..3edd1f24 100644 --- a/pkg/client/deploy/container_test.go +++ b/pkg/client/deploy/container_test.go @@ -186,15 +186,66 @@ func TestEvalContainerSpecChange_Volumes(t *testing.T) { Type: api.VolumeTypeVolume, VolumeOptions: &api.VolumeOptions{ Name: "data", - Driver: &mount.Driver{ - Name: "local", - }, }, }, }, }, expected: ContainerUpToDate, }, + { + name: "change volume driver to local", + current: api.ServiceSpec{ + Volumes: []api.VolumeSpec{ + { + Name: "data", + Type: api.VolumeTypeVolume, + }, + }, + }, + new: api.ServiceSpec{ + Volumes: []api.VolumeSpec{ + { + Name: "data", + Type: api.VolumeTypeVolume, + VolumeOptions: &api.VolumeOptions{ + Name: "data", + Driver: &mount.Driver{ + Name: api.VolumeDriverLocal, + }, + }, + }, + }, + }, + // TODO: this doesn't really require a recreate, only a spec update would be sufficient. + expected: ContainerNeedsRecreate, + }, + { + name: "change volume driver to custom", + current: api.ServiceSpec{ + Volumes: []api.VolumeSpec{ + { + Name: "data", + Type: api.VolumeTypeVolume, + }, + }, + }, + new: api.ServiceSpec{ + Volumes: []api.VolumeSpec{ + { + Name: "data", + Type: api.VolumeTypeVolume, + VolumeOptions: &api.VolumeOptions{ + Name: "data", + Driver: &mount.Driver{ + Name: "custom", + }, + }, + }, + }, + }, + // TODO: this doesn't really require a recreate, only a spec update would be sufficient. + expected: ContainerNeedsRecreate, + }, { name: "change bind option CreateHostPath", current: api.ServiceSpec{ diff --git a/pkg/client/deploy/scheduler/volume.go b/pkg/client/deploy/scheduler/volume.go index 33f3678e..403ff068 100644 --- a/pkg/client/deploy/scheduler/volume.go +++ b/pkg/client/deploy/scheduler/volume.go @@ -138,6 +138,7 @@ func (s *VolumeScheduler) Schedule() (map[string][]api.VolumeSpec, error) { // For each volume that exists on any machine(s) (which shouldn't be created), intersect each service's // eligible machines that use the volume with the machines the volume is located on. + // // Service name -> list of processed volume names (quoted) to format the error message. quotedServiceVolumes := make(map[string][]string) for volumeName, volumeMachines := range s.existingVolumeMachines { diff --git a/pkg/client/deploy/scheduler/volume_test.go b/pkg/client/deploy/scheduler/volume_test.go index 171f0eed..ff5576f8 100644 --- a/pkg/client/deploy/scheduler/volume_test.go +++ b/pkg/client/deploy/scheduler/volume_test.go @@ -521,6 +521,11 @@ func TestVolumeScheduler_Schedule(t *testing.T) { { Name: "vol2", Type: api.VolumeTypeVolume, + VolumeOptions: &api.VolumeOptions{ + Driver: &mount.Driver{ + Name: api.VolumeDriverLocal, + }, + }, }, { Name: "vol3", @@ -547,15 +552,17 @@ func TestVolumeScheduler_Schedule(t *testing.T) { { Name: "vol2", Type: api.VolumeTypeVolume, + VolumeOptions: &api.VolumeOptions{ + Driver: &mount.Driver{ + Name: api.VolumeDriverLocal, + }, + }, }, { Name: "vol4-alias", Type: api.VolumeTypeVolume, VolumeOptions: &api.VolumeOptions{ Name: "vol4", - Driver: &mount.Driver{ - Name: api.VolumeDriverLocal, - }, }, }, }, @@ -570,7 +577,7 @@ func TestVolumeScheduler_Schedule(t *testing.T) { ContainerPath: "/data2", }, { - VolumeName: "vol5", + VolumeName: "vol5-with-driver", ContainerPath: "/data5", }, }, @@ -587,8 +594,14 @@ func TestVolumeScheduler_Schedule(t *testing.T) { }, }, { - Name: "vol5", + Name: "vol5-with-driver", Type: api.VolumeTypeVolume, + VolumeOptions: &api.VolumeOptions{ + Name: "vol5", + Driver: &mount.Driver{ + Name: api.VolumeDriverLocal, + }, + }, }, }, }, @@ -598,6 +611,11 @@ func TestVolumeScheduler_Schedule(t *testing.T) { { Name: "vol5", Type: api.VolumeTypeVolume, + VolumeOptions: &api.VolumeOptions{ + Driver: &mount.Driver{ + Name: api.VolumeDriverLocal, + }, + }, }, }, "machine3": { diff --git a/pkg/client/service.go b/pkg/client/service.go index 415348f4..2984ced1 100644 --- a/pkg/client/service.go +++ b/pkg/client/service.go @@ -51,12 +51,18 @@ func (cli *Client) RunService(ctx context.Context, spec api.ServiceSpec) (api.Ru // Create the missing volumes on the scheduled machines. for machineID, volumes := range scheduledVolumes { for _, v := range volumes { - _, err = cli.CreateVolume(ctx, machineID, volume.CreateOptions{ - Name: v.Name, - Driver: v.VolumeOptions.Driver.Name, - DriverOpts: v.VolumeOptions.Driver.Options, - }) - if err != nil { + opts := volume.CreateOptions{ + Name: v.Name, + } + if v.VolumeOptions != nil { + if v.VolumeOptions.Driver != nil { + opts.Driver = v.VolumeOptions.Driver.Name + opts.DriverOpts = v.VolumeOptions.Driver.Options + } + opts.Labels = v.VolumeOptions.Labels + } + + if _, err = cli.CreateVolume(ctx, machineID, opts); err != nil { return resp, fmt.Errorf("create volume '%s': %w", v.Name, err) } } diff --git a/test/e2e/assert.go b/test/e2e/assert.go index 5f8ac2c5..ea2600dc 100644 --- a/test/e2e/assert.go +++ b/test/e2e/assert.go @@ -1,7 +1,6 @@ package e2e import ( - "reflect" "slices" "strconv" "strings" @@ -104,7 +103,15 @@ func assertContainerMountsMatchSpec(t *testing.T, mounts []mount.Mount, spec api assert.Len(t, mounts, len(expectedMounts), "Expected %d mounts", len(expectedMounts)) for i, m := range mounts { - assert.True(t, reflect.DeepEqual(m, expectedMounts[i]), "Expected mount type=%s,src=%s,dst=%s to match spec") + // The mount generated from the spec may not define the driver which means to use the default driver + // if creating a new volume or mounting an existing one no matter what its driver is. + // So skip driver comparison if the driver is not set in the spec. + if expectedMounts[i].VolumeOptions != nil && expectedMounts[i].VolumeOptions.DriverConfig == nil && + m.VolumeOptions != nil { + expectedMounts[i].VolumeOptions.DriverConfig = m.VolumeOptions.DriverConfig + } + assert.Equal(t, expectedMounts[i], m, + "Expected mount type=%s,src=%s,dst=%s to match spec", m.Type, m.Source, m.Target) } } diff --git a/test/e2e/service_test.go b/test/e2e/service_test.go index 0c45fd45..ac03cd1c 100644 --- a/test/e2e/service_test.go +++ b/test/e2e/service_test.go @@ -595,15 +595,8 @@ func TestDeployment(t *testing.T) { serviceName := "test-replicated-with-volume-single-machine" volumeName := serviceName t.Cleanup(func() { - err := cli.RemoveService(ctx, serviceName) - if !errors.Is(err, api.ErrNotFound) { - assert.NoError(t, err) - } - - err = cli.RemoveVolume(ctx, c.Machines[1].Name, volumeName, false) - if !errors.Is(err, api.ErrNotFound) { - assert.NoError(t, err) - } + removeServices(t, cli, serviceName) + removeVolumes(t, cli, volumeName) }) vol, err := cli.CreateVolume(ctx, c.Machines[1].Name, volume.CreateOptions{Name: volumeName})