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 10, 2020
1 parent ee8c5d6 commit 7f748a2
Show file tree
Hide file tree
Showing 2 changed files with 137 additions and 48 deletions.
179 changes: 131 additions & 48 deletions frontend/public/components/storage/attach-storage.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,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 +62,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((err) => setError(err))
: setClaimVolumeMode(_.get(newPVCObj, 'spec.volumeMode', ''));
}, [newPVCObj, obj, claimName, showCreatePVC, claimVolumeMode, namespace]);

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

const getDevicePaths = (podTemplate: any): string[] => {
const containers: ContainerSpec[] = _.get(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 +164,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,6 +191,37 @@ export const AttachStorageForm: React.FC<AttachStorageFormProps> = (props) => {
}
return patch;
}, []);
};

const getVolumeDevicePatches = (): any[] => {
const device = {
name: volumeName,
devicePath,
};
const containers: ContainerSpec[] = _.get(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 = (pvClaimName: string) => {
const patches =
claimVolumeMode === 'Block' ? getVolumeDevicePatches() : getVolumeMountPatches();
const volume = {
name: volumeName,
persistentVolumeClaim: {
Expand Down Expand Up @@ -254,52 +313,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="mount-path-help">
Device path for the 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 7f748a2

Please sign in to comment.