Bug #15734
closed
[a-d-c] needs to populate node.json in the container log collection
Description
Node manager drops a file called node.json in the container log collection, with an output like this:
{
"created_at": "2017-04-12T04:37:11.016719000Z",
"crunch_worker_state": "busy",
"domain": "qr1hi.arvadosapi.com",
"etag": "xyrne3ax1barycf21f0xw49i",
"first_ping_at": "2019-06-01T18:53:52.714980000Z",
"hostname": "compute3",
"href": "/nodes/qr1hi-7ekkf-z6lokkesrtdw9ia",
"ip_address": "10.26.64.21",
"job_uuid": null,
"kind": "arvados#node",
"last_ping_at": "2019-06-01T18:56:02.166756000Z",
"modified_at": "2019-06-01T18:56:02.170027000Z",
"modified_by_client_uuid": null,
"modified_by_user_uuid": "qr1hi-tpzed-000000000000000",
"nameservers": [
"10.26.0.11"
],
"owner_uuid": "qr1hi-tpzed-000000000000000",
"properties": {
"cloud_node": {
"price": 0.192,
"size": "Standard_D4s_v3"
},
"total_cpu_cores": 4,
"total_ram_mb": 16018,
"total_scratch_mb": 32747
},
"slot_number": 3,
"status": "running",
"uuid": "qr1hi-7ekkf-z6lokkesrtdw9ia"
}
This information originates from the Arvados node record. That table is no longer used by a-d-c. We need the 'properties' section to be available in that file for container cost accounting. A-d-c should populate the node.json file like this:
{
"properties": {
"cloud_node": {
"price": 0.192,
"size": "Standard_D4s_v3"
},
"total_cpu_cores": 4,
"total_ram_mb": 16018,
"total_scratch_mb": 32747
},
}
instead of just dropping an empty file in the container logs. If it makes sense to change the node.json format that is fine, as long as the new format is easily machine readable.
Related issues
Updated by Ward Vandewege over 4 years ago
- Target version changed from To Be Groomed to 2019-11-06 Sprint
Updated by Tom Clegg over 4 years ago
- Status changed from New to In Progress
- Assigned To set to Tom Clegg
15734-dispatchcloud-node-info @ 9481ff4a22314c0d5acffe78fbc7595278414e6f -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1606/
crunch-run copies the relevant InstanceTypes config entry into node.json if it was dispatched via arvados-dispatch-cloud. This means the format differs depending on whether you're using crunch-dispatch-slurm or arvados-dispatch-cloud.
Updated by Peter Amstutz over 4 years ago
Tom Clegg wrote:
15734-dispatchcloud-node-info @ 9481ff4a22314c0d5acffe78fbc7595278414e6f -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1606/
crunch-run copies the relevant InstanceTypes config entry into node.json if it was dispatched via arvados-dispatch-cloud. This means the format differs depending on whether you're using crunch-dispatch-slurm or arvados-dispatch-cloud.
+ // In order to stay compatible with recent dev/experimental
+ // versions of crunch-run, we need to pass a map with string
+ // values only, so we JSON-encode the instance type
+ // record. Once worker images are updated we can skip this and
+ // just pass {"InstanceType": wkr.instType}.
I found this comment quite confusing. I think what you saying is that versions of crunch-run prior to this one will fail if given a non-string value, so the a workaround is for dispatcher to pass the json text nested in a string and write it directly to the file instead of decoding and reencoding it.
This reminds me that the idea of provisioning a known copy of crunch-run on newly booted nodes would neatly avoid these kinds of compatibility problems. We discussed this, is there actually a ticket for that?
Rest LGTM.
Updated by Tom Clegg over 4 years ago
- Related to Idea #15759: [arvados-dispatch-cloud] deploy/run correct version of crunch-run binary on worker nodes added
Updated by Tom Clegg over 4 years ago
- Status changed from In Progress to Resolved
- % Done changed from 0 to 100
Applied in changeset arvados|4cadc4832ed4dd58e841628232c472e99b6f7d1b.
Updated by Tom Clegg over 4 years ago
Note in order for this to work, you need to update crunch-run on the worker image, as well as updating arvados-dispatch-cloud.
Updated by Tom Clegg over 4 years ago
- Status changed from Resolved to In Progress
- a-d-c passes InstanceType map on stdin of crunch-run
- crunch-run reads stdin into a map[string]interface{} and saves string values to environment
- crunch-run starts a child
crunch-run -no-detach; child inherits string values from the map via environment vars -- but not the map[string]interface{} itself - crunch-run (parent) exits
- crunch-run (child) looks for InstanceType in runner.dispatchEnv, which is always empty because it's only populated in the parent process
Updated by Tom Clegg over 4 years ago
- abandon hope of passing as a struct in future
- when logging, use the env var (which is propagated to the detached crunch-run child) instead of a map (which isn't)
Updated by Peter Amstutz over 4 years ago
Tom Clegg wrote:
15734-dispatchcloud-node-info @ f5f4c5f6088707c86c43ca2bd70a70c14699e450 -- https://ci.curoverse.com/view/Developer/job/developer-run-tests/1615/
- abandon hope of passing as a struct in future
- when logging, use the env var (which is propagated to the detached crunch-run child) instead of a map (which isn't)
Darn, the interaction with "detach" was not obvious to either of us.
I wish we had a better way to test this than rolling it out to staging clusters (eg #15370)
Technically, couldn't we pipe dispatchEnv to the detached process the same way the parent gets it from c-d-c?
However, I think the simpler approach of propagating the plain string is probably better in this case.
LGTM (but I have no way of realistically testing it)
Updated by Tom Clegg over 4 years ago
- Status changed from In Progress to Resolved
Applied in changeset arvados|e87f5931f41937cf210cdebd42783c9dc8c8c723.
Updated by Ward Vandewege over 4 years ago
Tom Clegg wrote:
Applied in changeset arvados|e87f5931f41937cf210cdebd42783c9dc8c8c723.
Confirmed working, see https://workbench.qr1hi.arvadosapi.com/container_requests/qr1hi-xvhdp-w4xk395ewnppx07#Log
Updated by Tom Morris over 4 years ago
- Status changed from Resolved to In Progress
- Target version changed from 2019-11-06 Sprint to 2019-11-20 Sprint
The output in the collection that Ward pointed to isn't compatible with the previous format and won't work with any of the existing cost scripts. The key naming, structure, and units have all changed.
New:
{
Name: "Standard_D1_v2",
ProviderType: "Standard_D1_v2",
VCPUs: 1,
RAM: 3584000000,
Scratch: 50000000000,
IncludedScratch: 50000000000,
AddedScratch: 0,
Price: 0.057,
Preemptible: false
}
Original:
{
"properties": {
"cloud_node": {
"price": 0.192,
"size": "Standard_D4s_v3"
},
"total_cpu_cores": 4,
"total_ram_mb": 16018,
"total_scratch_mb": 32747
},
}
Updated by Ward Vandewege over 4 years ago
- Status changed from In Progress to Resolved
Tom Morris wrote:
The output in the collection that Ward pointed to isn't compatible with the previous format and won't work with any of the existing cost scripts. The key naming, structure, and units have all changed.
Yes, that's by design. The old format was simply a dump of our (idiosynchratic) arvados node object.
We don't use those objects anymore with a-d-c. The format was undocumented anyway.
The new format is much more straightforward and includes more information, which was not available before. We only had internal tools consuming it anyway, so this is a good cleanup.
Oh, and TomC did check with the reviewer (me) about this before changing the format. Resolving this ticket again.