Add IPMI pools to /api/address/pools endpoint #521
Conversation
| IpmiInfo.AddressConfig.map(_.poolNames).getOrElse(Set()) | ||
| } else { | ||
| // TODO: Change this to only return active pools? | ||
| IpmiInfo.AddressConfig.map(_.poolNames).getOrElse(Set()) |
There was a problem hiding this comment.
This is the one thing I don't like about this currently. How IpAddress pools are handing this is just querying the DB and returning all unique fields. Since this is not in the DB at all for IPMI pools we would have to run a very expensive query to figure out what all the active pools are. The other option is writing a migration to go with an evolution which I believe is the correct path forward.
There was a problem hiding this comment.
I prefer reading the configured pools from the config for IPMI tbh. However, we would need to take into account the fact that IPMI addresses can be "loose", and not adherent to any pool in the config.
I am open to either, however an evolution will definitely complicate things. I also think if we track pool with an ipmi_info entry, we will need to update it when people hit /api/asset/:tag/ipmi to create a new address or update existing...
michaeljs1990
changed the title
|
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 :) |
|
That sounds good to me, i'll split this out into another endpoint. Do you have any preference on the name? |
|
Not really sure. Perhaps similar to https://tumblr.github.io/collins/api.html#api-ipam-get-address-pools, |
| @@ -85,4 +88,8 @@ trait IpmiApi { | |||
| ) | |||
| }(Permissions.IpmiApi.UpdateIpmi) | |||
|
|
|||
| // GET /api/ipmi/pools | |||
| def getIpmiAddressPools(all: String) = | |||
| GetPoolsAction(Truthy(all), Permissions.IpAddressApi.UpdateAddress, this) | |||
There was a problem hiding this comment.
this permission seems incorrect. We should use a separate "ipmi.get address pools" perm, and perhaps a separate one for ipmi network view from normal address pool view?
| format(pools) | ||
| val ipmiPools: Set[String] = | ||
| if (all) { | ||
| IpmiInfo.AddressConfig.map(_.poolNames).getOrElse(Set()) |
There was a problem hiding this comment.
Check that this handles mapping the default pool to the proper name correctly? I.e. if you have a config that uses an implicit network (not using the pools = {...} syntax)
Also, does this stuff need to be reverted now that you have a separate endpoint for ipmi pools?
| case ActionDataHolder(all) => | ||
| val pools: Set[String] = | ||
| if (all) { | ||
| IpmiInfo.AddressConfig.map(_.poolNames).getOrElse(Set()) |
There was a problem hiding this comment.
both arms of the conditional seem the same. Whats the purpose of the all param then?
| } | ||
| ) | ||
|
|
||
| protected def formatNetworkAddress(network: String): String = network match { |
There was a problem hiding this comment.
would be cool to refactor this to return an Option instead of a string error message, and do the user-view formatting at a higher level with flatMap or something to handle the nones
|
|
||
| override def validate(): Validation = { | ||
| var pools: Set[String] = { | ||
| val p = IpmiInfo.AddressConfig.filter(_.poolNames != Set()) |
There was a problem hiding this comment.
could probably be _.poolNames.isNonEmpty or something
There was a problem hiding this comment.
This is much nicer. I thought i tried this originally but I must have had some other error popping up the first time :(
| override def validate(): Validation = { | ||
| var pools: Set[String] = { | ||
| val p = IpmiInfo.AddressConfig.filter(_.poolNames != Set()) | ||
| if (p.isDefined) { |
There was a problem hiding this comment.
i think this could be more idiomatic like
IpmiInfo.AddressConfig.filter(_.poolNames.isNonEmpty) match {
case Set() => Set(IpmiInfo.AddressConfig.flatMap(_.defaultPoolName).getOrElse(""))
case _ => IpmiInfo.AddressConfig.map(_.poolNames).getOrElse(Set(""))
}
``` (syntax may be screwed up, but you get the drift)
Just a suggestion, feel free to disregard
There was a problem hiding this comment.
I am a pattern matching noob but this does make things look way nicer.
| if (p.isDefined) { | ||
| IpmiInfo.AddressConfig.map(_.poolNames).getOrElse(Set("")) | ||
| } else { | ||
| Set(IpmiInfo.AddressConfig.flatMap(_.defaultPoolName).getOrElse("")) |
There was a problem hiding this comment.
So, if you do not specify any pools to pull back, it only returns the default pool? What is the deal iwht the getOrElse?
There was a problem hiding this comment.
Yeah if no pools are defined in the configuration it trys to pull back the DEFAULT pool. The get or else was just because this returns an option and I felt dirty just using get.
| args(sequential = true) | ||
|
|
||
| "the REST API" should { | ||
| "Support getting ipmi pools" in new WithApplication with AssetApiHelper { |
There was a problem hiding this comment.
should probably test with and without the optional parameters?
There was a problem hiding this comment.
yes that would be best, I'll see if i can figure out how to switch out or change the configuration file it's reading from so both cases can be tested.
There was a problem hiding this comment.
The WithApplication can use something like FakeApplication and provide individual parameters? However, ive not had great experience overriding a config this way (i gave up overriding the IPAM pool configs :()
|
@michaeljs1990 this looks pretty awesome. It needs a) doc updates, and b) updates to the ruby client and collins-shell to make this accessible. |
|
Added collins shell support in the form of... |
|
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 |
This adds support for IPMI pools to be returned from the /api/ipmi/pools API endpoint. See comments above /ipmi/GetPoolsAction toAddressPools method for a better understanding of how this works. Also added a simple unit test based on the dev config.
|
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 |
|
@michaeljs1990 looks like my changes i landed created merge conflicts :( soz! |
|
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. |
|
hmmm, something broke with how the pool name is determined. Looking into it right now... yeah for unit tests. |
update the test config file to match what the unit test is expecting.
|
Ok, this should be all set for landing 🔥 🔥 |
|
@michaeljs1990 dope, thanks! Landing now. cc @roymarantz @defect |
|
awww yis! thanks for landing! |
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.
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