Bug #16997: arvados-server config-check doesn't sort Containers/InstanceTypes - Arvados

Actions

Copy link

Bug #16997

closed

arvados-server config-check doesn't sort Containers/InstanceTypes

Added by Ward Vandewege about 4 years ago. Updated about 3 years ago.

Status:

Resolved

Priority:

Normal

Assigned To:

Tom Clegg

Category:

Target version:

2021-04-28 bughunt sprint

Start date:

04/27/2021

Due date:

% Done:

100%

Estimated time:

(Total: 0.00 h)

Story points:

Release:

Arvados 2.2.2

Release relationship:

Auto

Description

This means that running `arvados-server` isn't deterministic:

workbench.su92l:/var/www/arvados-workbench/current/config# arvados-server --version
arvados-server 2.1.0~rc5 (go1.13.4)
workbench.su92l:~# arvados-server config-check
workbench.su92l:~# arvados-server config-check
Your configuration is relying on deprecated entries. Suggest making the following changes.
--- without-deprecated-configs
+++ relying-on-deprecated-configs
@@ -528,26 +528,6 @@
         RAM: 34359738368
         Scratch: 100000000000
         VCPUs: 4
-      Standard_E48_v3:
-        AddedScratch: 0
-        IncludedScratch: 1200000000000
-        Name: Standard_E48_v3
-        Preemptible: false
-        Price: 3.501001
-        ProviderType: Standard_E48_v3
-        RAM: 412316860416
-        Scratch: 1200000000000
-        VCPUs: 48
-      Standard_E48s_v3:
-        AddedScratch: 0
-        IncludedScratch: 768000000000
-        Name: Standard_E48s_v3
-        Preemptible: false
-        Price: 3.501
-        ProviderType: Standard_E48s_v3
-        RAM: 412316860416
-        Scratch: 768000000000
-        VCPUs: 48
       Standard_E4s_v3:
         AddedScratch: 0
         IncludedScratch: 64000000000
@@ -638,6 +618,26 @@
         RAM: 274877906944
         Scratch: 512000000000
         VCPUs: 32
+      Standard_E48_v3:
+        AddedScratch: 0
+        IncludedScratch: 1200000000000
+        Name: Standard_E48_v3
+        Preemptible: false
+        Price: 3.501001
+        ProviderType: Standard_E48_v3
+        RAM: 412316860416
+        Scratch: 1200000000000
+        VCPUs: 48
+      Standard_E48s_v3:
+        AddedScratch: 0
+        IncludedScratch: 768000000000
+        Name: Standard_E48s_v3
+        Preemptible: false
+        Price: 3.501
+        ProviderType: Standard_E48s_v3
+        RAM: 412316860416
+        Scratch: 768000000000
+        VCPUs: 48
       Standard_E64_v3:
         AddedScratch: 0
         IncludedScratch: 1600000000000

Subtasks 1 (1 open — 0 closed)

Actions

Copy link

Updated by Ward Vandewege about 4 years ago

Description updated (diff)

Actions

Copy link

Updated by Ward Vandewege about 4 years ago

Description updated (diff)

Actions

Copy link

Updated by Tom Clegg over 3 years ago

Assigned To set to Tom Clegg
Target version changed from To Be Groomed to 2021-04-28 bughunt sprint

Actions

Copy link

Updated by Tom Clegg over 3 years ago

Status changed from New to In Progress

Actions

Copy link

Updated by Tom Clegg over 3 years ago

I haven't been able to reproduce this yet (even with arvados 2.1.0 + go1.13) or figure out why keys would be printed in non-lexical order, but I added a test, and while investigating I found two different duplicate-warning bugs to fix.

config-check calls Load() twice, so "missing/invalid token" warnings were appearing twice
config-check had its own way of warning about missing tokens, which wasn't deleted when Load() started warning about them too

16997-sort-config-for-diff @ 727c8c37baa64fe63bec04aacea870ea47a7daf0 -- developer-run-tests: #2440

Actions

Copy link

Updated by Tom Clegg over 3 years ago

Mystery solved: go-yaml uses a fancy Less() function for sorting that is non-deterministic for keys like {"a32a", "a48a", "a4a"}. Sometimes we get the sort order {a4a < a32a < a48a}, sometimes we get {a32a < a48a < a4a}.

The real solution is to fix go-yaml's Less function again (other sorting bugs have been fixed before and other people have hit this same bug but it hasn't been updated since the version we're using, v2.2.4) ... but in the meantime here's a kludge to work around it.

16997-sort-config-for-diff @ 9a7efddc9cf8118a346c797365027f168a1e49fe -- developer-run-tests: #2441

Actions

Copy link

Updated by Ward Vandewege over 3 years ago

Tom Clegg wrote:

Mystery solved: go-yaml uses a fancy Less() function for sorting that is non-deterministic for keys like {"a32a", "a48a", "a4a"}. Sometimes we get the sort order {a4a < a32a < a48a}, sometimes we get {a32a < a48a < a4a}.

The real solution is to fix go-yaml's Less function again (other sorting bugs have been fixed before and other people have hit this same bug but it hasn't been updated since the version we're using, v2.2.4) ... but in the meantime here's a kludge to work around it.

16997-sort-config-for-diff @ 9a7efddc9cf8118a346c797365027f168a1e49fe -- developer-run-tests: #2441

The kludge works, LGTM. If you're planning to submit a PR upstream to fix the bug, can you add a comment to our code referencing that PR, with a note to remove the kludge once a fixed version is released upstream?

Actions

Copy link

Updated by Tom Clegg over 3 years ago

Here's the PR. https://github.com/go-yaml/yaml/pull/733

Here's a branch that adds the same test, but switches to our patched go-yaml instead of the awful kludge.

16997-sort-config-for-diff @ 7eccf2d2d8d45b5b6f10ba6cf123cba1cc90e4e3 -- developer-run-tests: #2442

Actions

Copy link

Updated by Ward Vandewege over 3 years ago

Tom Clegg wrote:

Here's the PR. https://github.com/go-yaml/yaml/pull/733

Here's a branch that adds the same test, but switches to our patched go-yaml instead of the awful kludge.

16997-sort-config-for-diff @ 7eccf2d2d8d45b5b6f10ba6cf123cba1cc90e4e3 -- developer-run-tests: #2442

LGTM, thanks!

Actions

Copy link

#10