Add code for running connectors in Agent by artem-shelkovnikov · Pull Request #2787 · elastic/connectors

artem-shelkovnikov · 2024-08-28T13:46:15Z

Part of https://github.com/elastic/search-team/issues/8096

This PR adds a way to run Connectors Service in Agent.

Entry point is connectors/agent/cli.py - if it's ran from an Agent component via the python in .venv/bin/python it will enable Connector Service running within an Agent (docker or not).

This PR adds bare bone logic: positive cases work, so expect tricky bugs to still be out there if we haven't accounted for anything.

I've tried to keep it very small, and I think there's no way to make it simpler/smaller :)

Additionally - I had to exclude tests for the component from make test because it would break make test for people who have no access to the python agent client repository - this one will be opened later so we can revert this part of the changeset. If you need to run tests there is a make target make test-agent - I kept high coverage for future.

Checklists

Pre-Review Checklist

this PR does NOT contain credentials of any kind, such as API keys or username/passwords (double check config.yml.example)
this PR has a meaningful title
this PR links to all relevant github issues that it fixes or partially addresses
if there is no GH issue, please create it. Each PR should have a link to an issue
this PR has a thorough description
Covered the changes with automated tests
Tested the changes locally
Added a label for each target release version (example: v7.13.2, v7.14.0, v8.0.0)

seanstory

Haven't really read the logic yet (since this is still in draft) but I like the structure/direction.

requirements/ftest.txt

connectors/services/job_cleanup.py

jedrazb

LGTM, couple of nits

connectors/agent/protocol.py

jedrazb · 2024-08-29T08:46:57Z

connectors/agent/component.py

+        )
+        self.opts = V2Options()
+        self.buffer = sys.stdin.buffer
+        self.agent_config = ConnectorsAgentConfiguration()


it took me a while to realise agent_config == (connector)service_config , should we rename to "service_config" or "component_config"? "Agent" references the process running the service component, so it's super accurate name in this context

Yeah I'll try renaming it, but I'm also not sure about this class - maybe we can even pass agent configuration inside, parse it and emit connectors configuration to just separate this out of the checkin handler.

I renamed it to ConnectorsAgentConfigurationWrapper and changed logic a bit + added code comments. I really could not make good names for stuff that I wrote here, open to suggestions

connectors/agent/service_manager.py

jedrazb · 2024-08-29T09:05:24Z

connectors/agent/service_manager.py

+                        f"Error while running services in ConnectorServiceManager: {e}"
+                    )
+                    raise
+        finally:


should we care about stopping running services here?

It should already be stopped - it's not possible to reach here without stopping. I can add additional step of verifying that everything stopped, but not sure it's needed

jedrazb

I meant to "comment", did you manage to run this with es_agent_client as dependency?

clicked approve instead of comment

artem-shelkovnikov · 2024-08-29T09:24:19Z

I meant to "comment", did you manage to run this with es_agent_client as dependency?

Oh yes, this branch is 100% functioning

artem-shelkovnikov · 2024-08-29T15:49:14Z

Makefile


 test: .venv/bin/pytest .venv/bin/elastic-ingest
-	.venv/bin/pytest --cov-report term-missing --cov-fail-under 92 --cov-report html --cov=connectors --fail-slow=$(SLOW_TEST_THRESHOLD) -sv tests
+	.venv/bin/pytest --cov-report term-missing --cov-fail-under 92 --cov-report html --cov=connectors --fail-slow=$(SLOW_TEST_THRESHOLD) -sv tests --ignore tests/agent


Ignoring tests/agent here cause repo that we build package from is private and it'll fail in CI. We can set it up in CI to pull the repo too, but occasional user of main can run into problems and I'd rather keep main clean and functioning

I'd challenge that we don't care about 2xx, in operational review discussions we've discussed that bumps in traffic

++

Sorry what's that quote? :D

lol looks like a paste error from a thread in https://github.com/elastic/cloud/pull/130868#pullrequestreview-2271519957

artem-shelkovnikov · 2024-08-29T15:50:00Z

connectors/config.py

    return configuration


+def add_defaults(config):


Not sure about method name 🤷

I like it. ;)

artem-shelkovnikov · 2024-08-29T15:50:22Z

requirements/agent.txt

@@ -0,0 +1 @@
+elastic-agent-client@git+https://github.com/elastic/python-elastic-agent-client@main


For now we just pick main, later we'll have more solid plan, hopefully

"Always releasable" enforced here 🤙

artem-shelkovnikov · 2024-08-29T15:50:48Z

tests/agent/test_component.py

+from connectors.agent.component import ConnectorsAgentComponent
+
+
+class StubMultiService:


Stub makes the test code much cleaner IMO so I went for it instead of a mock

jedrazb

LGTM. Couple of questions about error logging and doc improvements suggestion, nothing blocking I think

jedrazb · 2024-08-30T08:13:56Z

Makefile


 test: .venv/bin/pytest .venv/bin/elastic-ingest
-	.venv/bin/pytest --cov-report term-missing --cov-fail-under 92 --cov-report html --cov=connectors --fail-slow=$(SLOW_TEST_THRESHOLD) -sv tests
+	.venv/bin/pytest --cov-report term-missing --cov-fail-under 92 --cov-report html --cov=connectors --fail-slow=$(SLOW_TEST_THRESHOLD) -sv tests --ignore tests/agent


I'd challenge that we don't care about 2xx, in operational review discussions we've discussed that bumps in traffic

++

jedrazb · 2024-08-30T08:28:50Z

connectors/agent/config.py

+        the method returns False.
+        """
+
+        # TODO: find a good link to what this object is.


Trying to be helpful, so digged into proto def, https://github.com/elastic/elastic-agent-client/blob/main/elastic-agent-client.proto#L168

But it seems like source is a an arbitrary struct hehe (so we have to look somewhere else for the actual definition) and take a leap of faith now

Yeah judging by the protobuf files it's arbitrary:

// Source is the original configuration of this unit configuration object in the agent policy. // Only standard fields are defined as explicit types, additional fields can be parsed from source. // // This source field will almost always contain arbitrary unit configuration fields beyond those // explicitly defined in this message type. google.protobuf.Struct source = 1;

The way I interrogated this during my POC was to just add logging. I'd get a checkin event, deserialize it, re-serialize it as json, then dump it in the logs.

jedrazb · 2024-08-30T08:31:05Z

connectors/agent/protocol.py

+        """
+        msg = (
+            f"This connector component can't handle action requests. Received: {action}"
+        )


should we include the error log? I don't see any other "wrapper" logic that would surface this to the user / logs

It should log the error when called, right?

cc @seanstory will it terminate the process?

It should log the error. See: https://github.com/elastic/python-elastic-agent-client/blob/ba2525d23ed424c5740d9bbf87baff80a90533e7/elastic_agent_client/service/actions.py#L51-L61

It won't terminate, no. It just sends a message back through the protocol that the requested action failed. See this slack thread: https://elastic.slack.com/archives/C01QQ449KE1/p1719235054818189?thread_ts=1718987763.249969&cid=C01QQ449KE1

connectors/agent/protocol.py

jedrazb · 2024-08-30T08:34:34Z

connectors/agent/service_manager.py

+        """
+        if self._running:
+            msg = f"{self.__class__.__name__} is already running."
+            raise ServiceAlreadyRunningError(msg)


should we add the error log, I'm not sure we catch-and-log the exception anywhere

This one is not caught, it'll just crash the caller. Supposedly this case is "never gonna happen", so I'd just keep it like this.

jedrazb · 2024-08-30T08:35:34Z

requirements/agent.txt

@@ -0,0 +1 @@
+elastic-agent-client@git+https://github.com/elastic/python-elastic-agent-client@main


"Always releasable" enforced here 🤙

seanstory

Bunch of little nits, but I'm ok with addressing them in follow-ups rather than blocking this.

Awesome stuff. I want to see a demo an make some toasts!

seanstory · 2024-08-30T15:04:03Z

Makefile

-	.venv/bin/pytest --cov-report term-missing --cov-fail-under 92 --cov-report html --cov=connectors --fail-slow=$(SLOW_TEST_THRESHOLD) -sv tests
+	.venv/bin/pytest --cov-report term-missing --cov-fail-under 92 --cov-report html --cov=connectors --fail-slow=$(SLOW_TEST_THRESHOLD) -sv tests --ignore tests/agent
+
+test-agent: .venv/bin/pytest .venv/bin/elastic-ingest install-agent


I think we should add this to CI now, to avoid forgetting or letting it get too behind in coverage. I think that ElasticMachine should be able to clone the private repo. Hit me up if it's not working.

seanstory · 2024-08-30T15:04:34Z

connectors/agent/README.md

If we don't have anything to say here yet, let's just remove the file for now.

connectors/agent/cli.py

seanstory · 2024-08-30T15:10:06Z

connectors/agent/component.py

+from connectors.agent.config import ConnectorsAgentConfigurationWrapper
+from connectors.agent.protocol import ConnectorActionHandler, ConnectorCheckinHandler
+from connectors.agent.service_manager import ConnectorServiceManager
+from connectors.services.base import MultiService


This is using the MulitService from Connectors instead of from the elastic_agent_client. Which I think is ok (and probably for the best) but just wanted to flag in case it wasn't intentional. I hadn't tested using ActionsService or CheckinV2Service with the Connectors MultiService.

It's technically same code - we need to clean up the python-agent repo from some classes like this or move them to example namespace to not distribute these classes.

connectors/agent/config.py

seanstory · 2024-08-30T15:30:42Z

connectors/agent/protocol.py

+        self.agent_connectors_config_wrapper = agent_connectors_config_wrapper
+        self.service_manager = service_manager
+
+    async def apply_from_client(self):


This looks a lot like the "Fake example", but I'm worried we'll need to implement more here. This seems to only look at the first unit's config, but doesn't consider log_level, features, or anything else from the checkin service's sync_component or sync_units method.

connectors/agent/service_manager.py

seanstory · 2024-08-30T15:36:05Z

connectors/config.py

    return configuration


+def add_defaults(config):


I like it. ;)

tests/agent/test_agent_config.py

seanstory · 2024-08-30T15:39:17Z

tests/agent/test_cli.py

+
+    main()
+
+    loop.close()


nothing's asserted here. Intended?

Yup. main blocks so if the test fails the test will be killed by a timeout. I can add some asserts for log messages too, but not sure it's needed

Maybe just a comment then, with what you've written in this thread. 👍

jedrazb · 2024-09-03T10:04:24Z

connectors/agent/config.py

+            }
+
+            if source.fields.get("api_key"):
+                es_creds["api_key"] = source["api_key"]


@artem-shelkovnikov commenting so that it doesn't get lost, by default we would get Fleet-specific api key format, this should do the trick to turn it to our version:

if source.fields.get("api_key"): api_key = source["api_key"] # if beats_logstash_format if ":" in api_key: api_key = base64.b64encode(api_key.encode()).decode()

Verified that this works

Thanks for the investigation! I'm gonna add this code as well :)

seanstory

re-approve

github-actions · 2024-09-03T15:18:33Z

💔 Failed to create backport PR(s)

The backport operation could not be completed due to the following error:
There are no branches to backport to. Aborting.

The backport PRs will be merged automatically after passing CI.

To backport manually run:
backport --pr 2787 --autoMerge --autoMergeMethod squash

github-actions bot added auto-backport v8.16.0 labels Aug 28, 2024

seanstory reviewed Aug 28, 2024

View reviewed changes

requirements/ftest.txt Show resolved Hide resolved

connectors/services/job_cleanup.py Show resolved Hide resolved

jedrazb previously approved these changes Aug 29, 2024

View reviewed changes

jedrazb reviewed Aug 29, 2024

View reviewed changes

artem-shelkovnikov force-pushed the artem/add-connectors-agent branch from b075733 to 86525f6 Compare August 29, 2024 10:03

artem-shelkovnikov added 7 commits August 29, 2024 12:55

Adding base connectors agent component

deab57c

Fix pyright problem

1f19aba

Minor improvements and simplifications

d4f45fb

Step forward

b8d5704

Update requirement to make it work

7bc5e58

Adjustments in namespace + checking something for tests

200febb

Test coverage for service_manager.py

a058c7c

artem-shelkovnikov force-pushed the artem/add-connectors-agent branch from 60a1a86 to a058c7c Compare August 29, 2024 10:55

artem-shelkovnikov added 5 commits August 29, 2024 13:43

Class comments + subtle renames

6cc17c6

Class comments + cleanups + linter

cc1cf04

Test protocol.py

8d57bc7

Test more

9de374b

Autoformat

7d392f8

artem-shelkovnikov changed the title ~~Artem/add connectors agent~~ Add code for running connectors in Agent Aug 29, 2024

artem-shelkovnikov marked this pull request as ready for review August 29, 2024 15:26

artem-shelkovnikov requested a review from a team August 29, 2024 15:26

Move agent tests to a different make target for now

ccba574

artem-shelkovnikov commented Aug 29, 2024

View reviewed changes

jedrazb previously approved these changes Aug 30, 2024

View reviewed changes

Add a comment about check-in handler

97ca12e

artem-shelkovnikov dismissed jedrazb’s stale review via 97ca12e August 30, 2024 13:04

seanstory previously approved these changes Aug 30, 2024

View reviewed changes

jedrazb reviewed Sep 3, 2024

View reviewed changes

jedrazb previously approved these changes Sep 3, 2024

View reviewed changes

artem-shelkovnikov added 2 commits September 3, 2024 16:28

Merge branch 'main' into artem/add-connectors-agent

7f5e8a0

Add missing license headers

428b8ad

artem-shelkovnikov dismissed stale reviews from jedrazb and seanstory via 428b8ad September 3, 2024 14:30

seanstory approved these changes Sep 3, 2024

View reviewed changes

artem-shelkovnikov merged commit fd20dfb into main Sep 3, 2024

artem-shelkovnikov deleted the artem/add-connectors-agent branch September 3, 2024 15:18

artem-shelkovnikov mentioned this pull request Sep 4, 2024

Add elastic-agent-service container to packaging elastic/elastic-agent#5349

Merged

2 tasks

		@@ -0,0 +1 @@
		elastic-agent-client@git+https://github.com/elastic/python-elastic-agent-client@main

		from connectors.agent.component import ConnectorsAgentComponent


		class StubMultiService:

Conversation

artem-shelkovnikov commented Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Part of https://github.com/elastic/search-team/issues/8096

Checklists

Pre-Review Checklist

Uh oh!

seanstory left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jedrazb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jedrazb left a comment

Choose a reason for hiding this comment

Uh oh!

artem-shelkovnikov commented Aug 29, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jedrazb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

artem-shelkovnikov Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seanstory left a comment

Choose a reason for hiding this comment

artem-shelkovnikov commented Aug 28, 2024 •

edited

Loading

artem-shelkovnikov Aug 30, 2024 •

edited

Loading