Skip to content

Commit

Permalink
Updated fix for redeploy bug
Browse files Browse the repository at this point in the history
  • Loading branch information
@rajivnathan
rajivnathan committed Jun 10, 2020
1 parent 2fd8d5d commit b37843d
Show file tree
Hide file tree
Showing 5 changed files with 110 additions and 60 deletions.
16 changes: 16 additions & 0 deletions pkg/devfile/adapters/common/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,22 @@ type DevfileVolume struct {
Size string
}

// ComponentVolumesPair is a struct to hold the relationship between components and their volumes
// Use splices of this struct instead of a map because it is important to maintain order since iteration order of maps is not guaranteed.
// When maps were used the ordering of the volumes could change in the pod spec causing an unnecessary rollout of the deployment which can be slow.
type ComponentVolumesPair struct {
ComponentAlias string
Volumes []DevfileVolume
}

// VolumePVCPair is a struct to hold the relationship between volumes and their PVCs
// Use splices of this struct instead of a map because it is important to maintain order since iteration order of maps is not guaranteed.
// When maps were used the ordering of the volumes could change in the pod spec causing an unnecessary rollout of the deployment which can be slow.
type VolumePVCPair struct {
Volume string
PVC string
}

// Storage is a struct that is common to all the adapters
type Storage struct {
Name string
Expand Down
14 changes: 10 additions & 4 deletions pkg/devfile/adapters/common/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,20 +110,26 @@ func GetSupportedComponents(data data.DevfileData) []common.DevfileComponent {
}

// GetVolumes iterates through the components in the devfile and returns a map of component alias to the devfile volumes
func GetVolumes(devfileObj devfileParser.DevfileObj) map[string][]DevfileVolume {
// componentAliasToVolumes is a map of the Devfile Component Alias to the Devfile Component Volumes
componentAliasToVolumes := make(map[string][]DevfileVolume)
func GetVolumes(devfileObj devfileParser.DevfileObj) []ComponentVolumesPair {
// componentAliasToVolumes maps the Devfile Component Alias to the Devfile Component Volumes
componentAliasToVolumes := []ComponentVolumesPair{}
size := volumeSize
for _, comp := range GetSupportedComponents(devfileObj.Data) {
if len(comp.Container.VolumeMounts) != 0 {
volumes := []DevfileVolume{}
for _, volume := range comp.Container.VolumeMounts {
vol := DevfileVolume{
Name: volume.Name,
ContainerPath: volume.Path,
Size: size,
}
componentAliasToVolumes[comp.Container.Name] = append(componentAliasToVolumes[comp.Container.Name], vol)
volumes = append(volumes, vol)
}
volumeData := ComponentVolumesPair{
ComponentAlias: *comp.Alias,
Volumes: volumes,
}
componentAliasToVolumes = append(componentAliasToVolumes, volumeData)
}
}
return componentAliasToVolumes
Expand Down
8 changes: 6 additions & 2 deletions pkg/devfile/adapters/kubernetes/component/adapter.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ func (a Adapter) createOrUpdateComponent(componentExists bool) (err error) {
componentAliasToVolumes := adaptersCommon.GetVolumes(a.Devfile)

var uniqueStorages []common.Storage
volumeNameToPVCName := make(map[string]string)
volumeNameToPVCName := []common.VolumePVCPair{}
processedVolumes := make(map[string]bool)

// Get a list of all the unique volume names and generate their PVC names
Expand Down Expand Up @@ -200,7 +200,11 @@ func (a Adapter) createOrUpdateComponent(componentExists bool) (err error) {
Volume: vol,
}
uniqueStorages = append(uniqueStorages, pvc)
volumeNameToPVCName[vol.Name] = generatedPVCName
pairData := common.VolumePVCPair{
Volume: vol.Name,
PVC: generatedPVCName,
}
volumeNameToPVCName = append(volumeNameToPVCName, pairData)
}
}
}
Expand Down
16 changes: 8 additions & 8 deletions pkg/kclient/volumes.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,20 +76,20 @@ func AddVolumeMountToPodTemplateSpec(podTemplateSpec *corev1.PodTemplateSpec, vo
// AddPVCAndVolumeMount adds PVC and volume mount to the pod template spec
// volumeNameToPVCName is a map of volume name to the PVC created
// componentAliasToVolumes is a map of the Devfile component alias to the Devfile Volumes
func AddPVCAndVolumeMount(podTemplateSpec *corev1.PodTemplateSpec, volumeNameToPVCName map[string]string, componentAliasToVolumes map[string][]common.DevfileVolume) error {
for volName, pvcName := range volumeNameToPVCName {
generatedVolumeName, err := generateVolumeNameFromPVC(pvcName)
func AddPVCAndVolumeMount(podTemplateSpec *corev1.PodTemplateSpec, volumeNameToPVCName []common.VolumePVCPair, componentAliasToVolumes []common.ComponentVolumesPair) error {
for _, pair := range volumeNameToPVCName {
generatedVolumeName, err := generateVolumeNameFromPVC(pair.PVC)
if err != nil {
return errors.Wrapf(err, "Unable to generate volume name from pvc name")
}
AddPVCToPodTemplateSpec(podTemplateSpec, generatedVolumeName, pvcName)
AddPVCToPodTemplateSpec(podTemplateSpec, generatedVolumeName, pair.PVC)

// componentAliasToMountPaths is a map of the Devfile container alias to their Devfile Volume Mount Paths for a given Volume Name
componentAliasToMountPaths := make(map[string][]string)
for containerName, volumes := range componentAliasToVolumes {
for _, volume := range volumes {
if volName == volume.Name {
componentAliasToMountPaths[containerName] = append(componentAliasToMountPaths[containerName], volume.ContainerPath)
for _, volumeData := range componentAliasToVolumes {
for _, volume := range volumeData.Volumes {
if pair.Volume == volume.Name {
componentAliasToMountPaths[volumeData.ComponentAlias] = append(componentAliasToMountPaths[volumeData.ComponentAlias], volume.ContainerPath)
}
}
}
Expand Down
116 changes: 70 additions & 46 deletions pkg/kclient/volumes_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -340,8 +340,8 @@ func TestAddPVCAndVolumeMount(t *testing.T) {
namespace string
labels map[string]string
containers []corev1.Container
volumeNameToPVCName map[string]string
componentAliasToVolumes map[string][]common.DevfileVolume
volumeNameToPVCName []common.VolumePVCPair
componentAliasToVolumes []common.ComponentVolumesPair
wantErr bool
}{
{
Expand Down Expand Up @@ -372,34 +372,49 @@ func TestAddPVCAndVolumeMount(t *testing.T) {
Env: []corev1.EnvVar{},
},
},
volumeNameToPVCName: map[string]string{
"volume1": "volume1-pvc",
"volume2": "volume2-pvc",
"volume3": "volume3-pvc",
volumeNameToPVCName: []common.VolumePVCPair{
common.VolumePVCPair{
Volume: "volume1",
PVC: "volume1-pvc",
},
common.VolumePVCPair{
Volume: "volume2",
PVC: "volume2-pvc",
},
common.VolumePVCPair{
Volume: "volume3",
PVC: "volume3-pvc",
},
},
componentAliasToVolumes: map[string][]common.DevfileVolume{
"container1": []common.DevfileVolume{
{
Name: volNames[0],
ContainerPath: volContainerPath[0],
},
{
Name: volNames[0],
ContainerPath: volContainerPath[1],
},
{
Name: volNames[1],
ContainerPath: volContainerPath[2],
componentAliasToVolumes: []common.ComponentVolumesPair{
common.ComponentVolumesPair{
ComponentAlias: "container1",
Volumes: []common.DevfileVolume{
{
Name: volNames[0],
ContainerPath: volContainerPath[0],
},
{
Name: volNames[0],
ContainerPath: volContainerPath[1],
},
{
Name: volNames[1],
ContainerPath: volContainerPath[2],
},
},
},
"container2": []common.DevfileVolume{
{
Name: volNames[1],
ContainerPath: volContainerPath[1],
},
{
Name: volNames[2],
ContainerPath: volContainerPath[2],
common.ComponentVolumesPair{
ComponentAlias: "container2",
Volumes: []common.DevfileVolume{
{
Name: volNames[1],
ContainerPath: volContainerPath[1],
},
{
Name: volNames[2],
ContainerPath: volContainerPath[2],
},
},
},
},
Expand All @@ -414,19 +429,28 @@ func TestAddPVCAndVolumeMount(t *testing.T) {
"component": "frontend",
},
containers: []corev1.Container{},
volumeNameToPVCName: map[string]string{
"volume2": "volume2-pvc",
"volume3": "volume3-pvc",
volumeNameToPVCName: []common.VolumePVCPair{
common.VolumePVCPair{
Volume: "volume2",
PVC: "volume2-pvc",
},
common.VolumePVCPair{
Volume: "volume3",
PVC: "volume3-pvc",
},
},
componentAliasToVolumes: map[string][]common.DevfileVolume{
"container2": []common.DevfileVolume{
{
Name: volNames[1],
ContainerPath: volContainerPath[1],
},
{
Name: volNames[2],
ContainerPath: volContainerPath[2],
componentAliasToVolumes: []common.ComponentVolumesPair{
common.ComponentVolumesPair{
ComponentAlias: "container2",
Volumes: []common.DevfileVolume{
{
Name: volNames[1],
ContainerPath: volContainerPath[1],
},
{
Name: volNames[2],
ContainerPath: volContainerPath[2],
},
},
},
},
Expand Down Expand Up @@ -463,26 +487,26 @@ func TestAddPVCAndVolumeMount(t *testing.T) {

// check the volume mounts of the pod template spec containers
for _, container := range podTemplateSpec.Spec.Containers {
for testcontainerAlias, testContainerVolumes := range tt.componentAliasToVolumes {
if container.Name == testcontainerAlias {
for _, pair := range tt.componentAliasToVolumes {
if container.Name == pair.ComponentAlias {
// check if container has the correct number of volume mounts
if len(container.VolumeMounts) != len(testContainerVolumes) {
t.Errorf("Incorrect number of Volume Mounts found in the pod template spec container %v, expected: %v found: %v", container.Name, len(testContainerVolumes), len(container.VolumeMounts))
if len(container.VolumeMounts) != len(pair.Volumes) {
t.Errorf("Incorrect number of Volume Mounts found in the pod template spec container %v, expected: %v found: %v", container.Name, len(pair.Volumes), len(container.VolumeMounts))
}

// check if container has the specified volume
volumeMatched := 0
for _, volumeMount := range container.VolumeMounts {
for _, testVolume := range testContainerVolumes {
for _, testVolume := range pair.Volumes {
testVolumeName := testVolume.Name
testVolumePath := testVolume.ContainerPath
if strings.Contains(volumeMount.Name, testVolumeName) && volumeMount.MountPath == testVolumePath {
volumeMatched++
}
}
}
if volumeMatched != len(testContainerVolumes) {
t.Errorf("Failed to match Volume Mounts for pod template spec container %v, expected: %v found: %v", container.Name, len(testContainerVolumes), volumeMatched)
if volumeMatched != len(pair.Volumes) {
t.Errorf("Failed to match Volume Mounts for pod template spec container %v, expected: %v found: %v", container.Name, len(pair.Volumes), volumeMatched)
}
}
}
Expand Down

0 comments on commit b37843d

Please sign in to comment.