Add support for Cisco UCS Service Profile Discovery #420

uppalk1 · 2017-03-07T20:14:43Z

*Create base-task, task and job for a new workflow to
     discover UCS Service Profile

houndci-bot · 2017-03-07T20:14:53Z

spec/lib/jobs/ucs-service-profile-discovery-spec.js

+        ucsTool,
+        rackmountData,
+        rootData,
+        serverData,


'serverData' is defined but never used.

houndci-bot · 2017-03-07T20:14:53Z

spec/lib/jobs/ucs-service-profile-discovery-spec.js

+        graphId = uuid.v4(),
+        ucsJob,
+        ucsTool,
+        rackmountData,


'rackmountData' is defined but never used.

houndci-bot · 2017-03-07T20:14:53Z

lib/jobs/ucs-service-profile-discovery.js

+                throw error;
+            })
+            .then(function(data){
+                return [data, waterline.obms.upsertByNode(data.id,obm)]


Missing semicolon.

houndci-bot · 2017-03-07T20:14:53Z

lib/jobs/ucs-service-profile-discovery.js

+                    })
+                    .then(function(data) {
+                        assert.object(data);
+                        return data['ServiceProfile']['members']


['ServiceProfile'] is better written in dot notation.
['members'] is better written in dot notation.
Missing semicolon.

houndci-bot · 2017-03-07T20:14:53Z

lib/jobs/ucs-service-profile-discovery.js

+    urlParse = require('url-parse');
+
+module.exports = UcsServiceProfileDiscoveryJobFactory;
+di.annotate(UcsServiceProfileDiscoveryJobFactory, new di.Provide('Job.Ucs.Service.Profile.Discovery'));


Line is too long.

yyscamper · 2017-03-08T03:47:11Z

lib/jobs/ucs-service-profile-discovery.js

+
+        return self.getRoot()
+            .then(function(root) {
+                return [ root, self.createServiceProfile(root) ];


Since root is not used in following spread , so it better to be removed from the array.

yyscamper · 2017-03-08T03:54:03Z

lib/jobs/ucs-service-profile-discovery.js

+            protocol: protocol,
+            username: this.options.username,
+            password: this.options.password,
+            ucs: this.options.ucs,


Could you please teach me what's the difference between the setting of ucs and host? Because I see you use the ucs as the host in the query but rather than host

var host = this.settings.ucs; var url= "/login?host=" + host+ "&user="+ username + "&password=" + password;

My understanding is:

host is the ucs-service address, RackHD use the ucs-service to talk with UCS servers.

ucs is the target UCS server, the ucs-service proxies the request to UCS server.

Is correct?

@yyscamper that is correct.

yyscamper · 2017-03-08T03:55:30Z

lib/task-data/base-tasks/ucs-service-profile-discovery.js

+    properties: {
+        catalog: {}
+    }
+};


Please add the schema for the new base task.

@yyscamper can you point me to one that I could use as a reference ?

This is pretty simple, just put a JSON schema in the property optionsSchema, this is very alike with the JSON schema in swagger ui.

This is an example: https://github.com/RackHD/on-tasks/blob/master/lib/task-data/base-tasks/provide-catalog-data.js

@yyscamper We will revisit the schema soon for all ucs-service tasks. For now this may not be held as a gate for PR merge.

@uppalk1 : I am OK with your plan.

@yyscamper if ok with the plan, can you please help remove the -1 that currently blocks the PR from being merged?

yyscamper · 2017-03-08T06:20:51Z

lib/jobs/ucs-service-profile-discovery.js

+                        return data['ServiceProfile']['members']
+                    })
+                    .map(function(members){
+                        self.createNode(members, Constants.NodeTypes.Compute);


This promise has to be returned, otherwise this function will return earlier before all nodes are really created.

yyscamper · 2017-03-08T06:26:04Z

lib/jobs/ucs-service-profile-discovery.js

+        var node = {
+            type: type,
+            name: data.name,
+            associatedServer: data.associatedServer,


What's this property? I don't see it is defined in node model (https://github.com/RackHD/on-core/blob/master/lib/models/node.js). If it is not necessary, them remove it; if you think this is necessary, please explain the reason and then we can check whether it is suitable to create a new property.

@yyscamper I created a new relation called "associatedTo" instead of creating a new property

This is good.

yyscamper · 2017-03-08T06:35:11Z

lib/jobs/ucs-service-profile-discovery.js

+        assert.string(data.name);
+        assert.string(data.path);
+        var id;
+        id = [config.ucs, data.path];


Please use the plural of id or change to other better name.

Because when I see the code identifer: id in line 174, I thought the identifer need be array, but you wrongly gave it a single id. So I look back to check the node, and see the id is really an array.

My point is choosing a good variable name and don't bring surprise to other people who read the code.

yyscamper · 2017-03-08T06:42:31Z

lib/jobs/ucs-service-profile-discovery.js

+            .then(function(curNode) {
+                return waterline.nodes.updateOne(
+                    { id: curNode.id },
+                    node


In line 175, the relations is reset to be empty, since this line of code is to update the node document, so whether it is intentionally to reset the relations? And how the relations are built again?

@yyscamper Yes, the relation will be rebuilt with the discovery.

OK, thanks for explanation, could you please add some comment here

yyscamper · 2017-03-08T06:44:30Z

lib/jobs/ucs-service-profile-discovery.js

+        });
+    };
+
+    UcsServiceProfileDiscoveryJob.prototype.createNode = function(data, type) {


The function name is not accurate, the code is to "update or create" node, not just "create" node.

yyscamper · 2017-03-08T06:45:10Z

lib/task-data/base-tasks/ucs-service-profile-discovery.js

@@ -0,0 +1,17 @@
+// Copyright 2016, EMC, Inc.


Incorrect copyright date

yyscamper · 2017-03-08T06:47:08Z

lib/task-data/base-tasks/ucs-service-profile-discovery.js

+    injectableName: 'Task.Base.UCS.Service.Profile.Discovery',
+    runJob: 'Job.Ucs.Service.Profile.Discovery',
+    requiredOptions: [
+        'uri'


From your implementation, the username, password, ucs are also required options.

assert.string(this.options.uri); assert.string(this.options.username); assert.string(this.options.password); assert.string(this.options.ucs);

@yyscamper this will be refactored as part of https://rackhd.atlassian.net/browse/RAC-4389

Sound good.

yyscamper · 2017-03-08T06:48:22Z

lib/task-data/base-tasks/ucs-service-profile-discovery.js

+    requiredProperties: {
+    },
+    properties: {
+        catalog: {}


What's this property used for? I don't see any reason to define such property and I don't see any code is to create the catalog.

@yyscamper This is a place holder for follow on stories to create a catalog for UCS logical server (if we deem its necessary)

yyscamper · 2017-03-08T06:49:04Z

lib/task-data/tasks/ucs-service-profile-discovery.js

@@ -0,0 +1,17 @@
+// Copyright 2016, EMC, Inc.


Incorrect copyright date

yyscamper · 2017-03-08T06:52:29Z

lib/jobs/ucs-service-profile-discovery.js

+            ucs: this.options.ucs,
+            verifySSL: this.options.verifySSL || false
+        };
+        this.ucs = new UcsTool();


this.ucs and this.settings.ucs is a little confused here, please rename to another distinguish one

@yyscamper I don't understand your confusion. I'm creating an instance and assigning settings to it.

With the same variable name ucs, but when it is in this.settings it means an IP address, however it is a UCS tool instance if it is in this

yyscamper · 2017-03-08T06:55:19Z

spec/lib/jobs/ucs-service-profile-discovery-spec.js

+            username:'user',
+            password:'pass',
+            ucs:"1.1.1.1"
+        }, {}, graphId);


this is taskId, not graphId, though this doesn't break the unit-test, but don't bring confusion since unit-test is a kind of doc for the code.

yyscamper · 2017-03-08T06:56:06Z

spec/lib/jobs/ucs-service-profile-discovery-spec.js

+            ucs:"1.1.1.1"
+        }, {}, graphId);
+
+        sandbox.stub(ucsJob.ucs);


The correct usage is sandbox.stub(ucsJob, 'ucs')

yyscamper · 2017-03-08T07:01:56Z

spec/lib/jobs/ucs-service-profile-discovery-spec.js

+    });
+
+    afterEach(function() {
+        sandbox.restore();


All stubs are created once in before, but all are restored after the first test case finishes. So when the 2nd test case begins to run, actually all stubs are not existing, so what's the purpose to create so many stubs in advance?

Meanwhile, I see a lot stubs are never been used, if so, just remove them.

yyscamper · 2017-03-08T07:08:54Z

spec/lib/jobs/ucs-service-profile-discovery-spec.js

+            return ucsJob.logIn()
+                .then(function(data) {
+                    expect(data).to.deep.equal("cookie");
+                });


Actually, line 140-143 can be short for:

return ucsJob.logIn().to.become('cookie');

Your code is still OK, just for your awareness if you like it.

yyscamper · 2017-03-08T07:15:22Z

lib/jobs/ucs-service-profile-discovery.js

+            relations : []
+        };
+        // Update existing node  or create a new one
+        return waterline.nodes.needOne({identifiers:data.path})


I am thinking of a corner case, since the identifiers is [config.ucs, data.path], here you only find node document by data.path, but how about if the ucs server address is actually changed for the target node, then it means this node's obm service need be updated as well.

yyscamper · 2017-03-08T07:18:31Z

spec/lib/jobs/ucs-service-profile-discovery-spec.js

+                });
+        });
+    });
+});


I don't think the test coverage is good enough (this should be one of the reason why the coverage decrease a lot), I see a lot logic is actually not tested, for example:
(1) If ucs request error, then the job should fail.
(2) If input any invalid option, the job should fail.
(3) If ucs request returns multiple servers, then multiple nodes should be updated or created.
(4) The obm service should be created (or updated) when the node is created (or updated)

yyscamper

Please fix my comment and improve the test coverage.

anhou · 2017-03-08T07:25:11Z

spec/lib/jobs/ucs-service-profile-discovery-spec.js

+    var obm = {
+        config: {
+            uri: "http://10.240.19.220:7080/sys",
+            host: "10.240.19.220",


please use a faked IP

houndci-bot · 2017-03-09T17:12:44Z

spec/lib/jobs/ucs-service-profile-discovery-spec.js

+                    }
+                ],
+                "type": "compute"
+            }


Missing semicolon.

houndci-bot · 2017-03-09T17:12:45Z

spec/lib/jobs/ucs-service-profile-discovery-spec.js

+                "associatedServer": "test",
+                "name": "ls-test",
+                "path": "org-root/ls-test"
+            }


Missing semicolon.

houndci-bot · 2017-03-09T17:12:45Z

spec/lib/jobs/ucs-service-profile-discovery-spec.js

+                "associatedServer": "test",
+                "name": "ls-test",
+                "path": "org-root/ls-test"
+            }


Missing semicolon.

houndci-bot · 2017-03-09T17:12:45Z

lib/jobs/ucs-service-profile-discovery.js

+                                relationType: 'associatedTo',
+                                targets: [newNode.associatedServer]
+                            }];
+                            return self.updateRelations(newNode.id, relations)


Missing semicolon.

houndci-bot · 2017-03-09T17:12:45Z

lib/jobs/ucs-service-profile-discovery.js

+                    })
+                    .then(function(data) {
+                        assert.object(data);
+                        return data['ServiceProfile']['members'];


['ServiceProfile'] is better written in dot notation.
['members'] is better written in dot notation.

houndci-bot · 2017-03-09T17:40:11Z

spec/lib/jobs/ucs-service-profile-discovery-spec.js

+                "associatedServer": "test",
+                "name": "ls-test",
+                "path": "org-root/ls-test"
+            }


Missing semicolon.

JenkinsRHD · 2017-03-09T18:12:58Z

BUILD on-tasks #2136 : ABORTED

** BUILD build-config-setup #2551: SUCCESS
** BUILD unit-tests #12504: UNSTABLE
** BUILD SmokeTest-FIT #1386: SUCCESS
** BUILD Install-Ubuntu-14-04 #1349: ABORTED
** BUILD Install-ESXI-6.0 #800: ABORTED
** BUILD Install-CentOs-6.5 #1371: ABORTED
** BUILD SmokeTest-CIT #1580: SUCCESS
** BUILD on-taskgraph #2257: SUCCESS
- ** BUILD build-config-setup #2552: SUCCESS
- ** BUILD unit-tests #12505: SUCCESS
** BUILD on-http #3463: SUCCESS
- ** BUILD build-config-setup #2553: SUCCESS
- ** BUILD unit-tests #12506: SUCCESS

BUILD unit-tests #12504 Error Logs ▼

Test Name: ucs-service-profile-discovery-spec.js task-data expected properties should have valid default option values Error Details: Task.Ucs.Service.Profile.Discovery: JSON schema validation failed - data.uri should be string Stack Trace: Error: Task.Ucs.Service.Profile.Discovery: JSON schema validation failed - data.uri should be string at TaskOptionValidator.JsonSchemaValidator.validatePatternsSkipped (node_modules/on-core/lib/common/json-schema-validator.js:95:23) at TaskOptionValidator.validateContextSkipped (lib/utils/task-option-validator.js:9:2054) at lib/task.js:9:29334 at arrayEach (node_modules/lodash/index.js:1289:13) at Function.<anonymous> (node_modules/lodash/index.js:3345:13) at Function.Task.validateOptions (lib/task.js:9:28969) at lib/task.js:9:20104 at tryCatcher (node_modules/on-core/node_modules/bluebird/js/main/util.js:26:23) at Promise._settlePromiseFromHandler (node_modules/on-core/node_modules/bluebird/js/main/promise.js:510:31) at Promise._settlePromiseAt (node_modules/on-core/node_modules/bluebird/js/main/promise.js:584:18) at Promise._settlePromises (node_modules/on-core/node_modules/bluebird/js/main/promise.js:700:14) at Async._drainQueue (node_modules/on-core/node_modules/bluebird/js/main/async.js:123:16) at Async._drainQueues (node_modules/on-core/node_modules/bluebird/js/main/async.js:133:10) at Immediate.Async.drainQueues [as _onImmediate] (node_modules/on-core/node_modules/bluebird/js/main/async.js:15:14)

JenkinsRHD · 2017-03-09T18:48:29Z

BUILD on-tasks #2137 : ABORTED

** BUILD build-config-setup #2554: SUCCESS
** BUILD unit-tests #12507: SUCCESS
** BUILD Install-ESXI-6.0 #803: ABORTED
** BUILD Install-CentOs-6.5 #1374: ABORTED
** BUILD Install-Ubuntu-14-04 #1352: ABORTED
** BUILD SmokeTest-FIT #1389: SUCCESS
** BUILD SmokeTest-CIT #1583: SUCCESS
** BUILD on-taskgraph #2258: SUCCESS
- ** BUILD build-config-setup #2555: SUCCESS
- ** BUILD unit-tests #12508: SUCCESS
** BUILD on-http #3464: SUCCESS
- ** BUILD build-config-setup #2556: SUCCESS
- ** BUILD unit-tests #12509: SUCCESS

*Create base-task, task and job for a new workflow to discover UCS Service Profile Clean up ucs-service-profile-discovery test Address PR comments and improve unit test coverage Schema reference as a future enhancement

houndci-bot · 2017-03-09T19:47:43Z

spec/lib/jobs/ucs-service-profile-discovery-spec.js

+                "associatedServer": "test",
+                "name": "ls-test",
+                "path": "org-root/ls-test"
+            }


Missing semicolon.

JenkinsRHD · 2017-03-09T21:23:19Z

BUILD on-tasks #2138 : ABORTED

** BUILD build-config-setup #2561: SUCCESS
** BUILD unit-tests #12513: SUCCESS
** BUILD Install-Ubuntu-14-04 #1358: ABORTED
** BUILD Install-CentOs-6.5 #1380: ABORTED
** BUILD Install-ESXI-6.0 #808: ABORTED
** BUILD SmokeTest-CIT #1589: ABORTED
** BUILD SmokeTest-FIT #1395: ABORTED
** BUILD on-taskgraph #2260: SUCCESS
- ** BUILD build-config-setup #2562: SUCCESS
- ** BUILD unit-tests #12514: SUCCESS
** BUILD on-http #3467: SUCCESS
- ** BUILD build-config-setup #2563: SUCCESS
- ** BUILD unit-tests #12515: SUCCESS

geoff-reid · 2017-03-09T21:42:08Z

It look like the coveralls decreased coverage is due to a change in the way node-coveralls calculates the coverage percentage. See nickmerwin/node-coveralls#158

The actual coverage as reported by istanbul has increased.

from master: https://travis-ci.org/RackHD/on-tasks/jobs/206278092

=============================== Coverage summary ===============================
Statements   : 89.1% ( 6352/7129 )
Branches     : 73.79% ( 1681/2278 )
Functions    : 86.14% ( 1578/1832 )
Lines        : 89.15% ( 6343/7115 )
================================================================================

from this PR: https://travis-ci.org/RackHD/on-tasks/jobs/209488312

=============================== Coverage summary ===============================
Statements   : 89.19% ( 6422/7200 )
Branches     : 73.82% ( 1686/2284 )
Functions    : 86.29% ( 1598/1852 )
Lines        : 89.24% ( 6413/7186 )
================================================================================

geoff-reid · 2017-03-10T18:30:18Z

test this please

geoff-reid

Looks good, 👍

yyscamper · 2017-03-12T05:31:08Z

lib/jobs/ucs-service-profile-discovery.js

+            verifySSL: this.options.verifySSL || false
+        };
+        this.ucs = new UcsTool();
+        this.ucs.settings = this.settings;


It's even more confusing between this.settings.ucs and this.ucs.settings

yyscamper · 2017-03-13T02:13:36Z

spec/lib/jobs/ucs-service-profile-discovery-spec.js

+            ucsTool.clientRequest.onCall(1).resolves(rootData);
+            return ucsJob._run()
+                .then(function() {
+                     expect(waterline.nodes.updateOne).to.be.called.twice;


There is no called.twice or called.once, you should use calledTwice and calledOnce.

The twice or once actually has no assertion effect, actually you could append any arbitrary words after called. For example:
expect(xxxx).to.be.called.notexisting or expect(xxx).to.be.called.foobar will always make the mocha happy. so the mocha success doesn't reflect such error.

yyscamper

+1 after at least fix the called.once and called.twice

houndci-bot reviewed Mar 7, 2017

View reviewed changes

uppalk1 mentioned this pull request Mar 7, 2017

Add support for Cisco UCS Service Profile Discovery RackHD/on-taskgraph#223

Merged

yyscamper reviewed Mar 8, 2017

View reviewed changes

yyscamper suggested changes Mar 8, 2017

View reviewed changes

anhou reviewed Mar 8, 2017

View reviewed changes

houndci-bot reviewed Mar 9, 2017

View reviewed changes

Add support for UCS Service Profile Discovery

6deddb8

*Create base-task, task and job for a new workflow to discover UCS Service Profile Clean up ucs-service-profile-discovery test Address PR comments and improve unit test coverage Schema reference as a future enhancement

uppalk1 force-pushed the ucs-logicalServers branch from 594ff87 to 6deddb8 Compare March 9, 2017 19:47

houndci-bot reviewed Mar 9, 2017

View reviewed changes

Fix hound errors

96d5c1f

geoff-reid approved these changes Mar 10, 2017

View reviewed changes

yyscamper reviewed Mar 12, 2017

View reviewed changes

yyscamper reviewed Mar 13, 2017

View reviewed changes

yyscamper approved these changes Mar 13, 2017

View reviewed changes

Updated unit tests to align well with the function calls

14aa36d

geoff-reid approved these changes Mar 13, 2017

View reviewed changes

geoff-reid merged commit 0ebf95a into RackHD:master Mar 13, 2017

Add support for Cisco UCS Service Profile Discovery #420

Add support for Cisco UCS Service Profile Discovery #420

Conversation

uppalk1 commented Mar 7, 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

yyscamper Mar 8, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyscamper Mar 8, 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

yyscamper Mar 8, 2017 • edited Loading

Choose a reason for hiding this comment

yyscamper left a 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

JenkinsRHD commented Mar 9, 2017

JenkinsRHD commented Mar 9, 2017

Choose a reason for hiding this comment

JenkinsRHD commented Mar 9, 2017

geoff-reid commented Mar 9, 2017

geoff-reid commented Mar 10, 2017

geoff-reid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yyscamper left a comment

Choose a reason for hiding this comment

yyscamper Mar 8, 2017 •

edited

Loading

yyscamper Mar 8, 2017 •

edited

Loading

yyscamper Mar 8, 2017 •

edited

Loading