Remove nodeset from NodeRequest

To make things simpler for schedulers to handle node provisioned
events for node requests which they may not have in their local
pipeline state, we need to make the pipeline storage of node requests
simpler.  That starts by removing the nodeset object as an attribute
of the NodeRequest object.  This means that the scheduler can work
with a node request object without relying on having the associated
nodeset.  It also simplifies the ZooKeeper code that deserializes
NodeRequests (as it doesn't have to create fake NodeSet objects too).
And finally, it simplifies what must be stored in the pipeline
and queue item structures, which will also come in handy later.

Two tests designed to verify that the request->nodeset magic
deserialization worked have been removed since they are no longer
applicable.

Change-Id: I70ae083765d5cd9a4fd1afc2442bf22d6c52ba0b

3 files changed, 15 insertions, 25 deletions

diff --git a/zuul/executor/client.py b/zuul/executor/client.py
index 3a67a6493..9c7a7a345 100644
--- a/zuul/executor/client.py
+++ b/zuul/executor/client.py
@@ -61,7 +61,7 @@ class ExecutorClient(object):
             job, uuid, nodeset, item.change, dependent_changes)
 
         params = zuul.executor.common.construct_build_params(
-            uuid, self.sched, nodeset,
+            uuid, self.sched,
             job, item, pipeline, dependent_changes, merger_items,
             redact_secrets_and_keys=False)
         # TODO: deprecate and remove this variable?
@@ -111,9 +111,10 @@ class ExecutorClient(object):
         # availability zone we can get executor_zone from only the first
         # node.
         executor_zone = None
-        if params["nodes"] and params["nodes"][0].get('attributes'):
-            executor_zone = params[
-                "nodes"][0]['attributes'].get('executor-zone')
+        if len(nodeset.nodes):
+            node = nodeset.getNodes()[0]
+            if node.attributes:
+                executor_zone = node.attributes.get('executor-zone')
 
         zone_known = False
         if executor_zone:
diff --git a/zuul/executor/common.py b/zuul/executor/common.py
index da7d2ffe5..4156f3ba0 100644
--- a/zuul/executor/common.py
+++ b/zuul/executor/common.py
@@ -17,7 +17,7 @@ import os
 from zuul.lib import strings
 
 
-def construct_build_params(uuid, sched, nodeset, job, item, pipeline,
+def construct_build_params(uuid, sched, job, item, pipeline,
                            dependent_changes=[], merger_items=[],
                            redact_secrets_and_keys=True):
     """Returns a list of all the parameters needed to build a job.
@@ -124,16 +124,7 @@ def construct_build_params(uuid, sched, nodeset, job, item, pipeline,
         params['cleanup_playbooks'] = [make_playbook(x)
                                        for x in job.cleanup_run]
 
-    # TODO(corvus): Remove nodes and groups since they're included in
-    # nodeset
-    nodes = []
-    for node in nodeset.getNodes():
-        n = node.toDict()
-        n.update(dict(name=node.name, label=node.label))
-        nodes.append(n)
-    params['nodes'] = nodes
-    params['groups'] = [group.toDict() for group in nodeset.getGroups()]
-    params["nodeset"] = nodeset.toDict()
+    params["nodeset"] = job.nodeset.toDict()
     params['ssh_keys'] = []
     if pipeline.post_review:
         if redact_secrets_and_keys:
diff --git a/zuul/executor/server.py b/zuul/executor/server.py
index 22167d665..4df32c451 100644
--- a/zuul/executor/server.py
+++ b/zuul/executor/server.py
@@ -1104,8 +1104,7 @@ class AnsibleJob(object):
         if node_request_id:
             zk_nodepool = self.executor_server.nodepool.zk_nodepool
             self.node_request = zk_nodepool.getNodeRequest(
-                self.arguments["noderequest_id"], self.nodeset
-            )
+                self.arguments["noderequest_id"])
 
             if self.node_request is None:
                 self.log.error(
@@ -1122,10 +1121,11 @@ class AnsibleJob(object):
         if self.node_request:
             self.log.debug("Locking nodeset")
             try:
-                self.executor_server.nodepool.acceptNodes(self.node_request)
+                self.executor_server.nodepool.acceptNodes(
+                    self.node_request, self.nodeset)
             except Exception:
                 self.log.exception(
-                    "Error locking nodeset %s", self.node_request.nodeset
+                    "Error locking nodeset %s", self.nodeset
                 )
                 raise NodeRequestError
 
@@ -1133,13 +1133,13 @@ class AnsibleJob(object):
         if self.node_request:
             try:
                 self.executor_server.nodepool.returnNodeSet(
-                    self.node_request.nodeset,
+                    self.nodeset,
                     self,
                     zuul_event_id=self.zuul_event_id,
                 )
             except Exception:
                 self.log.exception(
-                    "Unable to return nodeset %s", self.node_request.nodeset
+                    "Unable to return nodeset %s", self.nodeset
                 )
 
     def _base_job_data(self):
@@ -1347,9 +1347,7 @@ class AnsibleJob(object):
         # start to run tasks on nodes (prepareVars in particular uses
         # Ansible to freeze hostvars).
         if self.node_request:
-            self.executor_server.nodepool.useNodeSet(
-                self.node_request.nodeset, self
-            )
+            self.executor_server.nodepool.useNodeSet(self.nodeset, self)
 
         # This prepares each playbook and the roles needed for each.
         self.preparePlaybooks(args)
@@ -3786,7 +3784,7 @@ class ExecutorServer(BaseMergeServer):
         if request is not None:
             self.log.debug("Got autohold %s", request)
             self.nodepool.holdNodeSet(
-                ansible_job.node_request.nodeset, request, ansible_job
+                ansible_job.nodeset, request, ansible_job
             )
             return True
         return False