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
30 changes: 5 additions & 25 deletions lib/api/nodes.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,27 +12,27 @@ di.annotate(nodesRouterFactory, new di.Inject(
'Services.Waterline',
'Protocol.TaskGraphRunner',
'Http.Services.RestApi',
'Http.Services.Api.Nodes',
'Services.Configuration',
'Task.Services.OBM',
'ipmi-obm-service',
'Logger',
'_',
'Errors',
'Promise'
'Errors'
)
);

function nodesRouterFactory (
waterline,
taskGraphProtocol,
rest,
nodeApiService,
configuration,
ObmService,
ipmiObmServiceFactory,
Logger,
_,
Errors,
Promise
Errors
) {
var router = express.Router();

Expand Down Expand Up @@ -209,27 +209,7 @@ function nodesRouterFactory (
router.delete('/nodes/:identifier', rest(function (req) {
return waterline.nodes.needByIdentifier(req.params.identifier)
.then(function (node) {
return taskGraphProtocol.getActiveTaskGraph( { target: node.id })
.then(function (graph) {
if (graph) {
throw new Errors.BadRequestError('Could not remove node ' + node.id +
', active workflow is running');
}
})
.then(function () {
return Promise.settle([
//lookups should be destoryed here, only clear node field
//as a workaround until the issue that when lookups are cleared
//it cannot be updated timely in nodes' next bootup is fixed
waterline.lookups.update({ node: node.id },{ node: '' } ),
waterline.nodes.destroy({ id: node.id }),
waterline.catalogs.destroy({ node: node.id }),
waterline.workitems.destroy({ node: node.id })
]);
})
.then(function () {
return node;
});
return nodeApiService.removeNode(node);
});
}));

Expand Down
132 changes: 132 additions & 0 deletions lib/services/nodes-api-service.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
// Copyright 2015, EMC, Inc.

'use strict';

var di = require('di');

module.exports = nodeApiServiceFactory;
di.annotate(nodeApiServiceFactory, new di.Provide('Http.Services.Api.Nodes'));
di.annotate(nodeApiServiceFactory,
new di.Inject(
'Protocol.TaskGraphRunner',
'Services.Waterline',
'Errors',
'Logger',
'_',
'Promise'
)
);
function nodeApiServiceFactory(
taskGraphProtocol,
waterline,
Errors,
Logger,
_,
Promise
) {
var logger = Logger.initialize(nodeApiServiceFactory);

function NodeApiService() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good, glad we have a node api service now. Just as a note it might make sense going forward for us to start moving the rest of logic in the node routes into here (switch/pdu discovery, etc.).

Copy link
Contributor

Choose a reason for hiding this comment

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

When I work on the bootstrap, I feel it is better if we can move the profile-api-service and template render service to the on-core, then some of the function (like rendering) can be shared in the job implementation (on-tasks).
Same to the nodes-api-service, in future we may have some internal logic to remove the nodes of a enclosure.

Of course, we can keep it here right now and move to on-core if we come across that situation.

Copy link
Member Author

Choose a reason for hiding this comment

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

@yyscamper I don't think it's proper be put into on-core, some of the service would run specific taskgraph, it's not good that it appears in on-core.

Copy link
Contributor

Choose a reason for hiding this comment

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

@anhou, the "Protocol.TaskGraphRunner" is just in on-core, so I think it's proper. Image future we implements the auto node removal, I think this node-api-service is exactly what we need for auto node removal.

Anyway, it's OK for me that move it to on-core after we come across that situation.

}

/**
* Find Enclosure nodes which enclose compute node
* @param {Object} node
* @return {Promise}
*/
NodeApiService.prototype._findEnclNodes = function(node) {
// Find the enclosure nodes who enclose this compute node
if (!_.has(node, 'relations')) {
return Promise.resolve();
}

var relation = _.find(node.relations, { relationType: 'enclosedBy' });
if (!relation || !_.has(relation, 'targets') ) {
return Promise.resolve();
}

return Promise.map(relation.targets, function (enclNodeId) {
return waterline.nodes.needByIdentifier(enclNodeId)
.catch(function (err) {
logger.error("Error Getting Enclosure Node", { error: err });
return;
});
});
};

/**
* Remove the relations between node and enclosure node, if enclosure node
* doesn't enclose any nodes, this enclosure node is also removed.
* @param {Object} node
* @param {Array} enclNodes
* @return {Promise}
*/
NodeApiService.prototype._removeEnclRelations = function(node, enclNodes) {
if (!enclNodes) {
return Promise.resolve();
}

// Remove the relationship between enclosure and compute node
return Promise.map(enclNodes, function(enclNode) {
if (!_.has(enclNode, 'relations')) {
return;
}

var index = _.findIndex(enclNode.relations, { relationType: 'encloses' });
if (index === -1 || !_.has(enclNode.relations[index], 'targets')) {
return;
}

if (_.indexOf(enclNode.relations[index].targets, node.id) !== -1) {
_.pull(enclNode.relations[index].targets, node.id);
}
// If Enclosure node doesn't have enclosed nodes
// remove this enclosure node, else remove relationships
if (enclNode.relations[index].targets.length === 0) {
return waterline.nodes.destroy({ id: enclNode.id });
} else {
return waterline.nodes.updateByIdentifier(
enclNode.id, { relations : enclNode.relations });
}
});
};

/**
* Remove node related data and remove its relations with other nodes
* @param {Object} node
* @return {Promise}
*/
NodeApiService.prototype.removeNode = function(node) {
var self = this;

return taskGraphProtocol.getActiveTaskGraph( { target: node.id })
.then(function (graph) {
if (graph) {
throw new Errors.BadRequestError('Could not remove node ' + node.id +
', active workflow is running');
}
})
.then(function () {
return Promise.settle([
//lookups should be destoryed here, only clear node field
//as a workaround until the issue that when lookups are cleared
//it cannot be updated timely in nodes' next bootup is fixed
waterline.lookups.update({ node: node.id },{ node: '' } ),
waterline.nodes.destroy({ id: node.id }),
waterline.catalogs.destroy({ node: node.id }),
waterline.workitems.destroy({ node: node.id })
]);
})
.then(function () {
return self._findEnclNodes(node);
})
.then(function (enclNodes) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to make the code in these promise blocks their own named functions, that way we can more easily test their functionality individually, and also have a more easily readable promise chain.

return self._removeEnclRelations(node, enclNodes);
})
.then(function () {
return node;
});
};

return new NodeApiService();
}
18 changes: 6 additions & 12 deletions spec/lib/api/nodes-spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ describe('Http.Api.Nodes', function () {
var waterline;
var ObmService;
var taskGraphProtocol;
var nodeApiService;
var Promise;
var Errors;

Expand All @@ -28,7 +29,9 @@ describe('Http.Api.Nodes', function () {
sinon.stub(ObmService.prototype, 'identifyOn');
sinon.stub(ObmService.prototype, 'identifyOff');
taskGraphProtocol = helper.injector.get('Protocol.TaskGraphRunner');
nodeApiService = helper.injector.get('Http.Services.Api.Nodes');
sinon.stub(taskGraphProtocol);
sinon.stub(nodeApiService);

Promise = helper.injector.get('Promise');
Errors = helper.injector.get('Errors');
Expand All @@ -50,6 +53,7 @@ describe('Http.Api.Nodes', function () {
resetStubs(waterline.workitems);
resetStubs(waterline.graphobjects);
resetStubs(taskGraphProtocol);
resetStubs(nodeApiService);

ObmService.prototype.identifyOn.reset();
ObmService.prototype.identifyOff.reset();
Expand Down Expand Up @@ -270,23 +274,13 @@ describe('Http.Api.Nodes', function () {
describe('DELETE /nodes/:identifier', function () {
it('should delete a node', function () {
waterline.nodes.needByIdentifier.resolves(node);
taskGraphProtocol.getActiveTaskGraph.resolves();
waterline.lookups.update.resolves();
waterline.nodes.destroy.resolves();
waterline.catalogs.destroy.resolves();
waterline.workitems.destroy.resolves();
nodeApiService.removeNode.resolves(node);

return helper.request().delete('/api/1.1/nodes/1234')
.expect('Content-Type', /^application\/json/)
.expect(200, node)
.expect(function () {
expect(taskGraphProtocol.getActiveTaskGraph).to.have.been.calledOnce;
expect(waterline.lookups.update).to.have.been.calledOnce;
expect(waterline.nodes.destroy).to.have.been.calledOnce;
expect(waterline.catalogs.destroy).to.have.been.calledOnce;
expect(waterline.workitems.destroy).to.have.been.calledOnce;
expect(waterline.nodes.destroy.firstCall.args[0])
.to.have.property('id').that.equals(node.id);
expect(nodeApiService.removeNode).to.have.been.calledOnce;
});
});

Expand Down
Loading