Enable discovery of UCS Chassis and Servers #403
Conversation
|
BUILD on-tasks #1998 : ABORTED
BUILD SmokeTest-CIT #587 Error Logs ▼Test Name: test_node_workflows_del_active Error Details: (500) Reason: Internal Server Error HTTP response headers: HTTPHeaderDict({'Content-Length': '86', 'X-Powered-By': 'Express', 'X-Warning': 'Deprecated API', 'Connection': 'keep-alive', 'ETag': 'W/"56-6cwmuEFBey6dLrbU/jLDJw"', 'Date': 'Tue, 14 Feb 2017 22:29:00 GMT', 'Access-Control-Allow-Origin': '*', 'Content-Type': 'application/json; charset=utf-8'}) HTTP response body: {"message":"Request Timed Out (on.task-graph-runner:methods.cancelTaskGraph:Object)."}-------------------- >> begin captured logging << -------------------- |
|
test this please |
|
BUILD on-tasks #2006 : UNSTABLE
BUILD Install-CentOs-6.5 #452 Error Logs ▼Test Name: test_nodes_discovery Error Details: timeout waiting for task discovery -------------------- >> begin captured logging << -------------------- tests.api.v1_1.nodes_tests: INFO: Wait start time: 2017-02-16 08:48:09.030081 amqp: DEBUG: Start from server, version: 0.9, properties: {u'information': u'Licensed under the MPL. See http://www.rabbitmq.com/', u'product': u'RabbitMQ', u'copyright': u'Copyright (C) 2007-2013 GoPivotal, Inc.', u'capabilities': {u'exchange_exchange_bindings': True, u'connection.blocked': True, u'authentication_failure_close': True, u'basic.nack': True, u'consumer_priorities': True, u'consumer_cancel_notify': True, u'publisher_confirms': True}, u'platform': u'Erlang/OTP', u'version': u'3.2.4'}, mechanisms: [u'AMQPLAIN', u'PLAIN'], locales: [u'en_US'] amqp: DEBUG: Open OK! kombu: INFO: Starting AMQP worker -> graph.finished.*> amqp: DEBUG: Start from server, version: 0.9, properties: {u'information': u'Licensed under the MPL. See http://www.rabbitmq.com/', u'product': u'RabbitMQ', u'copyright': u'Copyright (C) 2007-2013 GoPivotal, Inc.', u'capabilities': {u'exchange_exchange_bindings': True, u'connection.blocked': True, u'authentication_failure_close': True, u'basic.nack': True, u'consumer_priorities': True, u'consumer_cancel_notify': True, u'publisher_confirms': True}, u'platform': u'Erlang/OTP', u'version': u'3.2.4'}, mechanisms: [u'AMQPLAIN', u'PLAIN'], locales: [u'en_US'] amqp: DEBUG: Open OK! kombu.mixins: INFO: Connected to amqp://guest:**@127.0.0.1:9091// amqp: DEBUG: using channel_id: 1 amqp: DEBUG: Channel open tests.api.v1_1.nodes_tests: INFO: { "duration": "0:01:45.271149", "graph_name": "Graph.SKU.Discovery", "route_id": "1fd11e58-c992-441a-a578-2644cfd1bc0a", "status": "succeeded", "target": "58a5adaac2905382086b8369" } tests.api.v1_1.nodes_tests: INFO: { "duration": "0:01:46.860252", "graph_name": "Graph.SKU.Discovery", "route_id": "faf30d9d-bd3b-4780-9553-dcd077bc23a7", "status": "succeeded", "target": "58a5adadc2905382086b8375" } modules.worker: ERROR: subtask timeout after 1200 seconds, (id=discovery), stopping.. kombu: INFO: Stopping AMQP worker -> graph.finished.*> modules.worker: INFO: stopping subtask for discovery amqp: DEBUG: Closed channel #1 --------------------- >> end captured logging << --------------------- Stack Trace: File "/usr/lib/python2.7/unittest/case.py", line 331, in run testMethod() File "/usr/lib/python2.7/unittest/case.py", line 1043, in runTest self._testFunc() File "/home/jenkins/workspace/Templates/Install-CentOs-6.5/RackHD/test/.venv/on-build-config/local/lib/python2.7/site-packages/proboscis/case.py", line 296, in testng_method_mistake_capture_func compatability.capture_type_error(s_func) File "/home/jenkins/workspace/Templates/Install-CentOs-6.5/RackHD/test/.venv/on-build-config/local/lib/python2.7/site-packages/proboscis/compatability/exceptions_2_6.py", line 27, in capture_type_error func() File "/home/jenkins/workspace/Templates/Install-CentOs-6.5/RackHD/test/.venv/on-build-config/local/lib/python2.7/site-packages/proboscis/case.py", line 350, in func func(test_case.state.get_state()) File "/home/jenkins/workspace/Templates/Install-CentOs-6.5/RackHD/test/tests/api/v1_1/nodes_tests.py", line 123, in test_nodes_discovery message='timeout waiting for task {0}'.format(self.__task.id)) File "/home/jenkins/workspace/Templates/Install-CentOs-6.5/RackHD/test/.venv/on-build-config/local/lib/python2.7/site-packages/proboscis/asserts.py", line 67, in assert_false raise ASSERTION_ERROR(message) 'timeout waiting for task discovery\n-------------------- >> begin captured logging << --------------------\ntests.api.v1_1.nodes_tests: INFO: Wait start time: 2017-02-16 08:48:09.030081\namqp: DEBUG: Start from server, version: 0.9, properties: {u\'information\': u\'Licensed under the MPL. See http://www.rabbitmq.com/\', u\'product\': u\'RabbitMQ\', u\'copyright\': u\'Copyright (C) 2007-2013 GoPivotal, Inc.\', u\'capabilities\': {u\'exchange_exchange_bindings\': True, u\'connection.blocked\': True, u\'authentication_failure_close\': True, u\'basic.nack\': True, u\'consumer_priorities\': True, u\'consumer_cancel_notify\': True, u\'publisher_confirms\': True}, u\'platform\': u\'Erlang/OTP\', u\'version\': u\'3.2.4\'}, mechanisms: [u\'AMQPLAIN\', u\'PLAIN\'], locales: [u\'en_US\']\namqp: DEBUG: Open OK!\nkombu: INFO: Starting AMQP worker -> graph.finished.*>\namqp: DEBUG: Start from server, version: 0.9, properties: {u\'information\': u\'Licensed under the MPL. See http://www.rabbitmq.com/\', u\'product\': u\'RabbitMQ\', u\'copyright\': u\'Copyright (C) 2007-2013 GoPivotal, Inc.\', u\'capabilities\': {u\'exchange_exchange_bindings\': True, u\'connection.blocked\': True, u\'authentication_failure_close\': True, u\'basic.nack\': True, u\'consumer_priorities\': True, u\'consumer_cancel_notify\': True, u\'publisher_confirms\': True}, u\'platform\': u\'Erlang/OTP\', u\'version\': u\'3.2.4\'}, mechanisms: [u\'AMQPLAIN\', u\'PLAIN\'], locales: [u\'en_US\']\namqp: DEBUG: Open OK!\nkombu.mixins: INFO: Connected to amqp://guest:**@127.0.0.1:9091//\namqp: DEBUG: using channel_id: 1\namqp: DEBUG: Channel open\ntests.api.v1_1.nodes_tests: INFO: {\n "duration": "0:01:45.271149",\n "graph_name": "Graph.SKU.Discovery",\n "route_id": "1fd11e58-c992-441a-a578-2644cfd1bc0a",\n "status": "succeeded",\n "target": "58a5adaac2905382086b8369"\n}\ntests.api.v1_1.nodes_tests: INFO: {\n "duration": "0:01:46.860252",\n "graph_name": "Graph.SKU.Discovery",\n "route_id": "faf30d9d-bd3b-4780-9553-dcd077bc23a7",\n "status": "succeeded",\n "target": "58a5adadc2905382086b8375"\n}\nmodules.worker: ERROR: subtask timeout after 1200 seconds, (id=discovery), stopping..\nkombu: INFO: Stopping AMQP worker -> graph.finished.*>\nmodules.worker: INFO: stopping subtask for discovery\namqp: DEBUG: Closed channel #1\n--------------------- >> end captured logging << ---------------------' |
|
test this please |
|
BUILD on-tasks #2007 : UNSTABLE
BUILD SmokeTest-CIT #664 Error Logs ▼Test Name: test_node_workflows_del_active Error Details: (500) Reason: Internal Server Error HTTP response headers: HTTPHeaderDict({'Content-Length': '86', 'X-Powered-By': 'Express', 'X-Warning': 'Deprecated API', 'Connection': 'keep-alive', 'ETag': 'W/"56-6cwmuEFBey6dLrbU/jLDJw"', 'Date': 'Thu, 16 Feb 2017 14:46:02 GMT', 'Access-Control-Allow-Origin': '*', 'Content-Type': 'application/json; charset=utf-8'}) HTTP response body: {"message":"Request Timed Out (on.task-graph-runner:methods.cancelTaskGraph:Object)."}-------------------- >> begin captured logging << -------------------- |
|
test this please |
|
BUILD on-tasks #2008 : FAILURE
|
lib/jobs/ucs-discovery.js
Outdated
| id = [config.ucs, data.path]; | ||
| } | ||
| assert.string(data.name); | ||
| assert.string(data.path); |
There was a problem hiding this comment.
These two lines are duplicated with line 220-221
lib/jobs/ucs-discovery.js
Outdated
|
|
||
| var id; | ||
| if (type === 'compute') | ||
| { |
There was a problem hiding this comment.
A little style problem, this mark should be put at the end of its last line.
geoff-reid
force-pushed
the
discover_chassis
branch
from
1391be7 to
74d9eb1
Compare
| }; | ||
|
|
||
| // Update existing node or create a new one | ||
| return waterline.nodes.needOne({identifiers:data.macs}) |
There was a problem hiding this comment.
Please confirm whether these macs will always be stored in fixed order for the same ucs server. For example, if the server has mac1 and mac2, if first time it returns [mac1, mac2], but the second time it is [mac2, mac1]. These two array are not identical but this should be considered to be the same server.
There was a problem hiding this comment.
I will make sure the ucs-service always returns the macs in the same order.
lib/jobs/ucs-discovery.js
Outdated
| { id: curNode.id }, | ||
| node | ||
| ) | ||
| .then(function(data){ |
There was a problem hiding this comment.
And this then block is redundant, since it just returns what it receives, so please remove this then block.
| * @function createServers | ||
| * @description discovers all the chassis servers in the Cisco UCS | ||
| */ | ||
| UcsDiscoveryJob.prototype.createServers = function (root) { |
There was a problem hiding this comment.
Too many nesting levels for this function! it's hard to read, please split it into smaller functions to reduce the nesting levels.
There was a problem hiding this comment.
After examining the whole code, the cause that leads to tool meany nesting levels is that you mixed the code that creating node and updating node relations together, just split the node creation and node update to a smaller function, this function will look much more simple:
lib/jobs/ucs-discovery.js
Outdated
| var baseurl = this.settings.protocol+"://"+this.settings.host+ ":"+this.settings.port; | ||
| var url= baseurl + "/chassis?host=" + ucs+ "&user="+ username + "&password=" + password;/* jshint ignore:line */ | ||
| return Promise.try(function () { | ||
| if (_.has(root, 'Chassis')) { |
There was a problem hiding this comment.
If you change to below style, actually you can reduce on nesting level:
if (!_.has(root, 'Chassis')) {
return;
}
return self.ucs.clientRequest(url)
...
| }); | ||
| }; | ||
|
|
||
| UcsDiscoveryJob.prototype.updateRelations = function(nodeId, relations) { |
There was a problem hiding this comment.
The relations are always mutual relations, such as "encloses" vs. "enclosedBy". If if you change the node which has "encloses" relation, the its peer node need be updated as well.
There was a problem hiding this comment.
This is done with multiple calls to updateRelations() in the createServers() routine. The first call updates the compute node, the second call updates the chassis node.
lib/jobs/ucs-discovery.js
Outdated
| var password = this.settings.password; | ||
| var ucs = this.settings.ucs; | ||
| var baseurl = this.settings.protocol+"://"+this.settings.host+ ":"+this.settings.port; | ||
| var url= baseurl + "/chassis?host=" + ucs+ "&user="+ username + "&password=" + password;/* jshint ignore:line */ |
There was a problem hiding this comment.
A little style suggestion. Somewhere there is space both before and after the operator + , sometimes only after, sometimes neither, you should at least keep it in same style. Usually it's recommend to add spaces before and after operator.
lib/jobs/ucs-discovery.js
Outdated
| return self.createNode(data, 'compute') | ||
| .spread(function(newNode) { | ||
| var relations = [{ | ||
| relationType: 'encloseBy', |
There was a problem hiding this comment.
a typo here encloseBy => enclosuredBy. see https://github.com/RackHD/on-core/blob/master/lib/common/constants.js#L273
lib/jobs/ucs-discovery.js
Outdated
| return res.body; | ||
| }) | ||
| .map(function(chassisData) { | ||
| return self.createNode(chassisData, 'enclosure') |
There was a problem hiding this comment.
Please change node type to use the constant that defined at https://github.com/RackHD/on-core/blob/master/lib/common/constants.js#L328
lib/jobs/ucs-discovery.js
Outdated
| type: type, | ||
| name: data.path, | ||
| identifiers: id, | ||
| obm:[obm], |
There was a problem hiding this comment.
@geoff-reid This property should be removed. The obms are added below and live their own collection.
| throw error; | ||
| }) | ||
| .then(function(data){ | ||
| return [data, waterline.obms.upsertByNode(data.id,obm)]; |
There was a problem hiding this comment.
This is not a bug since this function is private and only used by your job, but may not a good design. This function will return Promise, but how should the caller know it should use the spread rather than the then to get the returned node document? The return value of waterline.obms.upsertByNode actually not be used anywhere, so it should be better hidden in this function itself.
For this comment, either change this or not is OK for me. Just give me two cents here.
There was a problem hiding this comment.
The node data is needed by the calling function, both promises need to be returned by the function.
spec/lib/jobs/ucs-discovery-spec.js
Outdated
|
|
||
| it('should not update a node', function() { | ||
| ucsTool.clientRequest.onCall(0).resolves([]); | ||
| ucsJob._run() |
There was a problem hiding this comment.
The promise must be returned! Otherwise this unit-test will always success.
I see all test cases in t his file have the same problems, please fix them all.
yyscamper
left a comment
yyscamper
left a comment
There was a problem hiding this comment.
There are many comments have to be fixed before merging.
geoff-reid
force-pushed
the
discover_chassis
branch
from
74d9eb1 to
b51215d
Compare
lib/jobs/ucs-discovery.js
Outdated
| return waterline.nodes.updateOne( | ||
| { id: curNode.id }, | ||
| node | ||
| ) |
lib/jobs/ucs-discovery.js
Outdated
| var username = this.settings.username; | ||
| var password = this.settings.password; | ||
| var ucs = this.settings.ucs; | ||
| var baseurl = this.settings.protocol + "://" + this.settings.host + ":" + this.settings.port; |
geoff-reid
force-pushed
the
discover_chassis
branch
from
b51215d to
54b8b38
Compare
|
BUILD on-tasks #2011 : UNSTABLE
BUILD unit-tests #11815 Error Logs ▼Test Name: Redfish Registries "before all" hook: start HTTP server Error Details: timeout of 5000ms exceeded. Ensure the done() callback is being called in this test. Stack Trace: Error: timeout of 5000ms exceeded. Ensure the done() callback is being called in this test. |
| throw error; | ||
| }) | ||
| .then(function(data){ | ||
| return [data, waterline.obms.upsertByNode(data.id,obm)]; |
There was a problem hiding this comment.
@geoff-reid It doesn't look like you ever use the fulfillment value from the obms upsert, so maybe you can just do this instead:
.tap(function(data) {
return waterline.obms.upsertByNode(data.id,obm);
});
That will return a promise that fulfills to data when the upsert promise fulfills..
|
test this please |
|
BUILD on-tasks #2012 : UNSTABLE
BUILD unit-tests #11818 Error Logs ▼Test Name: Http.Api.Nodes v1.1 "before all" hook: start HTTP server Error Details: timeout of 5000ms exceeded. Ensure the done() callback is being called in this test. Stack Trace: Error: timeout of 5000ms exceeded. Ensure the done() callback is being called in this test. at bound (domain.js:287:14) at runBound (domain.js:300:12) 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) |
yyscamper
left a comment
yyscamper
left a comment
There was a problem hiding this comment.
If the function createServers could be refactored to reduce the nesting levels, that will be much better.
|
test this please |
|
BUILD on-tasks #2030 : FAILURE
BUILD SmokeTest-CIT #721 Error Logs ▼Test Name: test_node_workflows_del_active Error Details: (500) Reason: Internal Server Error HTTP response headers: HTTPHeaderDict({'Content-Length': '86', 'X-Powered-By': 'Express', 'X-Warning': 'Deprecated API', 'Connection': 'keep-alive', 'ETag': 'W/"56-6cwmuEFBey6dLrbU/jLDJw"', 'Date': 'Fri, 17 Feb 2017 13:58:48 GMT', 'Access-Control-Allow-Origin': '*', 'Content-Type': 'application/json; charset=utf-8'}) HTTP response body: {"message":"Request Timed Out (on.task-graph-runner:methods.cancelTaskGraph:Object)."}-------------------- >> begin captured logging << -------------------- BUILD unit-tests #11896 Error Logs ▼Test Name: Http.Api.Tasks "before each" hook: set up mocks for "should send down tasks" Error Details: timeout of 2000ms exceeded. Ensure the done() callback is being called in this test. Stack Trace: Error: timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.Test Name: Http.Api.Versions "before all" hook: start HTTP server Test Name: Http.Api.Versions "after all" hook: stop HTTP server Test Name: Http.Api.Notification "before all" hook: start HTTP server Test Name: Http.Api.Login test with authentication enabled "before all" hook: start HTTPs server Test Name: Http.Api.Login test with authentication enabled "after all" hook: stop server, restore mock and configure |
|
test this please |
Merge pull request RackHD#403 from geoff-reid/discover_chassis Enable discovery of UCS Chassis and Servers
7 participants
@RackHD/corecommitters @tannoa2 @keedya @uppalk1