Dynamic Metadata

[epipho]

Updated 2/28/2015

• Added new PATCH method to the machines collection endpoint to allow changing a machines metadata via the http endpoint.
• API uses the jsonpatch format. Multiple machines can be modified at once, including machines that are not currently part of the cluster. "add", "replace", and "remove" operations are available.
• Metadata modified via the api is retained even if the machine leaves and rejoins the cluster.
• Dynamic metadata (metadata set by the user via the api) is merged with Machine metadata (metadata defined by the fleet config, env variables, or flags) using the following rules
  • Any keys that exist in only one of the two collections are added to the final collection as-is
  • Any keys that exist in both, the value set in the dynamic metadata is added to the final collection.
  • Any keys that are the string zero-value in the dynamic metadata are considered deleted and are not included in the final collection. This lets a user persistently delete a value set at configuration time, even if the machine leaves and rejoins the cluster.
• The reconcile will reschedule any units on machines that no longer meet the metadata requirements (pre-existing functionality)

Known Issues:

• Docs not complete

[epipho]

TODO:

• Change TTL to int (seconds) instead of time.Duration (ns)
• Tests tests tests tests

[epipho]

I think this is ready for the first pass of reviews.

[bcwaldon]

I haven't looked too far into the actual code, but I wanted to share some high-level feedback since this is likely to change some things around:

I'd like to see the PATCH endpoint implemented using JSON patch [0] rather than a custom request format. Additionally, we should consider implementing the PATCH at the root of the machines collection rather than per-machine.

What utility does the TTL on a specific key provide?

[0] http://jsonpatch.com/

[epipho]

Switching to JSONPatch is easy enough as is moving the PATCH at the root level to allow multiple machines to be updated at once. I'll have the changes made in a couple of days.

The original idea was to use it as kind of a health check, a process could check a node to see if it was suitable for a specific type of job and set the TTL'ed metadata. Furthur thoughts make it seem like the TTL idea is half baked / gold plating and maybe doesn't deserve to live, particularly with the new request format.

[bcwaldon]

@epipho I do like the idea, but I'm not sure if we should try to squeeze it in right now.

[epipho]

@bcwaldon

• Updated to use jsonpatch on the machines collection level. Metadata is now updated with the path format /machineid/metadata/key using the add/remove/replace ops.

[{"op": "add", "path": "/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa/metadata/foo", "value": "bar"},
     {"op": "replace", "path": "/bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb/metadata/ping", "value": "splat"}]

• TTL feature has been stashed away for another day
• Error handling: Currently the handler does as much as it can to validate the patch data before doing any work. Invalid ops, bad path format, or missing values will cause the request to fail with no effect on the data. If an error occurs during the apply phase, the request terminates immediately and returns the error. Because the changes cannot be made atomic as the metadata keys are split apart this could (rarely) result in a partial patch being applied. Any suggestions on how to handle this or what information should be returned to the caller when this happens?

[epipho]

Removing WIP. Ready for initial review and comments.

[umiller]

How do I test the dynamic metadata?
I am looking for a safe way to change the CoreOs machine metadata in run-time
Is this feature is the only way? (I blocked CoreOs updates on my cluster..)

[bcwaldon]

Let's call this SetMachineMetadata, since Update can mean the key must already exist in some contexts.

And there's no need to respecify string three times, just use SetMachineMetadata(machID, key, value string)

[bcwaldon]

@epipho In addition to the inline comments, I have a concern about how the metadata is initially making it into etcd. Machines leave and rejoin the cluster frequently, so metadata that was manually added by someone through etcd will get blown away. I wonder if the metadata published by a given machine should be managed independently from the metadata added through etcd? I think it makes sense for metadata manually added through etcd directly should persist until it is manually removed.

[epipho]

@bcwaldon Thank you for taking the time to do a very thorough review. I will address all the small stuff this weekend.

As for your question above:

Wouldn't that be true today since a machine leaving the cluster removes the /machineid/object value that currently holds the metadata?

I'm not sure how to best handle your request, I outlined a few possibilities below.

1. Metadata that is configured via fleet config (file, env variable, cloud config) is automatic, removed by machine leave, and cannot be manipulated at all. All other metdata values are untouched.

2. Metadata configured by fleet config can be manipulated, but will be reset by the machine rejoining. All metadata values are untouched when a machine leaves.

3. Metadata configured by fleet config is only added if the machine does not exist in the registry at all. All metadata values are untouched when a machine leaves.

4. Same as 3, but all metadata is removed when a machine leaves (as currently written).

I worry that separating the metadata will be confusing as some will be able to be manipulated via the api and some will not. Of all the suggestions outlined I like 3, treating the configured values as defaults if the machine has never been seen before but leaving it untouched otherwise.

[bcwaldon]

@epipho It's not clear to me how it should work, so I'd like to get an opinion from @jonboulle on this.

[Telvary]

+1 !

[rushmorem]

@epipho @bcwaldon @jonboulle What's the status on this?

[chr0n1x]

bump

[SamuelMarks]

👍

[dalbani]

It has been a while since this issue has seen some activity.
This feature would be of great use!

[dalbani]

Hi @epipho, I've been running your code for a month now and it works just fine. Thanks for your contribution!
I was wondering where I could find the code where you implemented the support for TTL?

[dalbani]

There's one point I was wondering about lately: the inability to set a property with an empty value.
For example, sending a PATCH request as:
[ { "op": "add", "path": "/e7afc665c2dc4a4b82082cd654fc6555/metadata/test", "value": "" } ]
Returns:
{"error":{"code":400,"message":"invalid value: add and replace require a value"}}

Aren't empty properties valid if configured via /etc/fleet/fleet.conf?

[dongsupark]

Just a short note:
My thought about this PR is simply, "sure, why not?".
It sounds good in general. With code reviews and writing additional tests, we might merge it. I don't think anyone is against this one.
But when I tried to rebase it on top of current master, I somehow gave up. It's a little messy. Especially the newline characters at the end of every line in the document... OMG.
If someone could rebase it for me, I would really appreciate, be happy to review the code, and test it too.

[dalbani]

I can definitely rebase the code for a start.

[dalbani]

Well, easier said than done, not being a GitHub expert...
I have imported this PR's code into a branch in my own repo, and made it up to date with master: https://github.com/dalbani/fleet/tree/dynamic-metadata
Am I supposed to submit a new pull request then?

[dongsupark]

@dalbani Yes, please create a new PR. That would be appropriate, as the original author doesn't seem to be active any more. I'll try to review & test that next week.
Actually what I wanted was rebasing this branch on top of master, not merging master into the branch. Anyway let's discuss about that, looking into the new PR.

[dalbani]

I have rebased on top of master, instead of merging, and created the new PR #1642.

[dongsupark]

Closed in favor of #1642. Thanks.