tumblr / collins

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

Add IPMI pools to /api/address/pools endpoint #521

Closed michaeljs1990 closed 7 years ago

michaeljs1990 commented 7 years ago

This adds support for programmatically discovering the currently configured pools. If no pools are configured and you have one top level IPMI pool nothing is returned such as with this config.

 ipmi {
   randomUsername = false
   username = "root"
   passwordLength = 16
   network="172.16.32.0/20"
   startAddress="172.16.32.20"
}

I believe this is the right action since you likely don't care about ipmi pools if you have not configured any and makes handling edge cases easier. Such as if you had pools configured as well as having a network set in the top level of the ipmi config. Also any default name we would give the top level pool could potentially be overwritten by a user leading to some strange cases.

Closes #515

todo

@byxorna

byxorna commented 7 years ago

When #513 lands, this will need to be rebased, as I split out the dev_base config from the test config.

I was thinking having a separate API endpoint for IPMI pools would make more sense, because the semantics of how IPMI pools and IP pools are reported/configured/tracked/updated are different. If we are only reporting the IPMI pools as configured in the play config, I would prefer them to be handled by another endpoint with a singular purpose :)

michaeljs1990 commented 7 years ago

That sounds good to me, i'll split this out into another endpoint. Do you have any preference on the name?

byxorna commented 7 years ago

Not really sure. Perhaps similar to https://tumblr.github.io/collins/api.html#api-ipam-get-address-pools, GET /api/ipmi/pools? Perhaps there can be some params to that endpoint to include/omit default pool, or include/omit actual IPs grouped by pools, and hopefully some way to indicate non-managed IP pools? (maybe the last thing is a huge timesink; maybe just stub out a param to show unmanaged IPMI network utilization and we can implement it later?)

byxorna commented 7 years ago

@michaeljs1990 this looks pretty awesome. It needs a) doc updates, and b) updates to the ruby client and collins-shell to make this accessible.

michaeljs1990 commented 7 years ago

Added collins shell support in the form of...

$ ./bin/collins-shell ipmi pools
Pool       Network         Gateway      Broadcast      Possible Addresses  Start Address  Specified Gateway
OOB-POD01  172.16.32.0/20  172.16.32.1  172.16.47.255                4094  172.16.32.20   Unspecified
michaeljs1990 commented 7 years ago

didn't bump the version since that would cause a conflict with the diff you are about to land...

edit: oops i accidentally did bump the version when testing but will update after #513 lands

michaeljs1990 commented 7 years ago

Thanks for suffering through me learning some Scala 😆 The last rebase I did just fixes a lint error where i had a backet off by one space in test/collins/controllers/IpamApiSpec.scala

byxorna commented 7 years ago

@michaeljs1990 looks like my changes i landed created merge conflicts :( soz!

michaeljs1990 commented 7 years ago

Thats ok :) was expecting it. Going to test locally github gave me a cool GUI conflict resolver that I had to try out.

I believe after that is done though someone else will have to land this as even before the conflict the only option I had was to close the PR.

michaeljs1990 commented 7 years ago

hmmm, something broke with how the pool name is determined. Looking into it right now... yeah for unit tests.

michaeljs1990 commented 7 years ago

Ok, this should be all set for landing 🔥 🔥

byxorna commented 7 years ago

@michaeljs1990 dope, thanks! Landing now. cc @roymarantz @defect

michaeljs1990 commented 7 years ago

awww yis! thanks for landing!