Skip to content

Commit

Permalink
Fixes bug attaching block storage to pods, deployments in attach-stor…
Browse files Browse the repository at this point in the history 
…age page

Using  block storage provisioners with RWX access mode incorrectly attaches the path as volumeMount in PodSpec instead of volumeDevice. This caused the pods to be in a state of ContainerCreation indefinitely. This  commit fixes this by  sending the object with the right configuration for block storage.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1853599

Signed-off-by: Vineet Badrinath <vbadrina@redhat.com>
  • Loading branch information
@vbnrh
vbnrh committed Aug 20, 2020
1 parent 902dd91 commit 083861c
Show file tree
Hide file tree
Showing 2 changed files with 141 additions and 51 deletions.
186 changes: 135 additions & 51 deletions frontend/public/components/storage/attach-storage.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import { CreatePVCForm } from './create-pvc';
import { PersistentVolumeClaimModel } from '../../models';
import { ContainerSelector } from '../container-selector';
import { PVCDropdown } from '../utils/pvc-dropdown';
import { PodTemplate } from '../../../public/module/k8s/types';

export const AttachStorageForm: React.FC<AttachStorageFormProps> = (props) => {
const [obj, setObj] = React.useState(null);
Expand All @@ -27,12 +28,14 @@ export const AttachStorageForm: React.FC<AttachStorageFormProps> = (props) => {
const [claimName, setClaimName] = React.useState('');
const [volumeName, setVolumeName] = React.useState('');
const [mountPath, setMountPath] = React.useState('');
const [devicePath, setDevicePath] = React.useState('');
const [subPath, setSubPath] = React.useState('');
const [mountAsReadOnly, setMountAsReadOnly] = React.useState(false);
const [selectedContainers, setSelectedContainers] = React.useState([]);
const [volumeAlreadyMounted, setVolumeAlreadyMounted] = React.useState(false);
const [error, setError] = React.useState('');
const [showCreatePVC, setShowCreatePVC] = React.useState('existing');
const [claimVolumeMode, setClaimVolumeMode] = React.useState('');
const [newPVCObj, setNewPVCObj] = React.useState(null);

const { kindObj, resourceName, namespace } = props;
Expand Down Expand Up @@ -60,12 +63,18 @@ export const AttachStorageForm: React.FC<AttachStorageFormProps> = (props) => {
claimName: newClaimName,
},
}) as any;

const newVolumeName = volume ? volume.name : newClaimName;
const newVolumeAlreadyMounted = !!volume;
setVolumeName(newVolumeName);
setVolumeAlreadyMounted(newVolumeAlreadyMounted);
}, [newPVCObj, obj, claimName, showCreatePVC]);
showCreatePVC === 'existing' && newClaimName.trim().length > 0
? k8sGet(PersistentVolumeClaimModel, newClaimName, namespace)
.then((pvc) => {
setClaimVolumeMode(pvc.spec.volumeMode);
})
.catch(setError)
: setClaimVolumeMode(newPVCObj?.spec?.volumeMode);
}, [newPVCObj, obj, claimName, showCreatePVC, claimVolumeMode, namespace]);

if (!kindObj || !_.includes(supportedKinds, kindObj.kind)) {
setError('Unsupported kind.');
Expand Down Expand Up @@ -117,6 +126,27 @@ export const AttachStorageForm: React.FC<AttachStorageFormProps> = (props) => {
validateMountPaths(event.currentTarget.value);
};

const getDevicePaths = (podTemplate: PodTemplate): string[] => {
const containers: ContainerSpec[] = podTemplate?.spec?.containers;
return containers.reduce((acc: string[], container: ContainerSpec) => {
if (!isContainerSelected(container)) {
return acc;
}
const devicePaths: string[] = _.map(container.volumeDevices, 'devicePath');
return acc.concat(devicePaths);
}, []);
};

const validateDevicePath = (path: string) => {
const existingDevicePaths = getDevicePaths(obj.spec.template);
const err = existingDevicePaths.includes(path) ? 'Device path is already in use.' : '';
setError(err);
};

const handleDevicePathChange: React.ReactEventHandler<HTMLInputElement> = (event) => {
setDevicePath(event.currentTarget.value);
validateDevicePath(event.currentTarget.value);
};
const handleSubPathChange: React.ReactEventHandler<HTMLInputElement> = (event) => {
setSubPath(event.currentTarget.value);
};
Expand All @@ -135,16 +165,15 @@ export const AttachStorageForm: React.FC<AttachStorageFormProps> = (props) => {
: Promise.resolve(claimName);
};

const getVolumePatches = (pvClaimName: string) => {
const getVolumeMountPatches = (): any[] => {
const mount = {
name: volumeName,
mountPath,
subPath,
readOnly: mountAsReadOnly,
};

const containers: ContainerSpec[] = _.get(obj, 'spec.template.spec.containers', []);
const patches = containers.reduce((patch, container, i) => {
return containers.reduce((patch, container, i) => {
// Only add to selected containers
if (isContainerSelected(container)) {
if (_.isEmpty(container.volumeMounts)) {
Expand All @@ -163,10 +192,41 @@ export const AttachStorageForm: React.FC<AttachStorageFormProps> = (props) => {
}
return patch;
}, []);
};

const getVolumeDevicePatches = (): any[] => {
const device = {
name: volumeName,
devicePath,
};
const containers: ContainerSpec[] = obj?.spec?.template?.spec?.containers;
return containers.reduce((patch, container, i) => {
// Only add to selected containers
if (isContainerSelected(container)) {
if (_.isEmpty(container.volumeMounts)) {
patch.push({
op: 'add',
path: `/spec/template/spec/containers/${i}/volumeDevices`,
value: [device],
});
} else {
patch.push({
op: 'add',
path: `/spec/template/spec/containers/${i}/volumeDevices/-`,
value: device,
});
}
}
return patch;
}, []);
};
const getVolumePatches = (pvcName: string) => {
const patches =
claimVolumeMode === 'Block' ? getVolumeDevicePatches() : getVolumeMountPatches();
const volume = {
name: volumeName,
persistentVolumeClaim: {
claimName: pvClaimName,
claimName: pvcName,
},
};

Expand All @@ -188,8 +248,8 @@ export const AttachStorageForm: React.FC<AttachStorageFormProps> = (props) => {
}
setInProgress(true);
createPVCIfNecessary().then(
(pvClaimName: string) => {
return k8sPatch(kindObj, obj, getVolumePatches(pvClaimName)).then((resource) => {
(pvcName: string) => {
return k8sPatch(kindObj, obj, getVolumePatches(pvcName)).then((resource) => {
setInProgress(false);
history.push(resourceObjPath(resource, referenceFor(resource)));
});
Expand Down Expand Up @@ -254,52 +314,76 @@ export const AttachStorageForm: React.FC<AttachStorageFormProps> = (props) => {
</div>
)}

<div className="form-group">
<label className="control-label co-required" htmlFor="mount-path">
Mount Path
</label>
<div>
<input
className="pf-c-form-control"
type="text"
onChange={handleMountPathChange}
aria-describedby="mount-path-help"
name="mountPath"
id="mount-path"
value={mountPath}
required
/>
<p className="help-block" id="mount-path-help">
Mount path for the volume inside the container.
</p>
{claimVolumeMode === 'Block' ? (
<div className="form-group">
<label className="control-label co-required" htmlFor="device-path">
Device Path
</label>
<div>
<input
className="pf-c-form-control"
type="text"
onChange={handleDevicePathChange}
aria-describedby="volume-device-help"
name="devicePath"
id="device-path"
value={devicePath}
required
/>
<p className="help-block" id="volume-device-help">
Device path for the block volume inside the container.
</p>
</div>
</div>
</div>
<Checkbox
label="Mount as read-only"
onChange={onMountAsReadOnlyChanged}
checked={mountAsReadOnly}
name="mountAsReadOnly"
/>
<div className="form-group">
<label className="control-label" htmlFor="subpath">
Subpath
</label>
<div>
<input
className="pf-c-form-control"
type="text"
onChange={handleSubPathChange}
aria-describedby="subpath-help"
id="subpath"
name="subPath"
value={subPath}
) : (
<div className="form-group">
<label className="control-label co-required" htmlFor="mount-path">
Mount Path
</label>
<div>
<input
className="pf-c-form-control"
type="text"
onChange={handleMountPathChange}
aria-describedby="mount-path-help"
name="mountPath"
id="mount-path"
value={mountPath}
required
/>
<p className="help-block" id="mount-path-help">
Mount path for the volume inside the container.
</p>
</div>
<Checkbox
label="Mount as read-only"
onChange={onMountAsReadOnlyChanged}
checked={mountAsReadOnly}
name="mountAsReadOnly"
/>
<p className="help-block" id="subpath-help">
Optional path within the volume from which it will be mounted into the container.
Defaults to the root of volume.
</p>
<div className="form-group">
<label className="control-label" htmlFor="subpath">
Subpath
</label>
<div>
<input
className="pf-c-form-control"
type="text"
onChange={handleSubPathChange}
aria-describedby="subpath-help"
id="subpath"
name="subPath"
value={subPath}
/>
<p className="help-block" id="subpath-help">
Optional path within the volume from which it will be mounted into the container.
Defaults to the root of volume.
</p>
</div>
</div>
</div>
</div>
)}

{!useContainerSelector && (
<p>
The volume will be mounted into all containers. You can{' '}
Expand Down
6 changes: 6 additions & 0 deletions frontend/public/module/k8s/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,11 @@ export type VolumeMount = {
subPathExpr?: string;
};

export type VolumeDevice = {
devicePath: string;
name: string;
};

type ProbePort = string | number;

export type ExecProbe = {
Expand Down Expand Up @@ -247,6 +252,7 @@ export type PodAffinity = {
export type ContainerSpec = {
name: string;
volumeMounts?: VolumeMount[];
volumeDevices?: VolumeDevice[];
env?: EnvVar[];
livenessProbe?: ContainerProbe;
readinessProbe?: ContainerProbe;
Expand Down

0 comments on commit 083861c

Please sign in to comment.