| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-07-27 | |||
| 16:42:40 | sean-k-mooney | we should revert the previous patch and start callign delete again on plamcnet | |
| 16:44:06 | melwitt | I'm cool with that too. I wasn't 100% sure whether there's internal cases where we would want the chance to know about a conflict | |
| 16:44:18 | sean-k-mooney | melwitt: basiclaly i think we have 2 options. mark the bug as invilad and adapt tempset for the 409, or always call placemtn with DELETE | |
| 16:44:29 | sean-k-mooney | i dont think we need a new arguement that default to ture | |
| 16:45:04 | bauzas | I see two different things here | |
| 16:45:07 | sean-k-mooney | melwitt: do we know of any internal cases this would protect form | |
| 16:45:20 | bauzas | 1/ a delete should always work and never return an exception, for sure | |
| 16:45:39 | bauzas | 2/ a racing delete could tho leave orphaned allocations and that's fine | |
| 16:45:46 | melwitt | sean-k-mooney: they're in the WIP patch, one of them was during a soft-delete reclaim periodic. I need to look again to see the other cases where we would pass force=False | |
| 16:46:19 | sean-k-mooney | melwitt: ack | |
| 16:46:29 | melwitt | but yeah for sure when it's an end user requesting delete we should ensure that cannot fail | |
| 16:48:22 | melwitt | bauzas: only adjacent to how this discussion came about, I would love for you to be the second reviewer on the placement consumer types series, if you might have interest | |
| 16:48:43 | bauzas | melwitt: I can help | |
| 16:48:59 | melwitt | \o/ | |
| 16:49:26 | bauzas | I mostly work on the generic mdev stuff but I'm not sure I'll be able to put all the changes before I leave for 3 weeks, so I'll maybe turn into reviews next week | |
| 16:50:29 | melwitt | gibi has gone through it thoroughly already (thank you gibi!) so it's in a nice shape now | |
| 16:51:24 | sean-k-mooney | i can try and take a look too but while i dicuss the api of placment alot i never really look at the code so millage will vary | |
| 16:52:14 | bauzas | melwitt: ping me your series so I'll open a tab and make it a priority | |
| 16:53:39 | melwitt | sean-k-mooney: ++ | |
| 16:54:08 | melwitt | bauzas: it's 3 patches starting here https://review.opendev.org/c/openstack/placement/+/669170 I will remove the -W, I put up a DNM nova change to run with the new stuff and it all worked well _except_ for hitting the generation conflict seemingly guaranteed | |
| 16:55:42 | opendevreview | Stephen Finucane proposed openstack/nova master: mypy: Add nova.cmd, nova.conf, nova.console https://review.opendev.org/c/openstack/nova/+/705657 | |
| 16:55:43 | opendevreview | Stephen Finucane proposed openstack/nova master: mypy: Add type annotations to top-level modules https://review.opendev.org/c/openstack/nova/+/705658 | |
| 16:55:44 | opendevreview | Stephen Finucane proposed openstack/nova master: trivial: Clean manager.Manager, service.Service signatures https://review.opendev.org/c/openstack/nova/+/764806 | |
| 16:55:44 | opendevreview | Stephen Finucane proposed openstack/nova master: WIP: Expand type hints for nova.block_device https://review.opendev.org/c/openstack/nova/+/742170 | |
| 16:57:15 | bauzas | melwitt: cool, will read tomorrow | |
| 16:57:42 | melwitt | thanks bauzas++ | |
| 16:59:26 | sean-k-mooney | melwitt: bauzas this is a summary of my view on the WIP patch https://review.opendev.org/c/openstack/nova/+/688802/2#message-70a28bde99719c723c954c947d0af17227a344e9 | |
| 17:00:00 | melwitt | thanks sean-k-mooney | |
| 17:00:13 | sean-k-mooney | melwitt: there are other reasonce lients can recive a 409 namely locked instances so we can go either way with it | |
| 17:00:28 | melwitt | I feel strongly that delete (by an end user) should never fail | |
| 17:00:46 | melwitt | internals can handle 409s if there is some reason to | |
| 17:00:47 | sean-k-mooney | right but the api contract is it must fail if the instance is locked | |
| 17:01:38 | melwitt | well, yeah, agree that is expected | |
| 17:01:42 | melwitt | that's the point (or one of the points) of locking | |
| 17:02:08 | sean-k-mooney | https://docs.openstack.org/api-ref/compute/?expanded=force-delete-server-forcedelete-action-detail#force-delete-server-forcedelete-action im kind of suprised force delete can also return a 409 | |
| 17:02:43 | melwitt | yeah :\ | |
| 17:02:48 | sean-k-mooney | force-delet i do expect to never fail | |
| 17:03:04 | sean-k-mooney | well 401/403/404 sire | |
| 17:03:07 | sean-k-mooney | *sure | |
| 17:03:10 | sean-k-mooney | but not 409 | |
| 17:03:38 | melwitt | yeah | |
| 17:08:24 | melwitt | fyi there's a new spec proposed around migrating instances between projects that I had -2ed the implementation proposal as this majorly needs a spec https://review.opendev.org/c/openstack/nova-specs/+/802034 | |
| 17:20:48 | sean-k-mooney | melwitt: oh i forgot to bring this up in the meeting | |
| 17:21:05 | sean-k-mooney | melwitt: bauzas how do you feel about approving/reviewing yoga specs now? | |
| 17:21:37 | sean-k-mooney | we normally wait untill around m3 but ^ and any other specs can be targeted again the yoga release now | |
| 17:21:43 | sean-k-mooney | we have the folder created | |
| 17:23:13 | sean-k-mooney | melwitt: and ya that needs a spec | |
| 17:23:36 | melwitt | I personally think review is welcome at any time. approval I think is natural post PTG at least but obviously it doesn't have to be | |
| 17:23:39 | sean-k-mooney | we sould also need support in every other serivce to move the ownership of the resouces | |
| 17:24:19 | sean-k-mooney | we have approved spec pre ptg before but mostly onse that missed the previous cycle | |
| 17:24:32 | melwitt | yeah. we've talked about it many times as we know, and some of the proposal were to only support the project move within nova and then let other tools call all the different projects' APIs | |
| 17:24:59 | sean-k-mooney | os-chown was the latest iteration of that right | |
| 17:25:34 | sean-k-mooney | https://github.com/kk7ds/oschown | |
| 17:26:03 | sean-k-mooney | i think there was a sepereate repo created form dansmith poc | |
| 17:26:19 | melwitt | yeah there is that and I didn't know about a separate repo | |
| 17:26:55 | sean-k-mooney | the only one i can find is os-migrate | |
| 17:27:02 | sean-k-mooney | but that is different | |
| 17:27:12 | sean-k-mooney | that is inter cloud migration | |
| 17:28:16 | sean-k-mooney | melwitt: so i think my issue with nova just doing the nova bits is we end up in a situration where for a time the ownser ship of each resouce is split between multipel porojects/users/domains | |
| 17:28:38 | opendevreview | Stephen Finucane proposed openstack/placement master: tests: Silence noisy tests https://review.opendev.org/c/openstack/placement/+/801101 | |
| 17:28:39 | opendevreview | Stephen Finucane proposed openstack/placement master: tox: Remove psycopg2 warning filter https://review.opendev.org/c/openstack/placement/+/800911 | |
| 17:28:39 | opendevreview | Stephen Finucane proposed openstack/placement master: setup: Replace dashes with underscores https://review.opendev.org/c/openstack/placement/+/801102 | |
| 17:28:40 | opendevreview | Stephen Finucane proposed openstack/placement master: db: Replace implicit conversion of SELECT into FROM https://review.opendev.org/c/openstack/placement/+/800910 | |
| 17:28:40 | opendevreview | Stephen Finucane proposed openstack/placement master: db: Replace 'as_scalar()' with 'scalar_subquery()' https://review.opendev.org/c/openstack/placement/+/801100 | |
| 17:28:41 | opendevreview | Stephen Finucane proposed openstack/placement master: db: Update 'select()' calls https://review.opendev.org/c/openstack/placement/+/801103 | |
| 17:28:41 | opendevreview | Stephen Finucane proposed openstack/placement master: db: Remove use of non-integer/slice indices https://review.opendev.org/c/openstack/placement/+/801104 | |
| 17:28:42 | opendevreview | Stephen Finucane proposed openstack/placement master: db: Replace deprecated 'FromClause.select().whereclause' parameter https://review.opendev.org/c/openstack/placement/+/801105 | |
| 17:28:42 | opendevreview | Stephen Finucane proposed openstack/placement master: db: Use explicit transactions https://review.opendev.org/c/openstack/placement/+/801106 | |
| 17:28:44 | opendevreview | Stephen Finucane proposed openstack/placement master: db: Remove unnecessary use of '_mapping' https://review.opendev.org/c/openstack/placement/+/801107 | |
| 17:28:44 | opendevreview | Stephen Finucane proposed openstack/placement master: tox: Enable SQLAlchemy 2.0 warnings https://review.opendev.org/c/openstack/placement/+/801108 | |
| 17:29:16 | sean-k-mooney | for example what happens if you move the ownwer ship of the neutron port of cinder volumes first before the nova instance | |
| 17:29:30 | sean-k-mooney | if we created the port and volume and delete on terminate is set | |
| 17:29:36 | sean-k-mooney | then you delete the instance | |
| 17:29:47 | melwitt | yeah... I think that's why we've nacked it in the past. if we add something like that, it needs a big fat warning on it. and maybe a dragon icon | |
| 17:29:48 | sean-k-mooney | what do we do then | |
| 17:30:05 | sean-k-mooney | but dragons are cool | |
| 17:30:21 | sean-k-mooney | we should put a honey badger on it instead | |
| 17:30:24 | melwitt | "if you do this and it messes stuff up, you are on your own" | |
| 17:30:33 | melwitt | hehe | |
| 17:31:07 | dansmith | how about we only allow the transition if the instance is locked? | |
| 17:31:24 | dansmith | I don't remember how much lock really restricts, and we've probably not done a good job of honoring lock everywhere, | |
| 17:31:39 | melwitt | yeah true dragons might attract people to call the API | |
| 17:31:43 | dansmith | but it would at least be something procedural | |
| 17:31:49 | sean-k-mooney | dansmith: admin bypass lock by default apparently which is fun | |
| 17:32:11 | dansmith | sean-k-mooney: sure, but this change ownership call could ... not | |
| 17:32:19 | sean-k-mooney | dansmith: but i get your point we could take some of the rough edges off with lock | |
| 17:32:43 | melwitt | conceptually I think that's a good idea. require some kind of freezing of it before allowing project migration | |
| 17:32:45 | dansmith | if a compute rebooted in the middle or something I think you'd still get magic smoke | |
| 17:33:30 | sean-k-mooney | i assume this woudl ba an admin only api by default too | |
| 17:33:46 | dansmith | for sure | |
| 17:33:58 | sean-k-mooney | although a domain admin might have a usecase for it to not be | |
| 17:34:02 | melwitt | yes, haha I can't imagine it being open to normal users by default | |
| 17:34:26 | sean-k-mooney | melwitt: well not project member anyway or likely even project admin | |
| 17:34:45 | sean-k-mooney | but in new rbac world i coudl see domain_admin or system_admin having a use case for it | |
| 17:35:03 | dansmith | not project admin either, I wouldn't think | |
| 17:35:41 | sean-k-mooney | unless you did it liek cinder? i think it cinder that has a volume transfer api | |
| 17:35:52 | sean-k-mooney | that allows you to transfer volumes between tenatns | |
| 17:35:58 | melwitt | yeah that would seem weird to be able to move something into another project if you're not admin in that project. I guess if you were project admin in the source and destination projects? | |
| 17:36:02 | sean-k-mooney | that has a request and accpet model | |
| 17:36:13 | dansmith | well, if you did that, I guess you could also freeze all operations on the instance if it had a pending transfer, | |