18290: address review comments.

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <ward@curii.com>

diff --git a/lib/config/config.default.yml b/lib/config/config.default.yml
index 8b51a85d9adf1a49eb7e93bbd7efdf316a87f743..52e35d83ff76296ee9d5f14202492e02e955375d 100644 (file)
--- a/lib/config/config.default.yml
+++ b/lib/config/config.default.yml
@@ -1026,7 +1026,7 @@ Clusters:
         # Template variables starting with % will be substituted as follows:
         #
         # %U uuid
-        # %C number of cpus
+        # %C number of VCPUs
         # %M memory in MB
         # %T tmp in MB
         #

diff --git a/lib/config/generated_config.go b/lib/config/generated_config.go
index 519d1a8e5f555627fab3b01aeca39dde40cadbb7..c58bbe1784ea48a5468613666ce4b834f143366b 100644 (file)
--- a/lib/config/generated_config.go
+++ b/lib/config/generated_config.go
@@ -1032,7 +1032,7 @@ Clusters:
         # Template variables starting with % will be substituted as follows:
         #
         # %U uuid
-        # %C number of cpus
+        # %C number of VCPUs
         # %M memory in MB
         # %T tmp in MB
         #

diff --git a/lib/lsf/dispatch.go b/lib/lsf/dispatch.go
index d17c458e8005b8837492caf2e6c2ba8ea158d502..6e35b7de929f8843bdd7bdb848698ca96c62a123 100644 (file)
--- a/lib/lsf/dispatch.go
+++ b/lib/lsf/dispatch.go
@@ -270,9 +270,7 @@ func (disp *dispatcher) bkill(ctr arvados.Container) {
 }
 
 func (disp *dispatcher) bsubArgs(container arvados.Container) ([]string, error) {
-       tmpArgs := []string{}
        args := []string{"bsub"}
-       tmpArgs = append(tmpArgs, disp.Cluster.Containers.LSF.BsubArgumentsList...)
 
        tmp := int64(math.Ceil(float64(dispatchcloud.EstimateScratchSpace(&container)) / 1048576))
        vcpus := container.RuntimeConstraints.VCPUs
@@ -280,16 +278,27 @@ func (disp *dispatcher) bsubArgs(container arvados.Container) ([]string, error)
                container.RuntimeConstraints.KeepCacheRAM+
                int64(disp.Cluster.Containers.ReserveExtraRAM)) / 1048576))
 
-       r := regexp.MustCompile(`([^%]|^)%([^%])`)
-       undoubleRE := regexp.MustCompile(`%%`)
-       for _, a := range tmpArgs {
-               tmp := r.ReplaceAllStringFunc(a, func(m string) string {
-                       parts := r.FindStringSubmatch(m)
-                       return parts[1] + disp.substitute(parts[2], container.UUID, vcpus, mem, tmp)
-               })
-               // handle escaped literal % symbols
-               tmp = undoubleRE.ReplaceAllString(tmp, "%")
-               args = append(args, tmp)
+       repl := map[string]string{
+               "%%": "%",
+               "%C": fmt.Sprintf("%d", vcpus),
+               "%M": fmt.Sprintf("%d", mem),
+               "%T": fmt.Sprintf("%d", tmp),
+               "%U": container.UUID,
+       }
+
+       re := regexp.MustCompile(`%.`)
+       var substitutionErrors string
+       for _, a := range disp.Cluster.Containers.LSF.BsubArgumentsList {
+               args = append(args, re.ReplaceAllStringFunc(a, func(s string) string {
+                       subst := repl[s]
+                       if len(subst) == 0 {
+                               substitutionErrors += fmt.Sprintf("Unknown substitution parameter %s in BsubArgumentsList, ", s)
+                       }
+                       return subst
+               }))
+       }
+       if len(substitutionErrors) != 0 {
+               return nil, fmt.Errorf("%s", substitutionErrors[:len(substitutionErrors)-2])
        }
 
        if u := disp.Cluster.Containers.LSF.BsubSudoUser; u != "" {
@@ -298,23 +307,6 @@ func (disp *dispatcher) bsubArgs(container arvados.Container) ([]string, error)
        return args, nil
 }
 
-func (disp *dispatcher) substitute(l string, uuid string, vcpus int, mem, tmp int64) string {
-       var arg string
-       switch l {
-       case "C":
-               arg = fmt.Sprintf("%d", vcpus)
-       case "T":
-               arg = fmt.Sprintf("%d", tmp)
-       case "M":
-               arg = fmt.Sprintf("%d", mem)
-       case "U":
-               arg = uuid
-       default:
-               arg = "%" + l
-       }
-       return arg
-}
-
 // Check the next bjobs report, and invoke TrackContainer for all the
 // containers in the report. This gives us a chance to cancel existing
 // Arvados LSF jobs (started by a previous dispatch process) that