Bug 1853599: Fixes bug attaching block storage PVC to deployments #6221

vbnrh · 2020-08-05T13:49:41Z

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

vbnrh · 2020-08-05T13:52:54Z

/assign cloudbehl

frontend/public/components/storage/attach-storage.tsx

cloudbehl

Add bugzilla id to the PR.

frontend/public/components/storage/attach-storage.tsx

openshift-ci-robot · 2020-08-10T10:29:25Z

@vbnrh: An error was encountered adding this pull request to the external tracker bugs for bug 1853599 on the Bugzilla server at https://bugzilla.redhat.com:

JSONRPC error 32000: There was an error reported for a GitHub REST call. URL: https://api.github.com/repos/openshift/console/pulls/6221 Error: 403 Forbidden at /loader/0x562531de83b0/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 111. at /loader/0x562531de83b0/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 111. eval {...} called at /loader/0x562531de83b0/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 98 Bugzilla::Extension::ExternalBugs::Type::GitHub::_do_rest_call('Bugzilla::Extension::ExternalBugs::Type::GitHub=HASH(0x56253e...', 'https://api.github.com/repos/openshift/console/pulls/6221', 'GET') called at /loader/0x562531de83b0/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 62 Bugzilla::Extension::ExternalBugs::Type::GitHub::get_data('Bugzilla::Extension::ExternalBugs::Type::GitHub=HASH(0x56253e...', 'Bugzilla::Extension::ExternalBugs::Bug=HASH(0x56253b89c840)') called at /loader/0x562531de83b0/Bugzilla/Extension/ExternalBugs/Bug.pm line 302 eval {...} called at /loader/0x562531de83b0/Bugzilla/Extension/ExternalBugs/Bug.pm line 302 Bugzilla::Extension::ExternalBugs::Bug::update_ext_info('Bugzilla::Extension::ExternalBugs::Bug=HASH(0x56253b89c840)', 1) called at /loader/0x562531de83b0/Bugzilla/Extension/ExternalBugs/Bug.pm line 125 Bugzilla::Extension::ExternalBugs::Bug::create('Bugzilla::Extension::ExternalBugs::Bug', 'HASH(0x56253cf820b8)') called at /var/www/html/bugzilla/extensions/ExternalBugs/Extension.pm line 877 Bugzilla::Extension::ExternalBugs::bug_start_of_update('Bugzilla::Extension::ExternalBugs=HASH(0x56253ca872b8)', 'HASH(0x562542172088)') called at /var/www/html/bugzilla/Bugzilla/Hook.pm line 21 Bugzilla::Hook::process('bug_start_of_update', 'HASH(0x562542172088)') called at /var/www/html/bugzilla/Bugzilla/Bug.pm line 1170 Bugzilla::Bug::update('Bugzilla::Bug=HASH(0x56253ca39e48)') called at /loader/0x562531de83b0/Bugzilla/Extension/ExternalBugs/WebService.pm line 88 Bugzilla::Extension::ExternalBugs::WebService::add_external_bug('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x56253bd801b8)') called at (eval 2601) line 1 eval ' $procedure->{code}->($self, @params) ;' called at /usr/share/perl5/vendor_perl/JSON/RPC/Legacy/Server.pm line 220 JSON::RPC::Legacy::Server::_handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x562541bf0a08)') called at /var/www/html/bugzilla/Bugzilla/WebService/Server/JSONRPC.pm line 295 Bugzilla::WebService::Server::JSONRPC::_handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x562541bf0a08)') called at /usr/share/perl5/vendor_perl/JSON/RPC/Legacy/Server.pm line 126 JSON::RPC::Legacy::Server::handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...') called at /var/www/html/bugzilla/Bugzilla/WebService/Server/JSONRPC.pm line 70 Bugzilla::WebService::Server::JSONRPC::handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...') called at /var/www/html/bugzilla/jsonrpc.cgi line 31 ModPerl::ROOT::Bugzilla::ModPerl::ResponseHandler::var_www_html_bugzilla_jsonrpc_2ecgi::handler('Apache2::RequestRec=SCALAR(0x56253bb74408)') called at /usr/lib64/perl5/vendor_perl/ModPerl/RegistryCooker.pm line 207 eval {...} called at /usr/lib64/perl5/vendor_perl/ModPerl/RegistryCooker.pm line 207 ModPerl::RegistryCooker::run('Bugzilla::ModPerl::ResponseHandler=HASH(0x56253bf27738)') called at /usr/lib64/perl5/vendor_perl/ModPerl/RegistryCooker.pm line 173 ModPerl::RegistryCooker::default_handler('Bugzilla::ModPerl::ResponseHandler=HASH(0x56253bf27738)') called at /usr/lib64/perl5/vendor_perl/ModPerl/Registry.pm line 32 ModPerl::Registry::handler('Bugzilla::ModPerl::ResponseHandler', 'Apache2::RequestRec=SCALAR(0x56253bb74408)') called at /var/www/html/bugzilla/mod_perl.pl line 139 Bugzilla::ModPerl::ResponseHandler::handler('Bugzilla::ModPerl::ResponseHandler', 'Apache2::RequestRec=SCALAR(0x56253bb74408)') called at (eval 2601) line 0 eval {...} called at (eval 2601) line 0
Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

In response to this:

Bug 1853599: Fixes bug attaching block storage PVC to deployments

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cloudbehl · 2020-08-10T10:53:58Z

/bugzilla refresh

openshift-ci-robot · 2020-08-10T10:54:04Z

@cloudbehl: This pull request references Bugzilla bug 1853599, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

frontend/public/components/storage/attach-storage.tsx

vbnrh · 2020-08-11T06:57:29Z

/assign @spadgett @rhamilto

spadgett · 2020-08-05T14:11:11Z

frontend/public/components/storage/attach-storage.tsx

+                value={devicePath}
+                required
+              />
+              <p className="help-block" id="mount-path-help">


Suggested change

<p className="help-block" id="mount-path-help">

<p className="help-block" id="volume-device-help">

spadgett · 2020-08-19T13:05:29Z

frontend/public/components/storage/attach-storage.tsx

    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)


Don't we already have all the PVCs since we get them for the dropdown? It would be nice to use the object we already have instead of requesting it again.

Currently the component only returns the name of the PVC and not the entire PVC object. We may have to change the entire component to return a list of objects for that to be possible.

How hard is it to update the ListDropdown onChange prop to take a 3rd optional argument with the resource? Then we could skip this get, and it could be useful in other contexts.

changed it to use obj from listdropdown. thanks for the input.

spadgett · 2020-08-19T13:05:47Z

frontend/public/components/storage/attach-storage.tsx

+    showCreatePVC === 'existing' && newClaimName.trim().length > 0
+      ? k8sGet(PersistentVolumeClaimModel, newClaimName, namespace)
+          .then((pvc) => {
+            setClaimVolumeMode(pvc?.spec?.volumeMode);


The optional chaining here shouldn't be necessary.

spadgett · 2020-08-19T13:05:58Z

frontend/public/components/storage/attach-storage.tsx

+          .then((pvc) => {
+            setClaimVolumeMode(pvc?.spec?.volumeMode);
+          })
+          .catch((err) => setError(err))


Suggested change

.catch((err) => setError(err))

.catch(setError)

spadgett · 2020-08-19T13:28:05Z

frontend/public/components/storage/attach-storage.tsx

@@ -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', []);


Use optional chaining instead of _.get

spadgett · 2020-08-19T13:29:25Z

frontend/public/components/storage/attach-storage.tsx

@@ -117,6 +125,27 @@ export const AttachStorageForm: React.FC<AttachStorageFormProps> = (props) => {
    validateMountPaths(event.currentTarget.value);
  };

+  const getDevicePaths = (podTemplate: any): string[] => {


We should have a TypeScript type for pod templates

spadgett · 2020-08-19T13:30:01Z

frontend/public/components/storage/attach-storage.tsx

+    const err = existingDevicePaths.includes(path) ? 'Device path is already in use.' : '';
+    setError(err);


Could this clear a different error that might have been previously set?

Yes it will set the last occurred error and display it to the user

But sometimes you call setError with '' as the value. It will clear any previous error in that case.

Ahh.... i got your point now. The changes should be suffice now

spadgett · 2020-08-19T13:31:19Z

frontend/public/components/storage/attach-storage.tsx

+      return patch;
+    }, []);
+  };
+  const getVolumePatches = (pvClaimName: string) => {


Suggested change

const getVolumePatches = (pvClaimName: string) => {

const getVolumePatches = (pvcName: string) => {

spadgett · 2020-08-19T13:31:35Z

frontend/public/components/storage/attach-storage.tsx

+      name: volumeName,
+      devicePath,
+    };
+    const containers: ContainerSpec[] = _.get(obj, 'spec.template.spec.containers', []);


Prefer optional chaining for new code

vbnrh · 2020-08-25T17:03:34Z

/retest

spadgett · 2020-08-25T15:00:41Z

frontend/public/components/storage/attach-storage.tsx

  const [newPVCObj, setNewPVCObj] = React.useState(null);
+  const [pvcResource, setPVCResource] = React.useState(null);


Suggested change

const [pvcResource, setPVCResource] = React.useState(null);

const [selectedPVC, setSelectedPVC] = React.useState<PersistentVolumeClaimKind>(null);

spadgett · 2020-09-03T19:28:49Z

frontend/public/components/storage/attach-storage.tsx

    const newVolumeName = volume ? volume.name : newClaimName;
    const newVolumeAlreadyMounted = !!volume;
    setVolumeName(newVolumeName);
    setVolumeAlreadyMounted(newVolumeAlreadyMounted);
-  }, [newPVCObj, obj, claimName, showCreatePVC]);
+    showCreatePVC === 'existing' && newClaimName.trim().length > 0


Why check newClaimName when it's an existing PVC?

const newClaimName =
showCreatePVC === 'existing' ? claimName : _.get(newPVCObj, 'metadata.name', '');`

newClaimName can represent both the exisiting and newer claim name.

spadgett · 2020-09-03T19:31:17Z

frontend/public/components/storage/attach-storage.tsx

@@ -135,16 +168,15 @@ export const AttachStorageForm: React.FC<AttachStorageFormProps> = (props) => {
      : Promise.resolve(claimName);
  };

-  const getVolumePatches = (pvClaimName: string) => {
+  const getVolumeMountPatches = (): any[] => {


Not specifying a type and letting TypeScript infer it would be better than any, but we do have a Patch type.

Suggested change

const getVolumeMountPatches = (): any[] => {

const getVolumeMountPatches = (): Patch[] => {

spadgett · 2020-09-03T19:31:37Z

frontend/public/components/storage/attach-storage.tsx

@@ -163,10 +195,41 @@ export const AttachStorageForm: React.FC<AttachStorageFormProps> = (props) => {
      }
      return patch;
    }, []);
+  };
+
+  const getVolumeDevicePatches = (): any[] => {


Suggested change

const getVolumeDevicePatches = (): any[] => {

const getVolumeDevicePatches = (): Patch[] => {

frontend/public/components/utils/list-dropdown.jsx

spadgett · 2020-09-03T19:35:58Z

frontend/public/components/utils/pvc-dropdown.tsx

@@ -21,7 +22,7 @@ export const PVCDropdown: React.FC<PVCDropdownProps> = (props) => {
 export type PVCDropdownProps = {
  namespace: string;
  selectedKey: string;
-  onChange: (string) => void;
+  onChange: (claimName: string, kindlabel?: string, pvc?: PersistentVolumeClaimKind) => void;


Suggested change

onChange: (claimName: string, kindlabel?: string, pvc?: PersistentVolumeClaimKind) => void;

onChange: (claimName: string, kindLabel?: string, pvc?: PersistentVolumeClaimKind) => void;

…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>

spadgett

/lgtm

openshift-ci-robot · 2020-09-08T16:40:02Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnehapk, spadgett, vbnrh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2020-09-08T17:51:43Z

@vbnrh: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-gcp-console	`48f057e`	link	`/test e2e-gcp-console`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2020-09-08T18:03:56Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-09-08T18:29:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-09-08T19:29:29Z

@vbnrh: All pull requests linked via external trackers have merged:

openshift/console#6221

Bugzilla bug 1853599 has been moved to the MODIFIED state.

In response to this:

Bug 1853599: Fixes bug attaching block storage PVC to deployments

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

vbnrh · 2020-09-14T08:10:36Z

/cherry-pick release-4.5

openshift-cherrypick-robot · 2020-09-14T08:10:40Z

@vbnrh: #6221 failed to apply on top of branch "release-4.5":

Applying: Fixes bug attaching block storage to pods, deployments in attach-storage page
Using index info to reconstruct a base tree...
M	frontend/public/components/storage/attach-storage.tsx
M	frontend/public/components/utils/list-dropdown.jsx
A	frontend/public/components/utils/pvc-dropdown.tsx
M	frontend/public/module/k8s/types.ts
Falling back to patching base and 3-way merge...
Auto-merging frontend/public/module/k8s/types.ts
CONFLICT (modify/delete): frontend/public/components/utils/pvc-dropdown.tsx deleted in HEAD and modified in Fixes bug attaching block storage to pods, deployments in attach-storage page. Version Fixes bug attaching block storage to pods, deployments in attach-storage page of frontend/public/components/utils/pvc-dropdown.tsx left in tree.
Auto-merging frontend/public/components/utils/list-dropdown.jsx
Auto-merging frontend/public/components/storage/attach-storage.tsx
CONFLICT (content): Merge conflict in frontend/public/components/storage/attach-storage.tsx
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fixes bug attaching block storage to pods, deployments in attach-storage page
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.5

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot requested review from rhamilto and TheRealJon August 5, 2020 13:49

openshift-ci-robot added the component/core Related to console core functionality label Aug 5, 2020

openshift-ci-robot assigned cloudbehl Aug 5, 2020

gnehapk reviewed Aug 6, 2020

View reviewed changes

frontend/public/components/storage/attach-storage.tsx Outdated Show resolved Hide resolved

gnehapk reviewed Aug 6, 2020

View reviewed changes

frontend/public/components/storage/attach-storage.tsx Outdated Show resolved Hide resolved

gnehapk reviewed Aug 6, 2020

View reviewed changes

frontend/public/components/storage/attach-storage.tsx Outdated Show resolved Hide resolved

cloudbehl reviewed Aug 6, 2020

View reviewed changes

vbnrh changed the title ~~Fixes bug attaching block storage to pods, deployments~~ Bug 1853599 : Fixes bug attaching block storage PVC to deployments Aug 10, 2020

vbnrh force-pushed the pvc-block-volume-fix branch from 78a869b to 7f748a2 Compare August 10, 2020 09:39

vbnrh changed the title ~~Bug 1853599 : Fixes bug attaching block storage PVC to deployments~~ Bug 1853599: Fixes bug attaching block storage PVC to deployments Aug 10, 2020

openshift-ci-robot added bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Aug 10, 2020

cloudbehl reviewed Aug 10, 2020

View reviewed changes

frontend/public/components/storage/attach-storage.tsx Outdated Show resolved Hide resolved

frontend/public/components/storage/attach-storage.tsx Outdated Show resolved Hide resolved

frontend/public/components/storage/attach-storage.tsx Outdated Show resolved Hide resolved

vbnrh force-pushed the pvc-block-volume-fix branch from 7f748a2 to 1f6124a Compare August 10, 2020 12:24

gnehapk approved these changes Aug 11, 2020

View reviewed changes

openshift-ci-robot assigned rhamilto and spadgett Aug 11, 2020

spadgett requested changes Aug 19, 2020

View reviewed changes

vbnrh force-pushed the pvc-block-volume-fix branch 2 times, most recently from 083861c to 4f0a2a6 Compare August 25, 2020 14:50

spadgett reviewed Sep 3, 2020

View reviewed changes

vbnrh force-pushed the pvc-block-volume-fix branch from 4f0a2a6 to 48f057e Compare September 7, 2020 10:13

spadgett approved these changes Sep 8, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 8, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 8, 2020

openshift-merge-robot merged commit 9cdb472 into openshift:master Sep 8, 2020

spadgett added this to the v4.6 milestone Sep 11, 2020

	<p className="help-block" id="mount-path-help">
	<p className="help-block" id="volume-device-help">

		const err = existingDevicePaths.includes(path) ? 'Device path is already in use.' : '';
		setError(err);

	const getVolumePatches = (pvClaimName: string) => {
	const getVolumePatches = (pvcName: string) => {

		const [newPVCObj, setNewPVCObj] = React.useState(null);
		const [pvcResource, setPVCResource] = React.useState(null);

	const [pvcResource, setPVCResource] = React.useState(null);
	const [selectedPVC, setSelectedPVC] = React.useState<PersistentVolumeClaimKind>(null);

	const getVolumeMountPatches = (): any[] => {
	const getVolumeMountPatches = (): Patch[] => {

	const getVolumeDevicePatches = (): any[] => {
	const getVolumeDevicePatches = (): Patch[] => {

	onChange: (claimName: string, kindlabel?: string, pvc?: PersistentVolumeClaimKind) => void;
	onChange: (claimName: string, kindLabel?: string, pvc?: PersistentVolumeClaimKind) => void;

Bug 1853599: Fixes bug attaching block storage PVC to deployments #6221

Bug 1853599: Fixes bug attaching block storage PVC to deployments #6221

Conversation

vbnrh commented Aug 5, 2020

vbnrh commented Aug 5, 2020

cloudbehl left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 10, 2020

cloudbehl commented Aug 10, 2020

openshift-ci-robot commented Aug 10, 2020

vbnrh commented Aug 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbnrh commented Aug 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Sep 8, 2020

openshift-ci-robot commented Sep 8, 2020

openshift-bot commented Sep 8, 2020

openshift-bot commented Sep 8, 2020

openshift-ci-robot commented Sep 8, 2020

vbnrh commented Sep 14, 2020

openshift-cherrypick-robot commented Sep 14, 2020