[ARVADOS] created: 242d3272391baeb95eb0a5e4e51627c2d54e7bc6
git at public.curoverse.com
git at public.curoverse.com
Wed Nov 4 12:21:54 EST 2015
at 242d3272391baeb95eb0a5e4e51627c2d54e7bc6 (commit)
commit 242d3272391baeb95eb0a5e4e51627c2d54e7bc6
Author: Brett Smith <brett at curoverse.com>
Date: Wed Nov 4 12:20:36 2015 -0500
7713: Node Manager blackholes broken nodes that can't shut down.
We are seeing situations on Azure where some nodes in an UNKNOWN state
cannot be shut down. The API call to destroy them always fails.
There are two related halves to this commit. In the first half,
after a cloud shutdown request fails, ComputeNodeShutdownActor checks
whether the node is broken. If it is, it cancels shutdown retries.
In the second half, the daemon checks for this shutdown outcome. When
it happens, it blacklists the broken node: it will immediately filter
it out of node lists from the cloud. It is no longer monitored in any
way or counted as a live node, so Node Manager will boot a replacement
for it.
This lets Node Manager create cloud nodes above max_nodes, up to the
number of broken nodes. We're reasonably bounded in for now because
only the Azure driver will ever declare a node broken. Other clouds
will never blacklist nodes this way.
diff --git a/services/nodemanager/arvnodeman/computenode/dispatch/__init__.py b/services/nodemanager/arvnodeman/computenode/dispatch/__init__.py
index 1c828c1..3c70877 100644
--- a/services/nodemanager/arvnodeman/computenode/dispatch/__init__.py
+++ b/services/nodemanager/arvnodeman/computenode/dispatch/__init__.py
@@ -154,6 +154,10 @@ class ComputeNodeShutdownActor(ComputeNodeStateChangeBase):
This actor simply destroys a cloud node, retrying as needed.
"""
+ # Reasons for a shutdown to be cancelled.
+ WINDOW_CLOSED = "shutdown window closed"
+ NODE_BROKEN = "cloud failed to shut down broken node"
+
def __init__(self, timer_actor, cloud_client, arvados_client, node_monitor,
cancellable=True, retry_wait=1, max_retry_wait=180):
# If a ShutdownActor is cancellable, it will ask the
@@ -167,6 +171,7 @@ class ComputeNodeShutdownActor(ComputeNodeStateChangeBase):
self._monitor = node_monitor.proxy()
self.cloud_node = self._monitor.cloud_node.get()
self.cancellable = cancellable
+ self.cancel_reason = None
self.success = None
def on_start(self):
@@ -180,7 +185,10 @@ class ComputeNodeShutdownActor(ComputeNodeStateChangeBase):
self.success = success_flag
return super(ComputeNodeShutdownActor, self)._finished()
- def cancel_shutdown(self):
+ def cancel_shutdown(self, reason):
+ self.cancel_reason = reason
+ self._logger.info("Cloud node %s shutdown cancelled: %s.",
+ self.cloud_node.id, reason)
self._finished(success_flag=False)
def _stop_if_window_closed(orig_func):
@@ -188,10 +196,7 @@ class ComputeNodeShutdownActor(ComputeNodeStateChangeBase):
def stop_wrapper(self, *args, **kwargs):
if (self.cancellable and
(not self._monitor.shutdown_eligible().get())):
- self._logger.info(
- "Cloud node %s shutdown cancelled - no longer eligible.",
- self.cloud_node.id)
- self._later.cancel_shutdown()
+ self._later.cancel_shutdown(self.WINDOW_CLOSED)
return None
else:
return orig_func(self, *args, **kwargs)
@@ -201,8 +206,11 @@ class ComputeNodeShutdownActor(ComputeNodeStateChangeBase):
@ComputeNodeStateChangeBase._retry()
def shutdown_node(self):
if not self._cloud.destroy_node(self.cloud_node):
- # Force a retry.
- raise cloud_types.LibcloudError("destroy_node failed")
+ if self._cloud.broken(self.cloud_node):
+ self._later.cancel_shutdown(self.NODE_BROKEN)
+ else:
+ # Force a retry.
+ raise cloud_types.LibcloudError("destroy_node failed")
self._logger.info("Cloud node %s shut down.", self.cloud_node.id)
arv_node = self._arvados_node()
if arv_node is None:
diff --git a/services/nodemanager/arvnodeman/computenode/dispatch/slurm.py b/services/nodemanager/arvnodeman/computenode/dispatch/slurm.py
index ec5014e..919b57f 100644
--- a/services/nodemanager/arvnodeman/computenode/dispatch/slurm.py
+++ b/services/nodemanager/arvnodeman/computenode/dispatch/slurm.py
@@ -43,7 +43,7 @@ class ComputeNodeShutdownActor(ShutdownActorBase):
# error are still being investigated.
@ShutdownActorBase._retry((subprocess.CalledProcessError, OSError))
- def cancel_shutdown(self):
+ def cancel_shutdown(self, reason):
if self._nodename:
if self._get_slurm_state() in self.SLURM_DRAIN_STATES:
# Resume from "drng" or "drain"
@@ -52,7 +52,7 @@ class ComputeNodeShutdownActor(ShutdownActorBase):
# Node is in a state such as 'idle' or 'alloc' so don't
# try to resume it because that will just raise an error.
pass
- return super(ComputeNodeShutdownActor, self).cancel_shutdown()
+ return super(ComputeNodeShutdownActor, self).cancel_shutdown(reason)
@ShutdownActorBase._retry((subprocess.CalledProcessError, OSError))
@ShutdownActorBase._stop_if_window_closed
diff --git a/services/nodemanager/arvnodeman/daemon.py b/services/nodemanager/arvnodeman/daemon.py
index 1d52073..a65e9a0 100644
--- a/services/nodemanager/arvnodeman/daemon.py
+++ b/services/nodemanager/arvnodeman/daemon.py
@@ -25,6 +25,7 @@ class _BaseNodeTracker(object):
def __init__(self):
self.nodes = {}
self.orphans = {}
+ self._blacklist = set()
# Proxy the methods listed below to self.nodes.
def _proxy_method(name):
@@ -43,6 +44,9 @@ class _BaseNodeTracker(object):
def add(self, record):
self.nodes[self.record_key(record)] = record
+ def blacklist(self, key):
+ self._blacklist.add(key)
+
def update_record(self, key, item):
setattr(self.nodes[key], self.RECORD_ATTR, item)
@@ -50,7 +54,9 @@ class _BaseNodeTracker(object):
unseen = set(self.nodes.iterkeys())
for item in response:
key = self.item_key(item)
- if key in unseen:
+ if key in self._blacklist:
+ continue
+ elif key in unseen:
unseen.remove(key)
self.update_record(key, item)
else:
@@ -346,11 +352,13 @@ class NodeManagerDaemonActor(actor_class):
self._begin_node_shutdown(record.actor, cancellable=False)
def node_finished_shutdown(self, shutdown_actor):
- success, cloud_node = self._get_actor_attrs(shutdown_actor, 'success',
- 'cloud_node')
+ cloud_node, success, cancel_reason = self._get_actor_attrs(
+ shutdown_actor, 'cloud_node', 'success', 'cancel_reason')
shutdown_actor.stop()
cloud_node_id = cloud_node.id
if not success:
+ if cancel_reason == self._node_shutdown.NODE_BROKEN:
+ self.cloud_nodes.blacklist(cloud_node_id)
del self.shutdowns[cloud_node_id]
elif cloud_node_id in self.booted:
self.booted.pop(cloud_node_id).actor.stop()
diff --git a/services/nodemanager/tests/test_computenode_dispatch.py b/services/nodemanager/tests/test_computenode_dispatch.py
index e718fc1..0bdb2cb 100644
--- a/services/nodemanager/tests/test_computenode_dispatch.py
+++ b/services/nodemanager/tests/test_computenode_dispatch.py
@@ -116,11 +116,12 @@ class ComputeNodeSetupActorTestCase(testutil.ActorTestMixin, unittest.TestCase):
class ComputeNodeShutdownActorMixin(testutil.ActorTestMixin):
def make_mocks(self, cloud_node=None, arvados_node=None,
- shutdown_open=True):
+ shutdown_open=True, node_broken=False):
self.timer = testutil.MockTimer()
self.shutdowns = testutil.MockShutdownTimer()
self.shutdowns._set_state(shutdown_open, 300)
self.cloud_client = mock.MagicMock(name='cloud_client')
+ self.cloud_client.broken.return_value = node_broken
self.arvados_client = mock.MagicMock(name='arvados_client')
self.updates = mock.MagicMock(name='update_mock')
if cloud_node is None:
@@ -201,6 +202,8 @@ class ComputeNodeShutdownActorTestCase(ComputeNodeShutdownActorMixin,
self.make_actor()
self.check_success_flag(False, 2)
self.assertFalse(self.cloud_client.destroy_node.called)
+ self.assertEqual(self.ACTOR_CLASS.WINDOW_CLOSED,
+ self.shutdown_actor.cancel_reason.get(self.TIMEOUT))
def test_shutdown_retries_when_cloud_fails(self):
self.make_mocks()
@@ -210,6 +213,15 @@ class ComputeNodeShutdownActorTestCase(ComputeNodeShutdownActorMixin,
self.cloud_client.destroy_node.return_value = True
self.check_success_flag(True)
+ def test_shutdown_cancelled_when_cloud_fails_on_broken_node(self):
+ self.make_mocks(node_broken=True)
+ self.cloud_client.destroy_node.return_value = False
+ self.make_actor(start_time=0)
+ self.check_success_flag(False, 2)
+ self.assertEqual(1, self.cloud_client.destroy_node.call_count)
+ self.assertEqual(self.ACTOR_CLASS.NODE_BROKEN,
+ self.shutdown_actor.cancel_reason.get(self.TIMEOUT))
+
def test_late_subscribe(self):
self.make_actor()
subscriber = mock.Mock(name='subscriber_mock')
diff --git a/services/nodemanager/tests/test_daemon.py b/services/nodemanager/tests/test_daemon.py
index 16f5604..bbfbe4b 100644
--- a/services/nodemanager/tests/test_daemon.py
+++ b/services/nodemanager/tests/test_daemon.py
@@ -449,6 +449,26 @@ class NodeManagerDaemonActorTestCase(testutil.ActorTestMixin,
self.daemon.node_can_shutdown(monitor).get(self.TIMEOUT)
self.assertTrue(shutdown_proxy.called)
+ def test_broken_node_blackholed_after_cancelled_shutdown(self):
+ cloud_node = testutil.cloud_node_mock(8)
+ wishlist = [testutil.MockSize(8)]
+ self.make_daemon([cloud_node], [testutil.arvados_node_mock(8)],
+ wishlist)
+ self.assertEqual(1, self.alive_monitor_count())
+ self.assertFalse(self.node_setup.start.called)
+ monitor = self.monitor_list()[0].proxy()
+ shutdown_proxy = self.node_shutdown.start().proxy
+ shutdown_proxy().cloud_node.get.return_value = cloud_node
+ shutdown_proxy().success.get.return_value = False
+ shutdown_proxy().cancel_reason.get.return_value = self.node_shutdown.NODE_BROKEN
+ self.daemon.update_server_wishlist([]).get(self.TIMEOUT)
+ self.daemon.node_can_shutdown(monitor).get(self.TIMEOUT)
+ self.daemon.node_finished_shutdown(shutdown_proxy()).get(self.TIMEOUT)
+ self.daemon.update_cloud_nodes([cloud_node]).get(self.TIMEOUT)
+ self.daemon.update_server_wishlist(wishlist).get(self.TIMEOUT)
+ self.stop_proxy(self.daemon)
+ self.assertEqual(1, self.node_setup.start.call_count)
+
def test_nodes_shutting_down_replaced_below_max_nodes(self):
cloud_node = testutil.cloud_node_mock(6)
self.make_daemon([cloud_node], [testutil.arvados_node_mock(6)])
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list