[RAC-6492] Task and job for adding hotspare via WSMAN #576

AlaricChan · 2017-12-20T06:27:28Z

Background
This story is to implement adding hot spare via WSMAN, replace the RACADM dependencies in Redfish API with WSMAN implementation.

Details

add task, base task and job implementation, using WSMAN API.
add unit tests for tasks and job

Reviewer
@yaolingling @pengz1 @nortonluo

houndci-bot · 2017-12-20T06:27:44Z

spec/lib/jobs/dell-wsman-add-hotspare-updatexml-spec.js

+        });
+
+        it('should catch error occurred while updating xml', function() {
+            updateXmlJob.dell = dellConfigs


Missing semicolon.

houndci-bot · 2017-12-20T06:27:44Z

spec/lib/jobs/dell-wsman-add-hotspare-updatexml-spec.js

+            hotspareType: 'ghs'
+        };
+        updateXmlJob = new UpdateXmlJob(options, {}, uuid.v4());
+        this.sandbox.stub(configuration, 'get').returns(dellConfigs);;


Unnecessary semicolon.

houndci-bot · 2017-12-20T06:27:44Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+                    var stateChild = doc.createElement('Attribute');
+                    stateChild.setAttribute('Name', 'RAIDPDState');
+                    stateChild.textContent = 'Ready';
+                    var breaklineChild = doc.createTextNode('\n');


'breaklineChild' is already defined.

houndci-bot · 2017-12-20T06:27:44Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+        } else {
+            //Update hotspare status of specified drive
+            for(var i = 0; i < components.length; i++) { //jshint ignore:line
+                var fqdd = components[i].getAttribute('FQDD');


'fqdd' is already defined.

houndci-bot · 2017-12-20T06:43:30Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+                    newDriveNode.appendChild(breaklineChild);
+                    newDriveNode.appendChild(stateChild);
+                    breaklineChild = doc.createTextNode('\n');
+                    newDriveNode.appendChild(breaklineChild);


'breaklineChild' used out of scope.

houndci-bot · 2017-12-20T06:43:30Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+                    breaklineChild = doc.createTextNode('\n');
+                    newDriveNode.appendChild(breaklineChild);
+                    newDriveNode.appendChild(stateChild);
+                    breaklineChild = doc.createTextNode('\n');


'breaklineChild' used out of scope.

houndci-bot · 2017-12-20T06:43:30Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+                    newDriveNode.appendChild(breaklineChild);
+                    newDriveNode.appendChild(statusChild);
+                    breaklineChild = doc.createTextNode('\n');
+                    newDriveNode.appendChild(breaklineChild);


'breaklineChild' used out of scope.

houndci-bot · 2017-12-20T06:43:30Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+                    breaklineChild = doc.createTextNode('\n');
+                    newDriveNode.appendChild(breaklineChild);
+                    newDriveNode.appendChild(statusChild);
+                    breaklineChild = doc.createTextNode('\n');


'breaklineChild' used out of scope.

houndci-bot · 2017-12-20T06:43:30Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+                    stateChild.setAttribute('Name', 'RAIDPDState');
+                    stateChild.textContent = 'Ready';
+                    breaklineChild = doc.createTextNode('\n');
+                    newDriveNode.appendChild(breaklineChild);


'breaklineChild' used out of scope.

houndci-bot · 2017-12-20T06:43:30Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+                    var stateChild = doc.createElement('Attribute');
+                    stateChild.setAttribute('Name', 'RAIDPDState');
+                    stateChild.textContent = 'Ready';
+                    breaklineChild = doc.createTextNode('\n');


'breaklineChild' used out of scope.

houndci-bot · 2017-12-20T06:43:30Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+                fqdd = components[i].getAttribute('FQDD');
+                if(fqdd === self.options.driveId) {
+                    var newDriveNode = doc.createElement('Component');
+                    newDriveNode.setAttribute('FQDD', fqdd);


'fqdd' used out of scope.

houndci-bot · 2017-12-20T06:43:30Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+            //Update hotspare status of specified drive
+            for(var i = 0; i < components.length; i++) { //jshint ignore:line
+                fqdd = components[i].getAttribute('FQDD');
+                if(fqdd === self.options.driveId) {


'fqdd' used out of scope.

houndci-bot · 2017-12-20T06:43:30Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+        } else {
+            //Update hotspare status of specified drive
+            for(var i = 0; i < components.length; i++) { //jshint ignore:line
+                fqdd = components[i].getAttribute('FQDD');


'fqdd' used out of scope.

houndci-bot · 2017-12-20T07:05:14Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+                    newNode.setAttribute('Name', 'RAIDdedicatedSpare');
+                    newNode.textContent = self.options.driveId;
+                    components[i].appendChild(newNode);
+                    var lineChild = doc.createTextNode('\n');


'lineChild' is defined but never used.

houndci-bot · 2017-12-20T07:05:14Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+                    newNode.textContent = self.options.driveId;
+                    components[i].appendChild(newNode);
+                    var lineChild = doc.createTextNode('\n');
+                    components[i].appendChild(breaklineChild);


'breaklineChild' used out of scope.

houndci-bot · 2017-12-20T07:05:15Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+                    var stateChild = doc.createElement('Attribute');
+                    stateChild.setAttribute('Name', 'RAIDPDState');
+                    stateChild.textContent = 'Ready';
+                    var breaklineChild = doc.createTextNode('\n');


'breaklineChild' was used before it was defined.

pengz1 · 2017-12-20T07:54:09Z

lib/jobs/dell-wsman-RAID.js

-        logger.info('Status from SCP Microservice for RAID operation:  ' + json["status"]);
-        if (json["status"] === "OK") {
+        var status = response.body.status;
+        logger.info('Status from SCP Microservice for RAID operation:  ' + status);


This log is not necessary.

pengz1 · 2017-12-20T07:57:44Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+    };
+
+    WsmanAddHotspareUpdateXmlJob.prototype.updateXmlForRaid = function() {
+        logger.info('Update xml for adding hotspare.');


This log is not necessary.

pengz1 · 2017-12-20T07:59:08Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+            );
+        } else {
+            throw new Error('Invalid shareType in smiConfig.json. The shareType should be 0 or 2.');
+        }


If you don't want process panic, it would be better put above code into a Promise.try()

pengz1 · 2017-12-20T07:59:15Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+                self.dell.shareFolder.password
+            );
+        } else {
+            throw new Error('Invalid shareType in smiConfig.json. The shareType should be 0 or 2.');


Use Promise.reject.

pengz1 · 2017-12-20T08:02:24Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+            if(!self.options.volumeId) {
+                throw new errors.NotFoundError('The volumeId should not be empty.');
+            }
+            for(var i = 0; i < components.length; i++) { //jshint ignore:line


You can use _.forEach to replace the "for" expression. This kind of expression (like i++) is not suggested by jshint

pengz1 · 2017-12-20T08:03:53Z

lib/jobs/dell-wsman-getXml.js

-        logger.info('Status from SCP Microservice for getXml:  ' + json["status"]);
-        if (json["status"] === "OK") {
+        var status = response.body.status;
+        logger.info('Status from SCP Microservice for getXml:  ' + status);


Unnecessary log

pengz1 · 2017-12-20T08:19:33Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+            }
+            for(var i = 0; i < components.length; i++) { //jshint ignore:line
+                var volumeFQDD = components[i].getAttribute('FQDD');
+                if(volumeFQDD === self.options.volumeId) {


Lines inside if are difficult to understand, you could put them into a function.

pengz1 · 2017-12-20T08:20:33Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+            //Update hotspare status of specified drive
+            for(var i = 0; i < components.length; i++) { //jshint ignore:line
+                var diskFQDD = components[i].getAttribute('FQDD');
+                if(diskFQDD === self.options.driveId) {


Lines inside if are difficult to understand, you could put them into a function.

pengz1 · 2017-12-20T08:22:54Z

lib/task-data/base-tasks/dell-wsman-add-hotspare-updatexml.js

+
+module.exports = {
+    friendlyName: "Dell Wsman Add Hotspare UpdateXml Base Task",
+    injectableName: "Task.Base.Dell.Wsman.Add.Hotspare.UpdateXml",


Just curious, if this UpdateXml is only used for AddHostSpare, you should not create a job for it but to put in into add host spare job.

It just follow the code structure design as how add-volume and delete-volume did, and reuse the common code.

Basically, A task for RackHD should be able to be executed independently with input options, it should not be coupled with other tasks.

Agree. But I prefer to create another story, to refactor these code structure in add hot spare, add volume and delete volume features.
@yaolingling

pengz1 · 2017-12-20T08:23:31Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+
+    function WsmanAddHotspareUpdateXmlJob(options, context, taskId) {
+        WsmanAddHotspareUpdateXmlJob.super_.call(this, logger, options, context, taskId);
+        assert.object(this.options);


This line is not necessary.

yaolingling · 2017-12-20T10:21:54Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+
+    WsmanAddHotspareUpdateXmlJob.prototype._run = function() {
+        var self = this;
+        if(!self.options.driveId) {


The type of driveId is string. It should be self.options.driveId === ''

yyscamper · 2017-12-21T04:50:58Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+                    self.dell.shareFolder.shareName,
+                    self.context.mountDir
+                );
+            } else if(self.dell.shareFolder.shareType === 2) {


shareType 0 and 2 is better to defined as a constants

Is it the common constants in on-core repo? @yyscamper
https://github.com/RackHD/on-core/blob/master/lib/common/constants.js

yyscamper · 2017-12-21T04:56:44Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+    /*
+     * Dedicated hotspare should be added into corresponding volume
+     */
+    WsmanAddHotspareUpdateXmlJob.prototype.UpdateVolumeElement = function(doc) {


naming style suggestion: the PascalCase style function name is usually used for constructer or class, for function, use camelCase instead.

The same for function UpdateDiskElement

yyscamper · 2017-12-21T05:01:22Z

spec/lib/jobs/dell-wsman-add-hotspare-updatexml-spec.js

+            };
+            updateXmlJob.dell = config;
+            this.sandbox.stub(smb2Clinet.prototype, 'readFile').resolves(xmlData);
+            this.sandbox.stub(smb2Clinet.prototype, 'writeFile');


since writeFile is a promise, so also stub it as promise

Does it mean that I should let the writeFile stub call resolves(), instead of leaving the stub call its default action, maybe returns() or any else? @yyscamper

I got a thought that, in this test case whether the writeFile return a promise or return a value, it is not a key point. Just let the UT ensure the promise chain could be fulfilled. Is that right?
Whatever, it could be funny to dig into the default behavior of stub.

I mean if the function is a promise, but you not stub it as a promise, your unit-test case may not find the potential problem.

Just like my purpose is to test A, but my test stubs it as B, so I am testing B, even B passes, I still cannot make sure A is OK.

pengz1 · 2017-12-26T09:39:08Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+                newDriveNode.appendChild(breakLineChild);
+                newDriveNode.appendChild(statusChild);
+
+                breakLineChild = doc.createTextNode('\n');


This line is duplicated with line 152

Each node is identified as a instance, could not be appended to multiple nodes.

pengz1 · 2017-12-26T09:39:12Z

lib/jobs/dell-wsman-add-hotspare-updatexml.js

+                newDriveNode.appendChild(breakLineChild);
+                newDriveNode.appendChild(stateChild);
+
+                breakLineChild = doc.createTextNode('\n');


This line is duplicated with line 152

houndci-bot reviewed Dec 20, 2017

View reviewed changes

AlaricChan force-pushed the raid-add-hotspare branch from 34ed83e to f1ad8ca Compare December 20, 2017 06:43

houndci-bot reviewed Dec 20, 2017

View reviewed changes

AlaricChan force-pushed the raid-add-hotspare branch 3 times, most recently from 7754838 to e241676 Compare December 20, 2017 07:05

houndci-bot reviewed Dec 20, 2017

View reviewed changes

AlaricChan force-pushed the raid-add-hotspare branch 2 times, most recently from c123e97 to a00160b Compare December 20, 2017 07:17

This was referenced Dec 20, 2017

[RAC-6492] Create graph to add hotspare via WSMAN RackHD/on-taskgraph#355

Merged

[RAC-6492] Port racadm based add-hotspare redfish API with wsman implementation RackHD/on-http#827

Merged

pengz1 added the run-test label Dec 20, 2017

AlaricChan force-pushed the raid-add-hotspare branch from a00160b to 4698335 Compare December 20, 2017 08:13

pengz1 reviewed Dec 20, 2017

View reviewed changes

AlaricChan force-pushed the raid-add-hotspare branch from 4698335 to ae68ccf Compare December 20, 2017 10:19

yaolingling reviewed Dec 21, 2017

View reviewed changes

yaolingling approved these changes Dec 21, 2017

View reviewed changes

yyscamper reviewed Dec 21, 2017

View reviewed changes

AlaricChan force-pushed the raid-add-hotspare branch from ae68ccf to c6d9326 Compare December 21, 2017 08:19

Task and job for adding hotspare via wsman

c6d9326

pengz1 reviewed Dec 26, 2017

View reviewed changes

pengz1 approved these changes Dec 26, 2017

View reviewed changes

pengz1 merged commit 263ad90 into RackHD:master Dec 27, 2017

[RAC-6492] Task and job for adding hotspare via WSMAN #576

[RAC-6492] Task and job for adding hotspare via WSMAN #576

Conversation

AlaricChan commented Dec 20, 2017

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

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

AlaricChan Dec 21, 2017 • edited Loading

Choose a reason for hiding this comment

yyscamper Dec 21, 2017 • edited Loading

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

AlaricChan Dec 21, 2017 •

edited

Loading

yyscamper Dec 21, 2017 •

edited

Loading