Fixed application patching to ensure that failures to patch on some nodes

    are always reported.

7 files changed, 120 insertions, 91 deletions

diff --git a/cpp/slice/IceGrid/Admin.ice b/cpp/slice/IceGrid/Admin.ice
index 246622b02dc..66c1e7d8537 100644
--- a/cpp/slice/IceGrid/Admin.ice
+++ b/cpp/slice/IceGrid/Admin.ice
@@ -331,8 +331,8 @@ interface Admin
      * doesn't exist.
      *
      **/
-    ["ami", "amd"] void patchApplication(string name, bool shutdown)
-	throws ApplicationNotExistException, PatchException;
+    ["ami", "amd"] bool patchApplication(string name, bool shutdown, out Ice::StringSeq reasons)
+	throws ApplicationNotExistException;
 
     /**
      *
diff --git a/cpp/src/IceGrid/AdminI.cpp b/cpp/src/IceGrid/AdminI.cpp
index 752d10a1bf7..25807ccde69 100644
--- a/cpp/src/IceGrid/AdminI.cpp
+++ b/cpp/src/IceGrid/AdminI.cpp
@@ -119,19 +119,13 @@ public:
 	    }
 
 	    ++_nFailure;
-	    _lastFailure = failure;
+	    _reasons.push_back(failure);
 	}
 
 	if((_nSuccess + _nFailure) == _count)
 	{
-	    if(_nSuccess > 0)
-	    {
-		_cb->ice_response();
-	    }
-	    else
-	    {
-		_cb->ice_exception(PatchException(_lastFailure));
-	    }
+	    sort(_reasons.begin(), _reasons.end());
+	    _cb->ice_response(!_nFailure, _reasons);
 	}
     }
 
@@ -143,7 +137,7 @@ private:
     const int _count;
     int _nSuccess;
     int _nFailure;
-    string _lastFailure;
+    Ice::StringSeq _reasons;    
 };
 typedef IceUtil::Handle<PatchAggregator> PatchAggregatorPtr;
 
@@ -176,11 +170,11 @@ public:
 	}
 	catch(const NodeNotExistException&)
 	{
-	    reason = "node doesn't exist";
+	    reason = "patch on node `" + _node + "' failed: node doesn't exist";
 	}
 	catch(const NodeUnreachableException&)
 	{
-	    reason = "node is not active";
+	    reason = "patch on node `" + _node + "' failed: node is not active";
 	}
 	catch(const Ice::Exception& ex)
 	{
@@ -235,11 +229,11 @@ public:
 	}
 	catch(const NodeNotExistException&)
 	{
-	    reason = "node `" + _node + "' doesn't exist";
+	    reason = "patch on node `" + _node + "' failed: node doesn't exist";
 	}
 	catch(const NodeUnreachableException&)
 	{
-	    reason = "node `" + _node + "' is not active";
+	    reason = "patch on node `" + _node + "' failed: node is not active";
 	}
 	catch(const Ice::Exception& ex)
 	{
@@ -321,7 +315,7 @@ AdminI::patchApplication_async(const AMD_Admin_patchApplicationPtr& amdCB,
 
     if(nodes.empty())
     {
-	amdCB->ice_response();
+	amdCB->ice_response(true, Ice::StringSeq());
 	return;
     }
 
diff --git a/cpp/src/IceGrid/Database.cpp b/cpp/src/IceGrid/Database.cpp
index 0ec22eef982..144544d3e46 100644
--- a/cpp/src/IceGrid/Database.cpp
+++ b/cpp/src/IceGrid/Database.cpp
@@ -331,7 +331,7 @@ Database::updateApplicationDescriptor(AdminSessionI* session, const ApplicationU
 	
 	reload(previous, helper, entries);
 	
-	_descriptors.put(StringApplicationDescriptorDict::value_type(update.name, helper.getDescriptor()));
+	_descriptors.put(StringApplicationDescriptorDict::value_type(update.name, helper.getDefinition()));
 
 	serial = ++_serial;
     }    
@@ -465,7 +465,7 @@ Database::instantiateServer(const string& application, const string& node, const
 	
 	reload(previous, helper, entries);
 	
-	_descriptors.put(StringApplicationDescriptorDict::value_type(application, helper.getDescriptor()));
+	_descriptors.put(StringApplicationDescriptorDict::value_type(application, helper.getDefinition()));
 
 	serial = ++_serial;
     }    
diff --git a/cpp/src/IceGrid/DescriptorHelper.cpp b/cpp/src/IceGrid/DescriptorHelper.cpp
index c5dc02b73cc..f489f2bb11e 100644
--- a/cpp/src/IceGrid/DescriptorHelper.cpp
+++ b/cpp/src/IceGrid/DescriptorHelper.cpp
@@ -1460,6 +1460,7 @@ ServiceInstanceHelper::ServiceInstanceHelper(const Ice::CommunicatorPtr& com, co
     {
 	throw DeploymentException("invalid service instance: no template defined");
     }
+
     if(_def.descriptor)
     {
 	_service = ServiceHelper(com, _def.descriptor);
@@ -1550,8 +1551,7 @@ ServiceInstanceHelper::print(Output& out) const
 	out << nl << "template = `" << _def._cpp_template << "'";
 	out << nl << "parameters";
 	out << sb;
-	for(StringStringDict::const_iterator p = _def.parameterValues.begin(); 
-	    p != _def.parameterValues.end(); ++p)
+	for(StringStringDict::const_iterator p = _def.parameterValues.begin(); p != _def.parameterValues.end(); ++p)
 	{
 	    out << nl << p->first << " = `" << p->second << "'";
 	}
@@ -1779,10 +1779,6 @@ NodeHelper::NodeHelper(const string& name, const NodeDescriptor& descriptor, con
     }
 
     //
-    // Other misc. validation...
-    //
-
-    //
     // Validate each property set references.
     //
     PropertySetDescriptorDict::const_iterator ps;
@@ -1878,22 +1874,16 @@ NodeHelper::diff(const NodeHelper& helper) const
 NodeDescriptor
 NodeHelper::update(const NodeUpdateDescriptor& update, const Resolver& appResolve) const
 {
-    NodeDescriptor def = _def; // TODO: XXX deep copy ?!
+    NodeDescriptor def;
     assert(update.name == _name);
 
     //
     // Update the variables, property sets, load factor, description.
     //
-    def.variables = updateDictElts(def.variables, update.variables, update.removeVariables);
-    def.propertySets = updateDictElts(def.propertySets, update.propertySets, update.removePropertySets);
-    if(update.loadFactor)
-    {
-	def.loadFactor = update.loadFactor->value;
-    }    
-    if(update.description)
-    {
-	def.description = update.description->value;
-    }
+    def.variables = updateDictElts(_def.variables, update.variables, update.removeVariables);
+    def.propertySets = updateDictElts(_def.propertySets, update.propertySets, update.removePropertySets);
+    def.loadFactor = update.loadFactor ? update.loadFactor->value : _def.loadFactor;
+    def.description = update.description ? update.description->value : _def.description;
     
     //
     // NOTE: It's important to create the resolver *after* updating
@@ -1915,7 +1905,7 @@ NodeHelper::update(const NodeUpdateDescriptor& update, const Resolver& appResolv
     ServerInstanceHelperDict::const_iterator r;
     ServerInstanceDescriptorSeq::const_iterator q;
 
-    set<string> removeServers(update.removeServers.begin(), update.removeServers.end());
+    set<string> removedServers(update.removeServers.begin(), update.removeServers.end());
 
     for(q = update.serverInstances.begin(); q != update.serverInstances.end(); ++q)
     {
@@ -1927,7 +1917,7 @@ NodeHelper::update(const NodeUpdateDescriptor& update, const Resolver& appResolv
     }
     for(r = _serverInstances.begin(); r != _serverInstances.end(); ++r)
     {
-	if(removeServers.find(r->first) != removeServers.end() ||
+	if(removedServers.find(r->first) != removedServers.end() ||
 	   serverInstances.find(r->first) != serverInstances.end())
 	{
 	    continue;
@@ -1957,7 +1947,7 @@ NodeHelper::update(const NodeUpdateDescriptor& update, const Resolver& appResolv
     }    
     for(r = _servers.begin(); r != _servers.end(); ++r)
     {
-	if(removeServers.find(r->first) != removeServers.end() ||
+	if(removedServers.find(r->first) != removedServers.end() ||
 	   serverInstances.find(r->first) != serverInstances.end())
 	{
 	    continue;
@@ -1977,13 +1967,10 @@ NodeHelper::update(const NodeUpdateDescriptor& update, const Resolver& appResolv
 	servers.insert(make_pair(helper.getId(), helper));
     }
 
-    def.serverInstances.clear();
     for(r = serverInstances.begin(); r != serverInstances.end(); ++r)
     {
 	def.serverInstances.push_back(r->second.getDefinition());
     }
-
-    def.servers.clear();
     for(r = servers.begin(); r != servers.end(); ++r)
     {
 	def.servers.push_back(r->second.getServerDefinition());
@@ -2009,7 +1996,7 @@ NodeHelper::getIds(multiset<string>& serverIds, multiset<string>& adapterIds, mu
 }
 
 const NodeDescriptor&
-NodeHelper::getDescriptor() const
+NodeHelper::getDefinition() const
 {
     return _def;
 }
@@ -2305,7 +2292,6 @@ ApplicationHelper::ApplicationHelper(const Ice::CommunicatorPtr& communicator, c
     //
     // Instantiate the nodes.
     //
-    _instance.nodes.clear();
     NodeHelperDict::const_iterator n;
     for(NodeDescriptorDict::const_iterator p = _def.nodes.begin(); p != _def.nodes.end(); ++p)
     {
@@ -2404,7 +2390,7 @@ ApplicationHelper::diff(const ApplicationHelper& helper) const
 	if(q == helper._nodes.end())
 	{
 	    NodeUpdateDescriptor nodeUpdate;
-	    const NodeDescriptor& node = p->second.getDescriptor();
+	    const NodeDescriptor& node = p->second.getDefinition();
 	    nodeUpdate.name = p->first;
 	    nodeUpdate.variables = node.variables;
 	    nodeUpdate.servers = node.servers;
@@ -2426,53 +2412,58 @@ ApplicationHelper::diff(const ApplicationHelper& helper) const
 ApplicationDescriptor
 ApplicationHelper::update(const ApplicationUpdateDescriptor& updt) const
 {
-    ApplicationDescriptor def = _def; // TODO: Deep copy??? XXX
-
-    if(updt.description)
-    {
-	def.description = updt.description->value;
-    }
-
-    def.replicaGroups =
-	updateSeqElts(def.replicaGroups, updt.replicaGroups, updt.removeReplicaGroups, GetReplicaGroupId());
-    
-    def.variables = updateDictElts(def.variables, updt.variables, updt.removeVariables);
-    def.propertySets = updateDictElts(def.propertySets, updt.propertySets, updt.removePropertySets);
-
-    if(updt.distrib)
-    {
-	def.distrib = updt.distrib->value;
-    }
-
-    def.serverTemplates = updateDictElts(def.serverTemplates, updt.serverTemplates, updt.removeServerTemplates);
-    def.serviceTemplates = updateDictElts(def.serviceTemplates, updt.serviceTemplates, updt.removeServiceTemplates);
+    ApplicationDescriptor def;
+    GetReplicaGroupId rg;
 
-    for(Ice::StringSeq::const_iterator r = updt.removeNodes.begin(); r != updt.removeNodes.end(); ++r)
-    {
- 	def.nodes.erase(*r);
-    }
+    def.name = _def.name;
+    def.description = updt.description ? updt.description->value : _def.description;
+    def.distrib = updt.distrib ? updt.distrib->value : _def.distrib;
+    def.replicaGroups = updateSeqElts(_def.replicaGroups, updt.replicaGroups, updt.removeReplicaGroups, rg);    
+    def.variables = updateDictElts(_def.variables, updt.variables, updt.removeVariables);
+    def.propertySets = updateDictElts(_def.propertySets, updt.propertySets, updt.removePropertySets);
+    def.serverTemplates = updateDictElts(_def.serverTemplates, updt.serverTemplates, updt.removeServerTemplates);
+    def.serviceTemplates = updateDictElts(_def.serviceTemplates, updt.serviceTemplates, updt.removeServiceTemplates);
 
     Resolver resolve(def, _communicator); // A resolver based on the *updated* application descriptor.
     for(NodeUpdateDescriptorSeq::const_iterator p = updt.nodes.begin(); p != updt.nodes.end(); ++p)
     {
-	NodeDescriptorDict::iterator q = def.nodes.find(p->name);
-	if(q == def.nodes.end())
+	NodeHelperDict::const_iterator q = _nodes.find(p->name);
+	if(q != _nodes.end()) // Updated node
+	{
+	    def.nodes.insert(make_pair(p->name, q->second.update(*p, resolve)));
+	}
+	else // New node
 	{
 	    NodeDescriptor desc;
 	    desc.variables = p->variables;
+	    if(!p->removeVariables.empty())
+	    {
+		resolve.exception("can't remove variables for node `" + p->name + "': node doesn't exist");
+	    }
 	    desc.propertySets = p->propertySets;
+	    if(!p->removePropertySets.empty())
+	    {
+		resolve.exception("can't remove property sets for node `" + p->name + "': node doesn't exist");
+	    }
 	    desc.servers = p->servers;
 	    desc.serverInstances = p->serverInstances;
+	    if(!p->removeServers.empty())
+	    {
+		resolve.exception("can't remove servers for node `" + p->name + "': node doesn't exist");
+	    }
 	    desc.loadFactor = p->loadFactor ? p->loadFactor->value : "";
 	    desc.description = p->description ? p->description->value : "";
 	    def.nodes.insert(make_pair(p->name, desc));
 	}
-	else
+    }
+    set<string> removedNodes(updt.removeNodes.begin(), updt.removeNodes.end());
+    for(NodeHelperDict::const_iterator n = _nodes.begin(); n != _nodes.end(); ++n)
+    {
+	if(removedNodes.find(n->first) != removedNodes.end() || def.nodes.find(n->first) != def.nodes.end())
 	{
-	    NodeHelperDict::const_iterator n = _nodes.find(q->first);
-	    assert(n != _nodes.end());
-	    q->second = n->second.update(*p, resolve); // The resolver is required to compute server IDs.
+	    continue; // Node was removed or updated.
 	}
+	def.nodes.insert(make_pair(n->first, n->second.getDefinition()));
     }
 
     return def;
@@ -2481,7 +2472,13 @@ ApplicationHelper::update(const ApplicationUpdateDescriptor& updt) const
 ApplicationDescriptor
 ApplicationHelper::instantiateServer(const string& node, const ServerInstanceDescriptor& instance) const
 {
-    ApplicationDescriptor def = _def; // TODO: Deep copy??? XXX
+    //
+    // Copy this application descriptor definition and add a server
+    // instance to the given node. The caller should then construct
+    // an application helper with the new application definition to
+    // ensure it's valid.
+    //
+    ApplicationDescriptor def = _def;
     NodeDescriptorDict::iterator q = def.nodes.find(node);
     if(q == def.nodes.end())
     {
@@ -2492,7 +2489,7 @@ ApplicationHelper::instantiateServer(const string& node, const ServerInstanceDes
     else
     {
 	q->second.serverInstances.push_back(instance);
-    }    
+    }
     return def;
 }
 
@@ -2523,7 +2520,7 @@ ApplicationHelper::getIds(set<string>& serverIds, set<string>& adapterIds, set<I
 }
 
 const ApplicationDescriptor&
-ApplicationHelper::getDescriptor() const
+ApplicationHelper::getDefinition() const
 {
     return _def;
 }
@@ -2846,7 +2843,8 @@ ApplicationHelper::printDiff(Output& out, const ApplicationHelper& helper) const
 }
 
 bool
-IceGrid::descriptorEqual(const Ice::CommunicatorPtr& com, const ServerDescriptorPtr& lhs, 
+IceGrid::descriptorEqual(const Ice::CommunicatorPtr& com, 
+			 const ServerDescriptorPtr& lhs, 
 			 const ServerDescriptorPtr& rhs)
 {
     IceBoxDescriptorPtr lhsIceBox = IceBoxDescriptorPtr::dynamicCast(lhs);
diff --git a/cpp/src/IceGrid/DescriptorHelper.h b/cpp/src/IceGrid/DescriptorHelper.h
index 8f372e71fc3..00c98979bfd 100644
--- a/cpp/src/IceGrid/DescriptorHelper.h
+++ b/cpp/src/IceGrid/DescriptorHelper.h
@@ -262,7 +262,7 @@ public:
     NodeDescriptor update(const NodeUpdateDescriptor&, const Resolver&) const;
 
     void getIds(std::multiset<std::string>&, std::multiset<std::string>&, std::multiset<Ice::Identity>&) const;
-    const NodeDescriptor& getDescriptor() const;
+    const NodeDescriptor& getDefinition() const;
     const NodeDescriptor& getInstance() const;
     void getServerInfos(const std::string&, std::map<std::string, ServerInfo>&) const;
     bool hasDistributions(const std::string&) const;
@@ -295,7 +295,7 @@ public:
     ApplicationDescriptor instantiateServer(const std::string&, const ServerInstanceDescriptor&) const;
 
     void getIds(std::set<std::string>&, std::set<std::string>&, std::set<Ice::Identity>&) const;
-    const ApplicationDescriptor& getDescriptor() const;
+    const ApplicationDescriptor& getDefinition() const;
     const ApplicationDescriptor& getInstance() const;
     void getDistributions(DistributionDescriptor&, std::vector<std::string>&,const std::string& = std::string()) const;
 
diff --git a/cpp/src/IceGrid/Parser.cpp b/cpp/src/IceGrid/Parser.cpp
index 0f584c4524b..ce70a4a73b7 100644
--- a/cpp/src/IceGrid/Parser.cpp
+++ b/cpp/src/IceGrid/Parser.cpp
@@ -189,15 +189,10 @@ Parser::addApplication(const list<string>& origArgs)
 	    //
 	    // Patch the application.
 	    //
-	    try
+	    Ice::StringSeq reasons;
+	    if(!_admin->patchApplication(app.name, true, reasons))
 	    {
-		_admin->patchApplication(app.name, true);
-	    }
-	    catch(const PatchException& ex)
-	    {
-		ostringstream s;
-		s << ex << ":\n" << ex.reason;
-		warning("the application was successfully added but the patch failed:\n" + s.str());
+		patchFailed(reasons);
 	    }
 	}
     }
@@ -373,7 +368,11 @@ Parser::patchApplication(const list<string>& origArgs)
     {
 	vector<string>::const_iterator p = args.begin();
 	string name = *p++;
-	_admin->patchApplication(name, opts.isSet("f") || opts.isSet("force"));
+	Ice::StringSeq reasons;
+	if(!_admin->patchApplication(name, opts.isSet("f") || opts.isSet("force"), reasons))
+	{
+	    patchFailed(reasons);
+	}
     }
     catch(const Ice::Exception& ex)
     {
@@ -1366,6 +1365,44 @@ Parser::invalidCommand(const string& s)
 }
 
 void
+Parser::patchFailed(const Ice::StringSeq& reasons)
+{
+    ostringstream os;
+    IceUtil::Output out(os);
+    out.setIndent(2);
+    out << "the patch failed on some nodes:\n";
+    for(Ice::StringSeq::const_iterator p = reasons.begin(); p != reasons.end(); ++p)
+    {
+	string reason = *p;
+	string::size_type beg = 0;
+	string::size_type end = reason.find_first_of("\n");
+	if(end == string::npos)
+	{
+	    end = reason.size();
+	}
+	out << "- " << reason.substr(beg, end - beg);
+	out.inc();
+	while(end < reason.size())
+	{
+	    beg = end + 1;
+	    end = reason.find_first_of("\n", beg);
+	    if(end == string::npos)
+	    {
+		end = reason.size();
+	    }
+	    out.nl();
+	    out << reason.substr(beg, end - beg);
+	}
+	out.dec();
+	if(p + 1 != reasons.end())
+	{
+	    out.nl();
+	}
+    }
+    warning(os.str());
+}
+
+void
 Parser::error(const char* s)
 {
     if(_commands.empty() && !isatty(fileno(yyin)))
diff --git a/cpp/src/IceGrid/Parser.h b/cpp/src/IceGrid/Parser.h
index e8f937a96ed..9a4aaa50614 100644
--- a/cpp/src/IceGrid/Parser.h
+++ b/cpp/src/IceGrid/Parser.h
@@ -124,7 +124,7 @@ public:
 
     void invalidCommand(const char*);
     void invalidCommand(const std::string&);
-
+    void patchFailed(const Ice::StringSeq&);
     void error(const char*);
     void error(const std::string&);