Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend LEAPP with actor configuration #870

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

abadger
Copy link
Member

@abadger abadger commented Jul 2, 2024

Provide a new unified solution to enable User-Configurable actors.

The point is to introduce mechanism how various parts of workflow could be configured, on level actors which should be able to easily access configuration related to them (e.g. via a framework API if designed in such a way). So users will not have to create their own custom actors for all possibilities we provide right now, but they could simply create/update configuration files to achieve what they want.

Issues:

  • Reading configs on RHEL7 (Python 2.7) fails, crashing the entire framework because glob.glob has no kw argument recursive=True

JIRA: OAMG-8803

@abadger abadger marked this pull request as draft July 2, 2024 08:01
@pirat89 pirat89 added this to the 8.10/9.5 milestone Jul 2, 2024
@pirat89 pirat89 modified the milestones: 8.10/9.5, 8.10/9.6 Aug 16, 2024
@abadger abadger force-pushed the actor-config branch 9 times, most recently from 3f26947 to b1f67db Compare September 13, 2024 08:52
@dkubek
Copy link
Member

dkubek commented Sep 16, 2024

Adding quick note here so we won't forget. We will want to include the config information also in leapp.db. If we make the Config serializable we could simply edit store_actor_metadata in leapp/utils/audit/__init__.py and it should work well enough.

Comment on lines 485 to 510
_get_attribute(actor, 'config_schemas', _is_type(Config), required=False,
default_value=actor.__doc__ or 'Description of the configuration used by this actor.'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will want here something similar like _is_config_tuple similar to the functions above. That is if I understand it correctly and we are allowing multiple config schema definitions per actor. Am I right @abadger ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! I'ce implemented such a function. The one change I made from how _is_model_tuple() and etc operate is to check for a Sequence instead of a Tuple. This allows Tuples, Lists, NamedTuples, to all satisfy the requirement rather than only tuples. If that shouldn't be the case, we can change it back.

@@ -86,6 +92,7 @@ def serialize(self):
'path': os.path.dirname(sys.modules[type(self).__module__].__file__),
'class_name': type(self).__name__,
'description': self.description or type(self).__doc__,
'config_schemas': [c.__name__ for c in self.config_schemas],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am thinking here, whether we only add the name of the config or a whole serialized representation as we do with dialogs. In my head c.serialize() makes more sense, however it is true that the schema is an entity that can be shared across actors opposed to dialog which is always specific to the given actor, so doing it this way has merit also. What do you think? @abadger @pirat89

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we keep it like this, this also brings to my attention the issue where we store config information in leappdb. Is config part of actor metadata? I argue that the config scheme (the name) is actor metadata, however the schema itself is not. Maybe we should create another table and store the schema information there instead of in metadata.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure whether name only or a serialization of the schema class is appropriate here. @pirat89 ?

I'm not sure what leappdb is used for either: Would we want to store both the config_schema in it and the actual actor config settings? Would we want to store just the config settings from config files or would we want to store the config dictionary that has been expanded to include default values as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason we would like to have this information in the database is for debugging and troubleshooting purposes (using for example leapp-inspector). My idea is that we would like to have a snapshot of the leapp configuration just as the user sees it. This means that in case something goes wrong for the user we can see what is the state of leapp when he provides the leapp.db file.

So I personally would store both the entire schema, so we can see the setup of the configuration as well as the actual values provided by the user. The format in which this is stored does not matters from my point of view only in terms of how easily we can work with it later when this information is retrieved from processing by other tools (such as the leapp-inspector I mentioned)

@@ -189,6 +190,7 @@ def mapped_actor_data(data):
})
return data

# TODO: Do we need to add configs here as well?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good question, I think we need to decide and have a discussion on the position of configs in the "hierarchy" of leapp (how tightly is a config definition coupled to given actor, what is its scope...). Currently I am thinking about Configs having similar status as a Model. Meaning it is an entity that has the whole repository as a scope and is not bound to a specific actor. From that point of view I think the information would fit here.

Could you maybe explain what is your view and thought process?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may be dragging in an unrelated discussion: I remember that we were figuring out how we could make sure that changes to a config schema that was shared amongst several different Actors would be caught and error before an upgrade started. IIRC, the strategy we decided upon was for the config schema to be copied into every Actor that wanted to use it. Then the framework could detect if these opies got out of sync and error out (forcing the user to fix the code before continuing).

Would that mean we do not want configs added here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be honest I am a bit lost about what we agreed on at this point 😂 However, in such a case I agree that it should not be added here and the information should be present only in conjunction with actors just as dialogs are

@abadger abadger force-pushed the actor-config branch 5 times, most recently from 1f75fe5 to bf68773 Compare September 19, 2024 09:19
@abadger
Copy link
Member Author

abadger commented Sep 19, 2024

Hey @vinzenz , I don't know if you have time but I was wondering if you recall where in the code I need to add something to recognize directories inside of the actor as being a plugin? I'm trying to add a configs/ directory that is pretty much the same as libraries/. I found the places in the code I need to modify to make a configs directory at the repository level work but I'm stll not recognizing a configs directory inside of each individual actor. I know I'm probably just missing a few line of code somewhere that sets it up but I can't seem to find it.

@@ -175,6 +175,7 @@ def serialize(self):
'class_name': self.class_name,
'description': self.description,
'tags': self.tags,
'config_schemas': self.config_schemas,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't 'configs': self.configs, missing here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought that serialize would need to be used for things that are class attributes rather than the directories but I'll give that a try.

Copy link
Member

@vinzenz vinzenz Sep 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just going through the PR and tried to see if I can find what's missing and just stumbled upon this.
From what I can tell just by looking at the PR, it does seem like you did it right. I will get a snapshot of your PR and try it and have a look if I can find the issue.

Btw this is a feature, that asks for adding snactor support.

Copy link
Member

@pirat89 pirat89 Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw this is a feature, that asks for adding snactor support.

You are right, however we decided to have updated snactor as a stretch goal for the next (spring) release and possibly finish it in the other one.

@dkubek dkubek changed the title Start figuring out where to add configuration Extend LEAPP with actor configuration Oct 10, 2024
This commit introduces multiple improvements and fixes for actor
configuration management in LEAPP, including configuration schema
support, API updates, validation improvements, and compatibility fixes.

- **Actor Configuration:**
  - Add actor configuration support
  - Introduce configuration schema attributes for LEAPP actors.
  - Create an API to load and validate actor configs against the schemas
    stored in actors.
  - Provide a function to retrieve actor-specified configurations.
  - Enable config directories at both repository-wide and actor-wide
    levels.
  - Add configs to `leappdb`.

- **Configuration Schema Support:**
  - Add `_is_config_sequence()` to validate sequences (lists, tuples) of
    configuration fields.
  - Add support for `StringMap` field types in `config_schemas`.

- **Testing and Linting:**
  - Separate linting and unittests for better CI control.

- **Dependency Management:**
  - Add `pyyaml` to requirements in `requirements.txt`, `setup.py`, and
    spec files.

JIRA: OAMG-8803

Co-authored-by: David Kubek <[email protected]>
If your actor has a configs folder with a schemas.py python module, import it
from the actor like this::

from leapp.libraries.actor import schemas
Copy link
Member

@matejmatuska matejmatuska Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe it should be leapp.configs.actor:

Suggested change
from leapp.libraries.actor import schemas
from leapp.configs.actor import schemas

Copy link
Member

@matejmatuska matejmatuska Oct 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I know this is still a draft I just noticed this while looking at the code

@MichalHe
Copy link
Member

/packit build

Copy link
Member

@MichalHe MichalHe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went through the code. It looks OK, but I cannot claim I am sure since the codebase is a bit hidden in "fog of war". @dkubek I will take care of the my own review suggestions. Can you comment on the comment where I tagged you explicitly?

leapp/actors/__init__.py Outdated Show resolved Hide resolved
_is_type(tuple)(actor, name, value)
if not all([True] + [isinstance(item, type) and issubclass(item, Model) for item in value]):
_is_type(Sequence)(actor, name, value)
if not all([True] + [isinstance(item, type) and issubclass(item, cls) for item in value]):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this True here? I expect this to be to avoid some edge-case related to empty sequences, but I am not sure as:

not all([]) = not True = False
not all([True] + []) = not True = False

It is even more confusing as all should logically be equivalent to True and bool(item1) and bool(item2) and bool(item3). Since True is neutral element of conjunction, one can just rewrite it to bool(item1) ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed this part to

all(isinstance(item, type) and issubclass(item, cls) for item in value)

removing the needless construction of list.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkubek could you, please, double-check this whether it looks OK and if so close this conversation?

leapp/actors/config.py Outdated Show resolved Hide resolved
leapp/actors/config.py Outdated Show resolved Hide resolved
leapp/actors/config.py Outdated Show resolved Hide resolved
leapp/actors/config.py Outdated Show resolved Hide resolved
leapp/configs/common/__init__.py Outdated Show resolved Hide resolved
leapp/models/fields/__init__.py Outdated Show resolved Hide resolved
Comment on lines +218 to +219
with self.injected_context():
result = q.get()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dkubek could you please comment here about the issue one would otherwise face when omitting self.injected_context()?

Comment on lines +658 to +659
# TODO(dkubek): Is configuration metadata? Does it make sense to include it here?
_metadata['config'] = config
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think it does. The actor.config contains the actual values (data, not metadata) the user supplied. In some sense, config is similar to messages - something specific to a single execution rather than something that statically describes the actor ("meta"). I definitely agree that the config instance needs to be stored in the database, but it is not metadata.

MichalHe and others added 5 commits October 29, 2024 21:14
Ignore pylint's options:
* too-many-lines
* too-many-positional-arguments
* use-yield-from

Some of these options cannot be used (python2.7 compat) or configured.
See: oamg/leapp-repository#1299
leapp internally performs os.chdir now, which is problematic in
tests. Add mocks for os.chdir where needed to not crash during
tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants