Bug 1759051: Ensure that pre-labelled nodes are not re-labelled #3326

parthdhanjal · 2019-11-11T07:37:43Z

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1759051
Signed-off-by: parthdhanjal dparth@redhat.com

openshift-ci-robot · 2019-11-11T07:37:58Z

Hi @parthdhanjal. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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 · 2019-11-11T08:44:42Z

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

+              history.push(
+                `/k8s/ns/${ocsProps.namespace}/clusterserviceversions/${
+                  ocsProps.clusterServiceVersion.metadata.name
+                }/${referenceForModel(OCSServiceModel)}/${ocsObj.metadata.name}`,


use resourcePathFromModel

@bipuladh We need ocs.openshift.io~v1~StorageCluster here, hence referenceForModel is being used. Output of resourcePathFromModel is /k8s/all-namespaces/ocs.openshift.io~v1~StorageCluster which is not needed here

resourcePathFromModel would replace the whole of the line and you can specify the namespace as per your need as well as the appName.

@bipuladh resourcePathFromModel would not give us the the same path. I do feel like maybe the path used here might actually be a bug. The path to k8s resources should be standardized, and this appears at first glance to be different from the usual format.

bipuladh · 2019-11-11T08:50:23Z

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

+        return k8sPatch(NodeModel, node, patch);
+      }
    });
+    promises.push(selectedNodes);


You are creating an array containing an array of promises. ([][])
I would do something in the lines of:

const promise = k8sPatch(NodeMode,node,patch); promises.push(promise);

bipuladh · 2019-11-11T08:52:07Z

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

+          history.push(
+            `/k8s/ns/${ocsProps.namespace}/clusterserviceversions/${
+              ocsProps.clusterServiceVersion.metadata.name
+            }/${referenceForModel(OCSServiceModel)}/${ocsObj.metadata.name}`,
+          );


I think it would be easier for you to use resourcePathFromModel.

bipuladh · 2019-11-11T08:55:47Z

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

+    if (promises) {
+      Promise.all(promises)
+        .then(() => {
+          k8sCreate(OCSServiceModel, ocsObj)
+            .then(() => {
+              history.push(
+                `/k8s/ns/${ocsProps.namespace}/clusterserviceversions/${
+                  ocsProps.clusterServiceVersion.metadata.name
+                }/${referenceForModel(OCSServiceModel)}/${ocsObj.metadata.name}`,
+              );
+            })
+            .catch((err) => {
+              setProgress(false);
+              setError(err.message);
+            });
+        })
+        .catch((err) => {
+          setProgress(false);
+          setError(err.message);
+        });
+    } else {
+      k8sCreate(OCSServiceModel, ocsObj)
+        .then(() => {
+          history.push(
+            `/k8s/ns/${ocsProps.namespace}/clusterserviceversions/${
+              ocsProps.clusterServiceVersion.metadata.name
+            }/${referenceForModel(OCSServiceModel)}/${ocsObj.metadata.name}`,
+          );
+        })
+        .catch((err) => {
+          setProgress(false);
+          setError(err.message);
+        });
+    }


no need for if else. Use finally

No need to use finally here either. Promise.all() will resolve if passed an empty array. You can remove all of the conditional logic around this, and it should work fine.

bipuladh · 2019-11-11T08:57:22Z

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

+    promises.push(selectedNodes);
+    return promises;


You are creating an array of array.

bipuladh · 2019-11-11T08:57:53Z

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

+            value: '',
+          },
+        ];
+        return k8sPatch(NodeModel, node, patch);


Suggested change

return k8sPatch(NodeModel, node, patch);

const promise = k8sPatch(NodeModel, node, patch);

promises.push(promise);

Creating a "const" array then pushing to it is a bit counterintuitive since constants should be immutable. I don't know why js allows this, but I think it's better practice to use a 'pure' approach like a reducer here:

const makeLabelNodesRequest = (selectedNodes: NodeKind[]): Promise<NodeKind>[] => _.reduce(selectedNodes, (accumulator, node) => { // patch could actually be defined as a constant at the top level scope since it is static. const patch = [ { op: 'add', path: '/metadata/labels/cluster.ocs.openshift.io~1openshift-storage', value: '', }, ]; return isLabelPresent(node) ? accumulator : [ ...accumulator, k8sPatch(NodeModel, node, patch) ]; }, []);

openshift-ci-robot · 2019-11-11T15:59:20Z

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

In response to this:

Bug 1759051: Ensure that pre-labelled nodes are not re-labelled

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.

TheRealJon

As @bipuladh pointed out, I think there is some flawed logic in makeLabelNodesRequest. You are returning a 2D array (which I don't think is the intention). See my comments on how this function could be improved. There are also a few "not new" things in node-list.tsx that I think should probably be addressed in a follow on at some point. I'll leave a separate comment for that.

TheRealJon · 2019-11-11T16:33:00Z

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

+            value: '',
+          },
+        ];
+        return k8sPatch(NodeModel, node, patch);


Creating a "const" array then pushing to it is a bit counterintuitive since constants should be immutable. I don't know why js allows this, but I think it's better practice to use a 'pure' approach like a reducer here:

const makeLabelNodesRequest = (selectedNodes: NodeKind[]): Promise<NodeKind>[] => _.reduce(selectedNodes, (accumulator, node) => { // patch could actually be defined as a constant at the top level scope since it is static. const patch = [ { op: 'add', path: '/metadata/labels/cluster.ocs.openshift.io~1openshift-storage', value: '', }, ]; return isLabelPresent(node) ? accumulator : [ ...accumulator, k8sPatch(NodeModel, node, patch) ]; }, []);

TheRealJon · 2019-11-11T17:08:52Z

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

+    if (promises) {
+      Promise.all(promises)
+        .then(() => {
+          k8sCreate(OCSServiceModel, ocsObj)
+            .then(() => {
+              history.push(
+                `/k8s/ns/${ocsProps.namespace}/clusterserviceversions/${
+                  ocsProps.clusterServiceVersion.metadata.name
+                }/${referenceForModel(OCSServiceModel)}/${ocsObj.metadata.name}`,
+              );
+            })
+            .catch((err) => {
+              setProgress(false);
+              setError(err.message);
+            });
+        })
+        .catch((err) => {
+          setProgress(false);
+          setError(err.message);
+        });
+    } else {
+      k8sCreate(OCSServiceModel, ocsObj)
+        .then(() => {
+          history.push(
+            `/k8s/ns/${ocsProps.namespace}/clusterserviceversions/${
+              ocsProps.clusterServiceVersion.metadata.name
+            }/${referenceForModel(OCSServiceModel)}/${ocsObj.metadata.name}`,
+          );
+        })
+        .catch((err) => {
+          setProgress(false);
+          setError(err.message);
+        });
+    }


No need to use finally here either. Promise.all() will resolve if passed an empty array. You can remove all of the conditional logic around this, and it should work fine.

TheRealJon · 2019-11-11T17:29:49Z

"Not new" issues that should probably be addressed at some point in a follow on.

node-list.tsx [line 313-315] unnecessary spread operator:

const promises = [];

promises.push(...makeLabelNodesRequest(selectedData));

Should just be:

const promises = makeLabelNodesRequest(selectedData);

node-list.tsx [line 319-320] Mutating deep object properties:

Instead of creating a static ocsObj, cloning, then mutating it, we should just use a function to build the object we need:

export const getOCSRequestData = (storageClassName: string): K8sResourceKind => ({
  apiVersion: 'ocs.openshift.io/v1',
  kind: 'StorageCluster',
  metadata: {
    name: 'ocs-storagecluster',
    namespace: 'openshift-storage',
  },
  spec: {
    managedNodes: false,
    storageDeviceSets: [
      {
        name: 'ocs-deviceset',
        count: 3,
        resources: {},
        placement: {},
        portable: true,
        dataPVCTemplate: {
          spec: {
            storageClassName,
            accessModes: ['ReadWriteOnce'],
            volumeMode: 'Block',
            resources: {
              requests: {
                storage: '1Ti',
              },
            },
          },
        },
      },
    ],
  },
});

Tainting logic that is commented out should not be in master
Use a single TODO comment, and save the logic in another branch. We shouldn't have commented code like this in master.

gnehapk · 2019-11-12T06:33:14Z

@TheRealJon
node-list.tsx [line 319-320] Mutating deep object properties: - This is a constant request data that needs to be send out while creating ocs service, expect one field which will be updated. Since there are not many fields getting updated, IMO its better to create a constant here instead of writing a separate method to do that.

gnehapk · 2019-11-12T06:59:27Z

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

-        },
-      ];
-      return k8sPatch(NodeModel, node, patch);
+    const promises = [];


@parthdhanjal makeLabelNodesRequest definition should be like this -

const makeLabelNodesRequest = (selectedNodes: NodeKind[]): Promise<NodeKind>[] => { return selectedNodes.map((node: NodeKind) => { const nodeLabel = isLabelPresent(node, ocsLabel); if (!nodeLabel) { const patch = [ { op: 'add', path: '/metadata/labels/cluster.ocs.openshift.io~1openshift-storage', value: '', }, ]; return k8sPatch(NodeModel, node, patch); } }); };

No need to create a promise array here and pass the ocs label explicitly so that isLabelPresent function should be generic.

gnehapk · 2019-11-12T07:00:56Z

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

-        setError(err.message);
-      });
+
+    if (promises) {


@parthdhanjal

Promise.all(promises) .then(() => { return k8sCreate(OCSServiceModel, ocsObj); }) .then(() => { history.push( `/k8s/ns/${ocsProps.namespace}/clusterserviceversions/${ ocsProps.clusterServiceVersion.metadata.name }/${referenceForModel(OCSServiceModel)}/${ocsObj.metadata.name}`, ); }) .catch((err) => { setProgress(false); setError(err.message); });

No need of if else and no need to write separate catch for both scenarios. It can be done in one like I have mentioned above.

gnehapk · 2019-11-12T07:01:22Z

frontend/packages/console-shared/src/selectors/node.ts

@@ -18,6 +18,9 @@ export const getNodeRoles = (node: NodeKind): string[] => {
  );
 };

+export const isLabelPresent = (node: NodeKind) =>


@parthdhanjal

export const isLabelPresent = (node: NodeKind, label: string) : boolean => _.has(node, ['metadata', 'labels', label]);

I would also argue that this function probably belongs in frontend/packages/console-shared/src/selectors/common.ts. Labels are common to all k8s resources, so this could be reused elsewhere. Also, I think 'hasLabel' is probably a more concise and accurate name.

export const hasLabel = <A extends K8sResourceKind = K8sResourceKind>(value: A, label: string) : boolean => _.has(value, ['metadata', 'labels', label]);

cloudbehl · 2019-11-12T13:15:33Z

/ok-to-test
/kind bug

gnehapk · 2019-11-12T13:35:07Z

@parthdhanjal Frontend tests are failing. Can you please check?

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

TheRealJon · 2019-11-12T17:04:11Z

@TheRealJon
node-list.tsx [line 319-320] Mutating deep object properties: - This is a constant request data that needs to be send out while creating ocs service, expect one field which will be updated. Since there are not many fields getting updated, IMO its better to create a constant here instead of writing a separate method to do that.

@gnehapk I have to disagree with this. If you need to change part of the object when you use it, then by definition it is not constant. The number of fields being updated doesn't matter. The important thing is that we are generating a dynamic object in a reusable way. The best way to do that is with a function.

gnehapk · 2019-11-13T11:36:58Z

/test images
/test e2e-gcp-console

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1759051 Signed-off-by: parthdhanjal <dparth@redhat.com>

parthdhanjal · 2019-11-25T12:20:42Z

/test e2e-gcp-console

parthdhanjal · 2019-11-25T15:14:06Z

/retest

gnehapk · 2019-11-26T03:43:12Z

/test e2e-gcp-console

benjaminapetersen · 2019-11-26T16:55:18Z

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

+    const patch = [
+      {
+        op: 'add',
+        path: '/metadata/labels/cluster.ocs.openshift.io~1openshift-storage',


Out of curiousity, where does '/metadata/labels/cluster.ocs.openshift.io~1openshift-storage' come from? the ~1openshift-storage seems a little strange to me.

benjaminapetersen · 2019-11-26T17:33:59Z

/lgtm

I believe @TheRealJon is going to create a few follow-up chores in Jira to address some of the items he mentioned above.

openshift-ci-robot · 2019-11-26T17:34:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benjaminapetersen, cloudbehl, gnehapk, parthdhanjal, TheRealJon

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~~ [benjaminapetersen]

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

benjaminapetersen · 2019-11-26T17:35:16Z

/retest

openshift-bot · 2019-11-26T17:42:29Z

/retest