From 8ee551ca8fef6bf56aa66f15370d3c4c1810c784 Mon Sep 17 00:00:00 2001 From: Pavel Sviderski Date: Sun, 20 Apr 2025 11:15:07 +1000 Subject: [PATCH] cleanup --- pkg/client/deploy/scheduler/volume.go | 114 ------------- pkg/client/deploy/scheduler/volume_test.go | 184 --------------------- test/e2e/service_test.go | 59 +++++++ 3 files changed, 59 insertions(+), 298 deletions(-) diff --git a/pkg/client/deploy/scheduler/volume.go b/pkg/client/deploy/scheduler/volume.go index 0436bf84..e9305989 100644 --- a/pkg/client/deploy/scheduler/volume.go +++ b/pkg/client/deploy/scheduler/volume.go @@ -193,46 +193,6 @@ func (s *VolumeScheduler) Schedule() (map[string][]api.VolumeSpec, error) { } return scheduledVolumes, nil - - //// For each missing volume (should be created only on one machine) - //machineToVolumeSpecs := make(map[string][]api.VolumeSpec) - //for volumeName, serviceNames := range missingVolumes { - // // Get the intersection of candidate machines for all services using this volume - // var intersection []string - // for i, serviceName := range serviceNames { - // if i == 0 { - // intersection = serviceEligibleMachines[serviceName] - // } else { - // intersection = s.intersectMachines(intersection, serviceEligibleMachines[serviceName]) - // } - // } - // - // if len(intersection) == 0 { - // return nil, fmt.Errorf("unable to find a machine where services %v can be placed together to share the missing volume %s", - // serviceNames, volumeName) - // } - // - // // Sort the intersection to ensure deterministic behavior - // sortedIntersection := make([]string, len(intersection)) - // copy(sortedIntersection, intersection) - // slices.Sort(sortedIntersection) - // - // // Choose the first machine in the sorted intersection to create the volume on - // machineID := sortedIntersection[0] - // - // // Get the volume spec for this volume name - // volumeSpec := volumeSpecs[volumeName] - // - // // Add the volume spec to the machine's list - // machineToVolumeSpecs[machineID] = append(machineToVolumeSpecs[machineID], volumeSpec) - // - // // Update the candidate machines for all services using this volume - // for _, serviceName := range serviceNames { - // serviceEligibleMachines[serviceName] = intersection - // } - //} - // - //return machineToVolumeSpecs, nil } // serviceEligibleMachinesWithoutVolumes returns a set of machine IDs where the service can be scheduled @@ -254,77 +214,3 @@ func (s *VolumeScheduler) serviceEligibleMachinesWithoutVolumes(spec api.Service return machineIDs, nil } - -// getAllVolumesAndSpecs returns a map of all volume names used by services and a map of volume names to their specs. -func (s *VolumeScheduler) getAllVolumesAndSpecs() (map[string]struct{}, map[string]api.VolumeSpec) { - volumes := make(map[string]struct{}) - volumeSpecs := make(map[string]api.VolumeSpec) - - for _, serviceSpec := range s.serviceSpecs { - for _, mount := range serviceSpec.Container.VolumeMounts { - if v, ok := serviceSpec.Volume(mount.VolumeName); ok && v.Type == api.VolumeTypeVolume { - volumeName := v.DockerVolumeName() - volumes[volumeName] = struct{}{} - volumeSpecs[volumeName] = v - } - } - } - - return volumes, volumeSpecs -} - -// getVolumeLocations returns a map of volume names to the list of machine IDs where they exist. -func (s *VolumeScheduler) getVolumeLocations(allVolumes map[string]struct{}) map[string][]string { - volumeLocations := make(map[string][]string) - - // Initialize the map with empty slices for all volumes - for volumeName := range allVolumes { - volumeLocations[volumeName] = []string{} - } - - // Populate the map with machine IDs where each volume exists - for _, machine := range s.machines { - for _, vol := range machine.Volumes { - if _, ok := volumeLocations[vol.Name]; ok { - volumeLocations[vol.Name] = append(volumeLocations[vol.Name], machine.Info.Id) - } - } - } - - return volumeLocations -} - -// getVolumeServices returns a map of volume names to the list of service names that use them. -func (s *VolumeScheduler) getVolumeServices(allVolumes map[string]struct{}) map[string][]string { - volumeServices := make(map[string][]string) - - // Initialize the map with empty slices for all volumes - for volumeName := range allVolumes { - volumeServices[volumeName] = []string{} - } - - for _, serviceSpec := range s.serviceSpecs { - serviceName := serviceSpec.Name - for _, mount := range serviceSpec.Container.VolumeMounts { - if v, ok := serviceSpec.Volume(mount.VolumeName); ok && v.Type == api.VolumeTypeVolume { - volumeName := v.DockerVolumeName() - if _, ok := allVolumes[volumeName]; ok { - volumeServices[volumeName] = append(volumeServices[volumeName], serviceName) - } - } - } - } - - return volumeServices -} - -// intersectMachines returns the intersection of two slices of machine IDs. -func (s *VolumeScheduler) intersectMachines(a, b []string) []string { - var result []string - for _, id := range a { - if slices.Contains(b, id) { - result = append(result, id) - } - } - return result -} diff --git a/pkg/client/deploy/scheduler/volume_test.go b/pkg/client/deploy/scheduler/volume_test.go index 0e239c5e..df970898 100644 --- a/pkg/client/deploy/scheduler/volume_test.go +++ b/pkg/client/deploy/scheduler/volume_test.go @@ -782,187 +782,3 @@ func TestVolumeScheduler_Schedule(t *testing.T) { }) } } - -func TestVolumeScheduler_getAllVolumesAndSpecs(t *testing.T) { - serviceSpecs := map[string]api.ServiceSpec{ - "service1": { - Name: "service1", - Container: api.ContainerSpec{ - VolumeMounts: []api.VolumeMount{ - { - VolumeName: "vol1", - ContainerPath: "/data", - }, - }, - }, - Volumes: []api.VolumeSpec{ - { - Name: "vol1", - Type: api.VolumeTypeVolume, - }, - }, - }, - "service2": { - Name: "service2", - Container: api.ContainerSpec{ - VolumeMounts: []api.VolumeMount{ - { - VolumeName: "vol2", - ContainerPath: "/data", - }, - }, - }, - Volumes: []api.VolumeSpec{ - { - Name: "vol2", - Type: api.VolumeTypeVolume, - }, - }, - }, - } - - // Convert map to slice - specsList := make([]api.ServiceSpec, 0, len(serviceSpecs)) - for _, spec := range serviceSpecs { - specsList = append(specsList, spec) - } - scheduler, err := NewVolumeSchedulerWithMachines(nil, specsList) - assert.NoError(t, err) - volumes, specs := scheduler.getAllVolumesAndSpecs() - - assert.Len(t, volumes, 2) - assert.Contains(t, volumes, "vol1") - assert.Contains(t, volumes, "vol2") - - assert.Len(t, specs, 2) - assert.Equal(t, api.VolumeSpec{Name: "vol1", Type: api.VolumeTypeVolume}, specs["vol1"]) - assert.Equal(t, api.VolumeSpec{Name: "vol2", Type: api.VolumeTypeVolume}, specs["vol2"]) -} - -func TestVolumeScheduler_getVolumeLocations(t *testing.T) { - machines := []*Machine{ - { - Info: &pb.MachineInfo{ - Id: "machine1", - Name: "machine1", - }, - Volumes: []volume.Volume{ - { - Name: "vol1", - }, - }, - }, - { - Info: &pb.MachineInfo{ - Id: "machine2", - Name: "machine2", - }, - Volumes: []volume.Volume{ - { - Name: "vol2", - }, - }, - }, - } - - allVolumes := map[string]struct{}{ - "vol1": {}, - "vol2": {}, - "vol3": {}, - } - - scheduler, err := NewVolumeSchedulerWithMachines(machines, nil) - assert.NoError(t, err) - locations := scheduler.getVolumeLocations(allVolumes) - - assert.Len(t, locations, 3) - assert.Equal(t, []string{"machine1"}, locations["vol1"]) - assert.Equal(t, []string{"machine2"}, locations["vol2"]) - assert.Empty(t, locations["vol3"]) -} - -func TestVolumeScheduler_getVolumeServices(t *testing.T) { - serviceSpecs := map[string]api.ServiceSpec{ - "service1": { - Name: "service1", - Container: api.ContainerSpec{ - VolumeMounts: []api.VolumeMount{ - { - VolumeName: "vol1", - ContainerPath: "/data", - }, - }, - }, - Volumes: []api.VolumeSpec{ - { - Name: "vol1", - Type: api.VolumeTypeVolume, - }, - }, - }, - "service2": { - Name: "service2", - Container: api.ContainerSpec{ - VolumeMounts: []api.VolumeMount{ - { - VolumeName: "vol1", - ContainerPath: "/data", - }, - { - VolumeName: "vol2", - ContainerPath: "/data2", - }, - }, - }, - Volumes: []api.VolumeSpec{ - { - Name: "vol1", - Type: api.VolumeTypeVolume, - }, - { - Name: "vol2", - Type: api.VolumeTypeVolume, - }, - }, - }, - } - - allVolumes := map[string]struct{}{ - "vol1": {}, - "vol2": {}, - } - - // Convert map to slice - specsList := make([]api.ServiceSpec, 0, len(serviceSpecs)) - for _, spec := range serviceSpecs { - specsList = append(specsList, spec) - } - scheduler, err := NewVolumeSchedulerWithMachines(nil, specsList) - assert.NoError(t, err) - services := scheduler.getVolumeServices(allVolumes) - - assert.Len(t, services, 2) - assert.ElementsMatch(t, []string{"service1", "service2"}, services["vol1"]) - assert.ElementsMatch(t, []string{"service2"}, services["vol2"]) -} - -func TestVolumeScheduler_intersectMachines(t *testing.T) { - scheduler := &VolumeScheduler{} - - a := []string{"machine1", "machine2", "machine3"} - b := []string{"machine2", "machine3", "machine4"} - - result := scheduler.intersectMachines(a, b) - assert.ElementsMatch(t, []string{"machine2", "machine3"}, result) - - // Empty intersection - c := []string{"machine5", "machine6"} - result = scheduler.intersectMachines(a, c) - assert.Empty(t, result) - - // One empty slice - result = scheduler.intersectMachines(a, []string{}) - assert.Empty(t, result) - result = scheduler.intersectMachines([]string{}, b) - assert.Empty(t, result) -} diff --git a/test/e2e/service_test.go b/test/e2e/service_test.go index 2fe0fb1d..a872f311 100644 --- a/test/e2e/service_test.go +++ b/test/e2e/service_test.go @@ -1388,6 +1388,65 @@ func TestServiceLifecycle(t *testing.T) { assert.Equal(t, spec.Ports, ports) }) + t.Run("3 replicas with volume auto-created", func(t *testing.T) { + t.Parallel() + + name := "test-3-replicas-volume-auto-created" + volumeName := name + t.Cleanup(func() { + err := cli.RemoveService(ctx, name) + if !errors.Is(err, api.ErrNotFound) { + assert.NoError(t, err) + } + + volumes, err := cli.ListVolumes(ctx, &api.VolumeFilter{Names: []string{volumeName}}) + require.NoError(t, err) + for _, v := range volumes { + err = cli.RemoveVolume(ctx, v.MachineID, v.Volume.Name, false) + assert.NoError(t, err) + } + }) + + volumes, err := cli.ListVolumes(ctx, &api.VolumeFilter{Names: []string{volumeName}}) + require.NoError(t, err) + assert.Len(t, volumes, 0, "Volume should not exist before service creation") + + spec := api.ServiceSpec{ + Name: name, + Mode: api.ServiceModeReplicated, + Container: api.ContainerSpec{ + Image: "portainer/pause:latest", + VolumeMounts: []api.VolumeMount{ + { + VolumeName: volumeName, + ContainerPath: "/data", + }, + }, + }, + Volumes: []api.VolumeSpec{ + { + Name: volumeName, + Type: api.VolumeTypeVolume, + }, + }, + } + resp, err := cli.RunService(ctx, spec) + require.NoError(t, err) + + svc, err := cli.InspectService(ctx, resp.ID) + require.NoError(t, err) + assertServiceMatchesSpec(t, svc, spec) + + volumes, err = cli.ListVolumes(ctx, &api.VolumeFilter{Names: []string{volumeName}}) + require.NoError(t, err) + assert.Len(t, volumes, 1, "Volume should be created automatically") + assert.Equal(t, volumeName, volumes[0].Volume.Name) + + machines := serviceMachines(svc) + assert.Equal(t, []string{volumes[0].MachineID}, machines.ToSlice(), + "Replicas should be on the same machine as the volume") + }) + t.Run("global mode", func(t *testing.T) { t.Parallel()