Merge branch '18638-noisy-errors-regarding-tests'
authorJavier Bértoli <jbertoli@curii.com>
Thu, 24 Mar 2022 18:44:43 +0000 (15:44 -0300)
committerJavier Bértoli <jbertoli@curii.com>
Thu, 24 Mar 2022 18:44:43 +0000 (15:44 -0300)
closes #18638
Arvados-DCO-1.1-Signed-off-by: Javier Bértoli <jbertoli@curii.com>

12 files changed:
doc/_includes/_container_scheduling_parameters.liquid
doc/admin/spot-instances.html.textile.liquid
lib/config/config.default.yml
lib/config/export.go
lib/config/load.go
lib/config/load_test.go
sdk/cwl/test_with_arvbox.sh
sdk/go/arvados/config.go
services/api/test/unit/container_request_test.rb
tools/salt-install/config_examples/multi_host/aws/states/shell_sudo_passwordless.sls [new file with mode: 0644]
tools/salt-install/config_examples/single_host/single_hostname/states/shell_sudo_passwordless.sls [new file with mode: 0644]
tools/salt-install/provision.sh

index be046173ad028b92fd9b26a4a2994f5b4c9fd295..636b6df59c455d5badac1bda9ded03434403207d 100644 (file)
@@ -11,5 +11,5 @@ Parameters to be passed to the container scheduler (e.g., Slurm) when running a
 table(table table-bordered table-condensed).
 |_. Key|_. Type|_. Description|_. Notes|
 |partitions|array of strings|The names of one or more compute partitions that may run this container. If not provided, the system will choose where to run the container.|Optional.|
-|preemptible|boolean|If true, the dispatcher will ask for a preemptible cloud node instance (eg: AWS Spot Instance) to run this container.|Optional. Default is false.|
+|preemptible|boolean|If true, the dispatcher should use a preemptible cloud node instance (eg: AWS Spot Instance) to run this container.  Whether a preemptible instance is actually used "depends on cluster configuration.":{{site.baseurl}}/admin/spot-instances.html|Optional. Default is false.|
 |max_run_time|integer|Maximum running time (in seconds) that this container will be allowed to run before being cancelled.|Optional. Default is 0 (no limit).|
index 3837f30d6da99519b8ba9017e28267a1f0e8742d..703e70fb8636afe4d99b2b356bc4a4326d0d46e7 100644 (file)
@@ -16,31 +16,48 @@ Currently Arvados supports preemptible instances using AWS and Azure spot instan
 
 h2. Configuration
 
-Add entries to @InstanceTypes@ that have @Preemptible: true@.  Typically you want to add both preemptible and non-preemptible entries for each cloud provider VM type.  The @Price@ for preemptible instances is the maximum bid price, the actual price paid is dynamic and will likely be lower.  For example:
+First, configure some @InstanceTypes@ that have @Preemptible: true@. For a preemptible instance, @Price@ determines the maximum bid price; the actual price paid is dynamic and will likely be lower.
+
+Typically you want to add both preemptible and non-preemptible entries for each cloud provider VM type. To do this automatically, use @PreemptiblePriceFactor@ to enable a preemptible version of each listed type, using the given factor to set the maximum bid price relative to the non-preemptible price. Alternatively, you can configure preemptible instance types explicitly. For example, the following two configurations are equivalent:
 
 <pre>
 Clusters:
   ClusterID:
+    Containers:
+      PreemptiblePriceFactor: 0.8
     InstanceTypes:
       m4.large:
-        Preemptible: false
         ProviderType: m4.large
         VCPUs: 2
         RAM: 8GiB
         AddedScratch: 32GB
         Price: 0.1
-      m4.large.spot:
-        Preemptible: true
+</pre>
+
+<pre>
+Clusters:
+  ClusterID:
+    InstanceTypes:
+      m4.large:
         ProviderType: m4.large
         VCPUs: 2
         RAM: 8GiB
         AddedScratch: 32GB
         Price: 0.1
+      m4.large.preemptible:
+        Preemptible: true
+        ProviderType: m4.large
+        VCPUs: 2
+        RAM: 8GiB
+        AddedScratch: 32GB
+        Price: 0.08
 </pre>
 
 Next, you can choose to enable automatic use of preemptible instances:
 
 <pre>
+Clusters:
+  ClusterID:
     Containers:
       AlwaysUsePreemptibleInstances: true
 </pre>
index 8bbc33ba08493b2147148e1fcdb8538a4a991c52..6512389815dd4e3d1d786b09cf1cd5b3eedd929d 100644 (file)
@@ -917,7 +917,16 @@ Clusters:
       #
       # This flag is ignored if no preemptible instance types are
       # configured, and has no effect on top-level containers.
-      AlwaysUsePreemptibleInstances: true
+      AlwaysUsePreemptibleInstances: false
+
+      # Automatically add a preemptible variant for every
+      # non-preemptible entry in InstanceTypes below. The maximum bid
+      # price for the preemptible variant will be the non-preemptible
+      # price multiplied by PreemptiblePriceFactor. If 0, preemptible
+      # variants are not added automatically.
+      #
+      # A price factor of 1.0 is a reasonable starting point.
+      PreemptiblePriceFactor: 0
 
       # PEM encoded SSH key (RSA, DSA, or ECDSA) used by the
       # cloud dispatcher for executing containers on worker VMs.
index db413b97bd3460a83afaddfc5276b027a6486a01..dae749c87470ecb5f4ab7c496ddf001c11abcdf3 100644 (file)
@@ -133,6 +133,7 @@ var whitelist = map[string]bool{
        "Containers.MaxDispatchAttempts":           false,
        "Containers.MaxRetryAttempts":              true,
        "Containers.MinRetryPeriod":                true,
+       "Containers.PreemptiblePriceFactor":        false,
        "Containers.ReserveExtraRAM":               true,
        "Containers.RuntimeEngine":                 true,
        "Containers.ShellAccess":                   true,
index 8d498af170f2180881fac496c82900b1bd764d7f..de43b9d2e2749b56ef37a100f57e57cd071d59e0 100644 (file)
@@ -285,6 +285,19 @@ func (ldr *Loader) Load() (*arvados.Config, error) {
                }
        }
 
+       // Preprocess/automate some configs
+       for id, cc := range cfg.Clusters {
+               ldr.autofillPreemptible("Clusters."+id, &cc)
+
+               if strings.Count(cc.Users.AnonymousUserToken, "/") == 3 {
+                       // V2 token, strip it to just a secret
+                       tmp := strings.Split(cc.Users.AnonymousUserToken, "/")
+                       cc.Users.AnonymousUserToken = tmp[2]
+               }
+
+               cfg.Clusters[id] = cc
+       }
+
        // Check for known mistakes
        for id, cc := range cfg.Clusters {
                for remote := range cc.RemoteClusters {
@@ -316,11 +329,6 @@ func (ldr *Loader) Load() (*arvados.Config, error) {
                                return nil, err
                        }
                }
-               if strings.Count(cc.Users.AnonymousUserToken, "/") == 3 {
-                       // V2 token, strip it to just a secret
-                       tmp := strings.Split(cc.Users.AnonymousUserToken, "/")
-                       cc.Users.AnonymousUserToken = tmp[2]
-               }
        }
        return &cfg, nil
 }
@@ -527,3 +535,21 @@ func (ldr *Loader) logExtraKeys(expected, supplied map[string]interface{}, prefi
                }
        }
 }
+
+func (ldr *Loader) autofillPreemptible(label string, cc *arvados.Cluster) {
+       if factor := cc.Containers.PreemptiblePriceFactor; factor > 0 {
+               for name, it := range cc.InstanceTypes {
+                       if !it.Preemptible {
+                               it.Preemptible = true
+                               it.Price = it.Price * factor
+                               it.Name = name + ".preemptible"
+                               if it2, exists := cc.InstanceTypes[it.Name]; exists && it2 != it {
+                                       ldr.Logger.Warnf("%s.InstanceTypes[%s]: already exists, so not automatically adding a preemptible variant of %s", label, it.Name, name)
+                                       continue
+                               }
+                               cc.InstanceTypes[it.Name] = it
+                       }
+               }
+       }
+
+}
index 1ede805b0085a668b40169af458243a304c12e93..5270dcccce8b9d95b5b5fdb1e6fe9ad15f2e0426 100644 (file)
@@ -305,8 +305,6 @@ func (s *LoadSuite) TestNoUnrecognizedKeysInDefaultConfig(c *check.C) {
 
 func (s *LoadSuite) TestNoWarningsForDumpedConfig(c *check.C) {
        var logbuf bytes.Buffer
-       logger := logrus.New()
-       logger.Out = &logbuf
        cfg, err := testLoader(c, `
 Clusters:
  zzzzz:
@@ -695,3 +693,72 @@ Clusters:
        _, err = ldr.Load()
        c.Assert(err, check.ErrorMatches, `there is no default storage class.*`)
 }
+
+func (s *LoadSuite) TestPreemptiblePriceFactor(c *check.C) {
+       yaml := `
+Clusters:
+ z1111:
+  InstanceTypes:
+   Type1:
+    RAM: 12345M
+    VCPUs: 8
+    Price: 1.23
+ z2222:
+  Containers:
+   PreemptiblePriceFactor: 0.5
+  InstanceTypes:
+   Type1:
+    RAM: 12345M
+    VCPUs: 8
+    Price: 1.23
+ z3333:
+  Containers:
+   PreemptiblePriceFactor: 0.5
+  InstanceTypes:
+   Type1:
+    RAM: 12345M
+    VCPUs: 8
+    Price: 1.23
+   Type1.preemptible: # higher price than the auto-added variant would use -- should generate warning
+    ProviderType: Type1
+    RAM: 12345M
+    VCPUs: 8
+    Price: 1.23
+    Preemptible: true
+   Type2:
+    RAM: 23456M
+    VCPUs: 16
+    Price: 2.46
+   Type2.preemptible: # identical to the auto-added variant -- so no warning
+    ProviderType: Type2
+    RAM: 23456M
+    VCPUs: 16
+    Price: 1.23
+    Preemptible: true
+`
+       var logbuf bytes.Buffer
+       cfg, err := testLoader(c, yaml, &logbuf).Load()
+       c.Assert(err, check.IsNil)
+       cc, err := cfg.GetCluster("z1111")
+       c.Assert(err, check.IsNil)
+       c.Check(cc.InstanceTypes["Type1"].Price, check.Equals, 1.23)
+       c.Check(cc.InstanceTypes, check.HasLen, 1)
+
+       cc, err = cfg.GetCluster("z2222")
+       c.Assert(err, check.IsNil)
+       c.Check(cc.InstanceTypes["Type1"].Preemptible, check.Equals, false)
+       c.Check(cc.InstanceTypes["Type1"].Price, check.Equals, 1.23)
+       c.Check(cc.InstanceTypes["Type1.preemptible"].Preemptible, check.Equals, true)
+       c.Check(cc.InstanceTypes["Type1.preemptible"].Price, check.Equals, 1.23/2)
+       c.Check(cc.InstanceTypes["Type1.preemptible"].ProviderType, check.Equals, "Type1")
+       c.Check(cc.InstanceTypes, check.HasLen, 2)
+
+       cc, err = cfg.GetCluster("z3333")
+       c.Assert(err, check.IsNil)
+       // Don't overwrite the explicitly configured preemptible variant
+       c.Check(cc.InstanceTypes["Type1.preemptible"].Price, check.Equals, 1.23)
+       c.Check(cc.InstanceTypes, check.HasLen, 4)
+       c.Check(logbuf.String(), check.Matches, `(?ms).*Clusters\.z3333\.InstanceTypes\[Type1\.preemptible\]: already exists, so not automatically adding a preemptible variant of Type1.*`)
+       c.Check(logbuf.String(), check.Not(check.Matches), `(?ms).*Type2\.preemptible.*`)
+       c.Check(logbuf.String(), check.Not(check.Matches), `(?ms).*(z1111|z2222)[^\n]*InstanceTypes.*`)
+}
index e43be6a61f6730ae59f58d2b53e333f3f4809529..d38414fc811d433acb5751613c9974f1bbc0b0c5 100755 (executable)
@@ -166,7 +166,7 @@ if [[ "$suite" = "integration" ]] ; then
    cd /usr/src/arvados/sdk/cwl/tests
    exec ./arvados-tests.sh $@
 elif [[ "$suite" = "conformance-v1.2" ]] ; then
-   exec cwltest --tool arvados-cwl-runner --test conformance_tests.yaml -N307 -- \$EXTRA
+   exec cwltest --tool arvados-cwl-runner --test conformance_tests.yaml -N307 $@ -- \$EXTRA
 else
    exec ./run_test.sh RUNNER=arvados-cwl-runner EXTRA="\$EXTRA" $@
 fi
index e0750bd8c54ae0a671f282ba9438d23e0bfb48ad..6c9324e478a273b68e83dcd7fb4e46150b8e0da1 100644 (file)
@@ -448,6 +448,7 @@ type ContainersConfig struct {
        StaleLockTimeout              Duration
        SupportedDockerImageFormats   StringSet
        AlwaysUsePreemptibleInstances bool
+       PreemptiblePriceFactor        float64
        RuntimeEngine                 string
        LocalKeepBlobBuffersPerVCPU   int
        LocalKeepLogsToContainerLog   string
index 9b35769ef2f0886ab3e1595d280b0a9a133fb83e..aa649e9106c4a403c1d2745c6a2a805a87e5f205 100644 (file)
@@ -1126,7 +1126,8 @@ class ContainerRequestTest < ActiveSupport::TestCase
     end
   end
 
-  test "Having preemptible_instances=true create a committed child container request and verify the scheduling parameter of its container" do
+  test "AlwaysUsePreemptibleInstances makes child containers preemptible" do
+    Rails.configuration.Containers.AlwaysUsePreemptibleInstances = true
     common_attrs = {cwd: "test",
                     priority: 1,
                     command: ["echo", "hello"],
diff --git a/tools/salt-install/config_examples/multi_host/aws/states/shell_sudo_passwordless.sls b/tools/salt-install/config_examples/multi_host/aws/states/shell_sudo_passwordless.sls
new file mode 100644 (file)
index 0000000..dbcc9c9
--- /dev/null
@@ -0,0 +1,27 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: Apache-2.0
+
+{%- set curr_tpldir = tpldir %}
+{%- set tpldir = 'arvados' %}
+{%- from "arvados/map.jinja" import arvados with context %}
+{%- set tpldir = curr_tpldir %}
+
+extra_shell_sudo_passwordless_sudo_pkg_installed:
+  pkg.installed:
+    - name: sudo
+
+extra_shell_sudo_passwordless_config_file_managed:
+  file.managed:
+    - name: /etc/sudoers.d/arvados_passwordless
+    - makedirs: true
+    - user: root
+    - group: root
+    - mode: '0440'
+    - replace: false
+    - contents: |
+        # This file managed by Salt, do not edit by hand!!
+        # Allow members of group sudo to execute any command without password
+        %sudo ALL=(ALL:ALL) NOPASSWD:ALL
+    - require:
+      - pkg: extra_shell_sudo_passwordless_sudo_pkg_installed
diff --git a/tools/salt-install/config_examples/single_host/single_hostname/states/shell_sudo_passwordless.sls b/tools/salt-install/config_examples/single_host/single_hostname/states/shell_sudo_passwordless.sls
new file mode 100644 (file)
index 0000000..dbcc9c9
--- /dev/null
@@ -0,0 +1,27 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: Apache-2.0
+
+{%- set curr_tpldir = tpldir %}
+{%- set tpldir = 'arvados' %}
+{%- from "arvados/map.jinja" import arvados with context %}
+{%- set tpldir = curr_tpldir %}
+
+extra_shell_sudo_passwordless_sudo_pkg_installed:
+  pkg.installed:
+    - name: sudo
+
+extra_shell_sudo_passwordless_config_file_managed:
+  file.managed:
+    - name: /etc/sudoers.d/arvados_passwordless
+    - makedirs: true
+    - user: root
+    - group: root
+    - mode: '0440'
+    - replace: false
+    - contents: |
+        # This file managed by Salt, do not edit by hand!!
+        # Allow members of group sudo to execute any command without password
+        %sudo ALL=(ALL:ALL) NOPASSWD:ALL
+    - require:
+      - pkg: extra_shell_sudo_passwordless_sudo_pkg_installed
index 53b67c6cd86111eb70ac385f9548f8ab9ac943f1..a26b3feaa3e0536266e9a4d73ed85c9196fbd792 100755 (executable)
@@ -518,7 +518,7 @@ if [ -d "${F_DIR}"/extra/extra ]; then
     # Same when using self-signed certificates.
     SKIP_SNAKE_OIL="dont_add_snakeoil_certs"
   fi
-  for f in $(ls "${F_DIR}"/extra/extra/*.sls | grep -v ${SKIP_SNAKE_OIL}); do
+  for f in $(ls "${F_DIR}"/extra/extra/*.sls | egrep -v "${SKIP_SNAKE_OIL}|shell_sudo_passwordless"); do
   echo "    - extra.$(basename ${f} | sed 's/.sls$//g')" >> ${S_DIR}/top.sls
   done
   # Use byo or self-signed certificates
@@ -548,6 +548,7 @@ if [ -z "${ROLES}" ]; then
     grep -q "custom_certs"    ${S_DIR}/top.sls || echo "    - extra.custom_certs" >> ${S_DIR}/top.sls
   fi
 
+  echo "    - extra.shell_sudo_passwordless" >> ${S_DIR}/top.sls
   echo "    - postgres" >> ${S_DIR}/top.sls
   echo "    - docker.software" >> ${S_DIR}/top.sls
   echo "    - arvados" >> ${S_DIR}/top.sls
@@ -757,6 +758,7 @@ else
       ;;
       "shell")
         # States
+        echo "    - extra.shell_sudo_passwordless" >> ${S_DIR}/top.sls
         grep -q "docker" ${S_DIR}/top.sls       || echo "    - docker.software" >> ${S_DIR}/top.sls
         grep -q "arvados.${R}" ${S_DIR}/top.sls || echo "    - arvados.${R}" >> ${S_DIR}/top.sls
         # Pillars