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

Issue/6025 implement discovery handler #6264

Closed
wants to merge 68 commits into from

Conversation

Hugo-Inmanta
Copy link
Contributor

@Hugo-Inmanta Hugo-Inmanta commented Jul 12, 2023

Description

  • implement DiscoveryHandler
  • Verify that the Id constructor and resource_str method are part of the stable API and documented.

Part of #6025

Self Check:

Strike through any lines that are not applicable (~~line~~) then check the box

  • Attached issue to pull request
  • Changelog entry
  • Type annotations are present
  • Code is clear and sufficiently documented
  • No (preventable) type errors (check using make mypy or make mypy-diff)
  • Sufficient test cases (reproduces the bug/tests the requested feature)
  • Correct, in line with design
  • End user documentation is included or an issue is created for end-user documentation (add ref to issue here: )
  • If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see test-fixes for more info)

# The DiscoveryHandler is generic in both the handler's Discovery Resource type (DRT)
# and the Unmanaged Resource type (URT) it reports to the server. The second has to be serializable.

# This class deploys instances of DRT and reports URT to the server.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

deploys instances of DRT
I'm not sure this is effectively the case with my implementation

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed, this is still missing. The main resource entry-point (at the moment, depends very slightly on the final outcome of the other PR) is deploy so that method should be implemented in this class to call discover_resources and report_discovered_resources.

def discover_resources(self, ctx: HandlerContext, discovery_resource: DRT) -> abc.Mapping[ResourceIdStr, URT]:
raise NotImplementedError

def report_discovered_resources(self, ctx: HandlerContext, resource: DRT) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm unsure of whether this logic should happen in a synchronous or asynchronous context.
The testcase currently uses the async version of this (async_report_discovered_resources) and passes.
I was having second thoughts about it and tried the synchronous version (report_discovered_resources) and it looks like it is hanging somewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

All agent ABC methods are synchronous so this should be synchronous as well. I would even go as far as to drop the async method. For testing purposes, using the sync client is more complicated because if both server and client run on the same thread, a synchronous call would hang the server so it can never respond. I'm pretty sure we have some fixtures to work with that, I'll add a suggestion to your test cases.

Comment on lines 84 to 85
# resource_handler.report_discovered_resources(ctx, discovery_resource)
await resource_handler.async_report_discovered_resources(ctx, discovery_resource)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment on the method themselves, I'm not confident about that part

Copy link
Contributor

@sanderr sanderr left a comment

Choose a reason for hiding this comment

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

Intermediate review: I haven't looked at test cases or inmanta.resources yet.

src/inmanta/agent/handler.py Outdated Show resolved Hide resolved
src/inmanta/agent/handler.py Outdated Show resolved Hide resolved
src/inmanta/agent/handler.py Outdated Show resolved Hide resolved
src/inmanta/agent/handler.py Outdated Show resolved Hide resolved
src/inmanta/agent/handler.py Outdated Show resolved Hide resolved
def discover_resources(self, ctx: HandlerContext, discovery_resource: DRT) -> abc.Mapping[ResourceIdStr, URT]:
raise NotImplementedError

def report_discovered_resources(self, ctx: HandlerContext, resource: DRT) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be deploy. Could optionally be split into discover -> serialize -> report, so deploy just has to chain them together. No strong preference on the last part, unless the serialization turns out to be very verbose.

try:
self.pre(ctx, resource)
# report to the server
discovered_resources: List[DiscoveredResource] = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be typed as lowercase list or abc.Sequence. You're not modifying it so abc.Sequence presents the clearer picture.

self.pre(ctx, resource)
# report to the server
discovered_resources: List[DiscoveredResource] = [
DiscoveredResource(discovered_resource_id=resource_id, values=values)
Copy link
Contributor

Choose a reason for hiding this comment

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

These types do not match, we should serialize here.

),
urt=str(URT),
resource_id=resource.id,
exception=f"{e.__class__.__name__}('{e}')",
Copy link
Contributor

Choose a reason for hiding this comment

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

I think repr() does pretty much this:

>>> e = Exception("hellow orld")
>>> str(e)
'hellow orld'
>>> repr(e)
"Exception('hellow orld')"

Same for other message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah turns out it is not the same regarding formatting:

e = Exception("An\nError\tMessage")

print(str(e))
print(repr(e))
print(f"{e.__class__.__name__}('{e}')")
An
Error	Message
Exception('An\nError\tMessage')
Exception('An
Error	Message')

And this test fails if we change it to repr

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. To be honest I personally prefer the repr output but if we were already using the other approach (I hadn't realized when I commented) I guess we should stick with that.

src/inmanta/agent/handler.py Outdated Show resolved Hide resolved
@Hugo-Inmanta
Copy link
Contributor Author

Still a couple of mypy errors that I didn't manage to handle on my own:

+ src/inmanta/agent/handler.py:1018: error: Argument 2 of "execute" is incompatible with supertype "HandlerAPI"; supertype defines the argument type as "Resource"  [override]
+ src/inmanta/agent/handler.py:1036: error: Argument 2 of "deploy" is incompatible with supertype "HandlerAPI"; supertype defines the argument type as "Resource"  [override]
+ src/inmanta/agent/handler.py:1036: note: This violates the Liskov substitution principle
+ src/inmanta/agent/handler.py:1036: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
+ src/inmanta/agent/handler.py:1071: error: Argument 2 of "execute" is incompatible with supertype "HandlerAPI"; supertype defines the argument type as "Resource"  [override]
+ src/inmanta/agent/handler.py:1071: note: This violates the Liskov substitution principle
+ src/inmanta/agent/handler.py:1071: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#incompatible-overrides
+ src/inmanta/agent/handler.py:1089: error: Argument 1 to "json_encode" has incompatible type "D"; expected "Union[Union[BaseModel, Union[UUID, StrictNonIntBool, int, float, datetime, str]], Sequence[Union[BaseModel, Union[UUID, StrictNonIntBool, int, float, datetime, str]]], Mapping[str, Union[BaseModel, Union[UUID, StrictNonIntBool, int, float, datetime, str]]], None]"  [arg-type]

Copy link
Contributor

@wouterdb wouterdb left a comment

Choose a reason for hiding this comment

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

Looks good overall

resource: R,
requires: abc.Mapping[ResourceIdStr, ResourceState],
) -> None:
self.execute(ctx, resource)
Copy link
Contributor

Choose a reason for hiding this comment

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

We may want to copy over some of the this code

 resources_in_unexpected_state = filter_resources_in_unexpected_state(requires)
        if resources_in_unexpected_state:
            ctx.set_status(const.ResourceState.skipped)
            ctx.warning(
                "Resource %(resource)s skipped because a dependency is in an unexpected state: %(unexpected_states)s",
                resource=resource.id.resource_version_str(),
                unexpected_states=str({rid: state.value for rid, state in resources_in_unexpected_state.items()}),
            )
            return

        failed_dependencies = [req for req, status in requires.items() if status != ResourceState.deployed]
        if not any(failed_dependencies):
            self.execute(ctx, resource)
            if _should_reload():
                self.do_reload(ctx, resource)
        else:
            ctx.set_status(const.ResourceState.skipped)
            ctx.info(
                "Resource %(resource)s skipped due to failed dependencies: %(failed)s",
                resource=resource.id.resource_version_str(),
                failed=str(failed_dependencies),
            )

To handle skip and failed dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I lifted this logic in the HandlerAPI's deploy in the other PR

# serialize resources and report to the server
discovered_resources: abc.Sequence[DiscoveredResource] = [
DiscoveredResource(discovered_resource_id=resource_id, values=json.loads(json_encode(values)))
for resource_id, values in self.discover_resources(ctx, resource).items()
Copy link
Contributor

Choose a reason for hiding this comment

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

purely a matter of taste, but I would put the self.discover_resources(ctx, resource) on a line of its own, as it is the main thing here. It is kind of hidden now.

try:
self.pre(ctx, resource)
# serialize resources and report to the server
discovered_resources: abc.Sequence[DiscoveredResource] = [
Copy link
Contributor

Choose a reason for hiding this comment

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

If we run a dryrun, do we want to execute all this?

def discover_resources(
self, ctx: HandlerContext, discovery_resource: MyDiscoveryResource
) -> abc.Mapping[ResourceIdStr, MyUnmanagedResource]:
dirs = os.listdir(self._top_dir_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why files, why not just an array of strings?

from inmanta.resources import DiscoveryResource, Id, resource


async def test_discovery_resource_handler(
Copy link
Contributor

Choose a reason for hiding this comment

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

I would also add tests for dryrun, get_fact and a few requires-provides scenarios.

@arnaudsjs arnaudsjs closed this Aug 21, 2023
@arnaudsjs arnaudsjs deleted the issue/6025-implement-discovery-handler branch August 21, 2023 06:17
inmantaci pushed a commit that referenced this pull request Aug 28, 2023
…#6025, PR #6415)

# Description

- [x] implement `DiscoveryHandler`
- [x] Verify that the `Id` constructor and `resource_str` method are part of the stable API and documented.

This PR replaces #6264

Part of #6025

# Self Check:

- [x] Attached issue to pull request
- [ ] Changelog entry
- [x] Type annotations are present
- [x] Code is clear and sufficiently documented
- [x] No (preventable) type errors (check using make mypy or make mypy-diff)
- [x] Sufficient test cases (reproduces the bug/tests the requested feature)
- [x] Correct, in line with design
- [x] End user documentation is included or an issue is created for end-user documentation (#6270)
- [ ] ~~If this PR fixes a race condition in the test suite, also push the fix to the relevant stable branche(s) (see [test-fixes](https://internal.inmanta.com/development/core/tasks/build-master.html#test-fixes) for more info)~~
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.

4 participants