| opendevreview | Jacob Anders proposed openstack/sushy master: [WIP] Remove optional attributes from insert_media https://review.opendev.org/c/openstack/sushy/+/802452 | 00:17 |
|---|---|---|
| opendevreview | Jacob Anders proposed openstack/sushy master: Change defaults - optional insert_media attributes https://review.opendev.org/c/openstack/sushy/+/802452 | 00:28 |
| opendevreview | Jacob Anders proposed openstack/sushy master: Change defaults - optional insert_media attributes https://review.opendev.org/c/openstack/sushy/+/802452 | 00:30 |
| opendevreview | Jacob Anders proposed openstack/ironic master: Remove hardcoded parameters from insert_media call https://review.opendev.org/c/openstack/ironic/+/802643 | 01:31 |
| TheJulia | janders: hey, it would be good if you revised commit messages to indicate why in the grand scheme of things and if there is a related rfe/story/bug/etc | 02:49 |
| janders | TheJulia: thanks, will include BZs | 02:50 |
| TheJulia | keep in mind with commit messages, it is good to be able to capture the overall why just from the commit log | 02:50 |
| TheJulia | NobodyCam: Ahh, yes. We took it to the max! | 02:52 |
| opendevreview | Jacob Anders proposed openstack/ironic master: Remove hardcoded parameters from insert_media call https://review.opendev.org/c/openstack/ironic/+/802643 | 04:19 |
| opendevreview | Jacob Anders proposed openstack/sushy master: Change defaults - optional insert_media attributes https://review.opendev.org/c/openstack/sushy/+/802452 | 04:19 |
| janders | TheJulia: added BZ references/links ^^ - apologies for forgetting these earlier | 04:20 |
| opendevreview | Merged openstack/ironic stable/train: Provide a path to set explicit ipxe bootloaders https://review.opendev.org/c/openstack/ironic/+/801669 | 04:34 |
| iurygregory | good morning Ironic! | 05:56 |
| cenne | good mornings iurygregory. | 06:02 |
| cenne | good morning Ironic! | 06:02 |
| iurygregory | morning cenne o/ | 06:03 |
| cenne | o/ :) | 06:03 |
| stendulker | iurygregory, cenne o/ | 06:49 |
| iurygregory | hey stendulker o/ | 06:50 |
| cenne | hey o/ stendulker | 06:54 |
| *** rpittau|afk is now known as rpittau | 07:02 | |
| rpittau | good morning ironic! o/ | 07:02 |
| iurygregory | morning rpittau o/ | 07:04 |
| rpittau | hey iurygregory :) | 07:05 |
| iurygregory | Hey everyone, so far we have 8 people that are planning to attend the MidCycle, if you are planning in participate please provide some input in the doodle https://doodle.com/poll/mhh959u4s4rturxi ( I will close the doodle tomorrow morning) | 07:13 |
| dtantsur | morning ironic | 07:14 |
| iurygregory | morning dtantsur | 07:14 |
| opendevreview | Dmitry Tantsur proposed openstack/ironic stable/wallaby: Fix regression in ramdisk deploy kernel parameters https://review.opendev.org/c/openstack/ironic/+/802664 | 07:29 |
| iurygregory | dtantsur, iscsi removal was like this https://classicprogrammerpaintings.com/post/624257359339995136/engineers-remove-dead-code-after-dropping-a ? hehehe | 07:29 |
| dtantsur | exactly :D | 07:30 |
| janders | good morning Ironic o/ | 08:00 |
| janders | stendulker rpittau thank you for prompt reviews, will fix up the things you pointed out later this evening | 08:00 |
| iurygregory | hey janders o/ | 08:04 |
| opendevreview | Merged openstack/ironic stable/victoria: Use openstack-tox for ironic-tox-unit-with-driver-libs https://review.opendev.org/c/openstack/ironic/+/802462 | 08:41 |
| opendevreview | Merged openstack/ironic bugfix/18.1: Fix regression in ramdisk deploy kernel parameters https://review.opendev.org/c/openstack/ironic/+/802464 | 08:41 |
| janders | stendulker rpittau w/r/t your comments in https://review.opendev.org/c/openstack/sushy/+/802452/6/sushy/resources/manager/virtual_media.py#96 - would you like me to change the "fixes" section of the release note to "upgrades", or keep it as-is and add a separate "upgrades" containing only the changed-defaults warning? | 08:41 |
| iurygregory | do we plan to backport the change? | 08:43 |
| rpittau | iurygregory: I don't think we can backport that | 08:43 |
| iurygregory | yeah, since we need to change default values etc =( | 08:43 |
| rpittau | yep :/ | 08:44 |
| rpittau | janders: I think we need to change from fixes to upgrades | 08:44 |
| janders | thank you iurygregory rpittau | 08:44 |
| janders | rpittau is it okay to reference downstream BZ in an upstream change like I did, or do I need to create a matching bug upstream and reference that? | 08:45 |
| dtantsur | we definitely need to backport that | 08:46 |
| dtantsur | the default values are wrong, why cannot we change them? | 08:46 |
| dtantsur | also -1 to not adding "fixes". it's a bug fix. | 08:47 |
| dtantsur | (even if it does have upgrade impact, a bug fix has to be mentioned together with the upgrade note) | 08:47 |
| dtantsur | janders, iurygregory, rpittau ^^ | 08:48 |
| iurygregory | if the defaults are wrong it should be fine to backport, on the ironic side we will probably need to add the parameters so we can set if the request has the information | 08:48 |
| iurygregory | yeah ++ to upgrade + fixes | 08:49 |
| dtantsur | I don't think we ever set these. I'm not even sure why they were added | 08:49 |
| dtantsur | (probably for completeness) | 08:49 |
| iurygregory | dtantsur, https://review.opendev.org/c/openstack/ironic/+/802643/2/ironic/drivers/modules/redfish/boot.py | 08:49 |
| iurygregory | we send inserted=True/write_protected=True | 08:50 |
| dtantsur | #facepalm | 08:50 |
| iurygregory | hehehe | 08:50 |
| dtantsur | oookay, just keel them with fire | 08:50 |
| dtantsur | ehmm, kill | 08:50 |
| iurygregory | (╯°□°)╯︵ ┻━┻ | 08:50 |
| rpittau | if we backport that, don't we potentially break everyuthing that relies on the old values ? | 08:51 |
| dtantsur | namely? | 08:51 |
| rpittau | I don't have a specific example, I'm just saying in general | 08:52 |
| dtantsur | in general it's possible. in practice we only know examples of the opposite. | 08:52 |
| rpittau | ok then | 08:53 |
| dtantsur | okay, if we go really paranoid, this is what we can do: | 08:53 |
| dtantsur | 1) make a sushy change that handles None values, but don't change the defaults | 08:53 |
| dtantsur | ehh, I started typing and realized it won't work :( | 08:54 |
| dtantsur | okay, another attempt: | 08:55 |
| iurygregory | maybe if ironic can send None we can handle in sushy to not include in the payload? | 08:55 |
| dtantsur | we cannot backport ^^^ | 08:55 |
| dtantsur | because it relies on a specific version of sushy | 08:55 |
| iurygregory | oh =( | 08:55 |
| dtantsur | old sushy will send "null" to the BMC | 08:55 |
| dtantsur | okay, so: | 08:55 |
| dtantsur | 1) change sushy to not send any values if they match the official defaults in the standard (true and true) | 08:55 |
| dtantsur | 2) change ironic to provide the values matching the standard (true and true again) | 08:56 |
| dtantsur | 3) change sushy to default to None only on master | 08:56 |
| iurygregory | interesting... (2 we are already doing this...) | 08:56 |
| dtantsur | 4) change ironic to not provide anything only on master | 08:56 |
| dtantsur | even better! | 08:56 |
| iurygregory | ++ makes sense to me | 08:56 |
| dtantsur | rpittau, janders, wdyt? | 08:57 |
| dtantsur | I know it's a bit more work, but I think it resolves the backporting concerns | 08:57 |
| janders | dtantsur re-reading ^^ the third time to make sure I got it right. I have no concerns about extra work if it gets the job done and doesn't risk breaking stuff. | 08:57 |
| rpittau | that would work with my level of paranoia :) | 08:58 |
| janders | certain degree of paranoia is healthy in engineering | 08:58 |
| iurygregory | step2 you can ignore since we are already sending true true by default :D | 08:58 |
| janders | otherwise we end up with quad-engine aircraft that can fly on two engines, but only if they are on two different wings | 08:59 |
| iurygregory | OMG | 08:59 |
| janders | (not quite in line with how gas turbines usually fail when they fail) | 08:59 |
| janders | so - im all for a healthy dose of paranoia :) | 09:00 |
| rpittau | well if they alternate between a set and another, possibly they can fly :P | 09:00 |
| janders | poor Concorde didn't fly far in 2000 | 09:00 |
| rpittau | ehm right | 09:00 |
| janders | so to sum up steps 1)-4) - we have 3) and 4) done, right? | 09:03 |
| janders | 2) is the current state without my patch (and this will remain outside master) | 09:03 |
| janders | so I will just need to implement 1) for the non-master patch right? | 09:04 |
| iurygregory | 1 needs to go to master I would say, and you can do the backports from it | 09:05 |
| dtantsur | yep | 09:05 |
| iurygregory | after 1 merges you can apply 3 4 | 09:05 |
| dtantsur | we always start on master, unless impossible | 09:05 |
| janders | ok! makes sense | 09:06 |
| janders | lastly - w/r/t https://review.opendev.org/c/openstack/ironic/+/802643/2/releasenotes/notes/remove-hardcoded-insert-media-attrs-6621a811f46cf8d3.yaml - should I just drop the release note entirely? | 09:06 |
| iurygregory | if you have 1 and 3 together you would need to remove things in the backport, so it's better to have a clean backport =) | 09:06 |
| dtantsur | janders: I think both 1 and 3 will have an impact worth mentioning in a release note | 09:07 |
| janders | dtantsur: ACK | 09:07 |
| janders | dinner break, will address remaining review bits and sort out change 1) in a bit | 09:21 |
| iurygregory | janders, take your time o/ enjoy the dinner | 09:22 |
| opendevreview | Riccardo Pittau proposed openstack/ironic master: Increase version of hacking and pycodestyle https://review.opendev.org/c/openstack/ironic/+/802675 | 09:33 |
| *** sshnaidm|afk is now known as sshnaidm | 09:45 | |
| opendevreview | Merged openstack/ironic stable/ussuri: Use openstack-tox for ironic-tox-unit-with-driver-libs https://review.opendev.org/c/openstack/ironic/+/802463 | 10:02 |
| janders | dtantsur: Should I add [DNM] tag on patches in change https://review.opendev.org/q/Ieab097c63f218c3c262f013c29495f5393dd3cfc (steps 3) and 4) from our plan) or is workflow-1 sufficient? | 10:06 |
| dtantsur | janders: W-1 is fully sufficient | 10:08 |
| janders | dtantsur: ty! | 10:08 |
| dtantsur | I only use [WIP] on patches that are not finished and [DNM] on patches that should never be merged | 10:08 |
| janders | dtantsur: ACK | 10:10 |
| opendevreview | Jacob Anders proposed openstack/sushy master: Change defaults - optional insert_media attributes https://review.opendev.org/c/openstack/sushy/+/802452 | 10:16 |
| opendevreview | Jacob Anders proposed openstack/ironic master: Remove hardcoded parameters from insert_media call https://review.opendev.org/c/openstack/ironic/+/802643 | 10:29 |
| janders | allright - I hope ^^ should sort out 3) and 4) from the discussion above, 2) should be all good without any changes, now onto 1) | 10:31 |
| dtantsur | janders: you'll need to rebase 3) on top of 1) | 10:37 |
| janders | dtantsur: makes sense | 10:38 |
| iurygregory | yeah, just to avoid merge conflict :D | 10:38 |
| dtantsur | and to figure out the release notes so that they don't contradict or duplicate each other | 10:38 |
| iurygregory | yup, make sense | 10:39 |
| dtantsur | like 1) fixes: support for X11, X12; upgrade: no longer send the fields when they match the standard defaults; 2) upgrade: changes the method default to None | 10:41 |
| janders | dtantsur: thanks! | 10:42 |
| janders | dtantsur: quick one - now that I am more mindful about the current defaults, I realised that sushy defaults to 'Inserted': True, 'WriteProtected': False while Ironic sets 'WriteProtected': True | 10:48 |
| janders | https://opendev.org/openstack/sushy/src/branch/master/sushy/resources/manager/virtual_media.py#L96 | 10:49 |
| janders | https://opendev.org/openstack/ironic/src/branch/master/ironic/drivers/modules/redfish/boot.py#L199 | 10:49 |
| janders | what was the reason for that, do you remember? | 10:50 |
| janders | (checking history too) | 10:50 |
| dtantsur | I suspect it was accidental.. | 10:53 |
| janders | dtantsur: ACK, thank you | 10:53 |
| opendevreview | Jacob Anders proposed openstack/sushy master: Removing optional fields from insert_media payload https://review.opendev.org/c/openstack/sushy/+/802690 | 12:05 |
| opendevreview | Jacob Anders proposed openstack/sushy master: Removing optional fields from insert_media payload https://review.opendev.org/c/openstack/sushy/+/802690 | 12:07 |
| janders | ^^ change 1) as per the plan above; first pass | 12:08 |
| janders | now - updating commit message/release note for 3) | 12:08 |
| opendevreview | Jacob Anders proposed openstack/sushy master: Change defaults - optional insert_media attributes https://review.opendev.org/c/openstack/sushy/+/802452 | 12:18 |
| janders | dtantsur rpittau iurygregory: w/r/t sushy vMedia fixes I think I have 1) 3) and 4) ready (and 2) shouldn't require any code changes) | 12:25 |
| janders | 1) https://review.opendev.org/c/openstack/sushy/+/802690 | 12:26 |
| janders | 3) https://review.opendev.org/c/openstack/sushy/+/802452 | 12:27 |
| janders | 4) https://review.opendev.org/c/openstack/ironic/+/802643 | 12:27 |
| janders | let me know if these make sense to you and I will do more testing (and fixing if required) tomorrow | 12:27 |
| janders | see you tomorrow Ironic o/ | 12:27 |
| iurygregory | janders, tks! good night! | 12:28 |
| rpittau | bye janders :) | 12:29 |
| janders | thank you for your insights iurygregory rpittau dtantsur | 12:30 |
| opendevreview | Jacob Anders proposed openstack/sushy master: Removing optional fields from insert_media payload https://review.opendev.org/c/openstack/sushy/+/802690 | 12:38 |
| opendevreview | Bob Fournier proposed openstack/sushy-tools master: Add additional BIOS Settings https://review.opendev.org/c/openstack/sushy-tools/+/802695 | 12:44 |
| TheJulia | good morning | 13:05 |
| dtantsur | morning TheJulia | 13:08 |
| iurygregory | good morning TheJulia | 13:16 |
| dtantsur | a friendly reminder: the bugfix CI is still not configured: https://review.opendev.org/c/openstack/ironic/+/801876 https://review.opendev.org/c/openstack/ironic-inspector/+/801873 https://review.opendev.org/c/openstack/ironic-python-agent/+/801898 | 15:32 |
| opendevreview | Dmitry Tantsur proposed openstack/bifrost master: [WIP] Move Nginx code to a new role bifrost-nginx-install https://review.opendev.org/c/openstack/bifrost/+/802532 | 15:52 |
| opendevreview | Dmitry Tantsur proposed openstack/bifrost master: DNM test the upgrade job https://review.opendev.org/c/openstack/bifrost/+/800673 | 16:00 |
| opendevreview | Dmitry Tantsur proposed openstack/bifrost bugfix/11.0: Install libvirt-python from source instead of a wheel https://review.opendev.org/c/openstack/bifrost/+/802749 | 16:05 |
| rpittau | good night! o/ | 16:21 |
| *** rpittau is now known as rpittau|afk | 16:21 | |
| opendevreview | Dmitry Tantsur proposed openstack/bifrost master: Keep sushy-emulator state directory in /var/lib https://review.opendev.org/c/openstack/bifrost/+/802754 | 16:37 |
| opendevreview | Dmitry Tantsur proposed openstack/bifrost master: Keep sushy-emulator state directory in /var/lib https://review.opendev.org/c/openstack/bifrost/+/802754 | 16:44 |
| dtantsur | o/ | 16:51 |
| TheJulia | goodnight dtantsur | 16:51 |
| opendevreview | Merged openstack/sushy-tools master: Add additional BIOS Settings https://review.opendev.org/c/openstack/sushy-tools/+/802695 | 17:02 |
| opendevreview | Merged openstack/ironic bugfix/18.1: Configure CI for bugfix/18.1 https://review.opendev.org/c/openstack/ironic/+/801876 | 17:14 |
| opendevreview | Merged openstack/ironic-inspector bugfix/10.7: Configure CI for bugfix/10.7 https://review.opendev.org/c/openstack/ironic-inspector/+/801873 | 17:14 |
| opendevreview | Merged openstack/ironic-python-agent bugfix/8.1: Configure CI for bugfix/8.1 https://review.opendev.org/c/openstack/ironic-python-agent/+/801898 | 17:14 |
| opendevreview | Merged openstack/python-ironic-inspector-client master: Update min version of tox to use allowlist https://review.opendev.org/c/openstack/python-ironic-inspector-client/+/796398 | 17:22 |
| opendevreview | Merged openstack/virtualbmc master: Update min version of tox to use allowlist https://review.opendev.org/c/openstack/virtualbmc/+/796401 | 17:24 |
| iurygregory | Hey everyone, so far we have 9 people that are planning to attend the MidCycle, if you are planning in participate please provide some input in the doodle https://doodle.com/poll/mhh959u4s4rturxi ( I will close the doodle tomorrow morning my time 10am UTC+2) | 17:42 |
| stevebaker | good morning | 20:42 |
| janders | good morning Ironic o/ | 21:11 |
| TheJulia | good morning stevebaker janders | 21:42 |
| janders | hey TheJulia | 21:45 |
| janders | TheJulia I dropped some messages w/r/t vMedia attribute tweaks on the internal channel - if you have time I'd like to chat further (getting coffee/bfast :) | 21:46 |
| TheJulia | Lets chat here, and honestly coffee does sound really good :) | 21:50 |
| * TheJulia goes and makes an espresso based beverage | 21:50 | |
| * TheJulia returns with a chocolate raspberry soy latte | 21:59 | |
| TheJulia | well, I guess it is more mocha raspberry soy.... | 21:59 |
| janders | OK, back | 22:24 |
| TheJulia | wb | 22:25 |
| janders | w/r/t removing vMedia InsertMedia optional attributes - the original change was https://review.opendev.org/c/openstack/sushy/+/802452 | 22:27 |
| TheJulia | okay | 22:27 |
| janders | I CCed Aija and Shivanand on this one, got some good feedback | 22:27 |
| janders | but in discussion on IRC Riccardo/Iury/Dmitry had concerns about backporting this and that is how we came up with two changes doing the same thing in different ways | 22:28 |
| janders | short summary of this discussion is in that JIRA comment I shared in the internal channel in case you wanted to have a look | 22:29 |
| janders | the idea behind all this is we specify optional attributes to InsertMedia Redfish calls that aren't needed and their presence breaks SuperMicro vMedia | 22:30 |
| TheJulia | I saw it, that entire jira story is basically the context your missing upstream | 22:30 |
| janders | so why not remove these optional attributes | 22:30 |
| TheJulia | well, the root seems to be 1) make it compliant to the standard and 2) also get supermicro gear working | 22:31 |
| TheJulia | at least, 2021.1 schema it seems | 22:31 |
| janders | yeah bit of a moving target isn't it :) | 22:31 |
| janders | but 2021.1 it is for the time being | 22:31 |
| TheJulia | Extreme Programming Moving Target, The Television Show! | 22:31 |
| janders | bring it on! :) | 22:31 |
| * TheJulia wonders how gets producer credentials | 22:31 | |
| TheJulia | s/how/who/ | 22:32 |
| opendevreview | Merged openstack/ironic-specs master: Clean up released features/items https://review.opendev.org/c/openstack/ironic-specs/+/801650 | 22:32 |
| janders | w/r/t testing - I haven't tested these particular changes to sushy as I am waiting for the implementation details to settle (there's still a lot of movement as reviews come in) | 22:32 |
| janders | but | 22:32 |
| janders | I did test it on the RF call level | 22:32 |
| janders | and haven't found a machine that needs these attributes | 22:32 |
| janders | (tested on Dell, HP and a few SuperMicros) | 22:33 |
| TheJulia | that is good, would be ideal to document on at least the commit message | 22:33 |
| janders | OK! I can do that | 22:33 |
| TheJulia | if they *are* entirely redundant, then gives us more evidence and comfort that they can just be removed on a backport espescialy if they are redundant to the spec | 22:33 |
| janders | now - with the context - if it were an upstream RFE, I'd create a story, tasks, etc | 22:34 |
| TheJulia | It is all about setting context in that few paragraphs | 22:34 |
| TheJulia | It still is, and should be tracked as such | 22:34 |
| janders | this is a very-downstream (not even OSP) bug at the moment | 22:34 |
| janders | so what is the right way to go about this? | 22:34 |
| TheJulia | Doesn't hurt to take 5 minutes dump some information in and go from there. It does sound like a bug in the behavior where the spec also seems to be simplier *now* | 22:35 |
| janders | do we still need a story for a bug reported against OCP | 22:35 |
| TheJulia | Yes, because RH entries can be made private and have private posts | 22:35 |
| janders | ok, I can do that | 22:36 |
| TheJulia | so either the commit messages need to provide every bit of context required, or we need to have a little information upstream to explain what/why | 22:36 |
| TheJulia | There is always some overlap there, but it goes quick :) | 22:36 |
| janders | just was concerned about how many different entities we'll end up with with for a trivial fix (BZ, GH for ironic-images re-spin, story, multiple changes) | 22:37 |
| janders | but I guess it is what it is | 22:37 |
| janders | s/GH/GH PR | 22:37 |
| TheJulia | one story, two tasks, the bz for downstream tracking | 22:37 |
| TheJulia | downstream pr for the re-spin once the patches make it through the build pipeline | 22:38 |
| janders | OK, I can do this | 22:39 |
| janders | I will focus on https://review.opendev.org/c/openstack/sushy/+/802690 first as it should be pretty close and that's the one that will be needed for 4.8 fixes | 22:40 |
| TheJulia | It gets worse for downstream backports of course if you have to clone stuff :\ | 22:40 |
| janders | yeah | 22:40 |
| TheJulia | janders: cool, well anything else? | 22:40 |
| janders | 80-90% of the work on this one will be creating tracking bits, but whatever gets the job done :) | 22:40 |
| TheJulia | that is the way with all backport heavy things | 22:41 |
| TheJulia | that and waiting for pipelines | 22:41 |
| janders | just quickly checking if you had any specific comments w/r/t master-only fix | 22:41 |
| TheJulia | I'd prefer whatever gets backported lands to master first | 22:41 |
| TheJulia | Personally | 22:42 |
| janders | yeah that's the plan | 22:42 |
| TheJulia | if that can't be the case, then the need is typically just explaining why | 22:42 |
| janders | I think we covered all the comments regarding https://review.opendev.org/c/openstack/sushy/+/802452 in the discussion about the backportable one too | 22:42 |
| janders | thanks TheJulia | 22:42 |
| TheJulia | Yeah, that is what I seemed to garner | 22:43 |
| janders | so - for the story | 22:43 |
| janders | 1) does it need to be against Ironic or sushy specifically? | 22:44 |
| TheJulia | janders: I'd file it against ironic and just assign one of the tasks to sushy | 22:44 |
| janders | ok! you answered my second question before I asked it | 22:44 |
| janders | :D | 22:44 |
| TheJulia | Excellent! | 22:44 |
| TheJulia | Well with that being done, I think it is time to go exercise | 22:44 |
| janders | and then both backportable and non-backportable patches go against the sushy task? | 22:44 |
| TheJulia | yeah | 22:45 |
| janders | I think that is a fine plan! :) | 22:45 |
| janders | have a good evening TheJulia, enjoy! | 22:45 |
| janders | (I will get my share of hill climbing later today) | 22:45 |
| *** pmannidi|AFK is now known as pmannidi | 23:08 | |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!