Check cubby targets before executing jobs

common/src/jobs.rs

@@ -4,7 +4,7 @@
 
 //! Signed job requests.
 
-use std::collections::{BTreeMap, BTreeSet, HashMap};
+use std::collections::{BTreeMap, HashMap};
 use std::fmt;
 use std::io::Error as IoError;
 use std::ops::Deref;
@@ -421,15 +421,6 @@ impl JobStartRequest {
         }
     }
 
-    /// Whether this job runs on `baseboard`. The signed target must match
-    /// `sleds`, so the sled that accepted the job cannot widen it.
-    /// Interactive jobs run only on their single named baseboard.
-    pub fn runs_on(&self, baseboard: &BaseboardId, sleds: &BTreeSet<BaseboardId>) -> bool {
-        sleds.contains(baseboard)
-            && self.target.matches(sleds)
-            && (!self.mode.is_interactive() || self.target.single_baseboard() == Some(baseboard))
-    }
-
     pub fn job_id(&self) -> &JobId {
         &self.job_id
     }
@@ -449,6 +440,10 @@ impl JobStartRequest {
     pub fn target(&self) -> &Target {
         &self.target
     }
+
+    pub fn target_matches_mode(&self) -> bool {
+        self.target.is_single() || !self.is_interactive()
+    }
 }
 
 impl ToBeSigned for JobStartRequest {
@@ -524,13 +519,17 @@ pub enum JobStatus {
 pub enum SkipReason {
     SessionSuperseded,
     SessionEnded,
+    NotTargeted,
+    CubbyUnknown,
 }
 
 impl fmt::Display for SkipReason {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         f.write_str(match self {
             Self::SessionSuperseded => "another session has superseded this one",
             Self::SessionEnded => "the session ended before the job could start",
+            Self::NotTargeted => "the job does not target this sled",
+            Self::CubbyUnknown => "this sled does not know its cubby yet",
         })
     }
 }
@@ -833,7 +832,6 @@ impl FromStr for JobOutputStream {
 mod test {
     use super::*;
     use crate::keys::{EccR, EccS, EncodedSignature};
-    use crate::targets::SledId;
 
     /// A sequence number encodes to four words, parses back liberally,
     /// and orders numerically.
@@ -1001,52 +999,6 @@ mod test {
         assert_eq!(server.next_job_id(), next);
     }
 
-    /// Interactive jobs run only on their single named baseboard.
-    #[test]
-    fn interactive_targets() {
-        let sled = |serial: &str| BaseboardId {
-            part_number: "913".to_string(),
-            serial_number: serial.to_string(),
-        };
-        let me = sled("me");
-        let sleds = BTreeSet::from([me.clone()]);
-        let job = |mode, target| {
-            JobStartRequest::new(JobId::random(), SessionId::random(), "true", mode, target)
-        };
-        use JobMode::*;
-        let just_me = Target::Sleds(vec![SledId::Baseboard(me.clone())]);
-        assert!(job(Interactive, just_me.clone()).runs_on(&me, &sleds));
-        assert!(job(Batch, just_me).runs_on(&me, &sleds));
-        assert!(!job(Interactive, Target::All).runs_on(&me, &sleds));
-        assert!(job(Batch, Target::All).runs_on(&me, &sleds));
-        let my_cubby = Target::Sleds(vec![SledId::Cubby(14)]);
-        assert!(!job(Interactive, my_cubby.clone()).runs_on(&me, &sleds));
-        assert!(job(Batch, my_cubby).runs_on(&me, &sleds));
-    }
-
-    /// A job runs only on the sleds its target resolved to, and only
-    /// if the signed target matches them.
-    #[test]
-    fn resolved_targets() {
-        let sled = |serial: &str| BaseboardId {
-            part_number: "913".to_string(),
-            serial_number: serial.to_string(),
-        };
-        let (me, you) = (sled("me"), sled("you"));
-        let job = |target| {
-            JobStartRequest::new(
-                JobId::random(),
-                SessionId::random(),
-                "true",
-                JobMode::Batch,
-                target,
-            )
-        };
-        assert!(!job(Target::All).runs_on(&me, &BTreeSet::from([you.clone()])));
-        let just_you = Target::from(you.clone());
-        assert!(!job(just_you).runs_on(&me, &BTreeSet::from([me.clone(), you])));
-    }
-
     /// Requested limits may narrow the default ceiling, never widen it.
     #[test]
     fn limits_clamp() {

common/src/targets.rs

@@ -7,8 +7,8 @@
 //! One grammar serves signed job requests, the client, and the proxy:
 //! `*` means every sled in the rack, and a comma-separated list names
 //! sleds by baseboard ID or cubby number. The sled that accepts a job
-//! resolves its target once, to the baseboards it can reach, and every
-//! sled runs the job against that resolution.
+//! resolves its target once, to the baseboards it can reach. Sleds that
+//! receive the job only run it if the signed target includes them.
 
 use std::collections::{BTreeMap, BTreeSet};
 use std::fmt;
@@ -104,22 +104,14 @@ impl Target {
         }
     }
 
-    /// Whether every sled in `sleds` is one this target names. Each cubby
-    /// matches any one sled.
-    pub fn matches(&self, sleds: &BTreeSet<BaseboardId>) -> bool {
+    /// Whether this target includes `baseboard` or `cubby`.
+    pub fn matches(&self, baseboard: &BaseboardId, cubby: Option<u8>) -> bool {
         match self {
             Self::All => true,
-            Self::Sleds(named) => {
-                let mut baseboards = BTreeSet::new();
-                let mut cubbies = BTreeSet::new();
-                for sled in named {
-                    match sled {
-                        SledId::Baseboard(baseboard) => baseboards.insert(baseboard.clone()),
-                        SledId::Cubby(cubby) => cubbies.insert(*cubby),
-                    };
-                }
-                sleds.difference(&baseboards).count() <= cubbies.len()
-            }
+            Self::Sleds(sleds) => sleds.iter().any(|sled| match sled {
+                SledId::Baseboard(listed) => listed == baseboard,
+                SledId::Cubby(listed) => Some(*listed) == cubby,
+            }),
         }
     }
 
@@ -311,21 +303,15 @@ mod test {
     #[test]
     fn targets_match() {
         let brm31 = baseboard("913-0000019:BRM42220031");
-        let brm40 = baseboard("913-0000019:BRM42220040");
-        let both = BTreeSet::from([brm31.clone(), brm40.clone()]);
-        let matches = |target: &str, sleds: &BTreeSet<BaseboardId>| {
-            target.parse::<Target>().unwrap().matches(sleds)
-        };
-
-        assert!(matches("*", &both));
-        assert!(matches(
-            "913-0000019:BRM42220031",
-            &BTreeSet::from([brm31.clone()])
-        ));
-        assert!(matches("913-0000019:BRM42220031", &BTreeSet::new()));
-        assert!(!matches("913-0000019:BRM42220031", &both));
-        assert!(matches("14", &BTreeSet::from([brm40.clone()])));
-        assert!(!matches("14", &both));
-        assert!(matches("913-0000019:BRM42220031,14", &both));
+        let matches =
+            |target: &str, cubby| target.parse::<Target>().unwrap().matches(&brm31, cubby);
+
+        assert!(matches("*", None));
+        assert!(matches("913-0000019:BRM42220031", None));
+        assert!(!matches("913-0000019:BRM42220040", Some(14)));
+        assert!(matches("913-0000019:BRM42220040,14", Some(14)));
+        assert!(matches("14", Some(14)));
+        assert!(!matches("14", Some(15)));
+        assert!(!matches("14", None));
     }
 }

server/src/manager.rs

@@ -420,30 +420,26 @@ impl JobManager {
         job: SignedJob,
         params: JobStartParams,
     ) -> Result<BTreeSet<BaseboardId>, JobError> {
-        // A forged job must not take a signed job's place in the chain.
+        // Check the job signature.
         self.state
             .borrow()
             .certs()
             .verify(&job)
             .map_err(JobError::UnverifiedJob)?;
-        self.session_check(job.payload().session_id())?;
 
-        // A broader target would orphan jobs on unattached sleds.
-        let payload = job.payload();
-        if payload.is_interactive() && payload.target().single_baseboard().is_none() {
-            return Err(JobError::InteractiveTarget);
-        }
-
-        // Reject job IDs we already know about, rather than silently
-        // queuing a resubmission that can never advance the session's
-        // job chain. Without this, a caller that resubmits an
-        // already-completed job ID and waits would see the *original*
-        // job's status returned as if it belonged to this submission.
+        // Reject jobs we already know about.
         let job_id = job.job_id().to_owned();
         if self.state.borrow().get_job_status(&job_id).is_some() {
             return Err(JobError::DuplicateJobId(job_id));
         }
 
+        // Check the session and target.
+        let payload = job.payload();
+        self.session_check(payload.session_id())?;
+        if !payload.target_matches_mode() {
+            return Err(JobError::InteractiveTarget);
+        }
+
         // Submit the job for execution.
         let sleds = self.reachable_targets(payload.target())?;
         let wait = params.wait.to_owned();

server/src/messages.rs

@@ -331,6 +331,8 @@ mod wire_format {
                 Event::Job(JobEvent::Skipped(_, _, reason)) => match reason {
                     SkipReason::SessionSuperseded => "job-skipped-event",
                     SkipReason::SessionEnded => "session-ended-skip-event",
+                    SkipReason::NotTargeted => "not-targeted-skip-event",
+                    SkipReason::CubbyUnknown => "cubby-unknown-skip-event",
                 },
             },
         }
@@ -477,13 +479,29 @@ mod wire_format {
                 )),
             ),
             Message::Event(
-                baseboard,
+                baseboard.clone(),
                 Event::Job(JobEvent::Skipped(
                     skip_job,
                     skip_when,
                     SkipReason::SessionEnded,
                 )),
             ),
+            Message::Event(
+                baseboard.clone(),
+                Event::Job(JobEvent::Skipped(
+                    skip_job,
+                    skip_when,
+                    SkipReason::NotTargeted,
+                )),
+            ),
+            Message::Event(
+                baseboard,
+                Event::Job(JobEvent::Skipped(
+                    skip_job,
+                    skip_when,
+                    SkipReason::CubbyUnknown,
+                )),
+            ),
         ]
     }
 

server/src/rack.rs

@@ -8,7 +8,7 @@ use std::collections::BTreeSet;
 
 use sled_hardware_types::BaseboardId;
 
-use sush_common::jobs::SignedJob;
+use sush_common::jobs::{JobStartRequest, SkipReason};
 use sush_common::targets::Cubbies;
 use sush_common::version::{VersionInfo, VersionMap};
 
@@ -17,6 +17,11 @@ pub struct Rack {
     /// The ID of the baseboard (sled) this server is running on.
     own_baseboard: BaseboardId,
 
+    /// The cubby this sled occupies, once MGS has reported it.
+    /// Does not change once known, since a sled cannot change
+    /// cubbies without rebooting.
+    own_cubby: Option<u8>,
+
     /// Sled baseboards by cubby number, as reported by MGS.
     cubbies: Cubbies,
 
@@ -32,13 +37,24 @@ impl Rack {
         Self {
             versions: [(own_baseboard.clone(), VersionInfo::current())].into(),
             own_baseboard,
+            own_cubby: None,
             cubbies: Default::default(),
             unknown_versions: Default::default(),
         }
     }
 
-    pub fn runs_here(&self, job: &SignedJob, sleds: &BTreeSet<BaseboardId>) -> bool {
-        job.runs_on(&self.own_baseboard, sleds)
+    /// Why `job` should not run on this sled, or `None` if it should.
+    /// The sled that accepted the job resolved its target to this sled,
+    /// but that resolution is unsigned so we must check it ourselves.
+    pub fn check_target(&self, job: &JobStartRequest) -> Option<SkipReason> {
+        let target = job.target();
+        if job.target_matches_mode() && target.matches(&self.own_baseboard, self.own_cubby) {
+            None
+        } else if self.own_cubby.is_none() {
+            Some(SkipReason::CubbyUnknown)
+        } else {
+            Some(SkipReason::NotTargeted)
+        }
     }
 
     pub fn own_baseboard(&self) -> &BaseboardId {
@@ -50,6 +66,12 @@ impl Rack {
     }
 
     pub fn set_cubbies(&mut self, cubbies: Cubbies) {
+        if let Some((&cubby, _)) = cubbies
+            .iter()
+            .find(|(_, baseboard)| **baseboard == self.own_baseboard)
+        {
+            self.own_cubby = Some(cubby);
+        }
         self.cubbies = cubbies;
     }
 
@@ -72,3 +94,58 @@ impl Rack {
         self.unknown_versions.insert(version)
     }
 }
+
+#[cfg(test)]
+mod test {
+    use sush_common::jobs::{JobId, JobMode, SessionId};
+    use sush_common::targets::Target;
+
+    use super::*;
+
+    #[test]
+    fn check_target() {
+        let me: BaseboardId = "913-0000019:BRM42220031".parse().unwrap();
+        let mut rack = Rack::new(me.clone());
+        let check_target = |rack: &Rack, mode, target: &str| {
+            rack.check_target(&JobStartRequest::new(
+                JobId::random(),
+                SessionId::random(),
+                "true",
+                mode,
+                target.parse::<Target>().unwrap(),
+            ))
+        };
+
+        use JobMode::*;
+        use SkipReason::*;
+
+        // Before this sled knows its cubby.
+        assert_eq!(check_target(&rack, Batch, "*"), None);
+        assert_eq!(check_target(&rack, Batch, "913-0000019:BRM42220031"), None);
+        assert_eq!(check_target(&rack, Batch, "14"), Some(CubbyUnknown));
+
+        // Once a cubby map places this sled in cubby 14, the sled keeps
+        // that cubby, even after a later map that omits it.
+        rack.set_cubbies(Cubbies::from([(14, me.clone())]));
+        rack.set_cubbies(Cubbies::new());
+        assert_eq!(check_target(&rack, Batch, "14"), None);
+        assert_eq!(check_target(&rack, Batch, "15"), Some(NotTargeted));
+        assert_eq!(
+            check_target(&rack, Batch, "913-0000019:BRM42220040"),
+            Some(NotTargeted)
+        );
+
+        // Interactive jobs run only on the single sled they target.
+        assert_eq!(
+            check_target(&rack, Interactive, "913-0000019:BRM42220031"),
+            None
+        );
+        assert_eq!(check_target(&rack, Interactive, "14"), None);
+        assert_eq!(check_target(&rack, Interactive, "15"), Some(NotTargeted));
+        assert_eq!(check_target(&rack, Interactive, "*"), Some(NotTargeted));
+        assert_eq!(
+            check_target(&rack, Interactive, "913-0000019:BRM42220031,14"),
+            Some(NotTargeted)
+        );
+    }
+}