connectors-ci: improve pytest result evaluation

[alafanechere]

What

Relates to #27425 , #28423

Original problem

When running pytest in a dagger container we face two challenges:

• When a pytest suite is empty or all tests are skipped pytest will return a 5 exit code. exit code >0 in Dagger raise ExecError, but we want to mark this step as skipped and not fail.
• When CAT (which is a pytest suite) is failing still need to perform after the facts operations to retrieve updated connector configurations from the container. When a container raises an ExecError we can't interact with it's filesystem without raising the same ExecError again.

How we overcame this problem so far

We installed a pytest plugin to always make pytest suite exit with code 0: pytest-custom_exit_code. We inferred success / failure / skipping of test by parsing the stdout log. This is not robust as the stdout content can change according to the pytest version or if a CAT run outputs unexpected stuff to stdout. This led to the false positive errors mentioned in #27425

The new approach to overcome this problem

We still make CAT exec artificially exit with a 0 exit code, but we store the original exit code to a file in the container filesystem: /exit_code. We get_step_result is run it will check if such a file exist on the container and return the in file exit code value instead of the pure container exit code. This allows us to capture the actual exit code that pytest returned.

How

The new approach mentioned above is mainly introduced via:

• a new hack: hacks.never_fail_exec function:
  

  airbyte/airbyte-ci/connectors/pipelines/pipelines/hacks.py

  Line 145 in ddb7345

  def never_fail_exec(command: List[str]) -> Callable:

• updating the Step.get_step_result function to read in file exit code if /exit_code file is available on the container.
  

  airbyte/airbyte-ci/connectors/pipelines/pipelines/bases.py

  Line 245 in ddb7345

  async def get_step_result(self, container: Container) -> StepResult:

I tried to thoroughly unit test these change to show how async test with a dagger client work:

airbyte/airbyte-ci/connectors/pipelines/tests/tests/test_common.py

Line 20 in ddb7345

class TestAcceptanceTests:

Additional refactor

Centralizing the AcceptanceTest logic

The logic to build the CAT container was in the environment.with_connector_acceptance_test function. I decided to move it to common.AcceptanceTest._build_connector_acceptance_test as environment functions are originally made to be reused in multiple steps. This function is very specific to AcceptanceTest.

Make all step write the stdout / stderr to a log file

(Relates to #28423)
We originally only wrote CAT output logs to a local directory. I figured that this logic can easily apply to all steps and would provide more debugging abilities to developers. Logs are written when the step results are built in get_step_results

airbyte/airbyte-ci/connectors/pipelines/pipelines/bases.py

Line 258 in ddb7345

await self.write_log_files(stdout, stderr)

Wrap acceptance test into its own dagger Pipeline.

(Relates to #28423)
I created Step specific dagger client in AcceptanceTest in order to showcase how we could make step execution wrapped into a dagger Pipeline: this will make the CAT dagger operations grouped in a explicit way in the dagger UI.

airbyte/airbyte-ci/connectors/pipelines/pipelines/bases.py

Line 112 in ddb7345

def dagger_client(self) -> Container:

Recommended reading order

1. airbyte/airbyte-ci/connectors/pipelines/pipelines/tests/common.py

   Line 176 in ddb7345

   class AcceptanceTests(PytestStep):

2. airbyte/airbyte-ci/connectors/pipelines/tests/tests/test_common.py

   Line 20 in ddb7345

   class TestAcceptanceTests:

[octavia-squidington-iii]

source-pokeapi test report (commit 9ae813c9e4) - ❌

⏲️ Total pipeline duration: 02mn32s

Step	Result
Validate airbyte-integrations/connectors/source-pokeapi/metadata.yaml	✅
Connector version semver check	✅
QA checks	✅
Code format checks	✅
Connector package install	✅
Build source-pokeapi docker image for platform linux/x86_64	✅
Unit tests	✅
Acceptance tests	❌

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-pokeapi test

[octavia-squidington-iii]

source-openweather test report (commit 9ae813c9e4) - ✅

⏲️ Total pipeline duration: 03mn46s

Step	Result
Validate airbyte-integrations/connectors/source-openweather/metadata.yaml	✅
Connector version semver check	✅
QA checks	✅
Code format checks	✅
Connector package install	✅
Build source-openweather docker image for platform linux/x86_64	✅
Unit tests	✅
Acceptance tests	✅

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-openweather test

[bnchrch]

A few comments but the only one I think is a hard blocker is referencing context.connector in Step

[bnchrch]

are our tests now passing for pipelines? Did I miss that PR?!

[alafanechere]

Sorry, it was a WIP push from a different branch: #28857

[bnchrch]

👍

[bnchrch]

Can we use connector here if this is our generic Step class?

[bnchrch]

Doing all this manual stderr/out handling and log writing makes me nervous.

Particularly now that they write to not just our ci folder but also the connector folder

My worries generally come down to If we keep writing log handling at a low level

1. We will have to keep repeating our logging code
2. More importantly we keep repeating if statments like if self.context.is_local and self.should_persist_stdout_stderr_logs: which if missed lead to tricky bugs
3. And if we go this route for too long, it will be very hard to go back as there will be alot of code to refactor.

This leads me to ask What is stopping us from moving all our log writing/handling to the dagger client boundary here:
https://github.com/airbytehq/airbyte/blob/master/airbyte-ci/connectors/pipelines/pipelines/pipelines/connectors.py#L85

For example a possible solution could be

1. Write our own logger that extends the typical python logger by adding pipeline specific context
   1. pipeline name
   2. pipeline step
   3. etc.
2. Write our own subclass of TextIOWrapper that based on the new pipeline context we stuff into the logger
   1. writes a log file per pipeline
   2. writes a log file per step
   3. writes a root log file representing everything
   4. Decides if we are also writing to stdout/stderr
   5. Decides if we need to send these logs anywhere else (e.g. sentry, datadog, cloudwatch etc)

If this is possible and is a good idea.

can we make a small step towards this now by

1. Moving the decision of whether or not we write to a log file the decision of a custom logger
2. Moving the writing logic to the logger

e.g. https://stackoverflow.com/questions/6386698/how-to-write-to-a-file-using-the-logging-python-module

[alafanechere]

@bnchrch let's keep the logging change for a different PR then.
But just to react on top of your comment:

My worries generally come down to If we keep writing log handling at a low level

This is not low level IMO because it's a change made at the Step class level, so we centralize all the logging logic on the base class.

Write our own logger that extends the typical python logger by adding pipeline specific context

This is what this change did: the step have a specific logger with pipeline and step name.

Write our own subclass of TextIOWrapper that based on the new pipeline context we stuff into the logger

In this context you mean parsing the dagger client logs. Splitting them into different per step/pipeline logs file will be very brittle as we have no control on the shape of the dagger logs, I'd rather ask the dagger team to provide such a feature and IMO it directly overlaps with the log access in the Dagger Web UI.
Moreover, if steps are cached their stdout are not shown in the logs (and replaced with CACHED).

can we make a small step towards this now by
Moving the decision of whether or not we write to a log file the decision of a custom logger
Moving the writing logic to the logger.

Yep you're right that using a combo of logging to stdout + write logic can definitely be handled at the logger level. Let's do it later and groom #28423 a bit more.
I think we should be clearer in differentiating what we mean by Dagger logs, steps logs, pipeline logs stp.

[bnchrch]

👍

[bnchrch]

well done function.

But Im starting to notice we have a lot of connector code, that isnt obvious based on the file structure.

For example this is in test/common

should we start making it more specific. e.g.:

• test/connector_acceptance/common.py
• actions/connectors/tests.py

[alafanechere]

The test/common module is originally made to receive connector testing step that are agnostic to the connector language. This is why AcceptanceTest belongs here.
The actions module was originally made to receive re-usable code. But I think I overused it and suffed action/environements with code that is not re-used in multiple context.

[bnchrch]

lgtm!