Feature #17187

[costanalyzer] feature improvements

Added by Ward Vandewege 11 months ago. Updated 5 months ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
12/06/2020
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
-
Release relationship:
Auto

Description

  • specify UUIDs on command line without having to insert the word "--uuid" before each one
  • specify collection UUID, meaning look up collection.properties.container_request and look up the cost of that CR
  • output the total dollar amount on stdout

Subtasks

Task #17196: review 17187-costanalyzer-updatesResolvedWard Vandewege

Associated revisions

Revision 01787232
Added by Ward Vandewege 10 months ago

Merge branch '17187-costanalyzer-updates'

closes #17187

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

Revision 6f27bbc6 (diff)
Added by Ward Vandewege 10 months ago

Costanalyzer bugfix: when a collection uuid is supplied, write the csv
files with the name of the associated container request uuid, not the
collection uuid. Also change tests to use a proper temporary directory
for the outputs, and do not reuse that directory between tests (which
hid the bug this commit fixed).

refs #17187

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

Revision f437716f (diff)
Added by Ward Vandewege 10 months ago

Costanalyzer bugfix: when a collection uuid is supplied, write the csv
files with the name of the associated container request uuid, not the
collection uuid. Also change tests to use a proper temporary directory
for the outputs, and do not reuse that directory between tests (which
hid the bug this commit fixed).

refs #17187

Arvados-DCO-1.1-Signed-off-by: Ward Vandewege <>

History

#1 Updated by Ward Vandewege 11 months ago

  • Description updated (diff)

#2 Updated by Ward Vandewege 11 months ago

  • Status changed from New to In Progress

#3 Updated by Ward Vandewege 11 months ago

Ready for review at 6f5431413448f52f3ae5c88553b7f7ee1532b9fd on branch 17187-costanalyzer-updates.

#4 Updated by Tom Clegg 11 months ago

This is way more convenient, thanks.

$ arvados-client costanalyzer -output /tmp/ca --uuid su92l-4zz18-hry71275jl44dui
...
8.95688399

Specifying multiple collections/containers would be even more convenient if I could do this:

$ arvados-client costanalyzer -output /tmp/ca su92l-4zz18-a5a0ydd2feqcobz su92l-4zz18-2fmt581375q8tv6 su92l-4zz18-w1usu1jeq0jc1wm su92l-4zz18-4g3kijra6qx19k4

(Seems like we could treat everything in flag.Args() as a UUID after parsing other flags, unless we're reserving those positional args for something else?)

After accidentally testing collection UUIDs on an older version I noticed bogus --uuid args are silently ignored:

$ arvados-client costanalyzer -output /tmp/ca --uuid bogus --uuid su92l-4zz18-hry71275jl44dui
...
8.95688399
$ echo $?
0

$ arvados-client costanalyzer -output /tmp/ca --uuid bogus
Nothing to do!
$ echo $?
0

Both of those cases should probably be considered errors.

This type assertion will panic on {"properties":{"container_request":["foo"]}} -- should do a 2-value type assertion and return an error instead.

               crUUID = value.(string)

Yet another feature request: now that I get total cost on stdout, I often don't even care about the details that get saved in the output dir, so it would be nice if -output=/dev/null or -output="" meant "don't bother writing any csv files" and I didn't need to create/specify a superfluous output dir -- perhaps that could even be the default?

#5 Updated by Ward Vandewege 10 months ago

Ready for another look at 856fd8070951c570464dbcc2785c9be689d315b9 on the 17187-costanalyzer-updates branch.

Tom Clegg wrote:

Specifying multiple collections/containers would be even more convenient if I could do this:
(Seems like we could treat everything in flag.Args() as a UUID after parsing other flags, unless we're reserving those positional args for something else?)

I've changed it that way; uuids must now be listed at the end of the option list (and no more need to say '-uuid' first).

After accidentally testing collection UUIDs on an older version I noticed bogus --uuid args are silently ignored:
Both of those cases should probably be considered errors.

Done.

This type assertion will panic on {"properties":{"container_request":["foo"]}} -- should do a 2-value type assertion and return an error instead.

Done, nice catch!

Yet another feature request: now that I get total cost on stdout, I often don't even care about the details that get saved in the output dir, so it would be nice if -output=/dev/null or -output="" meant "don't bother writing any csv files" and I didn't need to create/specify a superfluous output dir -- perhaps that could even be the default?

Sure, done. The '-output' argument is now optional.

#6 Updated by Tom Clegg 10 months ago

LGTM, thanks!

#7 Updated by Ward Vandewege 10 months ago

  • % Done changed from 0 to 100
  • Status changed from In Progress to Resolved

#8 Updated by Peter Amstutz 5 months ago

  • Release set to 38

Also available in: Atom PDF