From 309adbdd019e5ae8d64e9a9177523a40bf2ebc96 Mon Sep 17 00:00:00 2001 From: Maysun J Faisal Date: Thu, 15 Apr 2021 16:38:19 -0400 Subject: [PATCH] Refactor storage utils for devfile/library migration Signed-off-by: Maysun J Faisal --- pkg/devfile/adapters/common/types.go | 13 - pkg/devfile/adapters/common/utils.go | 38 -- pkg/devfile/adapters/common/utils_test.go | 155 --------- .../adapters/docker/component/adapter.go | 24 +- .../adapters/docker/component/utils.go | 17 - .../adapters/docker/storage/adapter.go | 24 -- .../adapters/docker/storage/adapter_test.go | 1 - pkg/devfile/adapters/docker/storage/utils.go | 115 ------ .../adapters/docker/storage/utils_test.go | 328 ------------------ .../adapters/kubernetes/component/adapter.go | 21 +- .../adapters/kubernetes/storage/utils.go | 79 +++-- .../adapters/kubernetes/storage/utils_test.go | 281 +++++++++------ pkg/envinfo/storage.go | 3 +- 13 files changed, 247 insertions(+), 852 deletions(-) delete mode 100644 pkg/devfile/adapters/docker/storage/adapter.go delete mode 100644 pkg/devfile/adapters/docker/storage/adapter_test.go diff --git a/pkg/devfile/adapters/common/types.go b/pkg/devfile/adapters/common/types.go index 1749d00204f..5c035d93ea9 100644 --- a/pkg/devfile/adapters/common/types.go +++ b/pkg/devfile/adapters/common/types.go @@ -15,19 +15,6 @@ type AdapterContext struct { Devfile devfileParser.DevfileObj // Devfile is the object returned by the Devfile parser } -// DevfileVolume is a struct for Devfile volume that is common to all the adapters -type DevfileVolume struct { - Name string - ContainerPath string - Size string -} - -// Storage is a struct that is common to all the adapters -type Storage struct { - Name string - Volume DevfileVolume -} - // PushParameters is a struct containing the parameters to be used when pushing to a devfile component type PushParameters struct { Path string // Path refers to the parent folder containing the source code to push up to a component diff --git a/pkg/devfile/adapters/common/utils.go b/pkg/devfile/adapters/common/utils.go index d97b19fe580..355f3fd9a88 100644 --- a/pkg/devfile/adapters/common/utils.go +++ b/pkg/devfile/adapters/common/utils.go @@ -7,9 +7,7 @@ import ( "k8s.io/klog" devfilev1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" - devfileParser "github.com/devfile/library/pkg/devfile/parser" parsercommon "github.com/devfile/library/pkg/devfile/parser/data/v2/common" - "github.com/openshift/odo/pkg/envinfo" ) // PredefinedDevfileCommands encapsulates constants for predefined devfile commands @@ -132,42 +130,6 @@ func getCommandsByGroup(commands []devfilev1.Command, groupType devfilev1.Comman return filteredCommands } -// GetVolumes iterates through the components in the devfile and returns a map of container name to the devfile volumes -func GetVolumes(devfileObj devfileParser.DevfileObj) (map[string][]DevfileVolume, error) { - containerComponents, err := devfileObj.Data.GetDevfileContainerComponents(parsercommon.DevfileOptions{}) - if err != nil { - return nil, err - } - volumeComponents, err := devfileObj.Data.GetDevfileVolumeComponents(parsercommon.DevfileOptions{}) - if err != nil { - return nil, err - } - - // containerNameToVolumes is a map of the Devfile container name to the Devfile container Volumes - containerNameToVolumes := make(map[string][]DevfileVolume) - for _, containerComp := range containerComponents { - for _, volumeMount := range containerComp.Container.VolumeMounts { - size := DefaultVolumeSize - - // check if there is a volume component name against the container component volume mount name - for _, volumeComp := range volumeComponents { - if volumeComp.Name == volumeMount.Name && len(volumeComp.Volume.Size) > 0 { - // If there is a volume size mentioned in the devfile, use it - size = volumeComp.Volume.Size - } - } - - vol := DevfileVolume{ - Name: volumeMount.Name, - ContainerPath: envinfo.GetVolumeMountPath(volumeMount), - Size: size, - } - containerNameToVolumes[containerComp.Name] = append(containerNameToVolumes[containerComp.Name], vol) - } - } - return containerNameToVolumes, nil -} - // IsRestartRequired checks if restart required for run command func IsRestartRequired(hotReload bool, runModeChanged bool) bool { if runModeChanged || !hotReload { diff --git a/pkg/devfile/adapters/common/utils_test.go b/pkg/devfile/adapters/common/utils_test.go index 94a31281763..e8c6b784909 100644 --- a/pkg/devfile/adapters/common/utils_test.go +++ b/pkg/devfile/adapters/common/utils_test.go @@ -13,161 +13,6 @@ import ( "github.com/devfile/library/pkg/testingutil" ) -func TestGetVolumes(t *testing.T) { - - size := "4Gi" - - tests := []struct { - name string - component []devfilev1.Component - wantContainerNameToVolumes map[string][]DevfileVolume - wantErr bool - }{ - { - name: "Case 1: Valid devfile with container referencing a volume component", - component: []devfilev1.Component{testingutil.GetFakeContainerComponent("comp1"), testingutil.GetFakeVolumeComponent("myvolume1", size)}, - wantContainerNameToVolumes: map[string][]DevfileVolume{ - "comp1": { - { - Name: "myvolume1", - Size: size, - ContainerPath: "/my/volume/mount/path1", - }, - }, - }, - }, - { - name: "Case 2: Valid devfile with container referencing multiple volume components", - component: []devfilev1.Component{ - testingutil.GetFakeVolumeComponent("myvolume1", size), - testingutil.GetFakeVolumeComponent("myvolume2", size), - testingutil.GetFakeVolumeComponent("myvolume3", size), - { - Name: "mycontainer", - ComponentUnion: devfilev1.ComponentUnion{ - Container: &devfilev1.ContainerComponent{ - Container: devfilev1.Container{ - Image: "image", - VolumeMounts: []devfilev1.VolumeMount{ - { - Name: "myvolume1", - Path: "/myvolume1", - }, - { - Name: "myvolume2", - Path: "/myvolume2", - }, - }, - }, - }, - }, - }, - }, - wantContainerNameToVolumes: map[string][]DevfileVolume{ - "mycontainer": { - { - Name: "myvolume1", - Size: size, - ContainerPath: "/myvolume1", - }, - { - Name: "myvolume2", - Size: size, - ContainerPath: "/myvolume2", - }, - }, - }, - }, - { - name: "Case 3: Valid devfile with container referencing no volume component", - component: []devfilev1.Component{testingutil.GetFakeContainerComponent("comp1"), testingutil.GetFakeVolumeComponent("myvolume2", size)}, - wantContainerNameToVolumes: map[string][]DevfileVolume{ - "comp1": { - { - Name: "myvolume1", - Size: "1Gi", - ContainerPath: "/my/volume/mount/path1", - }, - }, - }, - }, - { - name: "Case 4: Valid devfile with no container volume mounts", - component: []devfilev1.Component{ - testingutil.GetFakeVolumeComponent("myvolume2", size), - { - Name: "mycontainer", - ComponentUnion: devfilev1.ComponentUnion{ - Container: &devfilev1.ContainerComponent{ - Container: devfilev1.Container{ - Image: "image", - }, - }, - }, - }, - }, - wantContainerNameToVolumes: map[string][]DevfileVolume{}, - }, - { - name: "Case 5: Valid devfile with container referencing no volume mount path", - component: []devfilev1.Component{ - testingutil.GetFakeVolumeComponent("myvolume1", size), - { - Name: "mycontainer", - ComponentUnion: devfilev1.ComponentUnion{ - Container: &devfilev1.ContainerComponent{ - Container: devfilev1.Container{ - Image: "image", - VolumeMounts: []devfilev1.VolumeMount{ - { - Name: "myvolume1", - }, - }, - }, - }, - }, - }, - }, - wantContainerNameToVolumes: map[string][]DevfileVolume{ - "mycontainer": { - { - Name: "myvolume1", - Size: "4Gi", - ContainerPath: "/myvolume1", - }, - }, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - devObj := devfileParser.DevfileObj{ - Data: func() data.DevfileData { - devfileData, err := data.NewDevfileData(string(data.APIVersion200)) - if err != nil { - t.Error(err) - } - err = devfileData.AddComponents(tt.component) - if err != nil { - t.Error(err) - } - return devfileData - }(), - } - - containerNameToVolumes, err := GetVolumes(devObj) - if (err != nil) != tt.wantErr { - t.Errorf("GetVolumes() error = %v, wantErr %v", err, tt.wantErr) - } - - if !reflect.DeepEqual(containerNameToVolumes, tt.wantContainerNameToVolumes) { - t.Errorf("TestGetVolumes error - got %v wanted %v", containerNameToVolumes, tt.wantContainerNameToVolumes) - } - }) - } - -} - func TestIsEnvPresent(t *testing.T) { envName := "myenv" diff --git a/pkg/devfile/adapters/docker/component/adapter.go b/pkg/devfile/adapters/docker/component/adapter.go index 872e295ffd5..b8503f77dba 100644 --- a/pkg/devfile/adapters/docker/component/adapter.go +++ b/pkg/devfile/adapters/docker/component/adapter.go @@ -13,7 +13,6 @@ import ( devfilev1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" "github.com/openshift/odo/pkg/devfile/adapters/common" - "github.com/openshift/odo/pkg/devfile/adapters/docker/storage" "github.com/openshift/odo/pkg/devfile/adapters/docker/utils" "github.com/openshift/odo/pkg/lclient" "github.com/openshift/odo/pkg/log" @@ -33,14 +32,11 @@ type Adapter struct { Client lclient.Client *common.GenericAdapter - containerNameToVolumes map[string][]common.DevfileVolume - uniqueStorage []common.Storage - volumeNameToDockerVolName map[string]string - devfileBuildCmd string - devfileRunCmd string - supervisordVolumeName string - projectVolumeName string - containers []types.Container + devfileBuildCmd string + devfileRunCmd string + supervisordVolumeName string + projectVolumeName string + containers []types.Container } // getPod lazily records and retrieves the containers associated with the component associated with this adapter @@ -87,16 +83,6 @@ func (a Adapter) Push(parameters common.PushParameters) (err error) { return errors.Wrapf(err, "unable to determine if component %s exists", a.ComponentName) } - // Process the volumes defined in the devfile - a.containerNameToVolumes, err = common.GetVolumes(a.Devfile) - if err != nil { - return err - } - a.uniqueStorage, a.volumeNameToDockerVolName, err = storage.ProcessVolumes(&a.Client, a.ComponentName, a.containerNameToVolumes) - if err != nil { - return errors.Wrapf(err, "unable to process volumes for component %s", a.ComponentName) - } - a.devfileBuildCmd = parameters.DevfileBuildCmd a.devfileRunCmd = parameters.DevfileRunCmd diff --git a/pkg/devfile/adapters/docker/component/utils.go b/pkg/devfile/adapters/docker/component/utils.go index 477db776853..03bf05a65ba 100644 --- a/pkg/devfile/adapters/docker/component/utils.go +++ b/pkg/devfile/adapters/docker/component/utils.go @@ -42,15 +42,6 @@ func (a Adapter) createComponent() (err error) { // Loop over each container component and start a container for it for _, comp := range containerComponents { var dockerVolumeMounts []mount.Mount - for _, vol := range a.containerNameToVolumes[comp.Name] { - - volMount := mount.Mount{ - Type: mount.TypeVolume, - Source: a.volumeNameToDockerVolName[vol.Name], - Target: vol.ContainerPath, - } - dockerVolumeMounts = append(dockerVolumeMounts, volMount) - } err = a.pullAndStartContainer(dockerVolumeMounts, comp) if err != nil { return errors.Wrapf(err, "unable to pull and start container %s for component %s", comp.Name, componentName) @@ -83,14 +74,6 @@ func (a Adapter) updateComponent() (componentExists bool, err error) { } var dockerVolumeMounts []mount.Mount - for _, vol := range a.containerNameToVolumes[comp.Name] { - volMount := mount.Mount{ - Type: mount.TypeVolume, - Source: a.volumeNameToDockerVolName[vol.Name], - Target: vol.ContainerPath, - } - dockerVolumeMounts = append(dockerVolumeMounts, volMount) - } if len(containers) == 0 { log.Infof("\nCreating Docker resources for component %s", a.ComponentName) diff --git a/pkg/devfile/adapters/docker/storage/adapter.go b/pkg/devfile/adapters/docker/storage/adapter.go deleted file mode 100644 index 3b5298d2fff..00000000000 --- a/pkg/devfile/adapters/docker/storage/adapter.go +++ /dev/null @@ -1,24 +0,0 @@ -package storage - -import ( - "github.com/openshift/odo/pkg/devfile/adapters/common" - "github.com/openshift/odo/pkg/lclient" -) - -// Adapter is a storage adapter implementation for Kubernetes -type Adapter struct { - Client lclient.Client - common.AdapterContext -} - -// Create creates the component pvc storage if it does not exist -func (a *Adapter) Create(storages []common.Storage) (err error) { - - // createComponentStorage creates PVC from the unique Devfile volumes if it does not exist - err = CreateComponentStorage(&a.Client, storages, a.ComponentName) - if err != nil { - return err - } - - return -} diff --git a/pkg/devfile/adapters/docker/storage/adapter_test.go b/pkg/devfile/adapters/docker/storage/adapter_test.go deleted file mode 100644 index 82be0547ed0..00000000000 --- a/pkg/devfile/adapters/docker/storage/adapter_test.go +++ /dev/null @@ -1 +0,0 @@ -package storage diff --git a/pkg/devfile/adapters/docker/storage/utils.go b/pkg/devfile/adapters/docker/storage/utils.go index 458833ac19c..72ee3ba1df5 100644 --- a/pkg/devfile/adapters/docker/storage/utils.go +++ b/pkg/devfile/adapters/docker/storage/utils.go @@ -3,57 +3,13 @@ package storage import ( "fmt" - "github.com/docker/docker/api/types" "github.com/pkg/errors" - "k8s.io/klog" - "github.com/openshift/odo/pkg/devfile/adapters/common" - "github.com/openshift/odo/pkg/lclient" "github.com/openshift/odo/pkg/util" ) const volNameMaxLength = 45 -// CreateComponentStorage creates a Docker volume with the given list of storages if it does not exist, else it uses the existing volume -func CreateComponentStorage(Client *lclient.Client, storages []common.Storage, componentName string) (err error) { - - for _, storage := range storages { - volumeName := storage.Volume.Name - dockerVolName := storage.Name - - existingDockerVolName, err := GetExistingVolume(Client, volumeName, componentName) - if err != nil { - return err - } - - if len(existingDockerVolName) == 0 { - klog.V(2).Infof("Creating a Docker volume for %v", volumeName) - _, err := Create(Client, volumeName, componentName, dockerVolName) - if err != nil { - return errors.Wrapf(err, "Error creating Docker volume for "+volumeName) - } - } - } - - return -} - -// Create creates the Docker volume for the given volume name and component name -func Create(Client *lclient.Client, name, componentName, dockerVolName string) (*types.Volume, error) { - - labels := map[string]string{ - "component": componentName, - "storage-name": name, - } - - klog.V(2).Infof("Creating a Docker volume with name %v and labels %v", dockerVolName, labels) - vol, err := Client.CreateVolume(dockerVolName, labels) - if err != nil { - return nil, errors.Wrap(err, "unable to create Docker volume") - } - return &vol, nil -} - // GenerateVolName generates a Docker volume name from the Devfile volume name and component name func GenerateVolName(volName, componentName string) (string, error) { @@ -72,74 +28,3 @@ func GenerateVolName(volName, componentName string) (string, error) { return dockerVolName, nil } - -// GetExistingVolume checks if a Docker volume is present and return the name if it exists -func GetExistingVolume(Client *lclient.Client, volumeName, componentName string) (string, error) { - - volumeLabels := map[string]string{ - "component": componentName, - "storage-name": volumeName, - } - - klog.V(2).Infof("Checking Docker volume for volume %v and labels %v\n", volumeName, volumeLabels) - - vols, err := Client.GetVolumesByLabel(volumeLabels) - if err != nil { - return "", errors.Wrapf(err, "Unable to get Docker volume with selectors %v", volumeLabels) - } - if len(vols) == 1 { - klog.V(2).Infof("Found an existing Docker volume for volume %v and labels %v\n", volumeName, volumeLabels) - existingVolume := vols[0] - return existingVolume.Name, nil - } else if len(vols) == 0 { - return "", nil - } else { - err = errors.New("More than 1 Docker volume found") - return "", err - } -} - -// ProcessVolumes takes in a list of component volumes and for each unique volume in the devfile, creates a Docker volume name for it -// It returns a list of unique volumes, a mapping of devfile volume names to docker volume names, and an error if applicable -func ProcessVolumes(client *lclient.Client, componentName string, containerNameToVolumes map[string][]common.DevfileVolume) ([]common.Storage, map[string]string, error) { - var uniqueStorages []common.Storage - volumeNameToDockerVolName := make(map[string]string) - processedVolumes := make(map[string]bool) - - // Get a list of all the unique volume names and generate their Docker volume names - // we do not use the volume components which are unique here because - // not all volume components maybe referenced by a container component. - // We only want to create volumes which are going to be used by a container - for _, volumes := range containerNameToVolumes { - for _, vol := range volumes { - if _, ok := processedVolumes[vol.Name]; !ok { - processedVolumes[vol.Name] = true - - // Generate the volume Names - klog.V(2).Infof("Generating Docker volumes name for %v", vol.Name) - generatedDockerVolName, err := GenerateVolName(vol.Name, componentName) - if err != nil { - return nil, nil, err - } - - // Check if we have an existing volume with the labels, overwrite the generated name with the existing name if present - existingVolName, err := GetExistingVolume(client, vol.Name, componentName) - if err != nil { - return nil, nil, err - } - if len(existingVolName) > 0 { - klog.V(2).Infof("Found an existing Docker volume for %v, volume %v will be re-used", vol.Name, existingVolName) - generatedDockerVolName = existingVolName - } - - dockerVol := common.Storage{ - Name: generatedDockerVolName, - Volume: vol, - } - uniqueStorages = append(uniqueStorages, dockerVol) - volumeNameToDockerVolName[vol.Name] = generatedDockerVolName - } - } - } - return uniqueStorages, volumeNameToDockerVolName, nil -} diff --git a/pkg/devfile/adapters/docker/storage/utils_test.go b/pkg/devfile/adapters/docker/storage/utils_test.go index 114d3a7c731..932510f0f85 100644 --- a/pkg/devfile/adapters/docker/storage/utils_test.go +++ b/pkg/devfile/adapters/docker/storage/utils_test.go @@ -3,336 +3,8 @@ package storage import ( "strings" "testing" - - "github.com/openshift/odo/pkg/devfile/adapters/common" - "github.com/openshift/odo/pkg/lclient" ) -func TestCreateComponentStorage(t *testing.T) { - fakeClient := lclient.FakeNew() - fakeErrorClient := lclient.FakeErrorNew() - - testComponentName := "test" - volNames := [...]string{"vol1", "vol2"} - volSize := "5Gi" - - tests := []struct { - name string - storages []common.Storage - client *lclient.Client - wantErr bool - }{ - { - name: "Case 1: Multiple volumes defined, no Docker client error", - storages: []common.Storage{ - { - Name: "vol1", - Volume: common.DevfileVolume{ - Name: volNames[0], - Size: volSize, - }, - }, - { - Name: "vol2", - Volume: common.DevfileVolume{ - Name: volNames[1], - Size: volSize, - }, - }, - }, - client: fakeClient, - wantErr: false, - }, - { - name: "Case 1: Multiple volumes defined, Docker client error", - storages: []common.Storage{ - { - Name: "vol1", - Volume: common.DevfileVolume{ - Name: volNames[0], - Size: volSize, - }, - }, - { - Name: "vol2", - Volume: common.DevfileVolume{ - Name: volNames[1], - Size: volSize, - }, - }, - }, - client: fakeErrorClient, - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - err := CreateComponentStorage(tt.client, tt.storages, testComponentName) - if !tt.wantErr == (err != nil) { - t.Errorf("Storage adapter create unexpected error %v, wantErr %v", err, tt.wantErr) - } - }) - } - -} - -func TestStorageCreate(t *testing.T) { - fakeClient := lclient.FakeNew() - fakeErrorClient := lclient.FakeErrorNew() - - testComponentName := "test" - volNames := [...]string{"vol1", "vol2"} - volSize := "5Gi" - - tests := []struct { - name string - storage common.Storage - client *lclient.Client - wantErr bool - }{ - { - name: "Case 1: Valid volume, no Docker client error", - storage: common.Storage{ - Name: "vol1", - Volume: common.DevfileVolume{ - Name: volNames[0], - Size: volSize, - }, - }, - client: fakeClient, - wantErr: false, - }, - { - name: "Case 2: Docker client error", - storage: common.Storage{ - Name: "vol-name", - Volume: common.DevfileVolume{ - Name: volNames[0], - Size: volSize, - }, - }, - client: fakeErrorClient, - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - - // Create one of the test volumes - _, err := Create(tt.client, tt.storage.Volume.Name, testComponentName, tt.storage.Name) - if !tt.wantErr == (err != nil) { - t.Errorf("Docker volume create unexpected error %v, wantErr %v", err, tt.wantErr) - } - - }) - } - -} - -func TestProcessVolumes(t *testing.T) { - fakeClient := lclient.FakeNew() - fakeErrorClient := lclient.FakeErrorNew() - - testComponentName := "test" - volumeNames := []string{"vol1", "vol2", "vol3"} - volumePaths := []string{"/path1", "/path2", "/path3"} - volumeSizes := []string{"1Gi", "2Gi", "3Gi"} - tests := []struct { - name string - client *lclient.Client - aliasVolumeMapping map[string][]common.DevfileVolume - wantErr bool - wantStorage []common.Storage - }{ - { - name: "Case 1: No volumes defined", - aliasVolumeMapping: nil, - client: fakeClient, - wantStorage: nil, - wantErr: false, - }, - { - name: "Case 2: One volume defined, one component", - aliasVolumeMapping: map[string][]common.DevfileVolume{ - "some-component": []common.DevfileVolume{ - { - Name: volumeNames[0], - ContainerPath: volumePaths[0], - Size: volumeSizes[0], - }, - }, - }, - client: fakeClient, - wantStorage: []common.Storage{ - { - Volume: common.DevfileVolume{ - Name: volumeNames[0], - ContainerPath: volumePaths[0], - Size: volumeSizes[0], - }, - }, - }, - wantErr: false, - }, - { - name: "Case 3: Multiple volumes defined, one component", - aliasVolumeMapping: map[string][]common.DevfileVolume{ - "some-component": []common.DevfileVolume{ - { - Name: volumeNames[0], - ContainerPath: volumePaths[0], - Size: volumeSizes[0], - }, - { - Name: volumeNames[1], - ContainerPath: volumePaths[1], - Size: volumeSizes[1], - }, - { - Name: volumeNames[2], - ContainerPath: volumePaths[2], - Size: volumeSizes[2], - }, - }, - }, - client: fakeClient, - wantStorage: []common.Storage{ - { - Volume: common.DevfileVolume{ - Name: volumeNames[0], - ContainerPath: volumePaths[0], - Size: volumeSizes[0], - }, - }, - { - Volume: common.DevfileVolume{ - Name: volumeNames[1], - ContainerPath: volumePaths[1], - Size: volumeSizes[1], - }, - }, - { - Volume: common.DevfileVolume{ - Name: volumeNames[2], - ContainerPath: volumePaths[2], - Size: volumeSizes[2], - }, - }, - }, - wantErr: false, - }, - { - name: "Case 4: Multiple volumes defined, multiple components", - aliasVolumeMapping: map[string][]common.DevfileVolume{ - "some-component": []common.DevfileVolume{ - { - Name: volumeNames[0], - ContainerPath: volumePaths[0], - Size: volumeSizes[0], - }, - { - Name: volumeNames[1], - ContainerPath: volumePaths[1], - Size: volumeSizes[1], - }, - }, - "second-component": []common.DevfileVolume{ - { - Name: volumeNames[0], - ContainerPath: volumePaths[0], - Size: volumeSizes[0], - }, - }, - "third-component": []common.DevfileVolume{ - { - Name: volumeNames[1], - ContainerPath: volumePaths[1], - Size: volumeSizes[1], - }, - { - Name: volumeNames[2], - ContainerPath: volumePaths[2], - Size: volumeSizes[2], - }, - }, - }, - client: fakeClient, - wantStorage: []common.Storage{ - { - Volume: common.DevfileVolume{ - Name: volumeNames[0], - ContainerPath: volumePaths[0], - Size: volumeSizes[0], - }, - }, - { - Volume: common.DevfileVolume{ - Name: volumeNames[1], - ContainerPath: volumePaths[1], - Size: volumeSizes[1], - }, - }, - { - Volume: common.DevfileVolume{ - Name: volumeNames[2], - ContainerPath: volumePaths[2], - Size: volumeSizes[2], - }, - }, - }, - wantErr: false, - }, - { - name: "Case 5: Docker client error", - aliasVolumeMapping: map[string][]common.DevfileVolume{ - "some-component": []common.DevfileVolume{ - { - Name: volumeNames[0], - ContainerPath: volumePaths[0], - Size: volumeSizes[0], - }, - }, - }, - client: fakeErrorClient, - wantStorage: nil, - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - - // Create one of the test volumes - uniqueStorage, _, err := ProcessVolumes(tt.client, testComponentName, tt.aliasVolumeMapping) - if !tt.wantErr == (err != nil) { - t.Errorf("Docker volume create unexpected error %v, wantErr %v", err, tt.wantErr) - } - - storageLength := len(uniqueStorage) - wantStorageLength := len(tt.wantStorage) - if storageLength != wantStorageLength { - t.Errorf("expected %v, wanted %v", storageLength, wantStorageLength) - } - - if storageLength > 0 { - for i := range uniqueStorage { - var volExists bool - for j := range tt.wantStorage { - if uniqueStorage[i].Volume.Name == tt.wantStorage[j].Volume.Name && uniqueStorage[i].Volume.ContainerPath == tt.wantStorage[j].Volume.ContainerPath { - volExists = true - } - } - - if !volExists { - t.Errorf("expected %v, wanted %v", uniqueStorage[i].Volume, tt.wantStorage[i].Volume) - } - } - } - - }) - } - -} - func TestGenerateVolName(t *testing.T) { tests := []struct { diff --git a/pkg/devfile/adapters/kubernetes/component/adapter.go b/pkg/devfile/adapters/kubernetes/component/adapter.go index 73b79c3fc19..af4c3178533 100644 --- a/pkg/devfile/adapters/kubernetes/component/adapter.go +++ b/pkg/devfile/adapters/kubernetes/component/adapter.go @@ -418,31 +418,34 @@ func (a Adapter) createOrUpdateComponent(componentExists bool, ei envinfo.EnvSpe } initContainers = append(initContainers, supervisordInitContainer) - containerNameToVolumes, err := common.GetVolumes(a.Devfile) - if err != nil { - return err - } - var odoSourcePVCName string - volumeNameToPVCName := make(map[string]string) - // list all the pvcs for the component pvcs, err := a.Client.GetKubeClient().ListPVCs(fmt.Sprintf("%v=%v", "component", a.ComponentName)) if err != nil { return err } + volumeNameToVolInfo := make(map[string]storage.VolumeInfo) for _, pvc := range pvcs { // check if the pvc is in the terminating state if pvc.DeletionTimestamp != nil { continue } - volumeNameToPVCName[pvc.Labels[storagelabels.StorageLabel]] = pvc.Name + + generatedVolumeName, err := storage.GenerateVolumeNameFromPVC(pvc.Name) + if err != nil { + return errors.Wrapf(err, "Unable to generate volume name from pvc name") + } + + volumeNameToVolInfo[pvc.Labels[storagelabels.StorageLabel]] = storage.VolumeInfo{ + PVCName: pvc.Name, + VolumeName: generatedVolumeName, + } } // Get PVC volumes and Volume Mounts - containers, pvcVolumes, err := storage.GetPVCAndVolumeMount(containers, volumeNameToPVCName, containerNameToVolumes) + pvcVolumes, err := storage.GetVolumesAndVolumeMounts(a.Devfile, containers, volumeNameToVolInfo, parsercommon.DevfileOptions{}) if err != nil { return err } diff --git a/pkg/devfile/adapters/kubernetes/storage/utils.go b/pkg/devfile/adapters/kubernetes/storage/utils.go index bcc76b038ed..63f90f2db4d 100644 --- a/pkg/devfile/adapters/kubernetes/storage/utils.go +++ b/pkg/devfile/adapters/kubernetes/storage/utils.go @@ -3,48 +3,59 @@ package storage import ( "fmt" + devfileParser "github.com/devfile/library/pkg/devfile/parser" + parsercommon "github.com/devfile/library/pkg/devfile/parser/data/v2/common" componentlabels "github.com/openshift/odo/pkg/component/labels" - "github.com/openshift/odo/pkg/devfile/adapters/common" + "github.com/openshift/odo/pkg/envinfo" "github.com/openshift/odo/pkg/kclient" "github.com/openshift/odo/pkg/preference" "github.com/openshift/odo/pkg/storage" storagelabels "github.com/openshift/odo/pkg/storage/labels" "github.com/openshift/odo/pkg/util" - "github.com/pkg/errors" kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" corev1 "k8s.io/api/core/v1" ) -// GetPVCAndVolumeMount gets the PVC and updates the containers with the volume mount -// volumeNameToPVCName is a map of volume name to the PVC created -// containerNameToVolumes is a map of the Devfile container names to the Devfile Volumes -func GetPVCAndVolumeMount(containers []corev1.Container, volumeNameToPVCName map[string]string, containerNameToVolumes map[string][]common.DevfileVolume) ([]corev1.Container, []corev1.Volume, error) { +// VolumeInfo is a struct to hold the pvc name and the volume name to create a volume. +// To be moved to devfile/library. +type VolumeInfo struct { + PVCName string + VolumeName string +} + +// GetVolumesAndVolumeMounts gets the PVC volumes and updates the containers with the volume mounts. +// volumeNameToVolInfo is a map of the devfile volume name to the volume info containing the pvc name and the volume name. +// To be moved to devfile/library. +func GetVolumesAndVolumeMounts(devfileObj devfileParser.DevfileObj, containers []corev1.Container, volumeNameToVolInfo map[string]VolumeInfo, options parsercommon.DevfileOptions) ([]corev1.Volume, error) { + + containerComponents, err := devfileObj.Data.GetDevfileContainerComponents(options) + if err != nil { + return nil, err + } + var pvcVols []corev1.Volume - for volName, pvcName := range volumeNameToPVCName { - generatedVolumeName, err := generateVolumeNameFromPVC(pvcName) - if err != nil { - return nil, nil, errors.Wrapf(err, "Unable to generate volume name from pvc name") - } - pvcVols = append(pvcVols, getPVC(generatedVolumeName, pvcName)) + for volName, volInfo := range volumeNameToVolInfo { + pvcVols = append(pvcVols, getPVC(volInfo.VolumeName, volInfo.PVCName)) // containerNameToMountPaths is a map of the Devfile container name to their Devfile Volume Mount Paths for a given Volume Name containerNameToMountPaths := make(map[string][]string) - for containerName, volumes := range containerNameToVolumes { - for _, volume := range volumes { - if volName == volume.Name { - containerNameToMountPaths[containerName] = append(containerNameToMountPaths[containerName], volume.ContainerPath) + for _, containerComp := range containerComponents { + for _, volumeMount := range containerComp.Container.VolumeMounts { + if volName == volumeMount.Name { + containerNameToMountPaths[containerComp.Name] = append(containerNameToMountPaths[containerComp.Name], envinfo.GetVolumeMountPath(volumeMount)) } } } - containers = addVolumeMountToContainers(containers, generatedVolumeName, containerNameToMountPaths) + addVolumeMountToContainers(containers, volInfo.VolumeName, containerNameToMountPaths) } - return containers, pvcVols, nil + return pvcVols, nil } -// getPVC gets a pvc type volume with the given volume name and pvc name +// getPVC gets a pvc type volume with the given volume name and pvc name. +// To be moved to devfile/library. func getPVC(volumeName, pvcName string) corev1.Volume { return corev1.Volume{ @@ -57,36 +68,34 @@ func getPVC(volumeName, pvcName string) corev1.Volume { } } -// generateVolumeNameFromPVC generates a volume name based on the pvc name -func generateVolumeNameFromPVC(pvc string) (volumeName string, err error) { - volumeName, err = util.NamespaceOpenShiftObject(pvc, "vol") - if err != nil { - return "", err - } - return -} - // addVolumeMountToContainers adds the Volume Mounts in containerNameToMountPaths to the containers for a given pvc and volumeName -// containerNameToMountPaths is a map of a container name to an array of its Mount Paths -func addVolumeMountToContainers(containers []corev1.Container, volumeName string, containerNameToMountPaths map[string][]string) []corev1.Container { +// containerNameToMountPaths is a map of a container name to an array of its Mount Paths. +// To be moved to devfile/library. +func addVolumeMountToContainers(containers []corev1.Container, volumeName string, containerNameToMountPaths map[string][]string) { for containerName, mountPaths := range containerNameToMountPaths { - for i, container := range containers { - if container.Name == containerName { + for i := range containers { + if containers[i].Name == containerName { for _, mountPath := range mountPaths { - container.VolumeMounts = append(container.VolumeMounts, corev1.VolumeMount{ + containers[i].VolumeMounts = append(containers[i].VolumeMounts, corev1.VolumeMount{ Name: volumeName, MountPath: mountPath, SubPath: "", }, ) } - containers[i] = container } } } +} - return containers +// GenerateVolumeNameFromPVC generates a volume name based on the pvc name +func GenerateVolumeNameFromPVC(pvc string) (volumeName string, err error) { + volumeName, err = util.NamespaceOpenShiftObject(pvc, "vol") + if err != nil { + return "", err + } + return } // HandleEphemeralStorage creates or deletes the ephemeral volume based on the preference setting diff --git a/pkg/devfile/adapters/kubernetes/storage/utils_test.go b/pkg/devfile/adapters/kubernetes/storage/utils_test.go index 6e2ff1c3aac..23c117cf18d 100644 --- a/pkg/devfile/adapters/kubernetes/storage/utils_test.go +++ b/pkg/devfile/adapters/kubernetes/storage/utils_test.go @@ -1,10 +1,15 @@ package storage import ( - "strings" "testing" - "github.com/openshift/odo/pkg/devfile/adapters/common" + devfilev1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2" + "github.com/devfile/api/v2/pkg/attributes" + "github.com/devfile/library/pkg/devfile/generator" + devfileParser "github.com/devfile/library/pkg/devfile/parser" + "github.com/devfile/library/pkg/devfile/parser/data" + parsercommon "github.com/devfile/library/pkg/devfile/parser/data/v2/common" + "github.com/openshift/odo/pkg/testingutil" v1 "k8s.io/api/core/v1" ) @@ -35,7 +40,7 @@ func TestGetPVC(t *testing.T) { } } -func TestAddVolumeMountToPodTemplateSpec(t *testing.T) { +func TestAddVolumeMountToContainers(t *testing.T) { tests := []struct { podName string @@ -92,7 +97,8 @@ func TestAddVolumeMountToPodTemplateSpec(t *testing.T) { for _, tt := range tests { t.Run(tt.podName, func(t *testing.T) { - containers := addVolumeMountToContainers([]v1.Container{tt.container}, tt.volumeName, tt.containerMountPathsMap) + containers := []v1.Container{tt.container} + addVolumeMountToContainers(containers, tt.volumeName, tt.containerMountPathsMap) mountPathCount := 0 for _, container := range containers { @@ -110,105 +116,161 @@ func TestAddVolumeMountToPodTemplateSpec(t *testing.T) { } if mountPathCount != len(tt.containerMountPathsMap[tt.container.Name]) { - t.Errorf("Volume Mounts for %s have not been properly mounted to the podTemplateSpec", tt.volumeName) + t.Errorf("Volume Mounts for %s have not been properly mounted to the container", tt.volumeName) } }) } } -func TestGetPVCAndVolumeMount(t *testing.T) { +func TestGetVolumesAndVolumeMounts(t *testing.T) { - volNames := [...]string{"volume1", "volume2", "volume3"} - volContainerPath := [...]string{"/home/user/path1", "/home/user/path2", "/home/user/path3"} + type testVolumeMountInfo struct { + mountPath string + volumeName string + } tests := []struct { - name string - podName string - namespace string - labels map[string]string - containers []v1.Container - volumeNameToPVCName map[string]string - componentAliasToVolumes map[string][]common.DevfileVolume - wantErr bool + name string + components []devfilev1.Component + volumeNameToVolInfo map[string]VolumeInfo + wantContainerToVol map[string][]testVolumeMountInfo + wantErr bool }{ { - name: "Case: Valid case", - podName: "podSpecTest", - namespace: "default", - labels: map[string]string{ - "app": "app", - "component": "frontend", - }, - containers: []v1.Container{ - { - Name: "container1", + name: "One volume mounted", + components: []devfilev1.Component{testingutil.GetFakeContainerComponent("comp1"), testingutil.GetFakeContainerComponent("comp2")}, + volumeNameToVolInfo: map[string]VolumeInfo{ + "myvolume1": { + PVCName: "volume1-pvc", + VolumeName: "volume1-pvc-vol", }, - { - Name: "container2", - }, - }, - volumeNameToPVCName: map[string]string{ - "volume1": "volume1-pvc", - "volume2": "volume2-pvc", - "volume3": "volume3-pvc", }, - componentAliasToVolumes: map[string][]common.DevfileVolume{ - "container1": []common.DevfileVolume{ + wantContainerToVol: map[string][]testVolumeMountInfo{ + "comp1": { { - Name: volNames[0], - ContainerPath: volContainerPath[0], + mountPath: "/my/volume/mount/path1", + volumeName: "volume1-pvc-vol", }, + }, + "comp2": { { - Name: volNames[0], - ContainerPath: volContainerPath[1], + mountPath: "/my/volume/mount/path1", + volumeName: "volume1-pvc-vol", }, - { - Name: volNames[1], - ContainerPath: volContainerPath[2], + }, + }, + wantErr: false, + }, + { + name: "One volume mounted at diff locations", + components: []devfilev1.Component{ + { + Name: "container1", + ComponentUnion: devfilev1.ComponentUnion{ + Container: &devfilev1.ContainerComponent{ + Container: devfilev1.Container{ + VolumeMounts: []devfilev1.VolumeMount{ + { + Name: "volume1", + Path: "/path1", + }, + { + Name: "volume1", + Path: "/path2", + }, + }, + }, + }, }, }, - "container2": []common.DevfileVolume{ + }, + volumeNameToVolInfo: map[string]VolumeInfo{ + "volume1": { + PVCName: "volume1-pvc", + VolumeName: "volume1-pvc-vol", + }, + }, + wantContainerToVol: map[string][]testVolumeMountInfo{ + "container1": { { - Name: volNames[1], - ContainerPath: volContainerPath[1], + mountPath: "/path1", + volumeName: "volume1-pvc-vol", }, { - Name: volNames[2], - ContainerPath: volContainerPath[2], + mountPath: "/path2", + volumeName: "volume1-pvc-vol", }, }, }, wantErr: false, }, { - name: "Case: Error case", - podName: "podSpecTest", - namespace: "default", - labels: map[string]string{ - "app": "app", - "component": "frontend", - }, - containers: []v1.Container{ + name: "One volume mounted at diff container components", + components: []devfilev1.Component{ + { + Name: "container1", + ComponentUnion: devfilev1.ComponentUnion{ + Container: &devfilev1.ContainerComponent{ + Container: devfilev1.Container{ + VolumeMounts: []devfilev1.VolumeMount{ + { + Name: "volume1", + Path: "/path1", + }, + }, + }, + }, + }, + }, { Name: "container2", + ComponentUnion: devfilev1.ComponentUnion{ + Container: &devfilev1.ContainerComponent{ + Container: devfilev1.Container{ + VolumeMounts: []devfilev1.VolumeMount{ + { + Name: "volume1", + Path: "/path2", + }, + }, + }, + }, + }, }, }, - volumeNameToPVCName: map[string]string{ - "volume2": "", - "volume3": "volume3-pvc", + volumeNameToVolInfo: map[string]VolumeInfo{ + "volume1": { + PVCName: "volume1-pvc", + VolumeName: "volume1-pvc-vol", + }, }, - componentAliasToVolumes: map[string][]common.DevfileVolume{ - "container2": []common.DevfileVolume{ + wantContainerToVol: map[string][]testVolumeMountInfo{ + "container1": { { - Name: volNames[1], - ContainerPath: volContainerPath[1], + mountPath: "/path1", + volumeName: "volume1-pvc-vol", }, + }, + "container2": { { - Name: volNames[2], - ContainerPath: volContainerPath[2], + mountPath: "/path2", + volumeName: "volume1-pvc-vol", }, }, }, + wantErr: false, + }, + { + name: "Invalid case", + components: []devfilev1.Component{ + { + Name: "container1", + Attributes: attributes.Attributes{}.FromStringMap(map[string]string{ + "firstString": "firstStringValue", + }), + ComponentUnion: devfilev1.ComponentUnion{}, + }, + }, wantErr: true, }, } @@ -216,51 +278,76 @@ func TestGetPVCAndVolumeMount(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - containers, pvcVols, err := GetPVCAndVolumeMount(tt.containers, tt.volumeNameToPVCName, tt.componentAliasToVolumes) + devObj := devfileParser.DevfileObj{ + Data: func() data.DevfileData { + devfileData, err := data.NewDevfileData(string(data.APIVersion200)) + if err != nil { + t.Error(err) + } + err = devfileData.AddComponents(tt.components) + if err != nil { + t.Error(err) + } + return devfileData + }(), + } + + containers, err := generator.GetContainers(devObj, parsercommon.DevfileOptions{}) + if err != nil { + t.Errorf("TestGetVolumesAndVolumeMounts error - %v", err) + return + } + + var options parsercommon.DevfileOptions + if tt.wantErr { + options = parsercommon.DevfileOptions{ + Filter: map[string]interface{}{ + "firstString": "firstStringValue", + }, + } + } + + pvcVols, err := GetVolumesAndVolumeMounts(devObj, containers, tt.volumeNameToVolInfo, options) if !tt.wantErr && err != nil { - t.Errorf("TestGetPVCAndVolumeMount.AddPVCAndVolumeMount() unexpected error %v, wantErr %v", err, tt.wantErr) + t.Errorf("TestGetVolumesAndVolumeMounts unexpected error: %v", err) + return } else if tt.wantErr && err != nil { return } else if tt.wantErr && err == nil { - t.Error("TestGetPVCAndVolumeMount.AddPVCAndVolumeMount() expected error but got nil") + t.Error("TestGetVolumesAndVolumeMounts expected error but got nil") return } - // The total number of expected volumes is equal to the number of volumes defined in the devfile - expectedNumVolumes := len(tt.volumeNameToPVCName) + // check if the pvc volumes returned are correct + for _, volInfo := range tt.volumeNameToVolInfo { + matched := false + for _, pvcVol := range pvcVols { + if volInfo.VolumeName == pvcVol.Name && pvcVol.PersistentVolumeClaim != nil && volInfo.PVCName == pvcVol.PersistentVolumeClaim.ClaimName { + matched = true + } + } - // check the number of containers and volumes in the pod template spec - if len(containers) != len(tt.containers) { - t.Errorf("TestGetPVCAndVolumeMount error - Incorrect number of Containers found in the pod template spec, expected: %v found: %v", len(tt.containers), len(containers)) - return - } - if len(pvcVols) != expectedNumVolumes { - t.Errorf("TestGetPVCAndVolumeMount error - incorrect amount of pvc volumes in pod template spec expected %v, actual %v", expectedNumVolumes, len(pvcVols)) - return + if !matched { + t.Errorf("TestGetVolumesAndVolumeMounts error - could not find volume details %s in the actual result", volInfo.VolumeName) + } } - // check the volume mounts of the pod template spec containers + // check the volume mounts of the containers for _, container := range containers { - for testcontainerAlias, testContainerVolumes := range tt.componentAliasToVolumes { - if container.Name == testcontainerAlias { - // check if container has the correct number of volume mounts - if len(container.VolumeMounts) != len(testContainerVolumes) { - t.Errorf("TestGetPVCAndVolumeMount - Incorrect number of Volume Mounts found in the pod template spec container %v, expected: %v found: %v", container.Name, len(testContainerVolumes), len(container.VolumeMounts)) - } - - // check if container has the specified volume - volumeMatched := 0 - for _, volumeMount := range container.VolumeMounts { - for _, testVolume := range testContainerVolumes { - testVolumeName := testVolume.Name - testVolumePath := testVolume.ContainerPath - if strings.Contains(volumeMount.Name, testVolumeName) && volumeMount.MountPath == testVolumePath { - volumeMatched++ - } + if volMounts, ok := tt.wantContainerToVol[container.Name]; !ok { + t.Errorf("TestGetVolumesAndVolumeMounts error - did not find the expected container %s", container.Name) + return + } else { + for _, expectedVolMount := range volMounts { + matched := false + for _, actualVolMount := range container.VolumeMounts { + if expectedVolMount.volumeName == actualVolMount.Name && expectedVolMount.mountPath == actualVolMount.MountPath { + matched = true } } - if volumeMatched != len(testContainerVolumes) { - t.Errorf("TestGetPVCAndVolumeMount - Failed to match Volume Mounts for pod template spec container %v, expected: %v found: %v", container.Name, len(testContainerVolumes), volumeMatched) + + if !matched { + t.Errorf("TestGetVolumesAndVolumeMounts error - could not find volume mount details for path %s in the actual result for container %s", expectedVolMount.mountPath, container.Name) } } } diff --git a/pkg/envinfo/storage.go b/pkg/envinfo/storage.go index c511b0e1531..778da7e378c 100644 --- a/pkg/envinfo/storage.go +++ b/pkg/envinfo/storage.go @@ -183,7 +183,8 @@ func (ei *EnvInfo) GetStorageMountPath(storageName string) (string, error) { return "", nil } -// GetVolumeMountPath gets the volume mount's path +// GetVolumeMountPath gets the volume mount's path. +// To be moved to devfile/library. func GetVolumeMountPath(volumeMount devfilev1.VolumeMount) string { // if there is no volume mount path, default to volume mount name as per devfile schema if volumeMount.Path == "" {