16:02:56 <priteau> #startmeeting blazar
16:02:57 <openstack> Meeting started Thu Feb 25 16:02:56 2021 UTC and is due to finish in 60 minutes.  The chair is priteau. Information about MeetBot at http://wiki.debian.org/MeetBot.
16:02:58 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
16:03:00 <openstack> The meeting name has been set to 'blazar'
16:03:21 <priteau> #topic Roll call
16:04:19 <diurnalist> o7
16:04:47 <diurnalist> hi priteau
16:04:55 <priteau> Hello diurnalist
16:06:31 <priteau> #topic Usage enforcement patch
16:06:53 <priteau> So I've been looking at this patch
16:07:00 <priteau> Let me pull the link
16:07:09 <priteau> https://review.opendev.org/c/openstack/blazar/+/736993
16:08:04 <priteau> There's something I don't quite understand, which is the semantics of the length limit window
16:08:14 <priteau> The code is at https://review.opendev.org/c/openstack/blazar/+/736993/7/blazar/enforcement/filters/max_lease_duration_filter.py
16:08:47 <priteau> The help code says:
16:09:10 <priteau> Gives users a window towards the end of a lease to extend it again for the max lease duration. A value of -1 will allow users to extend leases beyond the maximum lease duration.
16:09:35 <diurnalist> yeah, I see what you mean now. that clearly doesn't work w/ this code
16:09:43 <priteau> I understand what is intended for the first part, e.g. allow extension only for the last 24 hours
16:09:55 <priteau> But I really don't see what was the goal for this -1 value
16:10:09 <priteau> Additionally, I don't see where in the code -1 is handled
16:10:26 <diurnalist> this is simply a bug, I think. we need an additional check for when the value is -1 (or 0, whichever we want to do). i think the intent was if it's -1 then the check always passes. but in this case, it stands to reason you could simply remove the filter entirely from the chain
16:11:33 <priteau> Well, there is the -1 for the max_lease_duration option
16:11:40 <priteau> That's fine, it means unlimited
16:11:58 <priteau> It's having a special case for the window that is strange
16:12:20 <priteau> My suggestion would be to remove that part of the code for now. We can put it back as a separate patch
16:12:53 <diurnalist> oh yeah, hah. yeah, i'm not sure what is up with having it work "beyond the maximum length window"
16:13:05 <priteau> I've got a local patch with unit tests for this filter
16:13:16 <diurnalist> i'd say, a value of -1 would maybe mean that you can extend the lease at _any_ point during the lease?
16:13:49 <priteau> I suppose so, that's would be the default behaviour
16:14:16 <diurnalist> that's what it does, effectively. maybe just the help text needs updating. although there is also an 'if' guarding that behavior entirely, so it would seem that 0 is really the value that does this
16:14:18 <priteau> And 0 would prevent extending it, which might be a feature?
16:14:42 <diurnalist> ah, good point, turning it off might be desirable
16:15:09 <priteau> Although it gets more complicated if you think about leases that were not created at the max to start with
16:15:21 <priteau> Say the length limit is 7 days
16:15:28 <priteau> I create a 1 day lease
16:15:45 <priteau> I should be able to extend it to 7 days no matter what is the value of the extension window
16:17:34 <diurnalist> up until that 7 days, you mean?
16:17:50 <diurnalist> that makes sense
16:18:20 <priteau> yes, up until it is 7 day long
16:19:04 <priteau> And, even beyond really, because:
16:19:15 <priteau> For active leases being updated, the limit applies between now and the new end date.
16:19:39 <priteau> So, if you extend after 0.5 day of activity, you're suppose to be able to reach 7.5
16:20:10 <diurnalist> i'm not sure of the semantics of -1 vs 0, either one can be used. so maybe it makes sense to have the rules like (a) if window is set, lease can be extended (for another period of <=max_len) within window of lease end. (b) if window is 0, lease can be extended at any time for another period of <=max_len. (c) window only applies to requests to extend past original start_date+max_len
16:22:55 <priteau> Why don't we create an Etherpad with a few examples and we agree on the semantics?
16:22:58 <diurnalist> ok
16:23:10 <priteau> Is it a filter that you would use in Chameleon?
16:27:15 <diurnalist> priteau: yes, we currently use it
16:27:21 <diurnalist> in combination w/ the external service filter
16:29:02 <priteau> So we may as well make it match your semantics
16:31:09 <priteau> It's quite a big patch, I've been thinking about how to make it easier to review
16:31:29 <priteau> I can see a few separate units
16:31:51 <priteau> 1) some refactoring that is independent of the implementation itself
16:31:57 <priteau> (or bug fixes)
16:32:25 <priteau> 2) refactor of the allocation query methods and its addition to all plugins
16:32:36 <priteau> 3) the common layer for enforcement
16:32:45 <priteau> 4) max lease duration
16:32:50 <priteau> 5) external service
16:33:23 <priteau> I'll see if splitting is more work than actually reviewing it all in one go
16:33:48 <priteau> I've also noticed the patch has a dependency on the soft delete functionality
16:33:58 <priteau> So I am thinking of reviving it first
16:34:36 <priteau> There were other things that I wanted to mention…
16:35:52 <priteau> For the external service, have you tested what happens if it is unreachable?
16:37:32 <priteau> And should we have timeouts?
16:38:02 <priteau> For example, if it accepts queries but without replying, could it block Blazar?
16:38:04 <diurnalist> It should fail closed, as in, proceed w/o blocking the lease. i don't believe there are configurable timeouts on it. maybe there are some common oslo options we could leverage for the client that gcalls the service
16:38:48 <diurnalist> i didn't think about it depending on soft-delete. does it really?
16:39:20 <diurnalist> i know there were some refactorings as part of (1) that were around lease deletion but i recall it was more about deleing events vs. leases
16:39:31 <priteau> Only as a hidden dependency, not explicitly
16:39:51 <priteau> For example here: https://review.opendev.org/c/openstack/blazar/+/736993/7/blazar/db/sqlalchemy/utils.py#111
16:40:06 <priteau> And also in the long standing bug you mention in the commit message
16:40:33 <priteau> In upstream Blazar, events are deleted with the lease so it doesn't matter that the state isn't changed
16:42:42 <diurnalist> ah, i see
16:42:56 <diurnalist> so it's only a long-standing bug in combo w/ soft-delete
16:43:15 <priteau> yes
16:43:25 <diurnalist> right, that deleted_at filter :facepalm:. do we have a patch for soft-delete already in there?
16:45:09 <priteau> An old one
16:45:09 <priteau> https://review.opendev.org/c/openstack/blazar/+/585807
16:45:09 <priteau> Outdated since it doesn't cover floating IPs
16:45:35 <diurnalist> yep, ok
16:46:11 <priteau> The main issue with it was around handling compute hosts that would be recreated
16:46:33 <priteau> Because their primary key is the Nova hypervisor ID
16:47:09 <priteau> So I think we wanted to un-soft-delete them
16:47:35 <priteau> The other comment was about handling DB purge, but we can worry about it in another patch
16:48:01 <priteau> If you have a newer local patch for this, feel free to upload on top
16:48:07 <diurnalist> ok
16:48:31 <diurnalist> i think i may actually. at some point i was doing some more git rewriting to try to push more of these patches down so they don't all depend on eachother so much
16:50:00 <priteau> The more we get merged, the easier it will be
16:50:31 <priteau> Are you using the external service for enforcement in Chameleon or still the build-in code I wrote ages ago?
16:53:23 <diurnalist> we're using the external service
16:54:52 <priteau> Great! Is the code open?
16:56:42 <diurnalist> for the external service itself, or the plugin? i thought the plugin was part of that gerrit patch
16:57:18 <priteau> For the external service itself
16:57:53 <diurnalist> it is not. though, i'm not sure there's a great reason for why
16:58:01 <diurnalist> i think you would have access though, I'll ping you a link
16:58:59 <priteau> Thanks
16:59:33 <priteau> Last question, do you think the term "filter" is appropriate? It's not quite like a Nova filter which lets some hosts pass but not all
17:00:19 <priteau> They're really policy modules
17:00:28 <priteau> Though it's not a great name either
17:00:40 <diurnalist> true, it's not really a filter in the strict sense
17:00:59 <diurnalist> more like a chain or something
17:01:34 <priteau> Something we can think about until next time
17:01:42 <priteau> We're out of time already
17:01:49 <priteau> Thanks Jason, useful chat
17:01:53 <priteau> #endmeeting