Merge branch '13752-index-all-filenames'
authorTom Clegg <tclegg@veritasgenetics.com>
Thu, 9 Aug 2018 13:38:32 +0000 (09:38 -0400)
committerTom Clegg <tclegg@veritasgenetics.com>
Thu, 9 Aug 2018 13:38:32 +0000 (09:38 -0400)
refs #13752

Arvados-DCO-1.1-Signed-off-by: Tom Clegg <tclegg@veritasgenetics.com>

21 files changed:
build/build.list
build/package-testing/deb-common-test-packages.sh
build/run-library.sh
sdk/cwl/arvados_cwl/fsaccess.py
sdk/cwl/setup.py
sdk/cwl/tests/13931-size-job.yml [new file with mode: 0644]
sdk/cwl/tests/13931-size.cwl [new file with mode: 0644]
sdk/cwl/tests/arvados-tests.sh
sdk/cwl/tests/arvados-tests.yml
sdk/cwl/tests/test_container.py
sdk/cwl/tests/test_job.py
sdk/cwl/tests/test_submit.py
sdk/cwl/tests/wf/expect_packed.cwl
sdk/dev-jobs.dockerfile
services/crunch-dispatch-slurm/crunch-dispatch-slurm.go
services/crunch-dispatch-slurm/crunch-dispatch-slurm_test.go
services/crunch-dispatch-slurm/squeue.go
services/keep-web/cache.go
services/keep-web/cache_test.go
services/keep-web/handler.go
services/keep-web/status_test.go

index ef6407031c41f286d47ad08e573f3020a48bf9e9..eb6ba220600e65b004c17e5bbef8de1f3030f4c2 100644 (file)
@@ -23,7 +23,7 @@ debian8,debian9,ubuntu1404,ubuntu1604,centos7|pyyaml|3.12|2|python|amd64
 debian8,debian9,ubuntu1404,ubuntu1604,centos7|rdflib|4.2.2|2|python|all
 debian8,debian9,ubuntu1404,centos7|shellescape|3.4.1|2|python|all
 debian8,debian9,ubuntu1404,ubuntu1604,centos7|mistune|0.7.3|2|python|all
-debian8,debian9,ubuntu1404,ubuntu1604,centos7|typing|3.5.3.0|2|python|all
+debian8,debian9,ubuntu1404,ubuntu1604,centos7|typing|3.6.2|2|python|all
 debian8,debian9,ubuntu1404,ubuntu1604,centos7|avro|1.8.1|2|python|all
 debian8,debian9,ubuntu1404,centos7|ruamel.ordereddict|0.4.9|2|python|amd64
 debian8,debian9,ubuntu1404,ubuntu1604,centos7|cachecontrol|0.11.7|2|python|all
@@ -40,6 +40,8 @@ centos7|python-daemon|2.1.2|1|python|all
 centos7|pbr|0.11.1|2|python|all
 centos7|pyparsing|2.1.10|2|python|all
 centos7|keepalive|0.5|2|python|all
+centos7|networkx|1.11|0|python|all
+centos7|psutil|5.0.1|0|python|all
 debian8,debian9,ubuntu1404,ubuntu1604,centos7|lockfile|0.12.2|2|python|all|--epoch 1
 debian8,debian9,ubuntu1404,ubuntu1604,centos7|subprocess32|3.5.1|2|python|all
 all|ruamel.yaml|0.14.12|2|python|amd64|--python-setup-py-arguments --single-version-externally-managed
@@ -50,3 +52,6 @@ all|futures|3.0.5|2|python|all
 all|future|0.16.0|2|python|all
 all|future|0.16.0|2|python3|all
 all|mypy-extensions|0.3.0|1|python|all
+all|prov|1.5.1|0|python|all
+all|bagit|1.6.4|0|python|all
+all|typing-extensions|3.6.5|0|python|all
index b4ea35c574b20a776960aeefad4d4e4d324a347e..900b091959107ace13d606f0a433d5d90c56f0d9 100755 (executable)
@@ -27,7 +27,13 @@ cd /tmp/opts
 
 export ARV_PACKAGES_DIR="/arvados/packages/$target"
 
-dpkg-deb -x $(ls -t "$ARV_PACKAGES_DIR/$1"_*.deb | head -n1) .
+if [[ -f $(ls -t "$ARV_PACKAGES_DIR/$1"_*.deb | head -n1) ]] ; then
+    debpkg=$(ls -t "$ARV_PACKAGES_DIR/$1"_*.deb | head -n1)
+else
+    debpkg=$(ls -t "$ARV_PACKAGES_DIR/processed/$1"_*.deb | head -n1)
+fi
+
+dpkg-deb -x $debpkg .
 
 while read so && [ -n "$so" ]; do
     echo
index c5a73cbe35a6116fdbed0b8f364f2af4f0e83df5..6ee57a4be5597114bbb0521d562883c3a1850399 100755 (executable)
@@ -60,7 +60,7 @@ version_from_git() {
     declare $(format_last_commit_here "git_ts=%ct git_hash=%h")
     ARVADOS_BUILDING_VERSION="$(git describe --abbrev=0).$(date -ud "@$git_ts" +%Y%m%d%H%M%S)"
     echo "$ARVADOS_BUILDING_VERSION"
-} 
+}
 
 nohash_version_from_git() {
     if [[ -n "$ARVADOS_BUILDING_VERSION" ]]; then
@@ -273,12 +273,15 @@ test_package_presence() {
           repo_subdir=${pkgname:0:1}
         fi
 
-        repo_pkg_list=$(curl -o - http://apt.arvados.org/pool/${D}/main/${repo_subdir}/)
+        repo_pkg_list=$(curl -s -o - http://apt.arvados.org/pool/${D}/main/${repo_subdir}/)
         echo ${repo_pkg_list} |grep -q ${complete_pkgname}
-        if [ $? -eq 0 ]; then
+        if [ $? -eq 0 ] ; then
           echo "Package $complete_pkgname exists, not rebuilding!"
           curl -o ./${complete_pkgname} http://apt.arvados.org/pool/${D}/main/${repo_subdir}/${complete_pkgname}
           return 1
+       elif test -f "$WORKSPACE/packages/$TARGET/processed/${complete_pkgname}" ; then
+          echo "Package $complete_pkgname exists, not rebuilding!"
+          return 1
         else
           echo "Package $complete_pkgname not found, building"
           return 0
index 15689a9010934cf2b8847ec08825cf30bd3e13eb..9a893df781f477dadac19264fb49cfa77b459bb7 100644 (file)
@@ -139,6 +139,17 @@ class CollectionFsAccess(cwltool.stdfsaccess.StdFsAccess):
         else:
             return super(CollectionFsAccess, self).exists(fn)
 
+    def size(self, fn):  # type: (unicode) -> bool
+        collection, rest = self.get_collection(fn)
+        if collection is not None:
+            if rest:
+                arvfile = collection.find(rest)
+                if isinstance(arvfile, arvados.arvfile.ArvadosFile):
+                    return arvfile.size()
+            raise IOError(errno.EINVAL, "Not a path to a file %s" % (fn))
+        else:
+            return super(CollectionFsAccess, self).size(fn)
+
     def isfile(self, fn):  # type: (unicode) -> bool
         collection, rest = self.get_collection(fn)
         if collection is not None:
index 0cab074d9a8a9755f941c6a59e226d4bd9d1e5f3..3836cf5a23cd9f5320aa9a66eb5d508e7ddb6165 100644 (file)
@@ -33,8 +33,8 @@ setup(name='arvados-cwl-runner',
       # Note that arvados/build/run-build-packages.sh looks at this
       # file to determine what version of cwltool and schema-salad to build.
       install_requires=[
-          'cwltool==1.0.20180615183820',
-          'schema-salad==2.7.20180501211602',
+          'cwltool==1.0.20180806194258',
+          'schema-salad==2.7.20180719125426',
           'typing >= 3.5.3',
           'ruamel.yaml >=0.13.11, <0.15',
           'arvados-python-client>=1.1.4.20180607143841',
diff --git a/sdk/cwl/tests/13931-size-job.yml b/sdk/cwl/tests/13931-size-job.yml
new file mode 100644 (file)
index 0000000..97b46dd
--- /dev/null
@@ -0,0 +1,3 @@
+fastq1:
+  class: File
+  location: keep:20850f01122e860fb878758ac1320877+71/sample1_S01_R1_001.fastq.gz
\ No newline at end of file
diff --git a/sdk/cwl/tests/13931-size.cwl b/sdk/cwl/tests/13931-size.cwl
new file mode 100644 (file)
index 0000000..aed1bd6
--- /dev/null
@@ -0,0 +1,10 @@
+cwlVersion: v1.0
+class: CommandLineTool
+inputs:
+  fastq1: File
+outputs:
+  out: stdout
+baseCommand: echo
+arguments:
+  - $(inputs.fastq1.size)
+stdout: size.txt
\ No newline at end of file
index 4869e3e524153af30feb6a654e65e2cac6c57f3f..8635aae65507fadb6be76d27156167855440ac68 100755 (executable)
@@ -12,4 +12,8 @@ fi
 if ! arv-get 4d8a70b1e63b2aad6984e40e338e2373+69 > /dev/null ; then
     arv-put --portable-data-hash secondaryFiles/hello.txt*
 fi
+if ! arv-get 20850f01122e860fb878758ac1320877+71 > /dev/null ; then
+    arv-put --portable-data-hash samples/sample1_S01_R1_001.fastq.gz
+fi
+
 exec cwltest --test arvados-tests.yml --tool arvados-cwl-runner $@ -- --disable-reuse --compute-checksum
index 8eac71886cbf643ca97db1e033b9ba2808b40137..21191a5b3e1553952e1031f9c1ef019db56ab1c4 100644 (file)
     out: null
   tool: wf-defaults/wf7.cwl
   doc: workflow level default in RunInSingleContainer
+
+- job: 13931-size-job.yml
+  output:
+    "out": {
+        "checksum": "sha1$5bf6e5357bd42a6b1d2a3a040e16a91490064d26",
+        "location": "size.txt",
+        "class": "File",
+        "size": 3
+    }
+  tool: 13931-size.cwl
+  doc: Test that size is set for files in Keep
index ae234414a3df90888cfbe9028c06aa5efbba9f55..3f8a32816ddccdad01c78eedfdce1ed0b2be5e64 100644 (file)
@@ -481,7 +481,8 @@ class TestContainer(unittest.TestCase):
 
         keepdocker.return_value = [("zzzzz-4zz18-zzzzzzzzzzzzzz3", "")]
         runner.api.collections().get().execute.return_value = {
-            "portable_data_hash": "99999999999999999999999999999993+99"}
+            "portable_data_hash": "99999999999999999999999999999994+99",
+            "manifest_text": ". 99999999999999999999999999999994+99 0:0:file1 0:0:file2"}
 
         document_loader, avsc_names, schema_metadata, metaschema_loader = cwltool.process.get_schema("v1.0")
 
index c110bc5d53cd4634656d93fab2937954be973d07..4473b88ca0d785dbb2eaff961bf64fd21c25c280 100644 (file)
@@ -389,6 +389,7 @@ class TestWorkflow(unittest.TestCase):
         metadata["cwlVersion"] = tool["cwlVersion"]
 
         mockcollection().portable_data_hash.return_value = "99999999999999999999999999999999+118"
+        mockcollectionreader().find.return_value = arvados.arvfile.ArvadosFile(mock.MagicMock(), "token.txt")
 
         arvtool = arvados_cwl.ArvadosWorkflow(runner, tool, loadingContext)
         arvtool.formatgraph = None
@@ -435,7 +436,8 @@ class TestWorkflow(unittest.TestCase):
   "fileblub": {
     "basename": "token.txt",
     "class": "File",
-    "location": "/keep/99999999999999999999999999999999+118/token.txt"
+    "location": "/keep/99999999999999999999999999999999+118/token.txt",
+    "size": 0
   },
   "sleeptime": 5
 }''')])
index cd46251300dfb95862cb7957f510e108dd78b281..d980db575dd8d6e3db1ac3dbb0f7709cb14a894e 100644 (file)
@@ -132,7 +132,8 @@ def stubs(func):
                     "listing": [{
                         "basename": "renamed.txt",
                         "class": "File",
-                        "location": "keep:99999999999999999999999999999998+99/file1.txt"
+                        "location": "keep:99999999999999999999999999999998+99/file1.txt",
+                        "size": 0
                     }],
                     'class': 'Directory'
                 },
@@ -164,7 +165,8 @@ def stubs(func):
                                   {
                                       'basename': 'renamed.txt',
                                       'class': 'File', 'location':
-                                      'keep:99999999999999999999999999999998+99/file1.txt'
+                                      'keep:99999999999999999999999999999998+99/file1.txt',
+                                      'size': 0
                                   }
                               ]}},
                         'cwl:tool': '3fffdeaa75e018172e1b583425f4ebff+60/workflow.cwl#main',
@@ -225,7 +227,8 @@ def stubs(func):
                         'z': {'basename': 'anonymous', 'class': 'Directory', 'listing': [
                             {'basename': 'renamed.txt',
                              'class': 'File',
-                             'location': 'keep:99999999999999999999999999999998+99/file1.txt'
+                             'location': 'keep:99999999999999999999999999999998+99/file1.txt',
+                             'size': 0
                             }
                         ]}
                     },
@@ -781,6 +784,7 @@ class TestSubmit(unittest.TestCase):
     @stubs
     def test_submit_file_keepref(self, stubs, tm, collectionReader):
         capture_stdout = cStringIO.StringIO()
+        collectionReader().find.return_value = arvados.arvfile.ArvadosFile(mock.MagicMock(), "blorp.txt")
         exited = arvados_cwl.main(
             ["--submit", "--no-wait", "--api=containers", "--debug",
              "tests/wf/submit_keepref_wf.cwl"],
index 7b3b4503efc239661f5b03b2afb0cfac3ca8cc4d..c84252c7b8c135b0eb6105881dab64f70424006b 100644 (file)
@@ -64,7 +64,8 @@
                                 "class": "File",
                                 "location": "keep:99999999999999999999999999999998+99/file1.txt",
                                 "nameext": ".txt",
-                                "nameroot": "renamed"
+                                "nameroot": "renamed",
+                                "size": 0
                             }
                         ]
                     },
index f9f1e967b94f7e589a60888261eae4a7916a88c1..aa1f18052f8afcbe289da18d597b6e66d62d3db6 100644 (file)
@@ -20,7 +20,7 @@ ENV DEBIAN_FRONTEND noninteractive
 
 RUN apt-get update -q && apt-get install -qy git python-pip python-virtualenv python-dev libcurl4-gnutls-dev libgnutls28-dev nodejs python-pyasn1-modules
 
-RUN pip install -U setuptools
+RUN pip install -U setuptools six
 
 ARG sdk
 ARG runner
index d1f19dd7b5702e2431471bc7ce2164f553a8cb11..b4103cc625a2badc3a3ab3f3d7458bac3f35e34e 100644 (file)
@@ -252,9 +252,6 @@ func (disp *Dispatcher) submit(container arvados.Container, crunchRunCommand []s
        crArgs = append(crArgs, container.UUID)
        crScript := strings.NewReader(execScript(crArgs))
 
-       disp.sqCheck.L.Lock()
-       defer disp.sqCheck.L.Unlock()
-
        sbArgs, err := disp.sbatchArgs(container)
        if err != nil {
                return err
@@ -355,10 +352,7 @@ func (disp *Dispatcher) runContainer(_ *dispatch.Dispatcher, ctr arvados.Contain
        }
 }
 func (disp *Dispatcher) scancel(ctr arvados.Container) {
-       disp.sqCheck.L.Lock()
        err := disp.slurm.Cancel(ctr.UUID)
-       disp.sqCheck.L.Unlock()
-
        if err != nil {
                log.Printf("scancel: %s", err)
                time.Sleep(time.Second)
index 719ec98d27aa19d65eceb7d3db3a46f506aed2f0..4ef4ba1d5d85a076a11dd0faf78c5b92d3641fcf 100644 (file)
@@ -116,7 +116,7 @@ func (s *IntegrationSuite) integrationTest(c *C,
        var containers arvados.ContainerList
        err = arv.List("containers", params, &containers)
        c.Check(err, IsNil)
-       c.Check(len(containers.Items), Equals, 1)
+       c.Assert(len(containers.Items), Equals, 1)
 
        s.disp.CrunchRunCommand = []string{"echo"}
 
index 24a056264475bd26d2ec65e4b5825302903af806..20305ab90abe91b150ae71a7749fd39c8e529548 100644 (file)
@@ -33,7 +33,8 @@ type SqueueChecker struct {
        queue          map[string]*slurmJob
        startOnce      sync.Once
        done           chan struct{}
-       sync.Cond
+       lock           sync.RWMutex
+       notify         sync.Cond
 }
 
 // HasUUID checks if a given container UUID is in the slurm queue.
@@ -42,11 +43,11 @@ type SqueueChecker struct {
 func (sqc *SqueueChecker) HasUUID(uuid string) bool {
        sqc.startOnce.Do(sqc.start)
 
-       sqc.L.Lock()
-       defer sqc.L.Unlock()
+       sqc.lock.RLock()
+       defer sqc.lock.RUnlock()
 
        // block until next squeue broadcast signaling an update.
-       sqc.Wait()
+       sqc.notify.Wait()
        _, exists := sqc.queue[uuid]
        return exists
 }
@@ -55,25 +56,30 @@ func (sqc *SqueueChecker) HasUUID(uuid string) bool {
 // container.
 func (sqc *SqueueChecker) SetPriority(uuid string, want int64) {
        sqc.startOnce.Do(sqc.start)
-       sqc.L.Lock()
-       defer sqc.L.Unlock()
-       job, ok := sqc.queue[uuid]
-       if !ok {
+
+       sqc.lock.RLock()
+       job := sqc.queue[uuid]
+       if job == nil {
                // Wait in case the slurm job was just submitted and
                // will appear in the next squeue update.
-               sqc.Wait()
-               if job, ok = sqc.queue[uuid]; !ok {
-                       return
-               }
+               sqc.notify.Wait()
+               job = sqc.queue[uuid]
+       }
+       needUpdate := job != nil && job.wantPriority != want
+       sqc.lock.RUnlock()
+
+       if needUpdate {
+               sqc.lock.Lock()
+               job.wantPriority = want
+               sqc.lock.Unlock()
        }
-       job.wantPriority = want
 }
 
 // adjust slurm job nice values as needed to ensure slurm priority
 // order matches Arvados priority order.
 func (sqc *SqueueChecker) reniceAll() {
-       sqc.L.Lock()
-       defer sqc.L.Unlock()
+       sqc.lock.RLock()
+       defer sqc.lock.RUnlock()
 
        jobs := make([]*slurmJob, 0, len(sqc.queue))
        for _, j := range sqc.queue {
@@ -133,12 +139,8 @@ func (sqc *SqueueChecker) Stop() {
 // queued). If it succeeds, it updates sqc.queue and wakes up any
 // goroutines that are waiting in HasUUID() or All().
 func (sqc *SqueueChecker) check() {
-       // Mutex between squeue sync and running sbatch or scancel.  This
-       // establishes a sequence so that squeue doesn't run concurrently with
-       // sbatch or scancel; the next update of squeue will occur only after
-       // sbatch or scancel has completed.
-       sqc.L.Lock()
-       defer sqc.L.Unlock()
+       sqc.lock.Lock()
+       defer sqc.lock.Unlock()
 
        cmd := sqc.Slurm.QueueCommand([]string{"--all", "--noheader", "--format=%j %y %Q %T %r"})
        stdout, stderr := &bytes.Buffer{}, &bytes.Buffer{}
@@ -191,18 +193,18 @@ func (sqc *SqueueChecker) check() {
                        // resolved the same way.
                        log.Printf("releasing held job %q (priority=%d, state=%q, reason=%q)", uuid, p, state, reason)
                        sqc.Slurm.Release(uuid)
-               } else if p < 1<<20 && replacing.wantPriority > 0 {
+               } else if state != "RUNNING" && p <= 2*slurm15NiceLimit && replacing.wantPriority > 0 {
                        log.Printf("warning: job %q has low priority %d, nice %d, state %q, reason %q", uuid, p, n, state, reason)
                }
        }
        sqc.queue = newq
-       sqc.Broadcast()
+       sqc.notify.Broadcast()
 }
 
 // Initialize, and start a goroutine to call check() once per
 // squeue.Period until terminated by calling Stop().
 func (sqc *SqueueChecker) start() {
-       sqc.L = &sync.Mutex{}
+       sqc.notify.L = sqc.lock.RLocker()
        sqc.done = make(chan struct{})
        go func() {
                ticker := time.NewTicker(sqc.Period)
@@ -214,6 +216,15 @@ func (sqc *SqueueChecker) start() {
                        case <-ticker.C:
                                sqc.check()
                                sqc.reniceAll()
+                               select {
+                               case <-ticker.C:
+                                       // If this iteration took
+                                       // longer than sqc.Period,
+                                       // consume the next tick and
+                                       // wait. Otherwise we would
+                                       // starve other goroutines.
+                               default:
+                               }
                        }
                }
        }()
@@ -223,9 +234,9 @@ func (sqc *SqueueChecker) start() {
 // names reported by squeue.
 func (sqc *SqueueChecker) All() []string {
        sqc.startOnce.Do(sqc.start)
-       sqc.L.Lock()
-       defer sqc.L.Unlock()
-       sqc.Wait()
+       sqc.lock.RLock()
+       defer sqc.lock.RUnlock()
+       sqc.notify.Wait()
        var uuids []string
        for u := range sqc.queue {
                uuids = append(uuids, u)
index b2bab78216eaf921a4710a5037d9af2231df31c5..8336b78f9ea9614af2796211d9ed89d58da741e8 100644 (file)
@@ -6,7 +6,6 @@ package main
 
 import (
        "sync"
-       "sync/atomic"
        "time"
 
        "git.curoverse.com/arvados.git/sdk/go/arvados"
@@ -26,7 +25,6 @@ type cache struct {
        MaxUUIDEntries       int
 
        registry    *prometheus.Registry
-       stats       cacheStats
        metrics     cacheMetrics
        pdhs        *lru.TwoQueueCache
        collections *lru.TwoQueueCache
@@ -34,17 +32,6 @@ type cache struct {
        setupOnce   sync.Once
 }
 
-// cacheStats is EOL - add new metrics to cacheMetrics instead
-type cacheStats struct {
-       Requests          uint64 `json:"Cache.Requests"`
-       CollectionBytes   uint64 `json:"Cache.CollectionBytes"`
-       CollectionEntries int    `json:"Cache.CollectionEntries"`
-       CollectionHits    uint64 `json:"Cache.CollectionHits"`
-       PDHHits           uint64 `json:"Cache.UUIDHits"`
-       PermissionHits    uint64 `json:"Cache.PermissionHits"`
-       APICalls          uint64 `json:"Cache.APICalls"`
-}
-
 type cacheMetrics struct {
        requests          prometheus.Counter
        collectionBytes   prometheus.Gauge
@@ -157,19 +144,6 @@ var selectPDH = map[string]interface{}{
        "select": []string{"portable_data_hash"},
 }
 
-func (c *cache) Stats() cacheStats {
-       c.setupOnce.Do(c.setup)
-       return cacheStats{
-               Requests:          atomic.LoadUint64(&c.stats.Requests),
-               CollectionBytes:   c.collectionBytes(),
-               CollectionEntries: c.collections.Len(),
-               CollectionHits:    atomic.LoadUint64(&c.stats.CollectionHits),
-               PDHHits:           atomic.LoadUint64(&c.stats.PDHHits),
-               PermissionHits:    atomic.LoadUint64(&c.stats.PermissionHits),
-               APICalls:          atomic.LoadUint64(&c.stats.APICalls),
-       }
-}
-
 // Update saves a modified version (fs) to an existing collection
 // (coll) and, if successful, updates the relevant cache entries so
 // subsequent calls to Get() reflect the modifications.
@@ -195,8 +169,6 @@ func (c *cache) Update(client *arvados.Client, coll arvados.Collection, fs arvad
 
 func (c *cache) Get(arv *arvadosclient.ArvadosClient, targetID string, forceReload bool) (*arvados.Collection, error) {
        c.setupOnce.Do(c.setup)
-
-       atomic.AddUint64(&c.stats.Requests, 1)
        c.metrics.requests.Inc()
 
        permOK := false
@@ -208,7 +180,6 @@ func (c *cache) Get(arv *arvadosclient.ArvadosClient, targetID string, forceRelo
                        c.permissions.Remove(permKey)
                } else {
                        permOK = true
-                       atomic.AddUint64(&c.stats.PermissionHits, 1)
                        c.metrics.permissionHits.Inc()
                }
        }
@@ -222,7 +193,6 @@ func (c *cache) Get(arv *arvadosclient.ArvadosClient, targetID string, forceRelo
                        c.pdhs.Remove(targetID)
                } else {
                        pdh = ent.pdh
-                       atomic.AddUint64(&c.stats.PDHHits, 1)
                        c.metrics.pdhHits.Inc()
                }
        }
@@ -239,7 +209,6 @@ func (c *cache) Get(arv *arvadosclient.ArvadosClient, targetID string, forceRelo
                // likely, the cached PDH is still correct; if so,
                // _and_ the current token has permission, we can
                // use our cached manifest.
-               atomic.AddUint64(&c.stats.APICalls, 1)
                c.metrics.apiCalls.Inc()
                var current arvados.Collection
                err := arv.Get("collections", targetID, selectPDH, &current)
@@ -268,7 +237,6 @@ func (c *cache) Get(arv *arvadosclient.ArvadosClient, targetID string, forceRelo
        }
 
        // Collection manifest is not cached.
-       atomic.AddUint64(&c.stats.APICalls, 1)
        c.metrics.apiCalls.Inc()
        err := arv.Get("collections", targetID, nil, &collection)
        if err != nil {
@@ -359,7 +327,6 @@ func (c *cache) lookupCollection(key string) *arvados.Collection {
                c.collections.Remove(key)
                return nil
        }
-       atomic.AddUint64(&c.stats.CollectionHits, 1)
        c.metrics.collectionHits.Inc()
        return ent.collection
 }
index cddeaf489763500b9e7230a75c2b19a4c25f40cf..d147573eec72d402faec43c21da86a010f13dc94 100644 (file)
@@ -5,17 +5,36 @@
 package main
 
 import (
+       "bytes"
+
        "git.curoverse.com/arvados.git/sdk/go/arvados"
        "git.curoverse.com/arvados.git/sdk/go/arvadosclient"
        "git.curoverse.com/arvados.git/sdk/go/arvadostest"
+       "github.com/prometheus/client_golang/prometheus"
+       "github.com/prometheus/common/expfmt"
        "gopkg.in/check.v1"
 )
 
+func (s *UnitSuite) checkCacheMetrics(c *check.C, reg *prometheus.Registry, regs ...string) {
+       mfs, err := reg.Gather()
+       c.Check(err, check.IsNil)
+       buf := &bytes.Buffer{}
+       enc := expfmt.NewEncoder(buf, expfmt.FmtText)
+       for _, mf := range mfs {
+               c.Check(enc.Encode(mf), check.IsNil)
+       }
+       mm := buf.String()
+       for _, reg := range regs {
+               c.Check(mm, check.Matches, `(?ms).*collectioncache_`+reg+`\n.*`)
+       }
+}
+
 func (s *UnitSuite) TestCache(c *check.C) {
        arv, err := arvadosclient.MakeArvadosClient()
        c.Assert(err, check.Equals, nil)
 
        cache := DefaultConfig().Cache
+       cache.registry = prometheus.NewRegistry()
 
        // Hit the same collection 5 times using the same token. Only
        // the first req should cause an API call; the next 4 should
@@ -29,11 +48,12 @@ func (s *UnitSuite) TestCache(c *check.C) {
                c.Check(coll.PortableDataHash, check.Equals, arvadostest.FooPdh)
                c.Check(coll.ManifestText[:2], check.Equals, ". ")
        }
-       c.Check(cache.Stats().Requests, check.Equals, uint64(5))
-       c.Check(cache.Stats().CollectionHits, check.Equals, uint64(4))
-       c.Check(cache.Stats().PermissionHits, check.Equals, uint64(4))
-       c.Check(cache.Stats().PDHHits, check.Equals, uint64(4))
-       c.Check(cache.Stats().APICalls, check.Equals, uint64(1))
+       s.checkCacheMetrics(c, cache.registry,
+               "requests 5",
+               "hits 4",
+               "permission_hits 4",
+               "pdh_hits 4",
+               "api_calls 1")
 
        // Hit the same collection 2 more times, this time requesting
        // it by PDH and using a different token. The first req should
@@ -49,11 +69,12 @@ func (s *UnitSuite) TestCache(c *check.C) {
        c.Check(coll2.ManifestText[:2], check.Equals, ". ")
        c.Check(coll2.ManifestText, check.Not(check.Equals), coll.ManifestText)
 
-       c.Check(cache.Stats().Requests, check.Equals, uint64(5+1))
-       c.Check(cache.Stats().CollectionHits, check.Equals, uint64(4+0))
-       c.Check(cache.Stats().PermissionHits, check.Equals, uint64(4+0))
-       c.Check(cache.Stats().PDHHits, check.Equals, uint64(4+0))
-       c.Check(cache.Stats().APICalls, check.Equals, uint64(1+1))
+       s.checkCacheMetrics(c, cache.registry,
+               "requests 6",
+               "hits 4",
+               "permission_hits 4",
+               "pdh_hits 4",
+               "api_calls 2")
 
        coll2, err = cache.Get(arv, arvadostest.FooPdh, false)
        c.Check(err, check.Equals, nil)
@@ -61,11 +82,12 @@ func (s *UnitSuite) TestCache(c *check.C) {
        c.Check(coll2.PortableDataHash, check.Equals, arvadostest.FooPdh)
        c.Check(coll2.ManifestText[:2], check.Equals, ". ")
 
-       c.Check(cache.Stats().Requests, check.Equals, uint64(5+2))
-       c.Check(cache.Stats().CollectionHits, check.Equals, uint64(4+1))
-       c.Check(cache.Stats().PermissionHits, check.Equals, uint64(4+1))
-       c.Check(cache.Stats().PDHHits, check.Equals, uint64(4+0))
-       c.Check(cache.Stats().APICalls, check.Equals, uint64(1+1))
+       s.checkCacheMetrics(c, cache.registry,
+               "requests 7",
+               "hits 5",
+               "permission_hits 5",
+               "pdh_hits 4",
+               "api_calls 2")
 
        // Alternating between two collections N times should produce
        // only 2 more API calls.
@@ -80,11 +102,12 @@ func (s *UnitSuite) TestCache(c *check.C) {
                _, err := cache.Get(arv, target, false)
                c.Check(err, check.Equals, nil)
        }
-       c.Check(cache.Stats().Requests, check.Equals, uint64(5+2+20))
-       c.Check(cache.Stats().CollectionHits, check.Equals, uint64(4+1+18))
-       c.Check(cache.Stats().PermissionHits, check.Equals, uint64(4+1+18))
-       c.Check(cache.Stats().PDHHits, check.Equals, uint64(4+0+18))
-       c.Check(cache.Stats().APICalls, check.Equals, uint64(1+1+2))
+       s.checkCacheMetrics(c, cache.registry,
+               "requests 27",
+               "hits 23",
+               "permission_hits 23",
+               "pdh_hits 22",
+               "api_calls 4")
 }
 
 func (s *UnitSuite) TestCacheForceReloadByPDH(c *check.C) {
@@ -92,17 +115,19 @@ func (s *UnitSuite) TestCacheForceReloadByPDH(c *check.C) {
        c.Assert(err, check.Equals, nil)
 
        cache := DefaultConfig().Cache
+       cache.registry = prometheus.NewRegistry()
 
        for _, forceReload := range []bool{false, true, false, true} {
                _, err := cache.Get(arv, arvadostest.FooPdh, forceReload)
                c.Check(err, check.Equals, nil)
        }
 
-       c.Check(cache.Stats().Requests, check.Equals, uint64(4))
-       c.Check(cache.Stats().CollectionHits, check.Equals, uint64(3))
-       c.Check(cache.Stats().PermissionHits, check.Equals, uint64(1))
-       c.Check(cache.Stats().PDHHits, check.Equals, uint64(0))
-       c.Check(cache.Stats().APICalls, check.Equals, uint64(3))
+       s.checkCacheMetrics(c, cache.registry,
+               "requests 4",
+               "hits 3",
+               "permission_hits 1",
+               "pdh_hits 0",
+               "api_calls 3")
 }
 
 func (s *UnitSuite) TestCacheForceReloadByUUID(c *check.C) {
@@ -110,15 +135,17 @@ func (s *UnitSuite) TestCacheForceReloadByUUID(c *check.C) {
        c.Assert(err, check.Equals, nil)
 
        cache := DefaultConfig().Cache
+       cache.registry = prometheus.NewRegistry()
 
        for _, forceReload := range []bool{false, true, false, true} {
                _, err := cache.Get(arv, arvadostest.FooCollection, forceReload)
                c.Check(err, check.Equals, nil)
        }
 
-       c.Check(cache.Stats().Requests, check.Equals, uint64(4))
-       c.Check(cache.Stats().CollectionHits, check.Equals, uint64(3))
-       c.Check(cache.Stats().PermissionHits, check.Equals, uint64(1))
-       c.Check(cache.Stats().PDHHits, check.Equals, uint64(3))
-       c.Check(cache.Stats().APICalls, check.Equals, uint64(3))
+       s.checkCacheMetrics(c, cache.registry,
+               "requests 4",
+               "hits 3",
+               "permission_hits 1",
+               "pdh_hits 3",
+               "api_calls 3")
 }
index d0ba431aa6312d64f44e518d5ca19d8826ad1c5c..bb77e5859449f5e7e4783d76d02120c359d51085 100644 (file)
@@ -91,14 +91,7 @@ func (h *handler) setup() {
 }
 
 func (h *handler) serveStatus(w http.ResponseWriter, r *http.Request) {
-       status := struct {
-               cacheStats
-               Version string
-       }{
-               cacheStats: h.Config.Cache.Stats(),
-               Version:    version,
-       }
-       json.NewEncoder(w).Encode(status)
+       json.NewEncoder(w).Encode(struct{ Version string }{version})
 }
 
 // updateOnSuccess wraps httpserver.ResponseWriter. If the handler
index 0a2b9eb988dce96c4791f7cbbaf0c602d5b16980..62db198dd9b9ef27618b9bfd04262b32ac2736f0 100644 (file)
@@ -30,7 +30,6 @@ func (s *UnitSuite) TestStatus(c *check.C) {
        var status map[string]interface{}
        err := json.NewDecoder(resp.Body).Decode(&status)
        c.Check(err, check.IsNil)
-       c.Check(status["Cache.Requests"], check.Equals, float64(0))
        c.Check(status["Version"], check.Not(check.Equals), "")
 }