| opendevreview | Alfredo Moralejo proposed openstack/watcher master: Only cancel non-terminal audits and action plans on sync https://review.opendev.org/c/openstack/watcher/+/1015384 | 09:12 |
|---|---|---|
| opendevreview | Alfredo Moralejo proposed openstack/watcher master: Add support to update status_message in update_audit_state https://review.opendev.org/c/openstack/watcher/+/1014342 | 09:14 |
| opendevreview | Alfredo Moralejo proposed openstack/watcher master: Version the parameters spec of strategies https://review.opendev.org/c/openstack/watcher/+/1014569 | 09:15 |
| opendevreview | Alfredo Moralejo proposed openstack/watcher master: Version the parameters spec of strategies https://review.opendev.org/c/openstack/watcher/+/1014569 | 13:17 |
| opendevreview | Alfredo Moralejo proposed openstack/watcher master: Version the parameters spec of strategies https://review.opendev.org/c/openstack/watcher/+/1014569 | 15:12 |
| amoralej_ | dviroel, reading https://review.opendev.org/c/openstack/watcher-specs/+/1007853/comment/335e1948_f26feca1/ | 16:02 |
| amoralej_ | wrt "We can, based on your suggestion, include also a always_skip_pinned policy for the Migrate Action, instead of adding a new paramter?" | 16:02 |
| amoralej_ | i'm not following that | 16:02 |
| amoralej_ | that should be up to the strategy | 16:02 |
| amoralej_ | the strategy should know which actions should be created | 16:03 |
| amoralej_ | if include_az_pinned_instances: False, it should simply not create those actions | 16:03 |
| dviroel | amoralej_: the same problem stays, one thing is what strategy creates, other is the instance properties at action execution time. The instance that wasn't pinned at audit execution time may be pinned at AP execution. Always skip policy would not allow any pinned instance to migrate, and be aligned with audit input_parameters. | 16:22 |
| dviroel | that was my intention with that | 16:23 |
| amoralej_ | for me, those are two totally different things | 16:24 |
| amoralej_ | " The instance that wasn't pinned at audit execution time may be pinned at AP execution. " | 16:24 |
| amoralej_ | for that, the easiest is to just add the pinned_az value to the action when was created | 16:24 |
| amoralej_ | and compare to the one actually in the instance at execution time | 16:25 |
| amoralej_ | if differs, the action should be skipped whatever the policy is probably | 16:25 |
| amoralej_ | imo, we are trying to do too many things with the policy parameter | 16:26 |
| dviroel | but i am not also sure that we also need to keep including parameters | 16:27 |
| amoralej_ | i think that's the easiest way to manage incorrect info in the model | 16:29 |
| amoralej_ | and would only be optional, to not affect other strategies | 16:29 |
| dviroel | TBH, I think that we need to be clear in documentation, and let user knows that what was defined at Audit execution time, may differ from the execution time too. And that manual actions like pin/unpin instances will have that effect | 16:29 |
| amoralej_ | but having logic to manage in different ways stale instances in the model depending on the rest of parameters... for me is overcomplicating it | 16:30 |
| amoralej_ | think in another case, currently if source_host != actual_source_host -> the action is skipped | 16:31 |
| amoralej_ | for the src_az to dst_az case, it may be the case that the instance has been moved from one host to another in the same src_az, so actually the instance still needs to be moved | 16:32 |
| amoralej_ | but i don't think we need to add logic to guess "ok, the instance has been moved to a different host after AP was created, but it's in the same AZ, so let's not skip it and migrate it" | 16:33 |
| amoralej_ | we should just skip it, and report it as skipped it | 16:33 |
| amoralej_ | user can always run the audit again | 16:33 |
| amoralej_ | actually, new instances may also have been created in the src_az, i.e. after creating the audit. I mean, an audit can produce inexact results specially if there has passed time betwen audit execution and ap execution | 16:34 |
| amoralej_ | actually, ideally, users should re-run the audit after hosts have been disabled, i.e. | 16:35 |
| amoralej_ | so, yeah, i think documenting that is good, actually, for any strategy can happen | 16:36 |
| dviroel | so yeah, I think that this behavior needs to be documented, to mention that any change in the related resources may cause Action to be skip or even fail. This is valid for pin/unpin, but as you mentioned also for a instance migrates in between audit solution and AP execution | 16:38 |
| amoralej_ | yep | 16:39 |
| dviroel | So i am not sure if we really need to freak out about that in the end | 16:39 |
| dviroel | I get your point on providing the pinned_az info to the Migrate Action too | 16:39 |
| amoralej_ | i'd say, if we can implement something simple, it's fine | 16:39 |
| dviroel | we could identify that an instance was pinned after the audit run | 16:40 |
| dviroel | or in a stale model | 16:40 |
| dviroel | but | 16:40 |
| dviroel | not sure if we should fix, or just make users aware of that | 16:40 |
| dviroel | maybe we should go simple | 16:40 |
| amoralej_ | i think skipping in that situation is fine | 16:41 |
| dviroel | the policy proposal is an extra thing in the end, allow users to Drain and also fix the previous pinning | 16:41 |
| amoralej_ | for me, it has value to let the users decide what to do with the instances migrated in terms of pinning | 16:41 |
| amoralej_ | initially, i think the idea was to always apply the "relocate" policy, right? | 16:42 |
| amoralej_ | for me that's the default and doing only that, i think the feature makes sense and is useful | 16:45 |
| amoralej_ | If we can easily add also the option to the users to define what to do with the pinned_az on destination, that's good and i see use cases for them | 16:45 |
| dviroel | sean-k-mooney: we are discussing it here too, just in case you are around :) | 16:51 |
| dviroel | amoralej_: right, my second concern was about changing the behavior of existing strategies | 16:51 |
| dviroel | the auto/relocate(in the spec) being the default would start to unpin/pin for all strategies in the end | 16:52 |
| amoralej_ | yes, i agree with that | 16:53 |
| sean-k-mooney | well it woudl only do that if your using AZ | 16:54 |
| sean-k-mooney | it woudlhave no effect if you were not passing a dest az right | 16:54 |
| amoralej_ | imo, th Migrate action should not try any unpin/pin, to not affect other strategies | 16:54 |
| sean-k-mooney | correct | 16:55 |
| sean-k-mooney | this woudl nto be somethign determind by the action | 16:55 |
| sean-k-mooney | it need to be spercfied by the stragy/desions enginv via the produce action plan | 16:55 |
| sean-k-mooney | actuions shoudl nto make desiones | 16:55 |
| amoralej_ | sean-k-mooney, we don't want zone_migrate to support repin using source_host destination_host ? | 16:55 |
| amoralej_ | i understood we want | 16:55 |
| sean-k-mooney | i dont think we shoudl no | 16:55 |
| sean-k-mooney | i mean we could but that does then change the behvior | 16:56 |
| amoralej_ | i understood we wanted those options to be also valid for host based migrations | 16:56 |
| sean-k-mooney | i was expecting us to need to opt into az aware migration via using the az | 16:56 |
| sean-k-mooney | if we are supporting it for all host based migrtion then the policy need to default differntly | 16:57 |
| amoralej_ | if we don't, it's much simpler, as you said, migrations by host keep current behavior, by az, new one | 16:57 |
| amoralej_ | that was the point, it's different the default value for the action and for the strategy | 16:58 |
| sean-k-mooney | we coudl supprot specifyign a dest az and host | 16:58 |
| sean-k-mooney | for change az and move to this host | 16:58 |
| sean-k-mooney | but the entrie point of the fature was tha tnova woudl choose the host when crossing azs | 16:58 |
| amoralej_ | yep | 16:59 |
| dviroel | we could initially not support the pin/unpin for dest-host | 17:00 |
| dviroel | i was thiking on making available for both | 17:01 |
| amoralej_ | So, i guess that's the first decission to make. if we want that include_az_pinned_instances and availability_zone_pinning_policy (or whatever name) applies only when using src_az and dst_az | 17:03 |
| dviroel | https://www.irccloud.com/pastebin/qEKz0iuA/ | 17:05 |
| dviroel | this is how it would looks like today. | 17:05 |
| dviroel | we would accept the 3 options for src_host/dest_host | 17:06 |
| dviroel | amoralej_: what is your opinion on that? | 17:08 |
| dviroel | I was originaly in favor of making to work with both AZ and Host | 17:08 |
| amoralej_ | me too, i understood we agreed on that, tbh | 17:08 |
| dviroel | but I was not expecting these issues | 17:08 |
| amoralej_ | but, it's also true that implementing the az options for host based strategies complicates it | 17:09 |
| sean-k-mooney | ```he central design decision of this proposal is that Watcher does not | 17:09 |
| sean-k-mooney | choose destination hosts. That choice is delegated to Nova, and depends on a | 17:09 |
| sean-k-mooney | new feature proposed by the `Nova migrate-to-availability-zone blueprint`_, | 17:09 |
| sean-k-mooney | which adds an optional ``availability_zone`` parameter to the move APIs. | 17:09 |
| sean-k-mooney | This keeps the strategy free of capacity accounting, avoids duplicating | 17:09 |
| sean-k-mooney | scheduler logic, and gives a real AZ guarantee that a "no destination" | 17:09 |
| sean-k-mooney | host cannot.``` | 17:09 |
| sean-k-mooney | that https://review.opendev.org/c/openstack/watcher-specs/+/1007853/8/specs/2027.1/approved/zone-migration-az-drain.rst#93 | 17:09 |
| sean-k-mooney | that to me ment thsi spec only added this fucntionality for moves where the AZ is specified | 17:10 |
| dviroel | sean-k-mooney: right, but at the same time we are improving some other pain points (e.g. counters, etc) | 17:11 |
| sean-k-mooney | we dotn have any usecase for movign to a specific host in a diffent az | 17:11 |
| dviroel | we don't | 17:11 |
| amoralej_ | "The following options are accepted on both node-pair and zone-pair entries: | 17:11 |
| amoralej_ | ``include_az_pinned_instances`` ..." | 17:11 |
| amoralej_ | i'm fine to decide now, anyway | 17:12 |
| dviroel | I took the decision to have it working for both in the end | 17:12 |
| dviroel | in my point of view, since these options were inside "compute_nodes" they could work for both | 17:12 |
| dviroel | same for "disable_source_services" | 17:13 |
| sean-k-mooney | fo waht its wroth i tink this is going to end up not happing this cycle | 17:13 |
| amoralej_ | yeah, for disable_source_services i see no conflict, we should support it in both | 17:13 |
| sean-k-mooney | its lookign more and more like we wil not have az for cold migration | 17:13 |
| sean-k-mooney | so we wont be able to proceed with the watcher feature | 17:13 |
| dviroel | ouch | 17:13 |
| sean-k-mooney | and im feeling pretty low because of that right now so i dont really have the engergy to go into this in deail now | 17:14 |
| amoralej_ | would we want to implement it only for active instances? | 17:14 |
| dviroel | it could work only for live, it would be another restriction for the AZ drain | 17:14 |
| sean-k-mooney | no | 17:14 |
| sean-k-mooney | i really dont think we shoudl | 17:14 |
| sean-k-mooney | tis a posibally but i dont think its reason to add AZ to only live migtion in nova | 17:14 |
| amoralej_ | ok, so let's discuss it on monday | 17:15 |
| dviroel | ack | 17:15 |
| sean-k-mooney | +1 for next week | 17:15 |
| dviroel | ack | 17:15 |
| amoralej_ | at least, now we know where to start the discussion from :) | 17:15 |
| sean-k-mooney | honestly though im pretty burntout now | 17:15 |
| dviroel | TL;DR; for me today is make it simple but functional | 17:16 |
| amoralej_ | no problem sean-k-mooney , takt time to rest | 17:16 |
| dviroel | sean-k-mooney: yeah, lets have that discussion next week | 17:16 |
| sean-k-mooney | exding the policy concept to support hosts is not really a problem | 17:17 |
| sean-k-mooney | we may have to change the allwoed options or have no default and reuiqre you set one if we woudl be changing az | 17:17 |
| sean-k-mooney | but lest regroup on monday | 17:18 |
| sean-k-mooney | if ye want to continue dicussisng it here go for it | 17:18 |
| dviroel | lets think a bit more and discuss on monday yeah | 17:19 |
| dviroel | if you folks have an idea, we can discuss and reach an agreement | 17:19 |
| dviroel | alread late for you guys | 17:20 |
| dviroel | for a friday at least | 17:20 |
| dviroel | take a rest o/ | 17:20 |
| -opendevstatus- NOTICE: The Gerrit service on review.opendev.org will be offline briefly while we reboot for system updates, but should return within a few minutes. | 17:43 | |
| amoralej_ | have a nice weekend! | 17:44 |
| dviroel | o/ | 18:13 |
Generated by irclog2html.py 4.1.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!