| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2017-03-24 | |||
| 14:27:28 | mriedem | can't you scope pylint? | |
| 14:27:31 | mriedem | or whitelist? | |
| 14:27:40 | mdbooth | It all over the place | |
| 14:28:12 | mdbooth | I disabled almost everything, and I still had 95% false positives | |
| 14:30:41 | mdbooth | It did find RemoteFilesystemDriver.copy_file for me | |
| 14:31:00 | mdbooth | Although in this case, somebody has clearly updated all the implementations and forgotten to update the superclass | |
| 14:31:26 | nkorabli | Hi there! I noticed a multitenancy issue, here is a bug report: https://bugs.launchpad.net/nova/+bug/1675791 | |
| 14:31:26 | openstack | Launchpad bug 1675791 in OpenStack Compute (nova) "Instance created by demo user(non-admin), shelved by admin and unshelved by demo user --> ends up in error state " [Undecided,New] | |
| 14:31:26 | nkorabli | Need to know your opinions, to start fixing it! :) Thanks in advance! | |
| 14:31:29 | mriedem | ok i see what you mean about abstractmethod not checking the signature | |
| 14:31:30 | mriedem | that sucks | |
| 14:31:33 | mriedem | i just tested locally | |
| 14:31:57 | mdbooth | mriedem: Anyway, how about the testclass generator? | |
| 14:32:01 | mdbooth | Overkill? | |
| 14:32:03 | mriedem | nkorabli: we already talked about this the other day | |
| 14:32:09 | leakypipes | np | |
| 14:32:15 | mdbooth | I think we could use it in a few places | |
| 14:32:16 | nkorabli | mriedem: Hi there! | |
| 14:32:29 | mriedem | nkorabli: the error during unshelve wasn't due to the tenant | |
| 14:32:38 | mriedem | it was most likely due to a failure to spawn the instance during unshelve | |
| 14:32:42 | mriedem | which put the instance into ERROR state | |
| 14:33:26 | nkorabli | mriedem: right, we did I just tested it with pure openstack (stable/newton) in devstack environment - and I see this error as well. | |
| 14:33:44 | mriedem | nkorabli: i've invalidated the bug, | |
| 14:33:51 | mriedem | nkorabli: have you checked the logs for errors during unshelve? | |
| 14:34:00 | mriedem | because there has to be an error in the logs if the instance is in error state | |
| 14:35:37 | nkorabli | I actually haven't found anything useful in the logs, but let my check one more time. | |
| 14:49:28 | bauwser | superdan: mriedem: quick question | |
| 14:49:39 | bauwser | superdan: mriedem: I'm about to clean up a bit of useless code in the conductor | |
| 14:50:25 | bauwser | superdan: mriedem: and I just notice that we do verify at every move operation if the affinity filters are present on the conductor side | |
| 14:51:03 | bauwser | superdan: mriedem: from my perspective, this is violating our policy to separate what's scheduler and what's conductor | |
| 14:51:38 | bauwser | if the operator disabled the affinity filters but had an instance that was previously booted by using an InstanceGroup, should we fail the move operation ? | |
| 14:51:45 | bauwser | my guts say no | |
| 14:51:59 | bauwser | johnthetubaguy: too ^ | |
| 14:52:39 | johnthetubaguy | maybe we shouldn't allow those filters to be disabled? | |
| 14:52:55 | johnthetubaguy | its not 100% clear to me | |
| 14:53:05 | bauwser | well | |
| 14:53:13 | bauwser | disabling a filter is an operator thing | |
| 14:53:33 | bauwser | in case they use instance groups, then why they should disable that ? | |
| 14:53:45 | bauwser | it's terrible for users | |
| 14:53:53 | johnthetubaguy | now you might want to disable instance groups, but that seems like it should be discoverable | |
| 14:54:06 | bauwser | because for example, say Bob created an instance and asked for this group A | |
| 14:54:23 | bauwser | then Max (operator) disabled the filters without asking Bob | |
| 14:54:24 | johnthetubaguy | as you can see, I am just a bit confused by the question really | |
| 14:54:46 | bauwser | then, the current situation is that if Bob wants to migrate its instance, it will be getting an exception | |
| 14:54:53 | bauwser | gibi: p | |
| 14:55:10 | johnthetubaguy | bob by default can't migrate his instance, so its a bit of an odd one for me | |
| 14:55:32 | johnthetubaguy | we should really just respect the filters, given the preferences | |
| 14:55:50 | bauwser | johnthetubaguy: https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L162-L163 | |
| 14:55:58 | johnthetubaguy | but seems like certain filters you shouldn't kill | |
| 14:56:11 | openstackgerrit | Vasyl Saienko proposed openstack/nova master: Implement interface attach/detach in ironic virt driver https://review.openstack.org/419975 | |
| 14:56:23 | johnthetubaguy | bauwser: thats just refreshing the list of instances in the group, at least I thought that was the intent there | |
| 14:56:23 | bauwser | johnthetubaguy: https://github.com/openstack/nova/blob/master/nova/scheduler/utils.py#L311-L315 | |
| 14:56:41 | bauwser | johnthetubaguy: nope, not the instances | |
| 14:56:51 | johnthetubaguy | oh, I see | |
| 14:56:53 | johnthetubaguy | thats odd | |
| 14:57:05 | bauwser | johnthetubaguy: the InstanceGroup.hosts is set by the nodes having instances in there | |
| 14:57:11 | johnthetubaguy | do we fail to boot with the same things I guess? | |
| 14:57:16 | bauwser | johnthetubaguy: but it's a lazy-loadable field | |
| 14:57:22 | mriedem | bauwser: looks like you added that https://github.com/openstack/nova/commit/111a852e79f0d9e54228d8e2724dc4183f737397 | |
| 14:57:54 | bauwser | mriedem: we did that in the past | |
| 14:58:00 | bauwser | mriedem: I just modified the callers | |
| 14:58:08 | johnthetubaguy | bauwser: that does do what I said right? | |
| 14:58:20 | johnthetubaguy | bauwser: the bit we need in there is re-populating the up to date group https://github.com/openstack/nova/blob/master/nova/scheduler/utils.py#L350 | |
| 14:58:33 | johnthetubaguy | bauwser: the other bits seem like nonsense for anything other than boot | |
| 14:58:48 | bauwser | johnthetubaguy: yup, we need to create those for boot | |
| 14:59:03 | bauwser | johnthetubaguy: but when we move that instance, then we get the existing field from the Spec object | |
| 14:59:11 | johnthetubaguy | bauwser: I would split out the boot checks, and call them separately in the boot flow right? | |
| 14:59:30 | bauwser | johnthetubaguy: given group.hosts is the only mutable field, it's fine given it's a lazy-loadable | |
| 14:59:43 | johnthetubaguy | I have no idea what you mean | |
| 14:59:51 | johnthetubaguy | sorry, I got lost | |
| 14:59:54 | bauwser | johnthetubaguy: my question is more about having permission to remove that call https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L162-L163 | |
| 15:00:11 | johnthetubaguy | bauwser: I just said, thats really bad | |
| 15:00:23 | bauwser | johnthetubaguy: that's bad to what ? :) | |
| 15:00:29 | johnthetubaguy | bauwser: we need to update the group with the latest, but we don't want to do the boot time only checks | |
| 15:00:46 | bauwser | johnthetubaguy: we don't need to update the group info | |
| 15:00:56 | johnthetubaguy | bauwser: because of lazy loading? | |
| 15:00:58 | bauwser | johnthetubaguy: because the group info is made of 3 things | |
| 15:01:10 | bauwser | 2 can't be modified and the 3rd is lazy-loadable | |
| 15:01:11 | bauwser | right | |
| 15:01:22 | johnthetubaguy | but its saved into the json blob in the DB? | |
| 15:01:45 | bauwser | johnthetubaguy: if you see the code https://github.com/openstack/nova/blob/master/nova/conductor/tasks/live_migrate.py#L162-L163 you will see that I don't update the Spec with that updated group already | |
| 15:01:54 | johnthetubaguy | I thought we had a bug where that list needed to be updated, because we persisted and out of date list | |
| 15:02:23 | johnthetubaguy | s/and/an/ | |
| 15:03:00 | bauwser | johnthetubaguy: that was fixed by https://github.com/openstack/nova/blob/master/nova/objects/instance_group.py#L282-L289 | |
| 15:03:03 | johnthetubaguy | I don't think we need the boot checks, I just through the filter params would need to be populated correctly, I guess I would have to go read more | |
| 15:03:33 | bauwser | I'm going to cut that call, and you could be reviewing it | |
| 15:03:36 | johnthetubaguy | bauwser: I am not convinced, but I guess you want some functional tests for that stuff really, then we can answer whats correct | |
| 15:04:59 | bauwser | johnthetubaguy: https://github.com/openstack/nova/commit/c48c1098cbcd7a9c7980d5fbe4668c38b16767f5 was missing a functional test but fixed the problem you mentioned | |
| 15:05:29 | mriedem | yeah seriously i'd write functional tests for anything we're removing | |
| 15:05:32 | mriedem | before removing it | |
| 15:05:40 | mriedem | because the unit tests just don't cut it for conductor <> scheduler interactions | |
| 15:05:41 | johnthetubaguy | what mriedem said, its safest | |
| 15:05:49 | mriedem | as we've been seeing lately with a lot of regressions | |
| 15:06:23 | mriedem | i'm still working on one for another regression | |
| 15:07:18 | mriedem | ooo and i think my regression test stumbled on another bug | |
| 15:08:42 | mriedem | ha, the fake virt driver doesn't set the image size on snapshot | |
| 15:08:54 | mriedem | something like that | |
| 15:09:23 | bauwser | mriedem: okay fair enough | |
| 15:10:04 | mriedem | oh it's the api code, not the fake virt driver | |
| 15:10:05 | mriedem | wtf | |