Skip to content

refactor: extract shared rewriteContainerVolumeMounts logic between storage provisioners #1698

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

Open
SAY-5 wants to merge 3 commits into
base: main
Choose a base branch
from
Open

refactor: extract shared rewriteContainerVolumeMounts logic between storage provisioners #1698

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3dfdbcb
refactor: extract shared rewriteContainerVolumeMounts logic between s…
SAY-5 Aug 25, 2026
44623db
docs: note rewriteContainerVolumeMounts must run once per PodAdditions
SAY-5 Sep 28, 2026
46c7ade
fix: drop unused overrides imports after rebase
SAY-5 Sep 28, 2026
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
79 changes: 4 additions & 75 deletions pkg/provision/storage/commonStorage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,6 @@ import (
"k8s.io/apimachinery/pkg/types"

"github.com/devfile/devworkspace-operator/apis/controller/v1alpha1"
"github.com/devfile/devworkspace-operator/pkg/constants"
devfileConstants "github.com/devfile/devworkspace-operator/pkg/library/constants"
"github.com/devfile/devworkspace-operator/pkg/library/overrides"
)

// The CommonStorageProvisioner provisions one PVC per namespace and configures all volumes in a workspace
Expand Down Expand Up @@ -139,76 +136,8 @@ func (p *CommonStorageProvisioner) rewriteContainerVolumeMounts(
workspace *dw.DevWorkspaceTemplateSpec,
restrictedFields []string,
) error {
devfileVolumes := map[string]dw.VolumeComponent{}

// Construct map of volume name -> volume Component
for _, component := range workspace.Components {
if component.Volume != nil {
if _, exists := devfileVolumes[component.Name]; exists {
return fmt.Errorf("volume component '%s' is defined multiple times", component.Name)
}
devfileVolumes[component.Name] = *component.Volume
}
}

// Containers in podAdditions may reference e.g. automounted volumes in their volumeMounts, and this is not an error
additionalVolumes := map[string]bool{}
for _, additionalVolume := range podAdditions.Volumes {
additionalVolumes[additionalVolume.Name] = true
}

// Containers in podAdditions may reference volumes defined in pod overrides, and this is not an error
overridesVolumes, err := overrides.GetVolumesFromOverrides(workspace, restrictedFields)
if err != nil {
return err
}
for volumeName, present := range overridesVolumes {
additionalVolumes[volumeName] = present
}

// Add implicit projects volume to support mountSources, if needed
if _, exists := devfileVolumes[devfileConstants.ProjectsVolumeName]; !exists {
projectsVolume := dw.VolumeComponent{}
projectsVolume.Size = constants.PVCStorageSize
devfileVolumes[devfileConstants.ProjectsVolumeName] = projectsVolume
}

// TODO: What should we do when a volume isn't explicitly defined?
rewriteVolumeMounts := func(containers []corev1.Container) error {
for cIdx, container := range containers {
for vmIdx, vm := range container.VolumeMounts {
volume, ok := devfileVolumes[vm.Name]
if !ok {
// Volume is defined outside of the devfile
if additionalVolumes[vm.Name] {
continue
}
// Should never happen as flattened Devfile is validated.
return fmt.Errorf("container '%s' references undefined volume '%s'", container.Name, vm.Name)
}
if !isEphemeral(&volume) {
containers[cIdx].VolumeMounts[vmIdx].SubPath = fmt.Sprintf("%s/%s", workspaceId, vm.Name)
containers[cIdx].VolumeMounts[vmIdx].Name = pvcName
}
}
}
return nil
}
if err := rewriteVolumeMounts(podAdditions.Containers); err != nil {
return err
}
if err := rewriteVolumeMounts(podAdditions.InitContainers); err != nil {
return err
}

podAdditions.Volumes = append(podAdditions.Volumes, corev1.Volume{
Name: pvcName,
VolumeSource: corev1.VolumeSource{
PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
ClaimName: pvcName,
},
},
})

return nil
return rewriteContainerVolumeMounts(workspaceId, pvcName, podAdditions, workspace, restrictedFields,
func(workspaceId, volumeName string) string {
return fmt.Sprintf("%s/%s", workspaceId, volumeName)
})
}
78 changes: 4 additions & 74 deletions pkg/provision/storage/perWorkspaceStorage.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,6 @@ import (
"github.com/devfile/devworkspace-operator/pkg/common"
"github.com/devfile/devworkspace-operator/pkg/constants"
"github.com/devfile/devworkspace-operator/pkg/dwerrors"
devfileConstants "github.com/devfile/devworkspace-operator/pkg/library/constants"
"github.com/devfile/devworkspace-operator/pkg/library/overrides"
"github.com/devfile/devworkspace-operator/pkg/library/overrides/restrictions"
nsconfig "github.com/devfile/devworkspace-operator/pkg/provision/config"
"github.com/devfile/devworkspace-operator/pkg/provision/sync"
Expand Down Expand Up @@ -104,78 +102,10 @@ func (p *PerWorkspaceStorageProvisioner) rewriteContainerVolumeMounts(
workspace *dw.DevWorkspaceTemplateSpec,
restrictedFields []string,
) error {
devfileVolumes := map[string]dw.VolumeComponent{}

// Construct map of volume name -> volume Component
for _, component := range workspace.Components {
if component.Volume != nil {
if _, exists := devfileVolumes[component.Name]; exists {
return fmt.Errorf("volume component '%s' is defined multiple times", component.Name)
}
devfileVolumes[component.Name] = *component.Volume
}
}

// Containers in podAdditions may reference e.g. automounted volumes in their volumeMounts, and this is not an error
additionalVolumes := map[string]bool{}
for _, additionalVolume := range podAdditions.Volumes {
additionalVolumes[additionalVolume.Name] = true
}

// Containers in podAdditions may reference volumes defined in pod overrides, and this is not an error
overridesVolumes, err := overrides.GetVolumesFromOverrides(workspace, restrictedFields)
if err != nil {
return err
}
for volumeName, present := range overridesVolumes {
additionalVolumes[volumeName] = present
}

// Add implicit projects volume to support mountSources, if needed
if _, exists := devfileVolumes[devfileConstants.ProjectsVolumeName]; !exists {
projectsVolume := dw.VolumeComponent{}
projectsVolume.Size = constants.PVCStorageSize
devfileVolumes[devfileConstants.ProjectsVolumeName] = projectsVolume
}

// TODO: What should we do when a volume isn't explicitly defined?
rewriteVolumeMounts := func(containers []corev1.Container) error {
for cIdx, container := range containers {
for vmIdx, vm := range container.VolumeMounts {
volume, ok := devfileVolumes[vm.Name]
if !ok {
// Volume is defined outside of the devfile
if additionalVolumes[vm.Name] {
continue
}
// Should never happen as flattened Devfile is validated.
return fmt.Errorf("container '%s' references undefined volume '%s'", container.Name, vm.Name)
}
if !isEphemeral(&volume) {
containers[cIdx].VolumeMounts[vmIdx].SubPath = vm.Name
containers[cIdx].VolumeMounts[vmIdx].Name = pvcName
}
}
}
return nil
}
if err := rewriteVolumeMounts(podAdditions.Containers); err != nil {
return err
}
if err := rewriteVolumeMounts(podAdditions.InitContainers); err != nil {
return err
}

podAdditions.Volumes = append(podAdditions.Volumes, corev1.Volume{
Name: pvcName,
VolumeSource: corev1.VolumeSource{
PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
ClaimName: pvcName,
},
},
})

return nil
return rewriteContainerVolumeMounts(workspaceId, pvcName, podAdditions, workspace, restrictedFields,
func(_, volumeName string) string {
return volumeName
})
}

func getPVCSize(workspace *common.DevWorkspaceWithConfig, namespacedConfig *nsconfig.NamespacedConfig) (*resource.Quantity, error) {
Expand Down
89 changes: 89 additions & 0 deletions pkg/provision/storage/shared.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
"github.com/devfile/devworkspace-operator/pkg/constants"
devfileConstants "github.com/devfile/devworkspace-operator/pkg/library/constants"
containerlib "github.com/devfile/devworkspace-operator/pkg/library/container"
"github.com/devfile/devworkspace-operator/pkg/library/overrides"
nsconfig "github.com/devfile/devworkspace-operator/pkg/provision/config"
)

Expand Down Expand Up @@ -338,3 +339,91 @@ func checkPVCTerminating(name, namespace string, api sync.ClusterAPI) (bool, err
}
return pvc.DeletionTimestamp != nil, nil
}

// rewriteContainerVolumeMounts rewrites the VolumeMounts in a set of PodAdditions so that each non-ephemeral
// volume mount becomes a subpath into the provided PVC. The subpath for each mount is computed by subPathFunc,
// allowing callers to implement the different PVC strategies (e.g. workspace-id-prefixed subpaths for the common
// strategy, and bare volume-name subpaths for the per-workspace strategy).
//
// It also adds the PVC-backed k8s Volume to podAdditions to accommodate the rewritten VolumeMounts.
Comment thread
tolusha marked this conversation as resolved.
// Must be called at most once per PodAdditions: the PVC-backed Volume is appended unconditionally.
func rewriteContainerVolumeMounts(
workspaceId, pvcName string,
podAdditions *v1alpha1.PodAdditions,
workspace *dw.DevWorkspaceTemplateSpec,
restrictedFields []string,
subPathFunc func(workspaceId, volumeName string) string,
Comment thread
tolusha marked this conversation as resolved.
) error {
devfileVolumes := map[string]dw.VolumeComponent{}

// Construct map of volume name -> volume Component
for _, component := range workspace.Components {
if component.Volume != nil {
if _, exists := devfileVolumes[component.Name]; exists {
return fmt.Errorf("volume component '%s' is defined multiple times", component.Name)
}
devfileVolumes[component.Name] = *component.Volume
}
}

// Containers in podAdditions may reference e.g. automounted volumes in their volumeMounts, and this is not an error
additionalVolumes := map[string]bool{}
for _, additionalVolume := range podAdditions.Volumes {
additionalVolumes[additionalVolume.Name] = true
}

// Containers in podAdditions may reference volumes defined in pod overrides, and this is not an error
overridesVolumes, err := overrides.GetVolumesFromOverrides(workspace, restrictedFields)

@tolusha tolusha Sep 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not a change request, just noting it while the context is fresh.

This branch has no test coverage. I checked every fixture under testdata/common-storage/ and testdata/perWorkspace-storage/ and none of them define pod overrides, so GetVolumesFromOverrides returns an empty map in every test and the additionalVolumes[vm.Name] escape below is never reached via the overrides path.

The gap is pre-existing and should not block this PR. Worth mentioning because the extraction makes it cheaper to close: it used to need two fixtures kept in sync, now a single one covers both strategies. Does it make sense to add one as a follow-up?

@SAY-5 SAY-5 Sep 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Yeah, a single pod-overrides fixture should cover both strategies now. I can do that as a follow-up.

if err != nil {
return err
}
for volumeName, present := range overridesVolumes {
additionalVolumes[volumeName] = present
}

// Add implicit projects volume to support mountSources, if needed
if _, exists := devfileVolumes[devfileConstants.ProjectsVolumeName]; !exists {
projectsVolume := dw.VolumeComponent{}
projectsVolume.Size = constants.PVCStorageSize
devfileVolumes[devfileConstants.ProjectsVolumeName] = projectsVolume
}

// TODO: What should we do when a volume isn't explicitly defined?
rewriteVolumeMounts := func(containers []corev1.Container) error {
for cIdx, container := range containers {
for vmIdx, vm := range container.VolumeMounts {
volume, ok := devfileVolumes[vm.Name]
if !ok {
// Volume is defined outside of the devfile
if additionalVolumes[vm.Name] {
continue
}
// Should never happen as flattened Devfile is validated.
return fmt.Errorf("container '%s' references undefined volume '%s'", container.Name, vm.Name)
}
if !isEphemeral(&volume) {
containers[cIdx].VolumeMounts[vmIdx].SubPath = subPathFunc(workspaceId, vm.Name)
containers[cIdx].VolumeMounts[vmIdx].Name = pvcName
}
}
}
return nil
}
if err := rewriteVolumeMounts(podAdditions.Containers); err != nil {
return err
}
if err := rewriteVolumeMounts(podAdditions.InitContainers); err != nil {
return err
}

podAdditions.Volumes = append(podAdditions.Volumes, corev1.Volume{
Name: pvcName,
VolumeSource: corev1.VolumeSource{
PersistentVolumeClaim: &corev1.PersistentVolumeClaimVolumeSource{
ClaimName: pvcName,
},
},
})

return nil
}