chore: do not set default volume driver to local, distinguish behaviour when it's set or not

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 {

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,20 +569,13 @@ 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
+			dm.Source = vol.DockerVolumeName()
-
-			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
-				}
-			}
 		case api.VolumeTypeTmpfs:
 			dm.TmpfsOptions = vol.TmpfsOptions
 		default:

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,10 +125,12 @@ func (v *VolumeSpec) MatchesDockerVolume(vol volume.Volume) bool {
 		return false
 	}
 
+	if spec.VolumeOptions.Driver != nil {
 		volDriver := vol.Driver
 		if volDriver == "" {
 			volDriver = VolumeDriverLocal
 		}
+
 		if spec.VolumeOptions.Driver.Name != volDriver {
 			return false
 		}
@@ -139,6 +138,7 @@ func (v *VolumeSpec) MatchesDockerVolume(vol volume.Volume) bool {
 		if !reflect.DeepEqual(spec.VolumeOptions.Driver.Options, vol.Options) {
 			return false
 		}
+	}
 
 	return true
 }

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{

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 {

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": {

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{
+				opts := volume.CreateOptions{
 					Name: v.Name,
-					Driver:     v.VolumeOptions.Driver.Name,
+				}
-					DriverOpts: v.VolumeOptions.Driver.Options,
+				if v.VolumeOptions != nil {
-				})
+					if v.VolumeOptions.Driver != nil {
-				if err != 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)
 				}
 			}

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)
 	}
 }
 

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)
+			removeServices(t, cli, serviceName)
-			if !errors.Is(err, api.ErrNotFound) {
+			removeVolumes(t, cli, volumeName)
-				assert.NoError(t, err)
-			}
-
-			err = cli.RemoveVolume(ctx, c.Machines[1].Name, volumeName, false)
-			if !errors.Is(err, api.ErrNotFound) {
-				assert.NoError(t, err)
-			}
 		})
 
 		vol, err := cli.CreateVolume(ctx, c.Machines[1].Name, volume.CreateOptions{Name: volumeName})