From: "Yujun Zhang (ZTE)" <zhangyujun+...@gmail.com> Date: Thursday, 11 January 2018 at 10:52
I have almost understood it thanks to your explanation. [Ifat] I liked the “almost” ;-) The confusion is mainly caused by the naming. I guess the main reason is that the scope evolves but the naming is not updated with it. For example 1. `vitrage_aggregated_state` actually applies for both resource state and alarm severity as defined in `value_properties`. So `vitrage_aggregated_values` could be a better name. [Ifat] For alarms we use ‘vitrage_aggregated_severity’ 2. For data source in static configuration, we may use `static.yaml` as a fallback. The name `default.yaml` will mislead user that it should be applied to data source configured in "types" but without a values configuration. [Ifat] We should decide whether we want the default values to apply also to “real” datasources. I think the risk is that people who write a new datasource will forget to add the values yaml file, and will believe that everything works fine with the default. Then, upon a specific failure (that doesn’t happen often) they will get UNDEFINED status. On the other hand, if they always get UNDEFINED, they will remember to add the correct yaml file. 3. The UNDEFINED value is named UNDEFINED_DATASOURCE = "undefined datasource", it is not a consistent type of severity and state enumeration. [Ifat] I didn’t understand this comment. 4. The behavior for data source defined in static without values configuration and data source defined in "types" without values configuration are inconsistent. The former will fallback to `default.yaml` but the latter will lead to undefined value. [Ifat] See my answer to #2. I know it is there for historical reasons and current developers may already get used to it, but it gives new contributors too many surprises. What do you think? Shall we amend them? On Tue, Jan 9, 2018 at 11:29 PM Afek, Ifat (Nokia - IL/Kfar Sava) <ifat.a...@nokia.com<mailto:ifat.a...@nokia.com>> wrote: Hi, I agree that the code is confusing… This is part of a change that was made in order to support default states for static entities. For example, in the static configuration yaml file you can add entities of types ‘switch’ and ‘br-ex’. In the past, in order to support states for these new types, you needed to add switch.yaml and br-ex.yaml under /etc/vitrage/datasources_values, which you would most likely copy&paste from another datasource. Now, we have under /etc/vitrage/datasources_values a default.yaml file that is used for all static entities. Back to the code, I believe this is the logic: • If the datasource is part of ‘types’ (as defined in vitrage.conf) and has states configuration – use it. This is the normal behavior. • If the datasource is not part of ‘types’, we understand that it was defined in a static configuration file. Use the default states configuration. I assume that it is somehow handled in the first part of the if statement (I’m not so familiar with that code) • If neither is true – it means that the datasource is “real” and not static, and was defined in vitrage.conf types. And it also means that its states configuration is missing, so the state is UNDEFINED. And to your questions: 1. the data source is not defined -> the default states should be used 2. data source defined but state config not exist -> UNDEFINED state 3. data source defined, state config exist but the state is not found. -> I believe that somewhere in the first part of the if statement you will get UNDEFINED Hope that’s more clear now. It might be a good idea to add some comments to that function… Best Regards, Ifat. From: "Yujun Zhang (ZTE)" <zhangyujun+...@gmail.com<mailto:zhangyujun%2b...@gmail.com>> Reply-To: "OpenStack Development Mailing List (not for usage questions)" <openstack-dev@lists.openstack.org<mailto:openstack-dev@lists.openstack.org>> Date: Tuesday, 9 January 2018 at 8:34 To: "OpenStack Development Mailing List (not for usage questions)" <openstack-dev@lists.openstack.org<mailto:openstack-dev@lists.openstack.org>> Subject: Re: [openstack-dev] [vitrage] rules in vitrage_aggregated_state() Forgot to paste the link to the related code: https://git.openstack.org/cgit/openstack/vitrage/tree/vitrage/entity_graph/mappings/datasource_info_mapper.py#n61 On Tue, Jan 9, 2018 at 2:34 PM Yujun Zhang (ZTE) <zhangyujun+...@gmail.com<mailto:zhangyujun%2b...@gmail.com>> wrote: Hi root causers I have been inspecting the code about aggregated state recently and have a question regarding the rules. The "not" operator in the if clause confuses me. If it is not a configured data source, how do we apply the aggregation rules? It seems this is handled in else clause. if datasource_name in self.datasources_state_confs or \ datasource_name not in self.conf.datasources.types: ... else: self.category_normalizer[vitrage_category].set_aggregated_value( new_vertex, self.UNDEFINED_DATASOURCE) self.category_normalizer[vitrage_category].set_operational_value( new_vertex, self.UNDEFINED_DATASOURCE) There are some test case describing the expected behavior. But I couldn't understand the design philosophy behind it. What is expected when 1. the data source is not defined 2. data source defined but state config not exist 3. data source defined, state config exist but the state is not found. Could somebody shed some light on it? -- Yujun Zhang -- Yujun Zhang __________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe<http://openstack-dev-requ...@lists.openstack.org?subject:unsubscribe> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev -- Yujun Zhang
__________________________________________________________________________ OpenStack Development Mailing List (not for usage questions) Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
