Manageiq: retirement_warn datatype set from dialog in service retirement isn't what we're testing

[d-m-u]

Since https://github.com/ManageIQ/manageiq/pull/16799 we've been able to set the retirement_warn field of a service from one of the dialog fields (with appropriate name). The issue is that retirement_warn is a bigint. Previous to 16799 this was fine, because the only values the UI allowed you to set retirement_warn were in number of days. But 16799 increased the granularity of the retirement time so there are now two issues: 1) any existing specs for retirement_warn (see https://github.com/ManageIQ/manageiq/pull/16799/files#diff-07af4c1ba33a7729ae73c31d62f4315dR97) shouldn't be expecting a datetime, they should be expecting an integer value (similar to how calling retirement_warn from the UI sets), and 2) it doesn't make any sense to be storing some number of days or hours in that field without having any idea of if we're talking about hours or days. At the moment there's nothing differentiating the units which seems bad.

If we retain the bigint that means we need to convert everything to hours, since hours (at the moment) are the smallest unit we're dealing with right now. The other approach is changing the type of the field to be a datetime but this got back to fine so that's gonna be interesting.

https://bugzilla.redhat.com/show_bug.cgi?id=1695219 is for this.

[d-m-u]

@eclarizio I was wondering if you could take a look at this and tell me your thoughts please.
@tinaafitz boo.

[eclarizio]

Do we have any other precedent for storing some kind of time based field as an int of "number of hours away" or "number of days" away from another time? I feel like the simplest solution (though probably the most difficult to push through depending on where we are in the cycle and/or what version needs it backported) would be the schema change.

[d-m-u]

@gmcculloug

[jrafanie]

Wow, this stinks. We definitely need to have retirement_warn be a datetime on master and have all the code expect this to be a datetime. For backport, we can't change the column on h/g/f so I can only think of one way:

On h/g/f:

• override retirement_warn= to accept a datetime and convert/persist as a bigint based on the retirement date.
• override retirement_warn to read the bigint in the db and convert/return a datetime based on the retirement date or the current time. The ability to have this value be relative to today vs. the retirement date could make this very hard. 🤷‍♂️

I don't know if that covers everything but it would interesting to see if that's even feasible.

[d-m-u]

retirement_warn is also an int on vms and orch_stacks and load balancers, which don't matter as much as services, but I feel like the field should be the same type across the four

[jrafanie]

retirement_warn is also an int on vms and orch_stacks, which don't matter as much as services, but I feel like the field should be the same type across the three

😢😢😢 😭 😭 😭

🏃 🏃 🏃 🏃 🏃 🏃 💣

[tinaafitz]

@d-m-u @jrafanie @gmcculloug Retirement_warn has always been used for the number of days relative to the retirement date. I would hate to change the behavior without checking with the field since they're likely using it in retirement emails. I'm going to ask Peter about it.

[pemcg]

Up to now we've always defined the retirement dates as a retirement _date_ and a number of _days_ warning or extension. This fits in with the API description: http://manageiq.org/docs/reference/fine/api/reference/services#service-retiring-future
and the formal and informal docs, e.g. http://talk.manageiq.org/t/define-time-until-retirement/1926 & https://cloudformsblog.redhat.com/2016/09/20/notify-vm-owner-of-upcoming-retirement/

I think that changing this behaviour would cause confusion and risk breaking existing code.

Peter

[tinaafitz]

Thanks @pemcg, I agree that we should leave the retirement_warn as the number of days.
@gmcculloug Are you okay with this?

[jrafanie]

What about the issue of storing hours in this field? I'm not sure of the details but I read it was we were storing either days or hours.

[jrafanie]

If we store only days, it's fine but we can't store both hours and days without it being really confusing.

If we need both ways, storing hours makes sense since you can easily convert days to hours.

[tinaafitz]

@jrafanie I think a recent code change might be optionally calculating based on hours, but I think we need to change the code back to days. I'm checking with @gmcculloug to make sure he's okay with that.

[d-m-u]

Fixed by 18637, see previous.
The original PR got backported to fine, will deal with that in a minute.