tumblr / collins

groovy kind of love
tumblr.github.com/collins
Apache License 2.0
573 stars 99 forks source link

Provisioning profiles endpoint should expose extra data #410

Open byxorna opened 8 years ago

byxorna commented 8 years ago

It is a common pattern to locate extra data with the provisioning profiles in profiles.yaml. For example, it is reasonable to have a profiles.yaml contain extra fields like what OS to provision, what VLAN the profile requires, what style of disk partitioning should be used, etc:


---
profiles:
    searchnode:
        label: "Search Server"
        prefix: "search"
        allow_suffix: true
        primary_role: "SEARCH"
        pool: "SEARCH_POOL"
        secondary_role: "MASTER"
        requires_primary_role: false
        requires_secondary_role: true
        requires_pool: false
        cobbler_profile: "centos-5.6-x86_64-db-v2"
        operating_system: "centos_5"
        vlans: ["search"]
        disks: hw_raid_10

When hitting the /api/provision/profiles endpoint, it omits all these juicy extra fields, which is very disappointing. Ideally these extra attributes could be made available to API consumers like provisioning systems to make appropriate decisions about how to do the needful. cc @Primer42 @defect @schallert @roymarantz

The data currently returned looks like:

#<Collins::Profile:0x007fdd1204d700 @options={:profile=>"devnode", :label=>"Dev Node", :prefix=>"dev", :suffix_allowed=>true, :primary_role=>"DEVELOPMENT", :requires_primary_role=>true, :pool=>"DEVELOPMENT", :requires_pool=>true, :secondary_role=>nil, :requires_secondary_role=>false}>

Perhaps there could be an extra field with extras: in it, and just dump json of all the other bits there?

defect commented 8 years ago

I agree, and it's something that's been bothering me as well. I took a quick look at the yaml parsing and while it would be nice to just take "left over" keys and collect them under an extras key in the json i think that might be tricky without doing a lot of snakeyaml magic.

What i think might be easier (i haven't tried it though) would be to make extras an "official" key and just keep the data there instead of under arbitrary top-level keys. I'll try to get a PoC going when i have some time over to show what i mean.

roymarantz commented 8 years ago

I'm wondering why extras is visible at all. To me it is an implementation detail that should be hidden and the "model" should be a flat key space. I know there can be (and IIRC is) duplicate keys, but I consider that a bug.

byxorna commented 8 years ago

@defect I agree on the extras key. Seems easiest. Only question I have is whether or not that field can be raw json data (i.e. arbitrarily nested objects) or if it has to be deserialized to a java object like Hash[String,String]. The API consumers are working with straight json, so id like to not remove any expressivity from the extras blob if possible. I was going to start knocking together a prototype as well :)

@roymarantz the extras is for other systems, not collins. the other systems are the implementation details of the provisioning systems.

roymarantz commented 8 years ago

@byxorna I guess that depends on what you mean by collins. I like to think of it as a KV store which supports provisioning, but not really part of the provisioning system. YMMV

defect commented 8 years ago

@byxorna I messed around a bit with it the other day, and having arbitrary object as the key seems less than trivial. I think we'll need to support at least lists and maps in the extras, but preferably just arbitrarily nested structures.

byxorna commented 8 years ago

@defect same with me. It looks to be really annoying, because the yaml is deserialized into a JProvisionerProfile. I couldnt find a way to mark a field as unparsed (ala json.RawMessage in go).

Heres a crazy thought: We could change the endpoint and instead of unmarshalling/remarshalling, we could just serve the yaml file straight away. It does't look like snakeyaml supports partial decoding on unknown fields, so thats one possibility. Another would be to not touch the existing JProfile nonsense, and add an additional YAML.load() step in the API action that would reparse the profiles.yaml and join in the extra data with the JProfile json? Or rewrite collins in a language that doesnt make this like punching yourself in the face.