Skip to content

Migrate devfile specific vol code from odo to devfile/library #81

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Apr 21, 2021
Merged

Migrate devfile specific vol code from odo to devfile/library #81

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74c6802
Migrate vol funcs from odo to devfile/library
maysunfaisal Apr 19, 2021
294012e
Update with PR feedback
maysunfaisal Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions pkg/devfile/generator/generators.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,3 +284,45 @@ func GetImageStream(imageStreamParams ImageStreamParams) imagev1.ImageStream {
}
return imageStream
}

// VolumeInfo is a struct to hold the pvc name and the volume name to create a volume.
type VolumeInfo struct {
PVCName string
VolumeName string
}

// VolumeParams is a struct that contains the required data to create Kubernetes Volumes and mount Volumes in Containers
type VolumeParams struct {
// Containers is a list of containers that needs to be updated for the volume mounts
Containers []corev1.Container

// VolumeNameToVolumeInfo is a map of the devfile volume name to the volume info containing the pvc name and the volume name.
VolumeNameToVolumeInfo map[string]VolumeInfo
}

// GetVolumesAndVolumeMounts gets the PVC volumes and updates the containers with the volume mounts.
func GetVolumesAndVolumeMounts(devfileObj parser.DevfileObj, volumeParams VolumeParams, options common.DevfileOptions) ([]corev1.Volume, error) {

containerComponents, err := devfileObj.Data.GetDevfileContainerComponents(options)
if err != nil {
return nil, err
}

var pvcVols []corev1.Volume
for volName, volInfo := range volumeParams.VolumeNameToVolumeInfo {
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 _, containerComp := range containerComponents {
for _, volumeMount := range containerComp.Container.VolumeMounts {
if volName == volumeMount.Name {
containerNameToMountPaths[containerComp.Name] = append(containerNameToMountPaths[containerComp.Name], GetVolumeMountPath(volumeMount))
}
}
}

addVolumeMountToContainers(volumeParams.Containers, volInfo.VolumeName, containerNameToMountPaths)
}
return pvcVols, nil
}
242 changes: 239 additions & 3 deletions pkg/devfile/generator/generators_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"reflect"
"testing"

"github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
v1 "github.com/devfile/api/v2/pkg/apis/workspaces/v1alpha2"
"github.com/devfile/api/v2/pkg/attributes"
"github.com/devfile/library/pkg/devfile/parser"
Expand All @@ -28,7 +27,7 @@ func TestGetContainers(t *testing.T) {
trueMountSources := true
falseMountSources := false

project := v1alpha2.Project{
project := v1.Project{
ClonePath: "test-project/",
Name: "project0",
ProjectSource: v1.ProjectSource{
Expand Down Expand Up @@ -189,7 +188,7 @@ func TestGetContainers(t *testing.T) {
DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{
DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{
Components: tt.containerComponents,
Projects: []v1alpha2.Project{
Projects: []v1.Project{
project,
},
},
Expand Down Expand Up @@ -228,3 +227,240 @@ func TestGetContainers(t *testing.T) {
}

}

func TestGetVolumesAndVolumeMounts(t *testing.T) {

type testVolumeMountInfo struct {
mountPath string
volumeName string
}

tests := []struct {
name string
components []v1.Component
volumeNameToVolInfo map[string]VolumeInfo
wantContainerToVol map[string][]testVolumeMountInfo
wantErr bool
}{
{
name: "One volume mounted",
components: []v1.Component{testingutil.GetFakeContainerComponent("comp1"), testingutil.GetFakeContainerComponent("comp2")},
volumeNameToVolInfo: map[string]VolumeInfo{
"myvolume1": {
PVCName: "volume1-pvc",
VolumeName: "volume1-pvc-vol",
},
},
wantContainerToVol: map[string][]testVolumeMountInfo{
"comp1": {
{
mountPath: "/my/volume/mount/path1",
volumeName: "volume1-pvc-vol",
},
},
"comp2": {
{
mountPath: "/my/volume/mount/path1",
volumeName: "volume1-pvc-vol",
},
},
},
wantErr: false,
},
{
name: "One volume mounted at diff locations",
components: []v1.Component{
{
Name: "container1",
ComponentUnion: v1.ComponentUnion{
Container: &v1.ContainerComponent{
Container: v1.Container{
VolumeMounts: []v1.VolumeMount{
{
Name: "volume1",
Path: "/path1",
},
{
Name: "volume1",
Path: "/path2",
},
},
},
},
},
},
},
volumeNameToVolInfo: map[string]VolumeInfo{
"volume1": {
PVCName: "volume1-pvc",
VolumeName: "volume1-pvc-vol",
},
},
wantContainerToVol: map[string][]testVolumeMountInfo{
"container1": {
{
mountPath: "/path1",
volumeName: "volume1-pvc-vol",
},
{
mountPath: "/path2",
volumeName: "volume1-pvc-vol",
},
},
},
wantErr: false,
},
{
name: "One volume mounted at diff container components",
components: []v1.Component{
{
Name: "container1",
ComponentUnion: v1.ComponentUnion{
Container: &v1.ContainerComponent{
Container: v1.Container{
VolumeMounts: []v1.VolumeMount{
{
Name: "volume1",
Path: "/path1",
},
},
},
},
},
},
{
Name: "container2",
ComponentUnion: v1.ComponentUnion{
Container: &v1.ContainerComponent{
Container: v1.Container{
VolumeMounts: []v1.VolumeMount{
{
Name: "volume1",
Path: "/path2",
},
},
},
},
},
},
},
volumeNameToVolInfo: map[string]VolumeInfo{
"volume1": {
PVCName: "volume1-pvc",
VolumeName: "volume1-pvc-vol",
},
},
wantContainerToVol: map[string][]testVolumeMountInfo{
"container1": {
{
mountPath: "/path1",
volumeName: "volume1-pvc-vol",
},
},
"container2": {
{
mountPath: "/path2",
volumeName: "volume1-pvc-vol",
},
},
},
wantErr: false,
},
{
name: "Invalid case",
components: []v1.Component{
{
Name: "container1",
Attributes: attributes.Attributes{}.FromStringMap(map[string]string{
"firstString": "firstStringValue",
}),
ComponentUnion: v1.ComponentUnion{},
},
},
wantErr: true,
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {

devObj := parser.DevfileObj{
Data: &v2.DevfileV2{
Devfile: v1.Devfile{
DevWorkspaceTemplateSpec: v1.DevWorkspaceTemplateSpec{
DevWorkspaceTemplateSpecContent: v1.DevWorkspaceTemplateSpecContent{
Components: tt.components,
},
},
},
},
}

containers, err := GetContainers(devObj, common.DevfileOptions{})
if err != nil {
t.Errorf("TestGetVolumesAndVolumeMounts error - %v", err)
return
}

var options common.DevfileOptions
if tt.wantErr {
options = common.DevfileOptions{
Filter: map[string]interface{}{
"firstString": "firstStringValue",
},
}
}

volumeParams := VolumeParams{
Containers: containers,
VolumeNameToVolumeInfo: tt.volumeNameToVolInfo,
}

pvcVols, err := GetVolumesAndVolumeMounts(devObj, volumeParams, options)
if !tt.wantErr && err != nil {
t.Errorf("TestGetVolumesAndVolumeMounts unexpected error: %v", err)
return
} else if tt.wantErr && err != nil {
return
} else if tt.wantErr && err == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we simplify these checks?

if (err != nil) != tt.wantErr {
  ...
}
if tt.wantErr && err != nil {
  return
}

t.Error("TestGetVolumesAndVolumeMounts expected error but got nil")
return
}

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

if !matched {
t.Errorf("TestGetVolumesAndVolumeMounts error - could not find volume details %s in the actual result", volInfo.VolumeName)
}
}

// check the volume mounts of the containers
for _, container := range containers {
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 !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)
}
}
}
}
})
}
}
43 changes: 43 additions & 0 deletions pkg/devfile/generator/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,3 +419,46 @@ func getBuildConfigSpec(buildConfigSpecParams BuildConfigSpecParams) *buildv1.Bu
},
}
}

// GetVolumeMountPath gets the volume mount's path.
func GetVolumeMountPath(volumeMount v1.VolumeMount) string {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason we want to make GetVolumeMountPath an exposed function?
non of the util functions are exposed. if it is expected to be exposed, then should be under generator.go

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

odo is using it in some of their code, i will move it to generators.go

// if there is no volume mount path, default to volume mount name as per devfile schema
if volumeMount.Path == "" {
volumeMount.Path = "/" + volumeMount.Name
}

return volumeMount.Path
}

// getPVC gets a pvc type volume with the given volume name and pvc name.
func getPVC(volumeName, pvcName string) corev1.Volume {

return corev1.Volume{
Name: volumeName,
VolumeSource: corev1.VolumeSource{
PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
ClaimName: pvcName,
},
},
}
}

// addVolumeMountToContainers adds the Volume Mounts in containerNameToMountPaths to the containers for a given 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) {

for containerName, mountPaths := range containerNameToMountPaths {
for i := range containers {
if containers[i].Name == containerName {
for _, mountPath := range mountPaths {
containers[i].VolumeMounts = append(containers[i].VolumeMounts, corev1.VolumeMount{
Name: volumeName,
MountPath: mountPath,
SubPath: "",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SubPath is omitempty, so no need to specifically set to ""

},
)
}
}
}
}
}
Loading