Ssh obm service #505

srinia6 · 2016-10-20T19:13:55Z

This PR Jenkins:depends on RackHD/on-core#222 and Jenkins:depends on RackHD/on-tasks#361

In this PR:

Move sshSettings from nodes.
Create /ibms and /ibms/{identifier} like the obms .
Backward compatible with 1.1 like obms.
Schema/view for ibms.
unit-test fix

@RackHD/corecommitters @dalebremner @brianparry @BillyAbildgaard @geoff-reid

rackhdmirror · 2016-10-20T19:14:02Z

@RackHD/corecommitters please review.

houndci-bot · 2016-10-20T19:14:06Z

lib/api/1.1/northbound/nodes.js

+                    password: 'REDACTED'
+                };
+            } else {
+                delete node['sshSettings'];


['sshSettings'] is better written in dot notation.

ignoring this.

it's recommended style also keep consistent with other codes

houndci-bot · 2016-10-21T19:27:45Z

lib/api/2.0/ibms.js

+                    .then(function (newNode) {
+                        return eventsProtocol.publishNodeAttrEvent(oldNode, newNode, 'ibms');
+                    })
+                    .catch(function (error) {


'error' is defined but never used.

houndci-bot · 2016-10-21T19:27:45Z

lib/api/2.0/ibms.js

+                        .then(function(newNode) {
+                            return eventsProtocol.publishNodeAttrEvent(oldNode, newNode, 'ibms');
+                        })
+                        .catch(function (error) {


'error' is defined but never used.

@srinia6 It's probably worth appending error.message to your base error message.

srinia6 · 2016-10-21T19:32:05Z

@RackHD/corecommitters - the unit test failures are due to on-core dependency.

srinia6 · 2016-10-21T20:08:12Z

FYI the unit-test failure is due to on-core dependency.

anhou · 2016-10-25T09:05:48Z

lib/api/2.0/ibms.js

+var ibmsPatchById = controller( function(req) {
+    var ibmId = req.swagger.params.identifier.value;
+    var values = req.swagger.params.body.value;
+    return waterline.ibms.needByIdentifier(ibmId)


duplicated codes, it also exists in some other places for ibm and obm. two places will be maintained, and easy to miss something if one place is changed. it needs some optimization, or move the common codes to /lib/services/xxxx. maybe it also could be waterline.[xxx].needByxxxx().

@anhou : I can create a ibm service and move the code for Patch and Delete. But the obm calls the obms waterline collection and ibms calls its own collection. Trying to think if thats what you meant? And not sure what you mean if one place changes ? They are 2 different collections that inherit from base class under on-core/models.

@anhou @srinia6 The duplicated code is in ibmsGet, ibmsPut, ibmsGetById, and ibmsGetAll. These are all basically one-liners, does it really make sense to create another layer of abstraction for this? The two verbs with some substance to them (patch and delete) are not common anyway. They could be moved to a service, but that does not avoid duplication.

some of the codes are copied from /lib/services/obm-api-services.js the code logic is the same. I think don't need another abstract layer to service api, if ibm and obm really almost the similar in document operation. maybe just a map/dict could reduce the duplication. maintain obm/ibm in /lib/services/bm-api-service.js or lib/services/nm-api-service.js(network management) all the document openration like waterline.obms.xxxx could be waterline.nm[type].xxx, just a method I could think of to de-duplication. or you could use other better ways to do it.

brianparry

@srinia6 Looks good overall, just a few questions / comments.

brianparry · 2016-10-25T15:11:02Z

lib/api/2.0/ibms.js

+var ibmsPatchById = controller( function(req) {
+    var ibmId = req.swagger.params.identifier.value;
+    var values = req.swagger.params.body.value;
+    return waterline.ibms.needByIdentifier(ibmId)


@anhou @srinia6 The duplicated code is in ibmsGet, ibmsPut, ibmsGetById, and ibmsGetAll. These are all basically one-liners, does it really make sense to create another layer of abstraction for this? The two verbs with some substance to them (patch and delete) are not common anyway. They could be moved to a service, but that does not avoid duplication.

brianparry · 2016-10-25T15:13:52Z

lib/api/2.0/ibms.js

+                        .then(function(newNode) {
+                            return eventsProtocol.publishNodeAttrEvent(oldNode, newNode, 'ibms');
+                        })
+                        .catch(function (error) {


@srinia6 It's probably worth appending error.message to your base error message.

brianparry · 2016-10-25T15:17:12Z

lib/api/2.0/ibms.js

+    return waterline.ibms.needByIdentifier(ibmId)
+        .then(function(oldIbm) {
+            /* Get nodes that need to publish events */
+            if (oldIbm.node && !values.nodeId) {


srinia6 Is this logic needed? Couldn't we just compare the old node to the new node and alert if anything has changed? Or maybe even just always alert?

I think this part of codes are copied from /lib/service/obm-api-service.js they are written and tested by me several months ago. the logic could be referenced from the unit test cases. the behavior is documented in RackHD/docs in 'event notification' section about obms.assigned/unassigned/updated

Thanks @anhou

@anhou - we have created another story to handle slightly similar code in this story - https://www.pivotaltracker.com/story/show/133740825

changev · 2016-10-27T03:22:28Z

@srinia6 a strange issue.
When get description by github api
It get

This PR Jenkins:depends on https://github.com/RackHD/on-core/pull/222 and Jenkins:depends on RackHD/on-tasks#361

RackHD/on-tasks#361 but not a URL.

I add sub line to correct this
jenkins: depends on RackHD/on-tasks#361

test this please

still not support branch feature/ibms not exists in all repos

Thanks.

benbp · 2016-10-27T19:24:13Z

lib/api/1.1/northbound/nodes.js

+        var obms = Promise.map(waterline.obms.find({node: node.id}), function(obm) {
+            return _.pick(obm.toJSON(), 'service', 'config');
+        });
+        var sshSettings = waterline.ibms.findByNode(node.id, 'ssh-ibm-service');


Since this is only for a 1.1 API backwards compatibility edge case, I'm not going to raise an issue. But in any 2.0 controller I would object to hardcoding for specific IBM service types, especially just for doing 'REDACT' which shouldn't be handled at this layer anyways.

Besides, the function name is _renderNodeObmSettings but the function covers both obm & ibm (ssh), so this doesn't match.

@benbp : Thanks ben! @yyscamper : I will rename the function.

yyscamper · 2016-11-04T06:57:52Z

@srinia6 : this is a reminder for you. When I worked on the issue https://github.com/RackHD/RackHD/issues/483, I found the obm setting was not added while manually adding node, so the node's discovery will fail. The solution is in my PR #524.

After the ibm is merged, and when you plan to move the snmpSettings into ibm, don't forget this issue.

yyscamper · 2016-11-04T06:59:07Z

static/schemas/ibms/ssh-ibm-service.json

+                    "type": "object",
+                    "properties": {
+                        "host": {
+                            "description": "BMC address",


This is not BMC address!

yyscamper · 2016-11-04T07:01:56Z

static/monorail-2.0.yaml

+    get:
+      description: 'Get a list of all In Band Management settings that have been associated
+
+        with nodes. 


This multiple lines style is very strange for me. this line should be put together with line630, the same for a lot below lines.

@yyscamper Because this pr has been sitting in review for ~14 days, it needs to be changed per the new master file. Will update it.

yyscamper · 2016-11-04T07:05:47Z

data/views/node.2.0.json

    "type": "<%=type%>",
-    "workflows": "<%=basepath %>/nodes/<%=id%>/workflows" 
+    "workflows": "<%=basepath %>/nodes/<%=id%>/workflows",
+    <% if (ibms === null) { %>


A minor suggestion, put the ibm together with obm will help people to compare the difference between them.

Since this has been tested with these changes, i plan to keep them as is for now. Will change them in the snmp story.

yyscamper · 2016-11-04T07:06:58Z

lib/api/1.1/northbound/nodes.js

-    )
+        'Assert',
+        'Errors'
+)


The previous indent is better, please remain it.

JenkinsRHD · 2016-11-04T08:44:41Z

BUILD on-http #2799 : FAILURE

** BUILD build-config-setup #146393: SUCCESS
** BUILD unit-tests #10007: UNSTABLE
** BUILD smoke-test #3737: FAILURE

BUILD unit-tests #10007 Error Logs ▼

Test Name: Http.Api.Ibms /api/2.0/ibms should return a list of IBM instances
Error Details: Should wrap property of object
Stack Trace: TypeError: Should wrap property of object
at Object.wrapMethod (node_modules/sinon/lib/sinon/util/core.js:67:23)
at Object.stub (node_modules/sinon/lib/sinon/stub.js:61:26)
at Context.<anonymous> (spec/lib/api/2.0/ibms-spec.js:106:26)

Test Name: Http.Api.Ibms /api/2.0/ibms should put an IBM instance
Error Details: Should wrap property of object
Stack Trace: TypeError: Should wrap property of object
at Object.wrapMethod (node_modules/sinon/lib/sinon/util/core.js:67:23)
at Object.stub (node_modules/sinon/lib/sinon/stub.js:61:26)
at Context.<anonymous> (spec/lib/api/2.0/ibms-spec.js:121:26)

Test Name: Http.Api.Ibms /api/2.0/ibms should 400 when put with unloaded schema
Error Details: Should wrap property of object
Stack Trace: TypeError: Should wrap property of object
at Object.wrapMethod (node_modules/sinon/lib/sinon/util/core.js:67:23)
at Object.stub (node_modules/sinon/lib/sinon/stub.js:61:26)
at Context.<anonymous> (spec/lib/api/2.0/ibms-spec.js:133:26)

Test Name: Http.Api.Ibms /api/2.0/ibms should 400 when put with missing field
Error Details: Should wrap property of object
Stack Trace: TypeError: Should wrap property of object
at Object.wrapMethod (node_modules/sinon/lib/sinon/util/core.js:67:23)
at Object.stub (node_modules/sinon/lib/sinon/stub.js:61:26)
at Context.<anonymous> (spec/lib/api/2.0/ibms-spec.js:145:26)

Test Name: Http.Api.Ibms /api/2.0/ibms/:id "before each" hook for "should get an IBM instance"
Error Details: Should wrap property of object
Stack Trace: TypeError: Should wrap property of object
at Object.wrapMethod (node_modules/sinon/lib/sinon/util/core.js:67:23)
at Object.stub (node_modules/sinon/lib/sinon/stub.js:61:26)
at Context.<anonymous> (spec/lib/api/2.0/ibms-spec.js:160:19)

Test Name: Http.Api.Ibms /api/2.0/ibms/:id "after each" hook for "should get an IBM instance"
Error Details: Cannot read property 'needByIdentifier' of undefined
Stack Trace: TypeError: Cannot read property 'needByIdentifier' of undefined
at Context.<anonymous> (spec/lib/api/2.0/ibms-spec.js:166:27)

Test Name: 2.0 Http.Api.Nodes GET /nodes/:identifier/ssh should return a list of the node's ssh settings
Error Details: Cannot read property 'findAllByNode' of undefined
Stack Trace: TypeError: Cannot read property 'findAllByNode' of undefined
at Context.<anonymous> (spec/lib/api/2.0/nodes-spec.js:383:27)

Test Name: 2.0 Http.Api.Nodes GET /nodes/:identifier/ssh should return a 404 if the node has no ssh settings
Error Details: Cannot read property 'findAllByNode' of undefined
Stack Trace: TypeError: Cannot read property 'findAllByNode' of undefined
at Context.<anonymous> (spec/lib/api/2.0/nodes-spec.js:400:27)

Test Name: 2.0 Http.Api.Nodes POST /nodes/:identifier/ssh should replace existing settings with a new set of ssh settings
Error Details: Cannot read property 'upsertByNode' of undefined
Stack Trace: TypeError: Cannot read property 'upsertByNode' of undefined
at Context.<anonymous> (spec/lib/api/2.0/nodes-spec.js:436:27)

Test Name: 2.0 Http.Api.Nodes POST /nodes/:identifier/ssh should add a new set of ssh settings if none exist
Error Details: Cannot read property 'upsertByNode' of undefined
Stack Trace: TypeError: Cannot read property 'upsertByNode' of undefined
at Context.<anonymous> (spec/lib/api/2.0/nodes-spec.js:449:27)

srinia6 · 2016-11-04T09:28:11Z

@yyscamper : Looking at your PR, that is no way related to this PR. #524 What issue are you talking about?

JenkinsRHD · 2016-11-04T19:35:02Z

BUILD on-http #2803 : FAILURE

** BUILD build-config-setup #146898: SUCCESS
** BUILD unit-tests #10011: UNSTABLE
** BUILD smoke-test #3741: FAILURE

BUILD unit-tests #10011 Error Logs ▼

Test Name: Http.Api.Ibms /api/2.0/ibms/definitions should get the ssh IBM schema
Error Details: expected 200 "OK", got 404 "Not Found" Response body: { message: '/api/2.0/ibms/definitions/ssh-ibm-service.json Not Found', status: '404', UUID: '12979859-636f-4346-b6b9-a23fb9a50467' } Error: expected 200 "OK", got 404 "Not Found" at Test._assertStatus (/home/jenkins/workspace/on-http/build/node_modules/supertest/lib/test.js:232:12) at Test._assertFunction (/home/jenkins/workspace/on-http/build/node_modules/supertest/lib/test.js:247:11) at Test.assert (/home/jenkins/workspace/on-http/build/node_modules/supertest/lib/test.js:148:18) at assert (/home/jenkins/workspace/on-http/build/node_modules/supertest/lib/test.js:127:12) at /home/jenkins/workspace/on-http/build/node_modules/supertest/lib/test.js:124:5 at Test.Request.callback (/home/jenkins/workspace/on-http/build/node_modules/supertest/node_modules/superagent/lib/node/index.js:703:3) at IncomingMessage.<anonymous> (/home/jenkins/workspace/on-http/build/node_modules/supertest/node_modules/superagent/lib/node/index.js:922:12) at emitNone (events.js:72:20) at IncomingMessage.emit (events.js:166:7) at endReadableNT (_stream_readable.js:921:12) at nextTickCallbackWith2Args (node.js:442:9) at process._tickDomainCallback (node.js:397:17)
Stack Trace: Error: expected 200 "OK", got 404 "Not Found"
at Test._assertStatus (node_modules/supertest/lib/test.js:232:12)
at Test._assertFunction (node_modules/supertest/lib/test.js:247:11)
at Test.assert (node_modules/supertest/lib/test.js:148:18)
at assert (node_modules/supertest/lib/test.js:127:12)
at node_modules/supertest/lib/test.js:124:5
at Test.Request.callback (node_modules/supertest/node_modules/superagent/lib/node/index.js:703:3)
at IncomingMessage.<anonymous> (node_modules/supertest/node_modules/superagent/lib/node/index.js:922:12)
at endReadableNT (_stream_readable.js:921:12)