Added ceph osd tree related test cases for OCS expand #3793

gnehapk · 2019-12-17T00:32:23Z

No description provided.

afreen23 · 2019-12-17T04:54:40Z

frontend/packages/ceph-storage-plugin/integration-tests/utils/helpers.ts

+export const getZoneIds = (nodes) =>
+  nodes.filter((node) => node.type === ZONE).map((node) => node.id);
+
+export const getNodeIds = (nodes) =>
+  nodes.filter((node) => node.type === HOST).map((node) => node.id);
+
+export const getOSDIds = (nodes) =>
+  nodes.filter((node) => node.type === OSD).map((node) => node.id);


Suggestion:

getIds = (nodes, type) => nodes.filter((node) => node.type === type).map((node) => node.id);

afreen23 · 2019-12-17T04:59:33Z

frontend/packages/ceph-storage-plugin/integration-tests/utils/helpers.ts

+export const getOSDIds = (nodes) =>
+  nodes.filter((node) => node.type === OSD).map((node) => node.id);
+
+export const getNewOSDIds = (nodes, osds) =>


afreen23 · 2019-12-17T04:59:51Z

frontend/packages/ceph-storage-plugin/integration-tests/utils/helpers.ts

+export const createOSDTreeMap = (nodes) => {
+  const tree = {};
+  nodes.forEach((node) => {
+    tree[node.id] = node;
+  });
+  return tree;
+};
+
+export const verifyZoneOSDMapping = (zones, osds, osdtree) => {
+  let filteredOsds = [];
+  zones.forEach((zone) => {
+    const hostId = osdtree[zone].children[0];
+    const len = osdtree[hostId].children.length;
+    filteredOsds = osds.filter((osd) => osd !== osdtree[hostId].children[len - 1]);
+  });
+
+  return filteredOsds.length === 0;
+};
+
+export const verifyNodeOSDMapping = (nodes, osds, osdtree) => {
+  let filteredOsds = [];
+  nodes.forEach((node) => {
+    const len = osdtree[node].children.length;
+    filteredOsds = osds.filter((osd) => osd !== osdtree[node].children[len - 1]);


frontend/packages/ceph-storage-plugin/integration-tests/utils/helpers.ts

gnehapk · 2019-12-18T02:28:13Z

@afreen23 @bipuladh Please review.

afreen23 · 2019-12-18T03:21:05Z

frontend/packages/ceph-storage-plugin/integration-tests/tests/2-tests/add-capacity.scenario.ts

@@ -92,6 +106,11 @@ if (clusterStatus && cephHealth) {
      await verifyFields();
      await click(confirmButton);

+      expansionObjects.osdtree = JSON.parse(
+        execSync(
+          `oc -n openshift-storage rsh  $(oc -n openshift-storage get pod | grep ceph-operator| awk '{print$1}') ceph --conf=/var/lib/rook/openshift-storage/openshift-storage.config osd tree --format=json`,


Suggested change

`oc -n openshift-storage rsh $(oc -n openshift-storage get pod | grep ceph-operator| awk '{print$1}') ceph --conf=/var/lib/rook/openshift-storage/openshift-storage.config osd tree --format=json`,

`oc -n ${NS} rsh $(oc -n ${NS} get pod | grep ceph-operator| awk '{print$1}') ceph --conf=/var/lib/rook/${NS}/${NS}.config osd tree --format=json`,

frontend/packages/ceph-storage-plugin/integration-tests/tests/2-tests/add-capacity.scenario.ts

afreen23 · 2019-12-18T03:58:54Z

frontend/packages/ceph-storage-plugin/integration-tests/tests/2-tests/add-capacity.scenario.ts

@@ -44,6 +54,10 @@ const expansionObjects = {
  updatedClusterJSON: {},
  previousPods: { items: [] as PodKind[] },
  updatedPods: { items: [] as PodKind[] },
+  osdtree: { nodes: [] as nodeType[] },


You have already defined osdTreeType below.

Suggested change

osdtree: { nodes: [] as nodeType[] },

osdtree: <osdTreeType>{},

or

Suggested change

osdtree: { nodes: [] as nodeType[] },

osdtree: {} as osdTreeType,

Both are different. osdtree has nodes which is of nodeType[] and not node.

You should have one structure for osdTree, if you are creating one for formatted , then name it properly.
It can cause readability issues.

AFAIK , this variable osdTree s the actual osd tree , so its type should be defined as OsdTreeType.
Then you are creating a map out of it, you can name it nodeMap or whatever you feel alongwith its type.

I will prefer not to create OsdTree type separately as its used just at one place. Ack on the map object and will rename it to FormattedOsdTreeType as it makes more sense.

afreen23 · 2019-12-18T03:59:43Z

frontend/packages/ceph-storage-plugin/integration-tests/tests/2-tests/add-capacity.scenario.ts

  previousPods: { items: [] as PodKind[] },
  updatedPods: { items: [] as PodKind[] },


you must define a proper type for this, since its being reused and it look more cleaner.

afreen23 · 2019-12-18T04:02:09Z

frontend/packages/ceph-storage-plugin/integration-tests/utils/helpers.ts

+  return filteredOsds.length === 0;
+};
+
+export type nodeType = {


I prefer to have type names defined in Pascal case. It helps in differentiating between variable names and keys defined within an object type.
Also omits the need of suffix: type

Suggested change

export type nodeType = {

export type Node = {

Ack for the Pascal case. Whats the reason for omitting type. Its being added to differentiate its type name from the variable name

You can add if you want, defining in Pascal works thats why I dont encourage.

afreen23 · 2019-12-18T04:02:25Z

frontend/packages/ceph-storage-plugin/integration-tests/utils/helpers.ts

+  primary_affinity?: number;
+};
+
+export type osdTreeType = {


Suggested change

export type osdTreeType = {

export type OsdTree = {

gnehapk · 2019-12-18T13:12:32Z

/test e2e-gcp-console

afreen23 · 2019-12-18T08:23:53Z

frontend/packages/ceph-storage-plugin/integration-tests/tests/2-tests/add-capacity.scenario.ts

  });
 }
+
+type Pod = {


Let be consistent.

Suggested change

type Pod = {

type PodType = {

afreen23 · 2019-12-18T08:24:08Z

frontend/packages/ceph-storage-plugin/integration-tests/tests/2-tests/add-capacity.scenario.ts

+  previousCnt: number;
+  updatedCnt: number;
+  updatedClusterJSON: {};
+  previousPods: Pod;


Suggested change

previousPods: Pod;

previousPods: PodType;

afreen23 · 2019-12-18T08:24:18Z

frontend/packages/ceph-storage-plugin/integration-tests/tests/2-tests/add-capacity.scenario.ts

+  updatedCnt: number;
+  updatedClusterJSON: {};
+  previousPods: Pod;
+  updatedPods: Pod;


Suggested change

updatedPods: Pod;

updatedPods: PodType;

gnehapk · 2019-12-18T15:43:25Z

@ebondare @rhrazdil @pumpmypic @hnallurv Please review.

frontend/packages/ceph-storage-plugin/integration-tests/tests/2-tests/add-capacity.scenario.ts

afreen23 · 2019-12-19T00:49:19Z

frontend/packages/ceph-storage-plugin/integration-tests/tests/2-tests/add-capacity.scenario.ts

@@ -92,6 +106,11 @@ if (clusterStatus && cephHealth) {
      await verifyFields();
      await click(confirmButton);

+      expansionObjects.osdTree = JSON.parse(


this should be fetched twice, once before expansion and once after ?

Not needed. Only need to test with the ceph osd tree reponse post expansion.

afreen23 · 2019-12-19T07:35:46Z

frontend/packages/ceph-storage-plugin/integration-tests/tests/2-tests/add-capacity.scenario.ts

+        execSync(
+          `oc -n ${NS} rsh  $(oc -n ${NS} get pod | grep ceph-operator| awk '{print$1}') ceph --conf=/var/lib/rook/${NS}/${NS}.config osd tree --format=json`,
+        ).toString(),
+      );
      const statusCol = storageClusterRow(uid).$('td:nth-child(4)');


This is not reliable, in 4.4 the status is in 3 column.
Use something else.

Don't see any other option. This test is written w.r.t 4.3 setup.

So, we are okay with breaking tests in 4.4+. This will cause timeout error, because it will never match "ready" status. I am not into integration testing but may be adding ids (though its touching core). There would be some way may be xpath ?
Or non reliable way would be to send seperate fixes for each branch.
@bipuladh might be able to help ?

My point of view to these inter-version changes in the UI is that once a PR is directed to console/master, it should be working in console/master.
I know it's hard to maintain upstream tests working, especially without a gating job that would enforce developers to fix tests that their changes break.

But if we implement test cases intentionally for old versions, it inevitably means that more time will be invested (or more like wasted, as it's not really an investment) into fixing the test for the next version.

Agree with @rhrazdil . I checked tests are not backported to 4.3/4.2 due to code freeze.
Hence it makes sense to write tests against master now and follow this suggested way to maintain.

For adding class to this status field, require changes in the common components i.e. OperandTableHeader inside OperandList which is being used by all the provided APIs views. Hence, I don't think that its a good idea to modify the common component just for adding class, as I am not sure, what impacts it might have.

shyRozen · 2019-12-22T11:40:45Z

/test e2e-gcp-console

afreen23

The logic looks good now

gnehapk · 2019-12-28T16:23:31Z

/test e2e-gcp-console

gnehapk · 2020-01-02T11:57:28Z

/test e2e-gcp-console

gnehapk · 2020-01-06T03:15:51Z

/test e2e-gcp-console

bipuladh · 2020-01-08T05:12:34Z

/approve

gnehapk · 2020-01-08T06:23:34Z

/lgtm

openshift-ci-robot · 2020-01-08T06:23:35Z

@gnehapk: you cannot LGTM your own PR.

In response to this:

/lgtm

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.

bipuladh · 2020-01-08T06:24:44Z

/lgtm

openshift-ci-robot · 2020-01-08T06:25:01Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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,gnehapk]

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 added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 17, 2019

openshift-ci-robot requested review from afreen23 and rawagner December 17, 2019 00:32

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. component/ceph Related to ceph-storage-plugin labels Dec 17, 2019

gnehapk force-pushed the expand-test branch from dc0a158 to fb62761 Compare December 17, 2019 00:35

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2019

gnehapk force-pushed the expand-test branch from fb62761 to e505f33 Compare December 17, 2019 00:42

afreen23 reviewed Dec 17, 2019

View reviewed changes

frontend/packages/ceph-storage-plugin/integration-tests/utils/helpers.ts Show resolved Hide resolved

afreen23 reviewed Dec 17, 2019

View reviewed changes

frontend/packages/ceph-storage-plugin/integration-tests/utils/helpers.ts Show resolved Hide resolved

gnehapk force-pushed the expand-test branch from e505f33 to 5ba2e44 Compare December 17, 2019 13:21

openshift-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 17, 2019

gnehapk force-pushed the expand-test branch from 5ba2e44 to ae22f8d Compare December 17, 2019 14:21

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 17, 2019

afreen23 suggested changes Dec 18, 2019

View reviewed changes

gnehapk force-pushed the expand-test branch from ae22f8d to 3e10510 Compare December 18, 2019 06:57

afreen23 reviewed Dec 18, 2019

View reviewed changes

gnehapk force-pushed the expand-test branch from 3e10510 to e411291 Compare December 18, 2019 13:53

afreen23 reviewed Dec 19, 2019

View reviewed changes

Added ceph osd tree related test cases for OCS expand

1bf4a9c

gnehapk force-pushed the expand-test branch from e411291 to 1bf4a9c Compare December 26, 2019 08:35

afreen23 approved these changes Dec 27, 2019

View reviewed changes

openshift-ci-robot assigned bipuladh Jan 8, 2020

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

openshift-merge-robot merged commit 68af0f5 into openshift:master Jan 8, 2020

spadgett added this to the v4.4 milestone Jan 14, 2020

	`oc -n openshift-storage rsh $(oc -n openshift-storage get pod \| grep ceph-operator\| awk '{print$1}') ceph --conf=/var/lib/rook/openshift-storage/openshift-storage.config osd tree --format=json`,
	`oc -n ${NS} rsh $(oc -n ${NS} get pod \| grep ceph-operator\| awk '{print$1}') ceph --conf=/var/lib/rook/${NS}/${NS}.config osd tree --format=json`,

	osdtree: { nodes: [] as nodeType[] },
	osdtree: <osdTreeType>{},

	osdtree: { nodes: [] as nodeType[] },
	osdtree: {} as osdTreeType,

		previousPods: { items: [] as PodKind[] },
		updatedPods: { items: [] as PodKind[] },

Added ceph osd tree related test cases for OCS expand #3793

Added ceph osd tree related test cases for OCS expand #3793

Conversation

gnehapk commented Dec 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnehapk commented Dec 18, 2019

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 Dec 18, 2019 • edited

Choose a reason for hiding this comment

afreen23 Dec 18, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afreen23 Dec 18, 2019 • 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

gnehapk commented Dec 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnehapk commented Dec 18, 2019

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 Dec 19, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afreen23 Dec 19, 2019 • edited

Choose a reason for hiding this comment

gnehapk Dec 24, 2019 • edited

Choose a reason for hiding this comment

shyRozen commented Dec 22, 2019

afreen23 left a comment • edited

Choose a reason for hiding this comment

gnehapk commented Dec 28, 2019

gnehapk commented Jan 2, 2020

gnehapk commented Jan 6, 2020

bipuladh commented Jan 8, 2020

gnehapk commented Jan 8, 2020

openshift-ci-robot commented Jan 8, 2020

bipuladh commented Jan 8, 2020

openshift-ci-robot commented Jan 8, 2020

afreen23 Dec 18, 2019 •

edited

afreen23 Dec 18, 2019 •

edited

afreen23 Dec 18, 2019 •

edited

afreen23 Dec 19, 2019 •

edited

afreen23 Dec 19, 2019 •

edited

gnehapk Dec 24, 2019 •

edited

afreen23 left a comment •

edited