Remove node finalizer on node cleanup

Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>

controller/src/util/node_watcher.rs

@@ -103,29 +103,27 @@ async fn reconcile_inner(event: Event<Node>, ctx: Arc<ControllerContext>) -> Res
  } else {
  // Node Modified
  drop(guard);
- call_handle_node_disappearance_if_needed(&node, ctx.clone()).await?;
+ handle_node_disappearance(&node, ctx.clone()).await?;
  }
  }
  Ok(Action::await_change())
  }
  Event::Cleanup(node) => {
  info!("handle_node - Deleted: {:?}", &node.name_unchecked());
- call_handle_node_disappearance_if_needed(&node, ctx.clone()).await?;
+ handle_node_disappearance(&node, ctx.clone()).await?;
  ctx.known_nodes.write().await.remove(&node.name_unchecked());
  Ok(Action::await_change())
  }
  }
 }
+
 /// This should be called for Nodes that are either !Ready or Deleted.
-/// This function ensures that handle_node_disappearance is called
-/// only once for any Node as it disappears.
-async fn call_handle_node_disappearance_if_needed(
- node: &Node,
- ctx: Arc<ControllerContext>,
-) -> anyhow::Result<()> {
+/// This function will clean up any Instances that reference a Node that
+/// was previously Running.
+async fn handle_node_disappearance(node: &Node, ctx: Arc<ControllerContext>) -> anyhow::Result<()> {
  let node_name = node.name_unchecked();
  trace!(
- "call_handle_node_disappearance_if_needed - enter: {:?}",
+ "handle_node_disappearance - enter: {:?}",
  &node.metadata.name
  );
  let last_known_state = ctx
@@ -136,20 +134,25 @@ async fn call_handle_node_disappearance_if_needed(
  .unwrap_or(&NodeState::Running)
  .clone();
  trace!(
- "call_handle_node_disappearance_if_needed - last_known_state: {:?}",
+ "handle_node_disappearance - last_known_state: {:?}",
  &last_known_state
  );
- // Nodes are updated roughly once a minute ... try to only call
- // handle_node_disappearance once for a node that disappears.
- //
- // Also, there is no need to call handle_node_disappearance if a
- // Node has never been in the Running state.
+
+ // If the node was running and no longer is, clear the node from
+ // each instance's nodes list and deviceUsage map.
  if last_known_state == NodeState::Running {
+ let api = ctx.client.all();
+ let instances: ObjectList<Instance> = api.list(&ListParams::default()).await?;
  trace!(
- "call_handle_node_disappearance_if_needed - call handle_node_disappearance: {:?}",
- &node.metadata.name
+ "handle_node_disappearance - found {:?} instances",
+ instances.items.len()
  );
- handle_node_disappearance(&node_name, ctx.clone()).await?;
+ for instance in instances.items {
+ let instance_name = instance.name_unchecked();
+ try_remove_nodes_from_instance(&node_name, &instance_name, &instance, api.as_ref())
+ .await?;
+ api.remove_finalizer(&instance, &node_name).await?;
+ }
  ctx.known_nodes
  .write()
  .await
@@ -174,37 +177,6 @@ fn is_node_ready(k8s_node: &Node) -> bool {
  })
 }
 
-/// This handles when a node disappears by clearing nodes from
-/// the nodes list and deviceUsage map.
-async fn handle_node_disappearance(
- vanished_node_name: &str,
- ctx: Arc<ControllerContext>,
-) -> anyhow::Result<()> {
- trace!(
- "handle_node_disappearance - enter vanished_node_name={:?}",
- vanished_node_name,
- );
- let api = ctx.client.all();
- let instances: ObjectList<Instance> = api.list(&ListParams::default()).await?;
- trace!(
- "handle_node_disappearance - found {:?} instances",
- instances.items.len()
- );
- for instance in instances.items {
- let instance_name = instance.name_unchecked();
-
- trace!(
- "handle_node_disappearance - make sure node is not referenced here: {:?}",
- &instance_name
- );
- try_remove_nodes_from_instance(vanished_node_name, &instance_name, &instance, api.as_ref())
- .await?;
- }
-
- trace!("handle_node_disappearance - exit");
- Ok(())
-}
-
 /// This attempts to remove nodes from the nodes list and deviceUsage
 /// map in an Instance. An attempt is made to update
 /// the instance in etcd, any failure is returned.
@@ -401,6 +373,9 @@ mod tests {
  }
  _ => false,
  });
+ instance_api_mock
+ .expect_remove_finalizer()
+ .returning(|_, _| Ok(()));
  mock.instance
  .expect_all()
  .return_once(move || Box::new(instance_api_mock));
@@ -445,6 +420,9 @@ mod tests {
  }
  _ => false,
  });
+ instance_api_mock
+ .expect_remove_finalizer()
+ .returning(|_, _| Ok(()));
  mock.instance
  .expect_all()
  .return_once(move || Box::new(instance_api_mock));
@@ -485,6 +463,9 @@ mod tests {
  }
  _ => false,
  });
+ instance_api_mock
+ .expect_remove_finalizer()
+ .returning(|_, _| Ok(()));
  mock.instance
  .expect_all()
  .return_once(move || Box::new(instance_api_mock));