| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-04-28 | |||
| 12:13:40 | sfinucan | johnthetubaguy: I _think_ that controller did nothing | |
| 12:13:58 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove 'os-flavor-rxtx' policy https://review.openstack.org/444036 | |
| 12:13:58 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove 'FlavorRxtxController' extension https://review.openstack.org/461010 | |
| 12:13:59 | openstackgerrit | Stephen Finucane proposed openstack/nova master: Remove useless API tests https://review.openstack.org/461011 | |
| 12:14:42 | sfinucan | So I've written those ^^^ to test as much (in the gate - weird "no db" issues if I try running 'tox -e api-samples' locally. If so, we can delete the policy as also doing nothing, IMO | |
| 12:44:31 | mriedem | o/ | |
| 12:45:42 | gibi | mriedem: hi! I think I figured out what happens with flavor-delete in https://review.openstack.org/#/c/460377 , I left comments in the patch | |
| 12:46:37 | mriedem | gibi: ah thanks | |
| 12:46:46 | mriedem | i must have been looking at the wrong main db api method | |
| 12:48:02 | mriedem | gibi: i'm not sure about changing it from [] to None | |
| 12:48:14 | mriedem | i'd have to change one of the notification samples at least | |
| 12:48:51 | gibi | mriedem: yes, but I think it is bugfix on that sample | |
| 12:49:08 | gibi | mriedem: before your patch flavor-delete always reported projects=[] | |
| 12:49:20 | gibi | mriedem: even if the flavor had some projects associated before the delete | |
| 12:49:26 | gibi | mriedem: and that is missleading | |
| 12:49:40 | gibi | mriedem: we use None (null) value in the notification to signal if data is missing | |
| 12:49:42 | mriedem | yeah | |
| 12:49:44 | mriedem | ok, | |
| 12:49:55 | mriedem | then i think i'll probably pull that part of the change out and do that first | |
| 12:50:07 | gibi | mriedem: sure, that would be cleaner | |
| 13:01:03 | johnthetubaguy | mriedem: was Nova v1.0 really a thing, I thought only the EC2 API was used back then? | |
| 13:01:55 | mriedem | johnthetubaguy: i found it in the 2010.1 release code in launchpad | |
| 13:02:14 | mriedem | https://launchpad.net/nova/austin/2010.1 | |
| 13:02:16 | johnthetubaguy | mriedem: curious | |
| 13:02:44 | mriedem | nova/api/__init__.py | |
| 13:03:01 | mriedem | http://paste.openstack.org/show/608325/ | |
| 13:06:03 | mriedem | gibi: oh, it can't be None | |
| 13:06:07 | mriedem | flavor.projects is not a nullable field | |
| 13:06:50 | johnthetubaguy | mriedem: crazy stuff, thanks. | |
| 13:07:04 | bauzas | mriedem: dansmith: FWIW, Monday May 1st is a French (and EU) holiday + I'll be on PTO by Tuesday | |
| 13:07:08 | mriedem | johnthetubaguy: yeah really, it was an interesting journey | |
| 13:07:42 | bauzas | mriedem: dansmith: so we should be discussing about the scheduler claims by either today or Wed | |
| 13:08:13 | mriedem | bauzas: ok...i haven't gone through those changes yet, and it's still < 1 cup of coffee here at this point | |
| 13:08:31 | mriedem | and john is already asking me about API versions from the austin release :) | |
| 13:09:02 | gibi | mriedem: :/ | |
| 13:09:43 | bauzas | mriedem: heh okay | |
| 13:09:56 | gibi | mriedem: we somehow ensure that the schema population detects the problem and defaults the payload field to None | |
| 13:10:08 | gibi | gibi: let me play with that patch a bit... | |
| 13:10:17 | mriedem | gibi: it used to do that because of the OrphanedObjectError i thought | |
| 13:10:46 | bauzas | mriedem: dansmith: the main discussion would be around knowing if we sould call placement by either conductor or scheduler (because of the problem I said by https://review.openstack.org/#/c/460455/ | |
| 13:11:17 | gibi | mriedem: yes, so basically what we have to do is to orphan the flavor object | |
| 13:13:09 | gibi | mriedem: so set flavor._context = None; here https://review.openstack.org/#/c/460377/2/nova/objects/flavor.py@544 | |
| 13:13:10 | mriedem | bauzas: gdi you're going to make me talk about this right now aren't you | |
| 13:13:16 | mriedem | gibi: yup did it and testing | |
| 13:13:46 | mriedem | bauzas: i think doing it in conductor is ok, we had also talked about this as an alternative, | |
| 13:13:58 | mriedem | and said we could do the retries in conductor if we return the hosts from the scheduler via select_destinations | |
| 13:15:10 | jaypipes | figleaf: ah, great point on the negative tests on that SQL. will add. | |
| 13:16:44 | mriedem | bauzas: comments inline | |
| 13:17:09 | openstackgerrit | John Garbutt proposed openstack/nova master: Update description to policies in remaining flavor APIs https://review.openstack.org/460509 | |
| 13:17:10 | mriedem | bauzas: fwiw dansmith always wanted to do it in conductor anyway, because that's normally where we do retries, and it makes scheduler more of a library, which i think is a long-term nice to have | |
| 13:19:21 | cdent | figleaf: can you sanity check the comments on the same thing you just reviewed? I feel like I'm maybe missing something, or something big is missing. | |
| 13:19:44 | bauzas | mriedem: okay | |
| 13:19:47 | bauzas | mriedem: looking | |
| 13:19:58 | bauzas | mriedem: also, you asked to rebase https://review.openstack.org/#/c/460113/1/nova/compute/resource_tracker.py | |
| 13:20:24 | bauzas | mriedem: shouldn't we rather maybe merge first ^ and then possibly add a new change for adding the needed modification ? | |
| 13:20:50 | bauzas | mriedem: I wouldn't like to wait for https://review.openstack.org/#/c/457782/ for that | |
| 13:21:13 | bauzas | mriedem: or I could write a follow-up that would be on top of the above if you agree | |
| 13:23:03 | mriedem | bauzas: i think https://review.openstack.org/#/c/457782/ is basically in, i was just waited for leakypipes to reply | |
| 13:23:14 | mriedem | about the spooning that's happening between the virt drivers and the RT | |
| 13:23:31 | bauzas | mriedem: of course, I tend to agree | |
| 13:24:19 | bauzas | mriedem: I just think that https://review.openstack.org/#/c/460113/ could just add the opt, and https://review.openstack.org/#/c/457782/ would have a follow-up adding the needed reserved stuff | |
| 13:24:43 | bauzas | mriedem: given I'm pretty operators won't use the new opt by the next weeks :) | |
| 13:24:57 | leakypipes | mriedem: I did reply... | |
| 13:25:00 | mriedem | leakypipes: i know | |
| 13:25:07 | leakypipes | ah, ok. thx luv. | |
| 13:25:08 | mriedem | leakypipes: and now i'm going to confront you about it | |
| 13:25:11 | mriedem | leakypipes: so https://review.openstack.org/#/c/457782/2/nova/virt/libvirt/driver.py | |
| 13:25:22 | leakypipes | heh, ok | |
| 13:25:38 | mriedem | leakypipes: would you be opposed to not setting reserved=0 for VCPU inventory in the libvirt driver and default that in the RT like we're doing for MEMORY_MB and DISK_GB? | |
| 13:25:45 | mriedem | that's i think the main thing bugging me, | |
| 13:25:58 | mriedem | is that the libvirt driver is setting reserved=0 for VCPU b/c it knows that the RT helper method isn't doing it | |
| 13:26:40 | mriedem | i think i'd like to get to the point that either the driver provides everything (which would be redundant between drivers), or the drivers only override what they need to, and the RT fills in all the blanks | |
| 13:27:14 | leakypipes | mriedem: I'd be totes ok with that. | |
| 13:27:27 | mriedem | if you make that change i'm +10 | |
| 13:27:35 | leakypipes | mriedem: we just never had any vcpus reserved option until bauzas' recent patch. | |
| 13:27:43 | leakypipes | mriedem: so it was just a hard-coded ugh. | |
| 13:27:50 | leakypipes | mriedem: happy to move it into the RT. | |
| 13:27:51 | bauzas | oh snap, it's Friday | |
| 13:29:01 | mriedem | leakypipes: ok cool, | |
| 13:29:09 | mriedem | left a comment and -1 for posterity :) | |
| 13:30:12 | bauwser | so many political folks want posterity... | |
| 13:30:24 | bauwser | could I just -1 them ? | |
| 13:31:23 | bauwser | life would be so much simple if our Parliaments were using Gerrit for voting laws | |
| 13:32:31 | figleaf | cdent: to which review are you referring? | |
| 13:33:12 | cdent | figleaf: sorry, I was thinking that would land on your brain at the same time as I typied, which is stupid: https://review.openstack.org/#/c/460798/ | |
| 13:33:43 | figleaf | cdent: on it | |
| 13:33:43 | cdent | figleaf: the key bit is not the bit about how the query works (unless you care) but the bit about aggregates not playing a part | |
| 13:35:00 | mriedem | gibi: ok got this working, patch coming shortly | |
| 13:35:07 | mriedem | once i can wordsmith the commit message | |
| 13:36:12 | cdent | mriedem, sdague: to follow up on the thing that sdague already fixed by changing the flags, my patch which does a greenthread sleep around the requests has passed 14 rechecks (as expected): https://review.openstack.org/#/c/459733/ | |
| 13:36:15 | figleaf | cdent: Hmmm... I see your point. I guess if the unit tests covered cases with multiple aggregates available, it would have shown up | |
| 13:36:21 | cdent | figleaf: yes | |
| 13:36:21 | gibi | mriedem: nice | |
| 13:36:41 | figleaf | cdent: I missed the missing stuff :) | |
| 13:36:51 | mriedem | cdent: do we need that if the keep_alive thing fixed it? | |
| 13:37:09 | leakypipes | bauwser: feel like +2ing https://review.openstack.org/#/c/460281/ and/or figleaf's https://review.openstack.org/#/c/453862/? | |
| 13:37:27 | cdent | mriedem: no, I'm just mentioning it for sake of data (that using eventlet in tests is somewhat bad...) | |
| 13:37:41 | bauwser | leakypipes: I *could* | |
| 13:37:43 | mriedem | cdent: ah ok | |
| 13:37:47 | bauwser | just need to rebase some stuff :) | |