Bug 1867400: Not allow creation of second storagecluster #7829

afreen23 · 2021-01-13T17:26:58Z

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

openshift-ci-robot · 2021-01-13T17:27:03Z

@afreen23: An error was encountered adding this pull request to the external tracker bugs for bug 1867400 on the Bugzilla server at https://bugzilla.redhat.com. We were able to detect the following conditions from the error:

The Bugzilla server failed to load data from GitHub when creating the bug. This is usually caused by rate-limiting, please try again later.

Full error message.


JSONRPC error 32000: There was an error reported for the RPC call to Jira: There was an error reported for a GitHub REST call. URL: https://api.github.com/repos/openshift/console/pulls/7829 Error: 403 Forbidden at /loader/0x556e19102468/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 111. at /loader/0x556e19102468/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 111. eval {...} called at /loader/0x556e19102468/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 98 Bugzilla::Extension::ExternalBugs::Type::GitHub::_do_rest_call('Bugzilla::Extension::ExternalBugs::Type::GitHub=HASH(0x556e2b...', 'https://api.github.com/repos/openshift/console/pulls/7829', 'GET') called at /loader/0x556e19102468/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 62 Bugzilla::Extension::ExternalBugs::Type::GitHub::get_data('Bugzilla::Extension::ExternalBugs::Type::GitHub=HASH(0x556e2b...', 'Bugzilla::Extension::ExternalBugs::Bug=HASH(0x556e2b425128)') called at /loader/0x556e19102468/Bugzilla/Extension/ExternalBugs/Bug.pm line 302 eval {...} called at /loader/0x556e19102468/Bugzilla/Extension/ExternalBugs/Bug.pm line 302 Bugzilla::Extension::ExternalBugs::Bug::update_ext_info('Bugzilla::Extension::ExternalBugs::Bug=HASH(0x556e2b425128)', 1) called at /loader/0x556e19102468/Bugzilla/Extension/ExternalBugs/Bug.pm line 125 Bugzilla::Extension::ExternalBugs::Bug::create('Bugzilla::Extension::ExternalBugs::Bug', 'HASH(0x556e2b4d8710)') called at /var/www/html/bugzilla/extensions/ExternalBugs/Extension.pm line 940 Bugzilla::Extension::ExternalBugs::bug_start_of_update('Bugzilla::Extension::ExternalBugs=HASH(0x556e2b589598)', 'HASH(0x556e2abda510)') called at /var/www/html/bugzilla/Bugzilla/Hook.pm line 21 Bugzilla::Hook::process('bug_start_of_update', 'HASH(0x556e2abda510)') called at /var/www/html/bugzilla/Bugzilla/Bug.pm line 1173 Bugzilla::Bug::update('Bugzilla::Bug=HASH(0x556e2b583048)') called at /loader/0x556e19102468/Bugzilla/Extension/ExternalBugs/WebService.pm line 88 Bugzilla::Extension::ExternalBugs::WebService::add_external_bug('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x556e2ab34480)') called at (eval 2738) 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(0x556e2b506738)') called at /var/www/html/bugzilla/Bugzilla/WebService/Server/JSONRPC.pm line 297 Bugzilla::WebService::Server::JSONRPC::_handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x556e2b506738)') 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(0x556e2b1caf48)') 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(0x556e2b5781b0)') called at /usr/lib64/perl5/vendor_perl/ModPerl/RegistryCooker.pm line 173 ModPerl::RegistryCooker::default_handler('Bugzilla::ModPerl::ResponseHandler=HASH(0x556e2b5781b0)') called at /usr/lib64/perl5/vendor_perl/ModPerl/Registry.pm line 32 ModPerl::Registry::handler('Bugzilla::ModPerl::ResponseHandler', 'Apache2::RequestRec=SCALAR(0x556e2b1caf48)') called at /var/www/html/bugzilla/mod_perl.pl line 139 Bugzilla::ModPerl::ResponseHandler::handler('Bugzilla::ModPerl::ResponseHandler', 'Apache2::RequestRec=SCALAR(0x556e2b1caf48)') called at (eval 2738) line 0 eval {...} called at (eval 2738) line 0 at /var/www/html/bugzilla/Bugzilla/Error.pm line 130. Bugzilla::Error::_throw_error('global/user-error.html.tmpl', 'ext_bz_rest_error', 'HASH(0x556e2b5b7d58)') called at /var/www/html/bugzilla/Bugzilla/Error.pm line 193 Bugzilla::Error::ThrowUserError('ext_bz_rest_error', 'HASH(0x556e2b5b7d58)') called at /loader/0x556e19102468/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 120 Bugzilla::Extension::ExternalBugs::Type::GitHub::_do_rest_call('Bugzilla::Extension::ExternalBugs::Type::GitHub=HASH(0x556e2b...', 'https://api.github.com/repos/openshift/console/pulls/7829', 'GET') called at /loader/0x556e19102468/Bugzilla/Extension/ExternalBugs/Type/GitHub.pm line 62 Bugzilla::Extension::ExternalBugs::Type::GitHub::get_data('Bugzilla::Extension::ExternalBugs::Type::GitHub=HASH(0x556e2b...', 'Bugzilla::Extension::ExternalBugs::Bug=HASH(0x556e2b425128)') called at /loader/0x556e19102468/Bugzilla/Extension/ExternalBugs/Bug.pm line 302 eval {...} called at /loader/0x556e19102468/Bugzilla/Extension/ExternalBugs/Bug.pm line 302 Bugzilla::Extension::ExternalBugs::Bug::update_ext_info('Bugzilla::Extension::ExternalBugs::Bug=HASH(0x556e2b425128)', 1) called at /loader/0x556e19102468/Bugzilla/Extension/ExternalBugs/Bug.pm line 125 Bugzilla::Extension::ExternalBugs::Bug::create('Bugzilla::Extension::ExternalBugs::Bug', 'HASH(0x556e2b4d8710)') called at /var/www/html/bugzilla/extensions/ExternalBugs/Extension.pm line 940 Bugzilla::Extension::ExternalBugs::bug_start_of_update('Bugzilla::Extension::ExternalBugs=HASH(0x556e2b589598)', 'HASH(0x556e2abda510)') called at /var/www/html/bugzilla/Bugzilla/Hook.pm line 21 Bugzilla::Hook::process('bug_start_of_update', 'HASH(0x556e2abda510)') called at /var/www/html/bugzilla/Bugzilla/Bug.pm line 1173 Bugzilla::Bug::update('Bugzilla::Bug=HASH(0x556e2b583048)') called at /loader/0x556e19102468/Bugzilla/Extension/ExternalBugs/WebService.pm line 88 Bugzilla::Extension::ExternalBugs::WebService::add_external_bug('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x556e2ab34480)') called at (eval 2738) 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(0x556e2b506738)') called at /var/www/html/bugzilla/Bugzilla/WebService/Server/JSONRPC.pm line 297 Bugzilla::WebService::Server::JSONRPC::_handle('Bugzilla::WebService::Server::JSONRPC::Bugzilla::Extension::E...', 'HASH(0x556e2b506738)') 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(0x556e2b1caf48)') 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(0x556e2b5781b0)') called at /usr/lib64/perl5/vendor_perl/ModPerl/RegistryCooker.pm line 173 ModPerl::RegistryCooker::default_handler('Bugzilla::ModPerl::ResponseHandler=HASH(0x556e2b5781b0)') called at /usr/lib64/perl5/vendor_perl/ModPerl/Registry.pm line 32 ModPerl::Registry::handler('Bugzilla::ModPerl::ResponseHandler', 'Apache2::RequestRec=SCALAR(0x556e2b1caf48)') called at /var/www/html/bugzilla/mod_perl.pl line 139 Bugzilla::ModPerl::ResponseHandler::handler('Bugzilla::ModPerl::ResponseHandler', 'Apache2::RequestRec=SCALAR(0x556e2b1caf48)') called at (eval 2738) line 0 eval {...} called at (eval 2738) line 0

Please contact an administrator to resolve this issue, then request a bug refresh with /bugzilla refresh.

In response to this:

Bug 1867400: Not allow creation of second storagecluster

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.

spadgett · 2021-01-15T13:45:23Z

/bugzilla refresh

openshift-ci-robot · 2021-01-15T13:45:30Z

@spadgett: This pull request references Bugzilla bug 1867400, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.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.

cloudbehl · 2021-01-17T12:08:42Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/install-page.tsx

  const [csv, csvLoaded, csvError] = useK8sWatchResource<ClusterServiceVersionKind>(csvResource);
  const [infra, infraLoaded, infraError] = useK8sGet<any>(InfrastructureModel, 'cluster');
+  const [sc, scLoaded, scLoadError] = useK8sGet<ListKind<StorageClusterKind>>(


can you use storageCluster instead of sc as it matches with storageClass.

sure, sounds good!

cloudbehl · 2021-01-18T04:48:50Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/existing-cluster-modal.tsx

+    params: { ns, appName },
+  } = match;
+  const { t } = useTranslation();
+  const [isOpen, setIsOpen] = React.useState(false);


Suggested change

const [isOpen, setIsOpen] = React.useState(false);

const [isOpen, setIsOpen] = React.useState(hasStorageCluster);

I have changed a bit of code now.
This suggestion does not holds now.PTAL again.

cloudbehl · 2021-01-18T04:53:03Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/install-page.tsx

@@ -136,6 +144,11 @@ const InstallCluster: React.FC<InstallClusterProps> = ({ match }) => {
      {mode === MODES.ATTACHED_DEVICES && (
        <CreateAttachedDevicesCluster match={match} mode={mode} />
      )}
+      <ExistingClusterModal
+        match={match}
+        clusterName={sc?.items?.[0]?.metadata?.name}


clusterName should be filtered out based on the item which has status != ignored.
Check this:
https://github.com/openshift/console/blob/master/frontend/packages/ceph-storage-plugin/src/features.ts#L116..L119

We want to block cluster creation , even if a single item is present.
And this condition is only valid to check when someone fires from CLI a cluster when another cluster is already present. It seems an edge case and i dropped it but no harm in putting a check there !!

bipuladh · 2021-02-04T04:08:34Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/existing-cluster-modal.tsx

+  const clusterName = storageCluster.items.find((sc) => sc.status.phase !== 'Ignored').metadata
+    .name;


Suggested change

const clusterName = storageCluster.items.find((sc) => sc.status.phase !== 'Ignored').metadata

.name;

const clusterName = storageCluster.items.find((sc) => sc.status.phase !== 'Ignored')?.metadata

?.name;

This component will be called when at least a cluster is present, hence it will never get to a situation where it can be undefined

bipuladh · 2021-02-04T04:08:44Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/existing-cluster-modal.tsx

+    params: { ns, appName },
+  } = match;
+  const { t } = useTranslation();
+  const [isOpen, setIsOpen] = React.useState(true);


Suggested change

const [isOpen, setIsOpen] = React.useState(true);

const [isOpen, setOpen] = React.useState(true);

I prefer full names :)

bipuladh · 2021-02-04T04:11:57Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/existing-cluster-modal.tsx

+      titleIconVariant="warning"
+      isOpen={isOpen}
+      variant="small"
+      onClose={() => history.goBack()}


Suggested change

onClose={() => history.goBack()}

onClose={() => resourcePathFromModel(ClusterServiceVersionModel, appName, ns)}

Let's return them back to the OLM page no matter how they reach here.

Its returning to Opeartor's page when clicked Go to Operator's page.
On cancel, it will just go back.

what if I come here through a URL link? wouldn't we go back to the last url?

yes, on closing the icon it will go and that's what we need to do. Its on closing the modal.
If you want to go to operator's page then you can click on "Go to operator's page"
Back to OLM page is explicit here. Refer the screenshot

history.goBack() will refer to the last url, if I am not mistaken, so if I reach here from say the dashboard pages by changing the url. Wouldn't closing it lead me back to dashboard?

@bipuladh here we are setting url to go back to the last url
And here we are setting to operator and cluster page.
A total of 3 urls.

Exactly my concern. If the user closes it, do we really want to send them to the last URL or to the OLM details page for OCS Operator?

Yes we want to send them to last url.
If he wants to go to operator page, he will choose operator page

bipuladh · 2021-02-04T06:07:18Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/existing-cluster-modal.tsx

+import { StorageClusterKind } from '../../types';
+import { OCSServiceModel } from '../../models';
+
+export const ExistingClusterModal: React.FC<ExistingClusterModalProps> = ({


Can we make this a default export of this file?

I used named because I want the name to be consistent all over.
Do you think we should make it default considering its a single component ?

I have updated but as a general rule I prefer to do named exports irrespective of a single file.
It messes with typos and intellisense, automcomplete. If this component want to be reused then anyone can export like ExistingClusterModal from './file' or existingClusterModal from './file'

bipuladh · 2021-02-04T06:09:56Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/existing-cluster-modal.tsx

+      onClose={() => history.goBack()}
+      isFullScreen={false}
+      actions={[
+        <Button key="confirm" variant="primary" onClick={onConfirm}>


aria-label for the button

There is text in the button so it will be used.
Adding aria-label will be redundant here, since both the text will be remain same. aria-label is good to add where we dont have clue of what button is doing like <button class="burger"></button> or <button>+</button>

makes sense. thanks

bipuladh · 2021-02-04T06:10:05Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/existing-cluster-modal.tsx

+        <Button key="confirm" variant="primary" onClick={onConfirm}>
+          {t('ceph-storage-plugin~Back to operator page')}
+        </Button>,
+        <Button key="cancel" variant="link" onClick={onCancel}>


aria-label for the button

Same as above #7829

afreen23 · 2021-02-04T06:50:07Z

frontend/packages/ceph-storage-plugin/src/components/ocs-install/existing-cluster-modal.tsx

+  const onConfirm = () => {
+    setIsOpen(false);
+    history.push(resourcePathFromModel(ClusterServiceVersionModel, appName, ns));
+  };
+
+  const onCancel = () => {
+    setIsOpen(false);
+    history.push(storageClusterPath);
+  };


@bipuladh here we are setting url to OLM page

afreen23 · 2021-02-04T08:10:16Z

/test ceph-storage-plugin

… mode in a single OCS Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1867400 Signed-off-by: Afreen Rahman <afrahman@redhat.com>

afreen23 · 2021-02-05T05:11:30Z

I have removed close button due to the concerns on redirection @bipuladh @cloudbehl @yuvalgalanti .
This wont be an issue since we have the respective urls for operator page and cluster page redirection present.
Hope this solves for all :)

bipuladh · 2021-02-05T05:31:04Z

/lgtm

openshift-ci-robot · 2021-02-05T05:31:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afreen23, bipuladh

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/packages/ceph-storage-plugin/OWNERS~~ [afreen23,bipuladh]

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

openshift-bot · 2021-02-05T06:19:56Z

/retest

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

openshift-ci-robot · 2021-02-05T09:10:26Z

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

openshift/console#7829

Bugzilla bug 1867400 has been moved to the MODIFIED state.

In response to this:

Bug 1867400: Not allow creation of second storagecluster

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 bipuladh and rawagner January 13, 2021 17:27

openshift-ci-robot added kind/i18n Indicates issue or PR relates to internationalization or has content that needs to be translated approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 13, 2021

afreen23 force-pushed the bug-fix-only-one-sc branch from 635e200 to 62ebafa Compare January 13, 2021 17:29

openshift-ci-robot added the bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. label Jan 15, 2021

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jan 15, 2021

cloudbehl reviewed Jan 18, 2021

View reviewed changes

afreen23 force-pushed the bug-fix-only-one-sc branch from 62ebafa to db62d63 Compare February 3, 2021 22:37

bipuladh suggested changes Feb 4, 2021

View reviewed changes

afreen23 force-pushed the bug-fix-only-one-sc branch from db62d63 to 7193e99 Compare February 4, 2021 06:43

afreen23 commented Feb 4, 2021

View reviewed changes

Bug 1867400: Not allow creation of second storagecluster of different…

4e6ee02

… mode in a single OCS Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1867400 Signed-off-by: Afreen Rahman <afrahman@redhat.com>

afreen23 force-pushed the bug-fix-only-one-sc branch from 7193e99 to 4e6ee02 Compare February 5, 2021 05:09

afreen23 requested review from bipuladh and cloudbehl February 5, 2021 05:20

openshift-ci-robot assigned bipuladh Feb 5, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 5, 2021

openshift-merge-robot merged commit 04fccb3 into openshift:master Feb 5, 2021

spadgett added this to the v4.7 milestone Feb 5, 2021

	const [isOpen, setIsOpen] = React.useState(false);
	const [isOpen, setIsOpen] = React.useState(hasStorageCluster);

		const clusterName = storageCluster.items.find((sc) => sc.status.phase !== 'Ignored').metadata
		.name;

	const [isOpen, setIsOpen] = React.useState(true);
	const [isOpen, setOpen] = React.useState(true);

	onClose={() => history.goBack()}
	onClose={() => resourcePathFromModel(ClusterServiceVersionModel, appName, ns)}

Bug 1867400: Not allow creation of second storagecluster #7829

Bug 1867400: Not allow creation of second storagecluster #7829

Conversation

afreen23 commented Jan 13, 2021

openshift-ci-robot commented Jan 13, 2021

spadgett commented Jan 15, 2021

openshift-ci-robot commented Jan 15, 2021

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

afreen23 Feb 4, 2021 • edited

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

afreen23 Feb 4, 2021 • edited

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

afreen23 Feb 4, 2021 • edited

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

afreen23 Feb 4, 2021 • edited

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

afreen23 commented Feb 4, 2021

afreen23 commented Feb 5, 2021

bipuladh commented Feb 5, 2021

openshift-ci-robot commented Feb 5, 2021

openshift-bot commented Feb 5, 2021

openshift-ci-robot commented Feb 5, 2021

afreen23 Feb 4, 2021 •

edited

afreen23 Feb 4, 2021 •

edited

afreen23 Feb 4, 2021 •

edited

afreen23 Feb 4, 2021 •

edited