16:00:45 <TheJulia> #startmeeting ironic_bfv
16:00:46 <openstack> Meeting started Thu May 18 16:00:45 2017 UTC and is due to finish in 60 minutes.  The chair is TheJulia. Information about MeetBot at http://wiki.debian.org/MeetBot.
16:00:48 <openstack> Useful Commands: #action #agreed #help #info #idea #link #topic #startvote.
16:00:50 <openstack> The meeting name has been set to 'ironic_bfv'
16:00:59 <derekh> o/
16:01:04 <hshiina> o/
16:01:16 <TheJulia> Our agenda, as always https://wiki.openstack.org/wiki/Meetings/Ironic-BFV
16:01:21 <TheJulia> #link https://wiki.openstack.org/wiki/Meetings/Ironic-BFV
16:01:36 <mjturek> o/
16:01:47 <TheJulia> #topic Announcements/Reminders
16:02:13 <TheJulia> The only thing that I really have is that I want to apologize for being insanely busy the last few weeks.
16:02:31 <mjturek> no worries TheJulia
16:03:07 <TheJulia> Anyone have any announcements?
16:03:08 <hshiina> TheJulia, never mind
16:03:40 <mjturek> cinder driver is close to merging
16:03:43 <TheJulia> \o/
16:03:45 <mjturek> we might need to do a follow up
16:03:56 <mjturek> thanks for all your help hshiina and TheJulia
16:04:17 <TheJulia> I saw, I noticed the boolean != none comments.  Thank you hshiina!
16:04:32 <hshiina> mjturek, TheJulia you're welcome
16:04:47 <TheJulia> Well, I guess we are safe to move on!
16:04:56 <dtantsur> I'd really, really like the patches to be smaller than this one
16:05:07 <dtantsur> it's hard to review and it has A LOT of unit tests
16:05:22 <mjturek> yeaaah understandable
16:05:28 <TheJulia> #topic Current Status
16:05:42 <TheJulia> Yeah, the tests are kind of... long. :(
16:06:29 <TheJulia> I think that one is the worst patch length wise
16:06:33 <TheJulia> FWIW
16:06:51 <TheJulia> #link https://etherpad.openstack.org/p/Ironic-BFV
16:06:53 <mjturek> there's a lot of redundancy in setup for each test, might be able to reduce that if it's something people want
16:07:07 <mjturek> but might not be worth it at this point
16:07:29 <TheJulia> Perhaps a follow-up might be good for that...
16:07:32 <dtantsur> yeah, let's finally approve it and move on :)
16:07:37 <mjturek> cool cool :)
16:07:40 <dtantsur> I won't survive reviewing it again :)
16:07:43 <mjturek> hahaha
16:07:55 <TheJulia> Looking through the statuses, looks like it is up to date
16:09:06 <TheJulia> It looks like some rebasing needs to be performed, and I need to update the deploy skip patch, I'll likely get to that while I'm in the air tomorrow.
16:09:31 <TheJulia> Sorry I didn't get to it this week :(
16:10:32 <TheJulia> I guess aside from getting the current rev landed and moving on to the next one, things look okay
16:11:53 <TheJulia> Minor correction, there is -1 on a prior rev of https://review.openstack.org/#/c/413324/ that needs to be looked at and addressed.  It was since rebased but not updated.
16:11:56 <TheJulia> I've updated the ether pad.
16:12:17 <TheJulia> Moving on...
16:12:19 <mjturek> Yuriy's?
16:12:19 <TheJulia> #topic Planning/Priorities
16:12:23 <TheJulia> mjturek: yes
16:12:27 <mjturek> ok cool
16:13:18 <TheJulia> Next up is https://review.openstack.org/#/c/406290/
16:13:24 <TheJulia> I just clicked the rebase button
16:13:33 <TheJulia> (and it worked \o/)
16:13:42 <mjturek> yaaay
16:13:45 <TheJulia> #info Next patch is https://review.openstack.org/#/c/406290/
16:14:14 * dtantsur wonders who we can drag into reviewing the cinder driver patch
16:14:52 <TheJulia> dtantsur: technically I shouldn't, but the code has morphed quite a bit from the first rev.  I guess I can at least give it a thumbs up from my point of view
16:15:03 <dtantsur> fair enough
16:15:46 <TheJulia> I'll look through the ones I've not touched recently that Joanna worked on and comment as such in the comments.
16:16:13 <hshiina> cinder driver patch depends on it: https://review.openstack.org/#/c/460250/
16:16:21 <hshiina> it also needs review
16:16:21 <mjturek> oh right
16:16:41 * dtantsur puts on his backlog
16:17:20 <TheJulia> hshiina: Thank you for raising that
16:17:40 <TheJulia> #info https://review.openstack.org/#/c/460250/ is required for the cinder driver to land.
16:18:18 <TheJulia> Beyond that, does anyone have any priorities or items that may be priorities that they wish to raise?
16:19:18 <TheJulia> hshiina: Could you update the list order in the etherpad for the revision you pointed out?  For some reason it is not working for me right now.
16:19:41 <hshiina> TheJulia, sure
16:19:45 <TheJulia> Thank you
16:20:05 <derekh> there are a few patches that could do with an update, that depend on old versions of parent patches
16:20:46 <derekh> I think this is the list and revision numbers they depend on http://paste.openstack.org/show/609943/
16:21:52 <TheJulia> derekh: Indeed, it would be a good time to rebase the outstanding ones that have few reviews right now.
16:23:01 <joanna> o/
16:23:17 <TheJulia> mjturek: I clicked the button on the storage attach/detach ops, are you going to look at the ipxe template updates?
16:23:21 <TheJulia> greetings joanna
16:23:32 <joanna> :)
16:23:46 <mjturek> TheJulia: sure, I'll take a look
16:24:35 <TheJulia> mjturek: Awesome.  I'll take care of the WIP one in the next few days and hopefully parent wise we should be in a fairly good position aside from client revisions
16:25:01 <TheJulia> Anyway, I think we've covered priorities and plans for the next week or so, If there is nothing else anyone has to raise, then we can move on to discussion.
16:25:09 <mjturek> sounds good
16:25:46 <TheJulia> #topic Discussion
16:25:59 <TheJulia> mjturek: You have two items it looks like.
16:26:09 <mjturek> so dtantsur raised two good questions I'd like to discuss
16:26:30 * mjturek grabs the first one
16:26:39 <mjturek> https://review.openstack.org/#/c/366197/45/ironic/drivers/modules/storage/cinder.py@210
16:26:47 <mjturek> #link https://review.openstack.org/#/c/366197/45/ironic/drivers/modules/storage/cinder.py@210
16:27:33 <mjturek> it seems that non-BFV deployment might be clobbered if no targets are defined in this case
16:28:25 <mjturek> should we be checking if no targets are defined and another storage interface is available?
16:28:30 <TheJulia> So... validate would need to be called by nova
16:29:02 <TheJulia> or the deployment (I think) (it is getting a little fuzzy at this point)
16:29:12 <TheJulia> but it is a good point, they shouldn't be clobbered if it is not explicitly bfv
16:29:27 <mjturek> cool
16:29:41 <dtantsur> I'm just checking that it's intended if storage_interface=cinder and iscsi_boot=True, then a node fails storage validation if used without connectors
16:29:51 <TheJulia> I think it needs an extra check
16:30:01 <TheJulia> for that there actually are connectors
16:30:07 <mjturek> multiple storage interfaces can be available right?
16:30:13 <TheJulia> One per node
16:30:51 <TheJulia> so yeah, I think it is just an extra conditional.
16:30:52 <mjturek> ahh, so should a node with cinder as the storage_interface be allowed to do non-BFV deployment?
16:31:01 <mjturek> I would've thought no
16:31:02 <dtantsur> yeah, this is my question ^^^
16:31:17 <dtantsur> and if the answer is "no", how do we tell nova about that?
16:31:28 <TheJulia> No, it should be possible
16:32:13 <TheJulia> We're doing whatever nova has requested, so nova does need to call validate, but at the same time validate only needs to trigger if there are actually connectors
16:32:19 <TheJulia> err
16:32:21 <TheJulia> not connectors
16:32:22 <TheJulia> targets
16:32:25 <TheJulia> it is okay to have connectors
16:32:35 <dtantsur> so what's the expected logic?
16:32:49 <dtantsur> should we just drop that check?
16:34:33 <TheJulia> Oh you know what
16:35:23 <TheJulia> That catches a general misconfiguration on the node
16:37:04 <TheJulia> sorry for the slow responses, juggling a conference call as well
16:37:33 <dtantsur> I hear you, I have an API-WG meeting in parallel (just finished though)
16:38:18 <TheJulia> If the node has the capability configured to iscsi boot, has a cinder storage interface conjured, but no connector info, then a deployment should fail.  What we should do is also add targets to be checked, I think.
16:38:34 <TheJulia> I may have broken it in two parts on purpose, I just don't remember right now
16:38:53 <dtantsur> aha, and we expect nova to set the targets, so this should not be a problem, right?
16:38:57 <TheJulia> Anyway, I'll look at and reply in depth
16:39:06 <TheJulia> dtantsur: exactly
16:39:21 <mjturek> ohhh
16:39:22 <dtantsur> ok, I think I start to understand
16:40:07 <mjturek> alright cool
16:40:37 <TheJulia> I think were in a good place there
16:40:41 <TheJulia> Next question?
16:40:51 <mjturek> https://review.openstack.org/#/c/366197/40/ironic/drivers/modules/storage/cinder.py@232
16:41:36 <mjturek> there've been a couple comments saying that _abort_attach_volume should be replaced with detach_volumes
16:41:55 <mjturek> the main difference seems to be the error handling and how many retries we do
16:42:30 <mjturek> so if we want to use action retries here, I think that's more fuel to drop the function and call self.detach_volumes instead
16:42:58 <TheJulia> I can agree with that :)
16:43:09 <mjturek> so is there any reason we would only want to retry once here?
16:43:27 <dtantsur> well, if there is no use retrying?
16:43:30 <dtantsur> like e.g. cinder is down
16:43:41 <dtantsur> not sure if it's a popular case though
16:44:32 <mjturek> alright, well in follow up I'll propose removing it
16:45:23 <mjturek> that sound good?
16:45:35 <TheJulia> Sounds good to me
16:45:48 <dtantsur> just make sure we still catch exceptions (to avoid masking the initial one)
16:45:59 <mjturek> ack :)
16:46:14 <mjturek> I'm good unless people have anything else!
16:46:48 * dtantsur hopes we finally land that patch
16:46:58 <TheJulia> I don't, just trying to remember why the retry detach logic
16:47:05 <TheJulia> I'm sure it will come to me at 3am
16:47:09 <mjturek> hahaha
16:47:21 <dtantsur> that's how it works
16:48:07 <TheJulia> #topic Open Discussion
16:48:14 <TheJulia> Anything else to chat about today?
16:48:41 <dtantsur> not from me
16:48:49 <mjturek> i'm good :)
16:48:57 <TheJulia> Awesome!
16:49:02 <TheJulia> Thank you everyone!
16:49:14 <mjturek> thanks all, ttyl
16:49:19 <hshiina> thanks
16:49:23 <TheJulia> #endmeeting