Add initial version of features mechanism #273

cescgina · 2022-05-24T14:35:47Z

This PR is marked as a draft while I finish writing tests, but it's ready for
review.

Features represent a new mechanism to query for information, particularly for
that information that can not be retrieved by a simple call combining
cli arguments.

Features are classes that inherit from the FeatureTemplate class and are
defined inside subpackages called 'features'. They must provide a
'query' method to be called, but the FeatureTemplate class provides on
that can be reused with slight tweaks in simple cases. The 'query' method
from The FeatureTemplate can be overriden for any given Feature, it's
not mandatory to call it.

Currently there are two
locations for features a general (cibyl/features, empty) and an openstack
specific (cibyl/plugins/openstack/features, three features provided, HA,
IPV4 and IPV6). Plugins that add features must add their paths to the
FeatureLoader class when the plugin is enabled.

In the orchestrator the features are loaded and executed (calling the
query method). If more than one feature is requested, their results are
combined. If the user uses other cli arguments like --jobs, the
intersection of the jobs that satisfy each feature will be finally
published. If not, for each system it will be simply published whether
each feature is present or not.

The result of a Feature query without --jobs is to add a Feature model
to the system queried. This should not be confused with any particular
Feature like (HA or IPV4). The Feature model is a ci model (similar to
Job or System) stored in cibyl/models/ci/base/feature.py and serves as
store for the result of a Feature query.

bregman-arie

Really great job. Very clean implementation. I had only really small comments but overall it looks great so far

cibyl/models/ci/base/feature.py

docs/source/development/features.rst

cibyl/plugins/openstack/features/network.py

bregman-arie

Should it give "False" for system that doesn't support the query?

ibyl --feature HA 
WARNING  cibyl.plugins.openstack.sources.jenkins Requesting deployment information for 1952 jobs will be based on the job name and approximate, restrict the query for more accurate results
ERROR    cibyl.features       Couldn't find any enabled source for the system zuul_cp that implements the function get_deployment.
Environment: osp_downstream_jenkins
  System: osp_jenkins
    HA feature: True
Environment: component_pipeline
  System: zuul_cp
    HA feature: False

Also, should it be case insensitive? so both cibyl --feature ha and cibyl --feature HA will work

bregman-arie · 2022-05-25T12:29:00Z

cibyl/models/ci/base/system.py

+            'attr_type': Feature,
+            'attribute_value_class': AttributeDictValue,
+            'arguments': [
+                Argument(name='--feature', arg_type=str,


What do you think about using "--features" instead of "--feature"? since it suppose to support more than one feature?

Makes sense, I'll change it

cibyl/orchestrator.py

cescgina · 2022-05-25T14:13:44Z

Should it give "False" for system that doesn't support the query?

Probably not, but I'm not sure what would be the best way to mark it. One option would be to print for each feature that failed to query a message, and then maybe a general one.

Environment: osp_phase2
ERROR    cibyl.features       Couldn't find any enabled source for the system osp_jenkins_phase2 that implements the function get_sth.
    System: osp_jenkins_phase2
    Test feature: No query was performed
    No query performed

Another would be to follow the same strategy that we have used until now with the normal queries and simply print a general message:

Environment: osp_phase2
ERROR    cibyl.features       Couldn't find any enabled source for the system osp_jenkins_phase2 that implements the function get_sth.
    System: osp_jenkins_phase2
    No query performed

What do you think, @bregman-arie?

Also, should it be case insensitive? so both cibyl --feature ha and cibyl --feature HA will work

I think that's a good idea and simple change, I'll implement it.

bregman-arie · 2022-05-25T15:02:27Z

Should it give "False" for system that doesn't support the query?

Probably not, but I'm not sure what would be the best way to mark it. One option would be to print for each feature that failed to query a message, and then maybe a general one.
Environment: osp_phase2
ERROR    cibyl.features       Couldn't find any enabled source for the system osp_jenkins_phase2 that implements the function get_sth.
    System: osp_jenkins_phase2
    Test feature: No query was performed
    No query performed
Another would be to follow the same strategy that we have used until now with the normal queries and simply print a general message:
Environment: osp_phase2
ERROR    cibyl.features       Couldn't find any enabled source for the system osp_jenkins_phase2 that implements the function get_sth.
    System: osp_jenkins_phase2
    No query performed
What do you think, @bregman-arie?

Also, should it be case insensitive? so both cibyl --feature ha and cibyl --feature HA will work

I think that's a good idea and simple change, I'll implement it.

My concern is that users sometimes focus on very specific lines and when a user sees this line:

HA: False

What comes into mind (at least for me) is that HA is not tested in that environment, but this is not what it necessarily means. We can print as many warning/error messages as we want before that line, but eventually, sooner or later, some user will take this as "HA is not tested in environment X".
I want us to think if there is a way to explicitly state that Cibyl doesn't knows if a feature is tested in that environment. Something like perhaps the spec where we use "N/A".

cescgina · 2022-05-25T15:04:41Z

Using the same message for not available as in the spec is a good idea, I'll use that. Thanks!

bregman-arie

Works flawlessly. Amazing job

jsanemet

I have found problems with the proposal. The code is tangled at times and I am not really fond of how FeatureTemplate has been designed. Take a look at my comments and we can discuss these.

cibyl/cli/query.py

cibyl/exceptions/__init__.py

cibyl/features/__init__.py

jsanemet · 2022-06-02T09:36:29Z

cibyl/features/__init__.py

+    """Load all features available, either general or product specific through
+    some plugin."""
+    # __path__ is a list
+    features_locations = __path__


Again, if you are going to use reflection, try to be as flexible as possible. Hard coding the path does not leave as much room for change.

The main way to introduce new paths for features will be thorugh plugins, that's way it's left as a class attribute. That's said I can certainly see the value in having an option to pass paths through the constructor, I'll add it.

I still do not like this solution. That is an static variable, which means that if you make a change to it you will be affecting all instances of the class. That is an easy error to happen and one that can be difficult to debug. I would suggest that you either go with a fully encapsulated object or you have this as a python module without classes, just functions and global variables.

The funny thing about Python, is that depending on the type the variable is shared by all instances or not:

feature_locations = 1 - This will not

feature_locations = [] - But this will

Yes, I'm aware of that, I did not think that would be a problem really, since I can't imagine a reason to have two instances of the class. Another option would be to make the FeatureLoader a Singleton class, but having everything at the module level is probably simpler and would achieve the same goal.

tests/unit/features/test_features.py

cibyl/orchestrator.py

cibyl/outputs/cli/ci/system/impls/base/colored.py

jsanemet · 2022-06-02T10:25:31Z

cibyl/plugins/openstack/__init__.py

@@ -68,6 +69,8 @@ def extend_models(self):
                    self.plugin_attributes_to_add)
            setattr(job_class, 'add_deployment',
                    add_deployment)
+        FeaturesLoader.features_locations.append(f"{__path__[0]}/features")


Another reason as to pass the path on the constructor. How easy it is to miss this change during debugging.

I'm not sure about this one, this follows the way we have been using the plugins. To make it more explicit I'll move it into a different method, for better visibility.

cibyl/outputs/cli/ci/system/impls/jobs/colored.py

jsanemet · 2022-06-02T10:30:56Z

cibyl/plugins/openstack/features/__init__.py

+
+class OpenstackFeatureTemplate(FeatureTemplate):
+    """Skeleton for an openstack specific feature."""
+    method_to_query = "get_deployment"


Have this be given through an abstract method, that is what they are for:

@abstractmethod def get_method_to_query(self): pass

I like the idea, thanks!

cescgina · 2022-06-02T13:19:06Z

Thanks for the thorough review @jsanemet! I've tried to address all the comments, and again sorry for the size of the PR.

jsanemet · 2022-06-03T13:27:20Z

cibyl/features/__init__.py

+    """Load all features available, either general or product specific through
+    some plugin."""
+    # __path__ is a list
+    features_locations = __path__


I still do not like this solution. That is an static variable, which means that if you make a change to it you will be affecting all instances of the class. That is an easy error to happen and one that can be difficult to debug. I would suggest that you either go with a fully encapsulated object or you have this as a python module without classes, just functions and global variables.

The funny thing about Python, is that depending on the type the variable is shared by all instances or not:

feature_locations = 1 - This will not

feature_locations = [] - But this will

cibyl/features/__init__.py

cibyl/orchestrator.py

jsanemet · 2022-06-03T13:30:24Z

I have gone through the changes and I still have some beef with the FeatureLoader class. Everything else is good to go I believe.

cescgina · 2022-06-03T14:00:58Z

I've updated the proposal moving to functionality of the FeatureLoader class to module-level functions

jsanemet

Great, everything is good to go for me.

Introduce a potential mechanism for features in cibyl. Features represent a new mechanism to query for information, particularly for that information that can not be retrieved by a simple call combining cli arguments. Features are classes that inherit from the FeatureTemplate class and are defined inside subpackages called 'features'. They must provide a 'query' method to be called, but the FeatureTemplate class provides on that can be reused with slight tweaks in simple cases. The 'query' method from The FeatureTemplate can be overriden for any given Feature, it's not mandatory to call it. Currently there are two locations for features a general (cibyl/features, empty) and an openstack specific (cibyl/plugins/openstack/features, three features provided, HA, IPV4 and IPV6). Plugins that add features must add their paths to the FeatureLoader class when the plugin is enabled. In the orchestrator the features are loaded and executed (calling the query method). If more than one feature is requested, their results are combined. If the user uses other cli arguments like --jobs, the intersection of the jobs that satisfy each feature will be finally published. If not, for each system it will be simply published whether each feature is present or not. The result of a Feature query without --jobs is to add a Feature model to the system queried. This should not be confused with any particular Feature like (HA or IPV4). The Feature model is a ci model (similar to Job or System) stored in cibyl/models/ci/base/feature.py and serves as store for the result of a Feature query.

cescgina added Core This issue changes/adds application core component Models Output 📝 Plugins 🔌 enhancement 💡 New feature or request labels May 24, 2022

cescgina requested a review from a team May 24, 2022 14:35

cescgina force-pushed the features branch from c7fa373 to 6d4b0d5 Compare May 24, 2022 15:58

bregman-arie reviewed May 25, 2022

View reviewed changes

cibyl/models/ci/base/feature.py Outdated Show resolved Hide resolved

cibyl/models/ci/base/feature.py Outdated Show resolved Hide resolved

docs/source/development/features.rst Outdated Show resolved Hide resolved

cibyl/plugins/openstack/features/network.py Outdated Show resolved Hide resolved

cescgina force-pushed the features branch 3 times, most recently from 630e047 to 01403bd Compare May 25, 2022 12:24

bregman-arie suggested changes May 25, 2022

View reviewed changes

cescgina force-pushed the features branch 2 times, most recently from 0015674 to b3a1b6d Compare May 25, 2022 13:46

cescgina marked this pull request as ready for review May 25, 2022 14:15

cescgina force-pushed the features branch from b3a1b6d to 4c0b11b Compare May 25, 2022 15:23

cescgina requested review from bregman-arie and a team May 26, 2022 07:08

bregman-arie approved these changes May 26, 2022

View reviewed changes

cescgina requested a review from a team May 26, 2022 10:15

cescgina force-pushed the features branch 2 times, most recently from 4df1969 to f7f8a65 Compare May 30, 2022 11:59

jsanemet suggested changes Jun 2, 2022

View reviewed changes

cescgina force-pushed the features branch from f7f8a65 to 082754c Compare June 2, 2022 13:17

cescgina requested a review from jsanemet June 2, 2022 13:19

jsanemet suggested changes Jun 3, 2022

View reviewed changes

cescgina force-pushed the features branch from 082754c to 15b1c3b Compare June 3, 2022 13:29

cescgina force-pushed the features branch from 15b1c3b to 82a7217 Compare June 3, 2022 14:00

jsanemet approved these changes Jun 6, 2022

View reviewed changes

bregman-arie force-pushed the features branch from 82a7217 to 65f50d4 Compare June 8, 2022 09:28

bregman-arie approved these changes Jun 8, 2022

View reviewed changes

bregman-arie merged commit 62846b3 into RedHatCRE:main Jun 8, 2022

cescgina deleted the features branch June 9, 2022 07:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial version of features mechanism #273

Add initial version of features mechanism #273

cescgina commented May 24, 2022

bregman-arie left a comment

bregman-arie left a comment

bregman-arie May 25, 2022

cescgina May 25, 2022

cescgina commented May 25, 2022

bregman-arie commented May 25, 2022

cescgina commented May 25, 2022

bregman-arie left a comment

jsanemet left a comment

jsanemet Jun 2, 2022

cescgina Jun 2, 2022

jsanemet Jun 3, 2022

cescgina Jun 3, 2022

jsanemet Jun 2, 2022

cescgina Jun 2, 2022

jsanemet Jun 2, 2022

cescgina Jun 2, 2022

cescgina commented Jun 2, 2022

jsanemet Jun 3, 2022

jsanemet commented Jun 3, 2022

cescgina commented Jun 3, 2022

jsanemet left a comment

Add initial version of features mechanism #273

Add initial version of features mechanism #273

Conversation

cescgina commented May 24, 2022

bregman-arie left a comment

Choose a reason for hiding this comment

bregman-arie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cescgina commented May 25, 2022

bregman-arie commented May 25, 2022

cescgina commented May 25, 2022

bregman-arie left a comment

Choose a reason for hiding this comment

jsanemet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cescgina commented Jun 2, 2022

Choose a reason for hiding this comment

jsanemet commented Jun 3, 2022

cescgina commented Jun 3, 2022

jsanemet left a comment

Choose a reason for hiding this comment