Add obm setting while posting a new node #524

yyscamper · 2016-11-04T04:41:31Z

This is a part of fix for https://github.com/RackHD/RackHD/issues/483

Root cause: After the obm is moved to a standalone model, we forget to add the obm setting while posting a new node, so some node catalog job will fail.

BTW: the unit-test error due to the node-cache is fixed as well in my PR.

@RackHD/corecommitters @iceiilin @cgx027 @pengz1 @manfrednde

yyscamper · 2016-11-04T04:45:10Z

spec/lib/services/upnp-service-spec.js

            helper.di.simpleWrapper(messenger, 'Services.Messenger'),
            helper.di.simpleWrapper(rx,'Rx'),
-            helper.di.requireWrapper('node-cache', 'node-cache')
+            helper.di.requireWrapper('node-cache', 'node-cache', undefined, __dirname)


If without this change, the unit-test will always fail due to following error:

1) UPnP Service "before all" hook: Error: dihelper incapable of finding specified file for require filename:node-cache, directories:(/home/rackhd/src/on-core/spec, /home/rackhd/src/on-core/lib) at _require (/home/rackhd/src/on-core/lib/di.js:492:13) at Object.requireWrapper (/home/rackhd/src/on-core/lib/di.js:518:13) at Context.<anonymous> (spec/lib/services/upnp-service-spec.js:49:23)

@brianparry

iceiilin · 2016-11-04T05:03:15Z

👍

srinia6 · 2016-11-04T09:25:16Z

@yyscamper Please see my comment under RackHD/RackHD#483

srinia6 · 2016-11-04T09:26:33Z

lib/services/nodes-api-service.js

            return waterline.nodes.create(body);
        }).tap(function(node) {
            return eventsProtocol.publishNodeEvent(node, 'added');
+        }).tap(function(node) {


This does not look right to me. We dont have obmSettings anymore. They come up as obms. @brianparry - to keep me honest here.

@srinia6: I examined the schema for POST /nodes (see https://github.com/RackHD/on-http/blob/master/static/monorail-2.0.yaml#L83), it doesn't tell how to specify the obm when manually adding a node.
And by looking at the rest code of postNode function, you will it still use the obmSettings (see https://github.com/yyscamper/on-http/blob/b31f6b199676172de8ddb37c0504de6df1cac197/lib/services/nodes-api-service.js#L308).

Anyway, we always need a way to let user specify the obm while manually adding a node, previously the obm document is bind with node, so it's pretty straightforward, now the obm is moved out, the situation is a little hard. If let user manually add the obm before creating node, but you don't know the node id; If bind obm document with node, then there is some what mismatch with our internal structure.

So @srinia6 @brianparry : What's your preference for how to specify the obm while adding a node?

@yyscamper The line you've referenced in nodes-api-service is a bug. node.obmSettings will always be undefined. A separate query to the obms collection is required to get the obms associated with that node. The autoDiscover case is one that we missed when refactoring obms, and I think this is a reasonable approach to deal with it. I think we should change the property in the body from obmSettings to obms though. This will match the name of the collection and the way this is displayed in the view.

srinia6

body.obmSettings does not exists in the API 2.0, and the proposed changes are now handled via obms route and/or node/id/obms route. Your code needs to be re-visited.

brianparry

Overall 👍 but I think we need to make some changes to the schema for request validation.

brianparry · 2016-11-04T17:34:30Z

lib/services/nodes-api-service.js

            return waterline.nodes.create(body);
        }).tap(function(node) {
            return eventsProtocol.publishNodeEvent(node, 'added');
+        }).tap(function(node) {


@yyscamper The line you've referenced in nodes-api-service is a bug. node.obmSettings will always be undefined. A separate query to the obms collection is required to get the obms associated with that node. The autoDiscover case is one that we missed when refactoring obms, and I think this is a reasonable approach to deal with it. I think we should change the property in the body from obmSettings to obms though. This will match the name of the collection and the way this is displayed in the view.

brianparry · 2016-11-04T18:08:10Z

lib/services/nodes-api-service.js

    NodeApiService.prototype.postNode = function(body) {
-        return Promise.resolve().then(function() {
+        return Promise.try (function() {
+            assert.string(body.type, 'node type is required');


@yyscamper This should be handled in the schema; we should be adding type to the list of required properties: https://github.com/RackHD/on-http/blob/master/static/schemas/2.0/node.2.0.json#L54. You shouldn't need to validate the type since we have a enum in the schema with all node types. The new obms request option should be added to the nodes schema as an array of objects. The tricky part here is that we should be referencing the obm schema (https://github.com/RackHD/on-http/tree/master/static/schemas/obms) here to validate the obms themselves.

yyscamper · 2016-11-10T10:05:55Z

@brianparry : Your comment has been resolved. Actually this is more difficult that I thought to move the validation code into schema. The challenge is:
(1) Those obm schemas are defined in different namespace.
(2) The schema for noop-obm-service is so free so that any arbitrary attributes can be put into config, so if I combines all obm schema with allOf, I tested arbitrary data can pass the validation.

So my solution is:
(1) Use the full schema name (namespace + filename) to reference the existing obm schemas.
(2) Set additionalProperties to false to obm schemas, so that the anyOf can take effect to screen out some invalid input.

JenkinsRHD · 2016-11-10T10:19:07Z

BUILD on-http #2820 : UNSTABLE

** BUILD build-config-setup #153156: SUCCESS
** BUILD unit-tests #10042: SUCCESS
** BUILD smoke-test #3758: UNSTABLE

BUILD smoke-test #3758 Error Logs ▼

Test Name: test_node_put_obm_by_node_id
Error Details: (400)
Reason: Bad Request
HTTP response headers: HTTPHeaderDict({'Content-Length': '125', 'X-Powered-By': 'Express', 'Connection': 'keep-alive', 'ETag': 'W/"7d-71y2OIahyQ02eFta/rMkng"', 'Date': 'Thu, 10 Nov 2016 10:18:24 GMT', 'Access-Control-Allow-Origin': '*', 'Content-Type': 'application/json; charset=utf-8'})
HTTP response body: {"message":"ValidationError: Additional properties not allowed","status":"400","UUID":"14266cde-66c5-4243-9ed5-fa1d07b24b05"}

-------------------- >> begin captured logging << --------------------
tests.api.v2_0.nodes_tests: INFO: {
"autoDiscover": "false",
"catalogs": "/api/2.0/nodes/58244868cf76e99d08c0a339/catalogs",
"id": "58244868cf76e99d08c0a339",
"identifiers": [],
"name": "Enclosure Node QTFCKJ51300099",
"obms": [],
"pollers": "/api/2.0/nodes/58244868cf76e99d08c0a339/pollers",
"relations": [
{
"info": null,
"relationType": "encloses",
"targets": [
"5824481fd28267a8081ed2ce"
]
}
],
"tags": "/api/2.0/nodes/58244868cf76e99d08c0a339/tags",
"type": "enclosure",
"workflows": "/api/2.0/nodes/58244868cf76e99d08c0a339/workflows"
}
--------------------- >> 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 "/tmp/.venv/local/lib/python2.7/site-packages/proboscis/case.py", line 296, in testng_method_mistake_capture_func
compatability.capture_type_error(s_func)
File "/tmp/.venv/local/lib/python2.7/site-packages/proboscis/compatability/exceptions_2_6.py", line 27, in capture_type_error
func()
File "/tmp/.venv/local/lib/python2.7/site-packages/proboscis/case.py", line 350, in func
func(test_case.state.get_state())
File "/home/jenkins/workspace/on-http/RackHD/test/tests/api/v2_0/nodes_tests.py", line 433, in test_node_put_obm_by_node_id
Api().nodes_put_obms_by_node_id(identifier=n.get('id'), body=self.__test_obm)
File "/tmp/.venv/local/lib/python2.7/site-packages/on_http_api2_0/apis/api_api.py", line 2026, in nodes_put_obms_by_node_id
callback=params.get('callback'))
File "/tmp/.venv/local/lib/python2.7/site-packages/on_http_api2_0/api_client.py", line 322, in call_api
response_type, auth_settings, callback)
File "/tmp/.venv/local/lib/python2.7/site-packages/on_http_api2_0/api_client.py", line 149, in __call_api
post_params=post_params, body=body)
File "/tmp/.venv/local/lib/python2.7/site-packages/on_http_api2_0/api_client.py", line 364, in request
body=body)
File "/tmp/.venv/local/lib/python2.7/site-packages/on_http_api2_0/rest.py", line 215, in PUT
body=body)
File "/tmp/.venv/local/lib/python2.7/site-packages/on_http_api2_0/rest.py", line 177, in request
raise ApiException(http_resp=r)
'(400)\nReason: Bad Request\nHTTP response headers: HTTPHeaderDict({'Content-Length': '125', 'X-Powered-By': 'Express', 'Connection': 'keep-alive', 'ETag': 'W/"7d-71y2OIahyQ02eFta/rMkng"', 'Date': 'Thu, 10 Nov 2016 10:18:24 GMT', 'Access-Control-Allow-Origin': '*', 'Content-Type': 'application/json; charset=utf-8'})\nHTTP response body: {"message":"ValidationError: Additional properties not allowed","status":"400","UUID":"14266cde-66c5-4243-9ed5-fa1d07b24b05"}\n\n-------------------- >> begin captured logging << --------------------\ntests.api.v2_0.nodes_tests: INFO: {\n "autoDiscover": "false",\n "catalogs": "/api/2.0/nodes/58244868cf76e99d08c0a339/catalogs",\n "id": "58244868cf76e99d08c0a339",\n "identifiers": [],\n "name": "Enclosure Node QTFCKJ51300099",\n "obms": [],\n "pollers": "/api/2.0/nodes/58244868cf76e99d08c0a339/pollers",\n "relations": [\n {\n "info": null,\n "relationType": "encloses",\n "targets": [\n "5824481fd28267a8081ed2ce"\n ]\n }\n ],\n "tags": "/api/2.0/nodes/58244868cf76e99d08c0a339/tags",\n "type": "enclosure",\n "workflows": "/api/2.0/nodes/58244868cf76e99d08c0a339/workflows"\n}\n--------------------- >> end captured logging << ---------------------'

yyscamper · 2016-11-23T17:59:48Z

@brianparry : As we talked last week. I have removed the newly created obm schema, as a follow-up, the schema validation need to be moved into the swagger fitting. Please review this PR, whether this is OK or not.

fix unit-test error due to node-cache

3c72b02

yyscamper commented Nov 4, 2016

View reviewed changes

yyscamper mentioned this pull request Nov 4, 2016

Ssh obm service #505

Merged

srinia6 reviewed Nov 4, 2016

View reviewed changes

srinia6 suggested changes Nov 4, 2016

View reviewed changes

brianparry suggested changes Nov 4, 2016

View reviewed changes

yyscamper force-pushed the fix-add-node branch from f5a2205 to fcbc3af Compare November 23, 2016 17:57

add obm settings while posting new node

fcbc3af

brianparry approved these changes Nov 23, 2016

View reviewed changes

brianparry merged commit d56dbd4 into RackHD:master Nov 23, 2016

JenkinsRHD mentioned this pull request Jan 18, 2017

Rac 1984 node/id/workflow returns appropriate response codes #551

Merged

yyscamper mentioned this pull request Jan 20, 2017

change obmSettings to obms RackHD/docs#371

Merged

JenkinsRHD mentioned this pull request Sep 13, 2017

RAC-5956 - Added some debugging for the post script processing to help address os-install hangs R… #763

Closed

Add obm setting while posting a new node #524

Add obm setting while posting a new node #524

Uh oh!

Conversation

yyscamper commented Nov 4, 2016

Uh oh!

yyscamper Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iceiilin commented Nov 4, 2016

Uh oh!

srinia6 commented Nov 4, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

srinia6 Nov 4, 2016

Choose a reason for hiding this comment

Uh oh!

yyscamper Nov 4, 2016

Choose a reason for hiding this comment

Uh oh!

brianparry Nov 4, 2016

Choose a reason for hiding this comment

Uh oh!

srinia6 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brianparry left a comment

Choose a reason for hiding this comment

Uh oh!

brianparry Nov 4, 2016

Choose a reason for hiding this comment

Uh oh!

brianparry Nov 4, 2016

Choose a reason for hiding this comment

Uh oh!

yyscamper commented Nov 10, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JenkinsRHD commented Nov 10, 2016

Uh oh!

yyscamper commented Nov 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

yyscamper Nov 4, 2016 •

edited

Loading

srinia6 commented Nov 4, 2016 •

edited

Loading

srinia6 left a comment •

edited

Loading

yyscamper commented Nov 10, 2016 •

edited

Loading