Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions data/views/ibms.2.0.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"id" : "<%=id%>",
<% if (node !== null) { %>
"node": "<%=basepath%>/nodes/<%=node%>",
<% } %>
"service": "<%= service %>",
"config": <%- JSON.stringify(config) %>
}
16 changes: 15 additions & 1 deletion data/views/node.2.0.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,5 +50,19 @@
<% } %>
<% } %>
"type": "<%=type%>",
"workflows": "<%=basepath %>/nodes/<%=id%>/workflows"
"workflows": "<%=basepath %>/nodes/<%=id%>/workflows",
<% if (ibms === null) { %>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"ibms": null,
<% } else { %>
"ibms":[
<% ibms.forEach(function (value, i, arr){ %>
{
"service": "<%=value.service%>",
"ref":"<%=basepath %>/ibms/<%=value.id%>"
}
<%= ( arr.length > 0 && i < arr.length-1 ) ? ',': '' %>
<% }); %>
]
<% } %>

}
81 changes: 58 additions & 23 deletions lib/api/1.1/northbound/nodes.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,8 @@ di.annotate(nodesRouterFactory, new di.Inject(
'Logger',
'Promise',
'_',
'Assert'
'Assert',
'Errors'
)
);

Expand All @@ -34,28 +35,41 @@ function nodesRouterFactory (
Logger,
Promise,
_,
assert
assert,
Errors
) {
var router = express.Router();

/**
* Renders obmSettings property into a node.
* Renders obmSettings and sshSettings properties into a node.
*
* @param {object} node - node model instance
* @return {object} Promise that fulfills to the rendered node
*/
function _renderNodeObmSettings(node) {
function _renderNodeMgmtSettings(node) {
// This function exists to allow backward compatibility of the 1.1 API
// with 2.0 API OBM resources.
// with 2.0 API OBM and SSH resources.
assert.object(node, 'node should be a node model instance.');
assert.ok(!node.obmSettings, 'node should not have obmSettings property');

node = _.omit(node.toJSON(), 'obms');
return Promise.map(waterline.obms.find({node: node.id}), function(obm) {
return _.pick(obm.toJSON(), 'service', 'config');
})
.then(function(obms) {
node = _.omit(node.toJSON(), ['obms', 'ibms']);

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');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

return Promise.all([obms, sshSettings])
.spread(function(obms, sshSettings) {
node.obmSettings = obms;
if (sshSettings) {
node.sshSettings = {
host: sshSettings.config.host,
user: sshSettings.config.user,
password: 'REDACTED'
};
} else {
delete node.sshSettings;
}
return node;
});
}
Expand All @@ -73,7 +87,7 @@ function nodesRouterFactory (
return nodeApiService.getAllNodes(req.query)
.then(function (nodes){
return Promise.map(nodes, function(node) {
return _renderNodeObmSettings(node);
return _renderNodeMgmtSettings(node);
});
});
}));
Expand Down Expand Up @@ -140,7 +154,7 @@ function nodesRouterFactory (
router.get('/nodes/:identifier', rest(function (req) {
return nodeApiService.getNodeById(req.params.identifier)
.then(function (node){
return _renderNodeObmSettings(node);
return _renderNodeMgmtSettings(node);
});
}));

Expand Down Expand Up @@ -184,7 +198,7 @@ function nodesRouterFactory (
})
.then(function() {
// lastly render obmSettings into the node.
return _renderNodeObmSettings(node);
return _renderNodeMgmtSettings(node);
});
}));

Expand Down Expand Up @@ -233,7 +247,7 @@ function nodesRouterFactory (
router.get('/nodes/:identifier/obm', rest(function (req) {
return nodeApiService.getNodeObmById(req.params.identifier)
.then(function (node){
return _renderNodeObmSettings(node)
return _renderNodeMgmtSettings(node)
.then(function (retNode){
return retNode.obmSettings;
});
Expand Down Expand Up @@ -284,7 +298,7 @@ function nodesRouterFactory (
});
})
.then(function() {
return _renderNodeObmSettings(node);
return _renderNodeMgmtSettings(node);
});
}));

Expand Down Expand Up @@ -328,10 +342,13 @@ function nodesRouterFactory (
*/

router.get('/nodes/:identifier/ssh', rest(function (req) {
return nodeApiService.getNodeSshById(req.params.identifier);
}, {
serializer: 'Serializables.V1.Ssh',
isObject: true
return nodeApiService.getNodeById(req.params.identifier)
.then(function(node) {
return _renderNodeMgmtSettings(node)
.then(function (retNode){
return retNode.sshSettings;
});
});
}));


Expand All @@ -353,14 +370,32 @@ function nodesRouterFactory (
*/

router.post('/nodes/:identifier/ssh', rest(function (req) {
return nodeApiService.postNodeSshById(req.params.identifier, req.body);
var node;
var newBody;

return Promise.try(function() {
if (req.body.host && req.body.user && req.body.password) {
return waterline.nodes.needByIdentifier(req.params.identifier);
} else {
throw new Errors.BadRequestError('Invalid SSH body');
}
})
.then(function(queryNode) {
node = queryNode;
newBody = {
nodeId: req.params.identifier,
service: 'ssh-ibm-service',
config: req.body
};
return waterline.ibms.upsertByNode(req.params.identifier, newBody);
})
.then(function() {
return _renderNodeMgmtSettings(node);
});
}, {
deserializer: 'Serializables.V1.Ssh',
serializer: 'Serializables.V1.Node',
renderOptions: { success: 201 }
}));


/**
* @api {get} /api/1.1/nodes/:identifier/catalogs GET /:id/catalogs
* @apiVersion 1.1.0
Expand Down
118 changes: 118 additions & 0 deletions lib/api/2.0/ibms.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,118 @@
// Copyright 2016, EMC, Inc.

'use strict';

var injector = require('../../../index.js').injector;
var controller = injector.get('Http.Services.Swagger').controller;
var schemaApiService = injector.get('Http.Api.Services.Schema');
var nameSpace = '/api/2.0/ibms/definitions/';
var waterline = injector.get('Services.Waterline');
var eventsProtocol = injector.get('Protocol.Events');
var Errors = injector.get('Errors');
var _ = injector.get('_'); // jshint ignore:line


// GET /api/2.0/ibms
var ibmsGet = controller(function(req) {
return waterline.ibms.find(req.query);
});

// PUT /api/2.0/ibms
var ibmsPut = controller({success: 201}, function(req) {
var nodeId = req.swagger.params.body.value.nodeId;
delete req.swagger.params.body.value.nodeId;
return waterline.ibms.upsertByNode(nodeId, req.swagger.params.body.value);
});

// GET /api/2.0/ibms/identifier
var ibmsGetById = controller(function(req) {
return waterline.ibms.needByIdentifier(req.swagger.params.identifier.value);
});

// PATCH /api/2.0/ibms/identifier
var ibmsPatchById = controller( function(req) {
var ibmId = req.swagger.params.identifier.value;
var values = req.swagger.params.body.value;
return waterline.ibms.needByIdentifier(ibmId)
Copy link
Member

@anhou anhou Oct 25, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@anhou anhou Oct 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.then(function(oldIbm) {
/* Get nodes that need to publish events */
if (oldIbm.node && !values.nodeId) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @anhou

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return Promise.all([waterline.nodes.getNodeById(oldIbm.node)]);
} else if (!oldIbm.node && values.nodeId) {
return Promise.all([waterline.nodes.getNodeById(values.node)]);
} else if (oldIbm.node && values.nodeId && oldIbm.node === values.nodeId) {
return Promise.all([waterline.nodes.getNodeById(oldIbm.node)]);
} else if (oldIbm.node && values.nodeId && oldIbm.node !== values.nodeId) {
return Promise.all([waterline.nodes.getNodeById(oldIbm.node),
waterline.nodes.getNodeById(values.nodeId)]);
}
})
.then(function(oldNodes) {
return waterline.ibms.updateByIdentifier(ibmId, values)
.tap(function() {
/* Publish events of nodes got beofre update */
_.forEach(oldNodes, function(oldNode) {
if (oldNode) {
/* asynchronous, don't wait promise return for performance*/
return waterline.nodes.getNodeById(oldNode.id)
.then(function(newNode) {
return eventsProtocol.publishNodeAttrEvent(oldNode, newNode, 'ibms');
})
.catch(function (error) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'error' is defined but never used.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

throw new Errors.BaseError('Error Occured', error.message);
});
}
});
});
});
});

// DELETE /api/2.0/ibms/identifier
var ibmsDeleteById = controller({success: 204}, function(req) {
var ibmId = req.swagger.params.identifier.value;
return waterline.ibms.needByIdentifier(ibmId)
.then(function (ibm) {
return waterline.nodes.getNodeById(ibm.node);
})
.then(function (oldNode) {
return waterline.ibms.destroyByIdentifier(ibmId)
.tap(function () {
if (oldNode) {
/* asynchronous, don't wait promise return for performance*/
waterline.nodes.getNodeById(oldNode.id)
.then(function (newNode) {
return eventsProtocol.publishNodeAttrEvent(oldNode, newNode, 'ibms');
})
.catch(function (error) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'error' is defined but never used.

throw new Errors.BaseError('Error Occured', error.message);
});
}
});
});

});

// GET /ibms/definitions
var ibmsDefinitionsGetAll = controller(function() {
return schemaApiService.getNamespace(nameSpace);
});

// GET /ibms/definitions/{name}
var ibmsDefinitionsGetByName = controller(function(req) {
var schemaUid = nameSpace + req.swagger.params.name.value;
var schema = schemaApiService.getSchema(schemaUid);
if (schema) {
return schema;
}
throw new Errors.NotFoundError(schemaUid + ' Not Found');
});

module.exports = {
ibmsGet: ibmsGet,
ibmsPut: ibmsPut,
ibmsGetById: ibmsGetById,
ibmsPatchById: ibmsPatchById,
ibmsDeleteById: ibmsDeleteById,
ibmsDefinitionsGetAll: ibmsDefinitionsGetAll,
ibmsDefinitionsGetByName: ibmsDefinitionsGetByName
};
13 changes: 11 additions & 2 deletions lib/fittings/rackhd_validator.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,24 @@ module.exports = function create(fittingDef) {
var schemaNameKey = fittingDef.schemaNameKey;
var operation = context.request.swagger.operation;
var schemaName = operation[schemaNameKey];
var suffix = null;
var service = null;
if (schemaName === 'obm') {
var suffix = '.json#/definitions/Obm';
var service = context.request.body.service;
suffix = '.json#/definitions/Obm';
service = context.request.body.service;
if (service) {
schemaName = service + suffix;
} else {
schemaName = 'noop-obm-service' + suffix;
}
}
else if (schemaName === 'ibm') {
suffix = '.json#/definitions/Ibm';
service = context.request.body.service;
if (service) {
schemaName = service + suffix;
}
}
validate(schemaName, context.request.body, next);
}
};
Expand Down
Loading