| opendevreview | chandan kumar proposed openstack/watcher-tempest-plugin master: Refactor: Extract common test utilities into shared base class https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/967522 | 06:36 |
|---|---|---|
| opendevreview | chandan kumar proposed openstack/watcher-tempest-plugin master: Improved continuous audit api test coverage https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/956004 | 06:55 |
| opendevreview | chandan kumar proposed openstack/watcher-tempest-plugin master: Refactor: Extract common test utilities into shared base class https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/967522 | 07:10 |
| opendevreview | chandan kumar proposed openstack/watcher-tempest-plugin master: Improved continuous audit api test coverage https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/956004 | 07:12 |
| opendevreview | David proposed openstack/watcher master: [DNM] Testing nodeset with three nodes (two computes + 1 controller) https://review.opendev.org/c/openstack/watcher/+/967331 | 10:31 |
| opendevreview | David proposed openstack/watcher master: [DNM] Testing nodeset with three nodes (two computes + 1 controller) https://review.opendev.org/c/openstack/watcher/+/967331 | 10:32 |
| opendevreview | David proposed openstack/watcher master: [DNM] Testing nodeset with three nodes (two computes + 1 controller) https://review.opendev.org/c/openstack/watcher/+/967331 | 10:33 |
| opendevreview | David proposed openstack/watcher master: [DNM] Testing nodeset with three nodes (two computes + 1 controller) https://review.opendev.org/c/openstack/watcher/+/967331 | 10:35 |
| opendevreview | chandan kumar proposed openstack/watcher-tempest-plugin master: Refactor: Extract common test utilities into shared base class https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/967522 | 10:40 |
| opendevreview | chandan kumar proposed openstack/watcher-tempest-plugin master: Add Continuous audit dummy strategy scenario test https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/955472 | 11:33 |
| opendevreview | chandan kumar proposed openstack/watcher-tempest-plugin master: Improved continuous audit api test coverage https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/956004 | 11:34 |
| opendevreview | chandan kumar proposed openstack/watcher-tempest-plugin master: Add Continuous audit dummy strategy scenario test https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/955472 | 11:43 |
| dviroel | sean-k-mooney: jgilaber: hi o/ - please revisit https://review.opendev.org/c/openstack/watcher/+/966226 once yo guys have some time, I solved the pending comment from jgilaber, let me known if we can proceed with it | 11:47 |
| amoralej | sean-k-mooney, dviroel jgilaber I'd like to discuss about https://review.opendev.org/c/openstack/watcher/+/967693 as I got contradictory feedback about adding the parameters for timeout in the method. While sean-k-mooney asked me explicitely to remove the parameter until we implement adding timeout parameter to the strategies/audits, the rest seems to be expeting to have parameters in the method for both timeout and | 11:57 |
| amoralej | polling interval | 11:57 |
| amoralej | the second open question is about the config sections where to put the parameters. I used watcher_applier as I found it more aligned with most of the config options which are per service, we could also introduce new per-action sections | 11:59 |
| sean-k-mooney | so i generally consider dead code to be a potential security issue. if we dont plan ot use it in teh short term i dont like adding supprot for future use likt that | 12:01 |
| sean-k-mooney | in this case it shoudl be safe | 12:01 |
| sean-k-mooney | but it a general anti patthern that i alwasy review for | 12:01 |
| amoralej | adding support to use it will not be backported, anyway | 12:02 |
| jgilaber | I suggested to add the parameters to the helper code and pass them from the action code (where for now we would read from the config) | 12:02 |
| sean-k-mooney | so if we were followign normal convetion the watcher.conf woudl not have watcher for any config section | 12:02 |
| jgilaber | in the future we would add the same to the action and propagate them from the audit parameter if set | 12:02 |
| sean-k-mooney | jgilaber: that an option i guess | 12:03 |
| sean-k-mooney | at least it woudl be used and the logic of which vaule to sue could be encaploatied hierh up | 12:03 |
| amoralej | at some point, we may have multiple actions using the migration helper function. Managing the defaults from config in the action may require duplicated defaults | 12:03 |
| sean-k-mooney | however the descion enging cant be assumed to have access to these config options | 12:03 |
| sean-k-mooney | so in either cae the value woudl be read form the config in the applier | 12:03 |
| jgilaber | +1, the strategy would only have access to the audit parameters (so no access in the current patch, only in a future one) | 12:04 |
| sean-k-mooney | so im not sold on addign this as an audit parmater yet | 12:04 |
| sean-k-mooney | it a possiblity but that corssing admin domain to a degree | 12:04 |
| sean-k-mooney | im not -1 on that idea eihter | 12:05 |
| sean-k-mooney | its just not somethign that is obviously the correct thing to do | 12:05 |
| sean-k-mooney | timeouts are not somethign nova allows ot be set at the api for exampel nor does cinder | 12:05 |
| sean-k-mooney | so i am not convice watcher shoudl either | 12:05 |
| dviroel | I think that we can forget about timeouts in audits for now | 12:06 |
| sean-k-mooney | my currernt view is this cycle we woudl have config only and we woudl reasses in a release or two if it really needs to eb in the audit parmaters or not | 12:06 |
| amoralej | so, if no actions no need to propagate as an action parameter | 12:06 |
| dviroel | i was raising that we should keep the helper interface, accepting the timeouts as parameters | 12:06 |
| sean-k-mooney | dviroel: right so i didnt eant to do that unless its used | 12:07 |
| sean-k-mooney | dviroel: if we move the reading of the config value out of the function and make timeout a required postional argument tehn that ok | 12:08 |
| dviroel | i prefer that way | 12:08 |
| dviroel | because is something that we plan to backport | 12:08 |
| sean-k-mooney | i dont want a partmer in the funciton signature that is not sue because when i read the code that imples that there is a way to configure it to a diffent value | 12:08 |
| sean-k-mooney | which is hwy the existign code was consuing when we dicsoverd it was never passed | 12:08 |
| amoralej | so, you problem with backporting is to break backwards compatibility? | 12:09 |
| dviroel | we could have any other custom action plugin out there, that is using this helper | 12:09 |
| amoralej | because in any case it was requested to change the name from retry (which was missleading) to timeout | 12:09 |
| sean-k-mooney | hum your talking about out of tree plugins | 12:09 |
| dviroel | yes | 12:10 |
| amoralej | it's unlikely I'd say, but it's possible | 12:10 |
| sean-k-mooney | thats a diffent angel. im trying to decied if this is part of our public contract and i really want to say no | 12:10 |
| sean-k-mooney | but i can see your point | 12:10 |
| dviroel | yeah, the point is if we can avoid that, why not maintain? | 12:10 |
| amoralej | technically, actions are plugable drivers and parameters are part of the contract, yes | 12:10 |
| sean-k-mooney | because the exissting names are inaccurate and this is creating techial debt as a result | 12:11 |
| sean-k-mooney | i.e. i would want to rename it form retry to timeout to refect it actull usage and sematic change | 12:11 |
| amoralej | dviroel, so https://review.opendev.org/c/openstack/watcher/+/967693/5/watcher/common/nova_helper.py is what you meant, right? | 12:11 |
| sean-k-mooney | dviroel: if we were to keep compaitblity | 12:12 |
| sean-k-mooney | isntead of a new timeout parmter we woudl add a retries config option | 12:12 |
| sean-k-mooney | so interval and retry not timeout and interval | 12:12 |
| sean-k-mooney | if we do that change | 12:12 |
| sean-k-mooney | then i think we can keep the current signiture with retry | 12:12 |
| amoralej | I can also do that, I feel configuring timeouts is more clear that retries, but can be done | 12:13 |
| sean-k-mooney | well we do both in diffent places in openstack | 12:13 |
| amoralej | should the polling interval also exposed in the method signature?, I'd say not if we are only doing this for compatibility | 12:14 |
| dviroel | yeah, i forgot about that today is a 'retry', i was just thinking on the timeout.. | 12:14 |
| sean-k-mooney | we tend to use retry + interval more when the interval is configurable | 12:14 |
| sean-k-mooney | and tend to just use tiemout if its not | 12:14 |
| sean-k-mooney | dviroel: jgilaber any opion on that? | 12:15 |
| sean-k-mooney | amoralej: by the way why are you not update resize_instance? https://review.opendev.org/c/openstack/watcher/+/967693/7/watcher/common/nova_helper.py#240 | 12:16 |
| amoralej | I took that as a different issue | 12:17 |
| amoralej | simply for scope | 12:17 |
| sean-k-mooney | cold migrate in nova is implented in terms fo a resize to tehe same flavor | 12:17 |
| sean-k-mooney | so eihter we shoudl include that or you shodul not be modifying watcher_non_live_migrate_instance | 12:17 |
| amoralej | I'd say resize it's not used ? | 12:18 |
| sean-k-mooney | we shoudl verify that and deleted it if so but that can be a diffent commit | 12:18 |
| amoralej | so, for me the scope of the bug is the Migrate Action | 12:18 |
| amoralej | which calls both | 12:18 |
| sean-k-mooney | the socpe of the bug was orginaly just live migrate | 12:19 |
| amoralej | that's why i updated live and non-live but not resize | 12:19 |
| sean-k-mooney | which is why i was wondering why you expaned it to cold migrate but not reize | 12:19 |
| sean-k-mooney | but ok | 12:19 |
| amoralej | I can do whatever you asked me tbh | 12:19 |
| dviroel | in the helper we can 1) move to retry + interval OR 2) add timeout and interval, keep the retry parameter (for backward compatibility) and translate it to a timeout inside the code? | 12:20 |
| sean-k-mooney | we do have a resize action for what its worth https://github.com/openstack/watcher/blob/8a884e3d5166678b85eaf264e518ef24b30085e5/watcher/applier/actions/resize.py#L27 | 12:20 |
| dviroel | maybe 1) is easierin the end | 12:20 |
| jgilaber | about the retry, we could keep the parameter without exposing it in the config, just exposing timeout and interval if we feel that is clearer for the user | 12:20 |
| amoralej | yes, i meant we don't use the resize action, iirc | 12:20 |
| jgilaber | if we read the config options in the action we could calculate the retry and pass the value to the helper method | 12:20 |
| amoralej | so, a requirement is to keep the retry parameter in the method | 12:21 |
| amoralej | for backwards compatibility | 12:21 |
| amoralej | we are mixing different questions. Let me try to summarize, please: | 12:22 |
| amoralej | 1. Want to maintain backwards compatibility in the method. | 12:23 |
| amoralej | 2. We can manage reading the new config options in the actions or in the helper method. Here I'm in favor of doing in the helper so that it applies to any action using it, unless we want to have different per-action defaults in config | 12:24 |
| amoralej | note that ^ it's not incompatible with setting timeout from the action in future via retry + interval parameters in the method | 12:25 |
| sean-k-mooney | ya we can do an or | 12:25 |
| amoralej | 3. we want to expose also the polling_interval in the method? | 12:26 |
| sean-k-mooney | retry = retry or conf... | 12:26 |
| * dviroel let us know when you finish your summary :) | 12:26 | |
| amoralej | 4. which section is better based on decissions in ^ ? | 12:26 |
| amoralej | 5. timeouts on resize? for me that's a different patch | 12:26 |
| amoralej | that was it :) | 12:26 |
| sean-k-mooney | so resize is useable via the actator stragey so it cant be assuemd to be unsued but we can adress it seperatly | 12:27 |
| sean-k-mooney | with the constraits you have list i would suggest keepign retry and adding interval as a new kwarg defaulting both to NONE | 12:28 |
| jgilaber | on number 2 my thoughts were the opposite, I would prefer separate action logic from the helper methods and read the config options from the action | 12:28 |
| amoralej | wrt retry or conf? yes, retry parameter will override conf | 12:28 |
| sean-k-mooney | then in the helper do retry = retry or CONF.whatever | 12:28 |
| amoralej | that's what i did with timeout in https://review.opendev.org/c/openstack/watcher/+/967693/5/watcher/common/nova_helper.py | 12:28 |
| amoralej | think in something similar but with retry and polling | 12:29 |
| sean-k-mooney | jgilaber: im ok with either but i woudl perfer the config to be used as the default not a hard code 120 | 12:29 |
| jgilaber | that's a fair point yes | 12:29 |
| dviroel | 2. Both ways works, it is about preference here I think. If you you want to provide defaults for the Helper, you should accept a configuration via kwargs (as we discussed above and in point 1.) | 12:30 |
| amoralej | I assume jgilaber option would be to have something like action_migrate.migrate_retry = X in the cfg | 12:30 |
| amoralej | and pass it to the helper retry | 12:30 |
| jgilaber | we should have the config default as the default so it kind of makes my point moot | 12:30 |
| dviroel | 4. If the default is used in the common helper, I don't think that the config option should live in the Applier in that case | 12:31 |
| dviroel | we could have an 'integrations' session to add config options to these integrations | 12:32 |
| dviroel | idea^ | 12:32 |
| amoralej | https://docs.openstack.org/watcher/latest/configuration/watcher.html those are the existing sections | 12:33 |
| sean-k-mooney | we could just put it in the default section | 12:33 |
| sean-k-mooney | or nova_client | 12:33 |
| amoralej | i was thinking about nova_client too | 12:33 |
| amoralej | althoug the existing options are very tied to the nova client options, i'm not sure | 12:34 |
| sean-k-mooney | so my concern with that is that section shoud go away eventually when we adopt hte openstack client | 12:34 |
| amoralej | right | 12:34 |
| sean-k-mooney | i have been holding my nose a bit with our configs because ther eare many antipatterns that i woudl liek to fix | 12:34 |
| amoralej | what if we create a new nova_helper ? | 12:34 |
| sean-k-mooney | but i has never felt woth the effort | 12:35 |
| sean-k-mooney | if we add a new section i shoudl jsut be [nova] | 12:35 |
| amoralej | what was the right why to namespace configs, you mentioned . was not correct, right? | 12:35 |
| sean-k-mooney | that part of it your not ment ot use . in the config path | 12:36 |
| amoralej | [nova] wfm and nova.migration_retry nova.migration_polling_interval | 12:36 |
| sean-k-mooney | that yep that what htey woudl be in this case | 12:36 |
| sean-k-mooney | service are not ment ot use the name of there service in there own config file | 12:36 |
| dviroel | +1, it should work for future work | 12:36 |
| jgilaber | if we put the options under some nova related section it makes more sense to read them in the helper | 12:36 |
| sean-k-mooney | so wather_applier shoudl just be [applier] | 12:37 |
| amoralej | jgilaber, yep, that's why i said, if we want to do it in the actions, we should add it to a different sections [actions] or similar | 12:37 |
| sean-k-mooney | when we refger to other services config section we default to eh service name i.e. [nova] [placement] ectra so the _client sufix is odd | 12:37 |
| amoralej | I understand the existing nova_client was because it was strictly created for the client initialization | 12:38 |
| sean-k-mooney | correct | 12:38 |
| amoralej | so, we should not add anything there | 12:38 |
| amoralej | anything else, i mean | 12:38 |
| sean-k-mooney | watcher also does not do htat in the standard way by useing keyston_auth with the ablity to over ried endpoint/urls per service | 12:39 |
| sean-k-mooney | watcher cant talk to a nova and placment that dont share a common ca for example | 12:40 |
| amoralej | so, back to the summary: about [1] I think we agree in keeping compatibility | 12:41 |
| amoralej | about [2] Is there agreement on doing the config options in the helper but overriding the retry and polling_interval with method parameters? | 12:41 |
| amoralej | and using [nova] as section | 12:42 |
| sean-k-mooney | +1 alhtough lets just call it interval instead of pooling_interval | 12:42 |
| jgilaber | I'm +1 to those 2 points | 12:42 |
| amoralej | ack | 12:42 |
| sean-k-mooney | i would perfer migrate_interval for the config option name also | 12:43 |
| amoralej | yes | 12:43 |
| amoralej | I'll remove polling from both | 12:43 |
| sean-k-mooney | polling is not commonly used in config files as a name | 12:43 |
| sean-k-mooney | ack | 12:43 |
| sean-k-mooney | if we are standardizing | 12:44 |
| amoralej | jgilaber, dviroel, about [3] and [4], exposing the interval and using [nova] is fine? | 12:44 |
| sean-k-mooney | do we want to align to thet data sources | 12:44 |
| sean-k-mooney | https://docs.openstack.org/watcher/latest/configuration/watcher.html#watcher_datasources.query_max_retries | 12:44 |
| sean-k-mooney | https://docs.openstack.org/watcher/latest/configuration/watcher.html#watcher_datasources.query_interval | 12:44 |
| sean-k-mooney | so migration_max_retries and migration_interval | 12:44 |
| amoralej | so max_retries instead of retry? note that the parameter will keep as retry | 12:44 |
| sean-k-mooney | yep and yep | 12:45 |
| amoralej | but it's not a problem | 12:45 |
| dviroel | amoralej: yes for [3] and [4] | 12:45 |
| jgilaber | exposing both as optional arguments while having the nova.* config opts as default wfm | 12:45 |
| amoralej | ack, thanks for your time! I will implement that | 12:46 |
| sean-k-mooney | cool i think we shoudl all be in alingment now so the next revision will likely be fine to proceed with. | 12:47 |
| sean-k-mooney | we shoudl loop back to resize after but we can do htat in a seperate bug/followup | 12:48 |
| dviroel | +1 | 12:48 |
| sean-k-mooney | as we mentioned before we likely need to examping all action and effectivly do the same | 12:48 |
| sean-k-mooney | at least assses if they are pooling ofr an async action and ensure that is configurable with standard parmater and config nameing scheme | 12:48 |
| sean-k-mooney | s/standard/consitent/ | 12:49 |
| sean-k-mooney | so for volume migration we can add migrate_max_retires and migrate_interval in the cinder section | 12:49 |
| amoralej | yes, I will take care of the resize too and will try to it similarly | 12:49 |
| sean-k-mooney | for what its worth i think resize can use the saem migration_* config options for now | 12:50 |
| sean-k-mooney | on the nova side | 12:50 |
| amoralej | for cinder, my problem is about what can be a good default | 12:50 |
| amoralej | as data migration can take very long times | 12:50 |
| amoralej | and that's expected | 12:50 |
| sean-k-mooney | amoralej: ya i dont know the answer there but problym start with a simlear default and we can alwasy increase it | 12:51 |
| sean-k-mooney | i have not looked to see if we have one hardcoded today | 12:51 |
| amoralej | neither me | 12:51 |
| sean-k-mooney | https://github.com/openstack/watcher/blob/master/watcher/common/cinder_helper.py#L151 | 12:52 |
| sean-k-mooney | so we do | 12:52 |
| sean-k-mooney | we lep for 10 seconds | 12:52 |
| sean-k-mooney | up to 10 times | 12:53 |
| sean-k-mooney | so 100s total | 12:53 |
| sean-k-mooney | oh no | 12:53 |
| sean-k-mooney | we sleep for 10 second indefintalty until it enters error ro success | 12:53 |
| sean-k-mooney | there is not retry limit today | 12:53 |
| sean-k-mooney | so we will need to choose a sane limit later. we could start with just making the interval configurable | 12:55 |
| sean-k-mooney | let leave that till later to decide | 12:55 |
| chandankumar | dviroel: hello, please add it to review list. https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/967522: Refactor: Extract common test utilities into shared base class, thank you! | 14:10 |
| chandankumar | dviroel: sean-k-mooney: https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/956004/ also (not super urgent),thank you! | 14:11 |
| opendevreview | Merged openstack/watcher-tempest-plugin master: Refactor: Extract common test utilities into shared base class https://review.opendev.org/c/openstack/watcher-tempest-plugin/+/967522 | 18:02 |
| *** haleyb is now known as haleyb|out | 22:51 | |
Generated by irclog2html.py 4.0.0 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!