connectors-ci: fix postgres integration testing #25942

alafanechere · 2023-05-10T01:48:25Z

What

Closes #25381
This PR is an attempt to make source-postgres integration java test pass in the Dagger pipeline context, and still making them work for original /test run or locally.

Original problems:

The container under test (source-postgres) could not reach the testcontainers databases that are spawned by the java tests
Flakyness on CDC tests.

How

Fixing networking issues

There were inconsistencies in the way test set up the source-postgres configuration to connect to the test database.
The Java test need to access to test databases to seed it with test data. In this context the java test should resolve the database host/port targetting the docker host and the exposed ports.
The source-postgres need to connect to test databases to READ. In this context, to resolve the database host/port the internal container ip/exposed ports must be used.

flowchart TD
    subgraph master ["Dagger engine"]
        itj["Gradle integrationTestJava"]
        subgraph dockerHost ["Docker host"]
            cut["source-postgres"]
            testcontainer["Test postgres database"]
            cut --"Resolve with docker host internal ip addresses and mapped ports"-->testcontainer
        end
        itj --"Resolve with docker host hostname and exposed ports for INSERT"--> testcontainer
    end

In practice:

To fill the source-postgres config we get the container internal ip address/exposed port by using HostPortResolver utility class:

    config = Jsons.jsonNode(ImmutableMap.builder()
        .put(JdbcUtils.HOST_KEY, HostPortResolver.resolveHost(container))
        .put(JdbcUtils.PORT_KEY, HostPortResolver.resolvePort(container))
        .put(JdbcUtils.DATABASE_KEY, container.getDatabaseName())
        .put(JdbcUtils.SCHEMAS_KEY, List.of(NAMESPACE))
        .put(JdbcUtils.USERNAME_KEY, container.getUsername())
        .put(JdbcUtils.PASSWORD_KEY, container.getPassword())
        .put("replication_method", replicationMethod)
        .put(JdbcUtils.SSL_KEY, false)
        .put("is_test", true)
        .build());

To seed the test database we use the host/exposed port by using the .getHost() / .getFirstMappedPort() on the container object:

    try (final DSLContext dslContext = DSLContextFactory.create(
        config.get(JdbcUtils.USERNAME_KEY).asText(),
        config.get(JdbcUtils.PASSWORD_KEY).asText(),
        DatabaseDriver.POSTGRESQL.getDriverClassName(),
        String.format(DatabaseDriver.POSTGRESQL.getUrlFormatString(),
            container.getHost(),
            container.getFirstMappedPort(),
            config.get(JdbcUtils.DATABASE_KEY).asText()),
        SQLDialect.POSTGRES)) {
      final Database database = new Database(dslContext);

      database.query(ctx -> {
        ctx.execute("CREATE TABLE id_and_name(id INTEGER  primary key, name VARCHAR(200));");
        ctx.execute("INSERT INTO id_and_name (id, name) VALUES (1,'picard'),  (2, 'crusher'), (3, 'vash');");
        ctx.execute("CREATE TABLE starships(id INTEGER primary key, name VARCHAR(200));");
        ctx.execute("INSERT INTO starships (id, name) VALUES (1,'enterprise-d'),  (2, 'defiant'), (3, 'yamato');");
        ctx.execute("SELECT pg_create_logical_replication_slot('" + SLOT_NAME_BASE + "', 'pgoutput');");
        ctx.execute("CREATE PUBLICATION " + PUBLICATION + " FOR ALL TABLES;");
        return null;
      });

Fixing CDC test flakyness

Increasing the value of INITIAL_WAITING_SECONDS to 30 seconds in the connector config made tests more stable. I observed the flakyness locally and in dagger pipelines.

Improving Gradle performance in Dagger Pipeline

I introduced the use of the Gradle S3 cache in Dagger pipeline to benefit from cross build caching in the CI.
Even if caching is enabled by default in our main gradle.properties file the cache is not used for local run according to settings.gradle. To enable the use of the gradle remote cache I added the required env var to the gradle container.

🚨 User Impact 🚨

According to my manual tests these changes make source-postgres:integrationTestJava pass locally, in dagger pipelines and we /test.
Please note that these inconsistency in how we connect to test databases are also present on other java connectors. I focused on source-posgres because it's GA. I might proceed in changing other connectors if this PR is 👍 and I spot other /test vs Dagger pipeline inconsistencies.

github-actions · 2023-05-10T01:48:44Z

alafanechere · 2023-05-10T01:49:34Z

/test connector=connectors/source-postgres

🕑 connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/4932259119
✅ connectors/source-postgres https://github.com/airbytehq/airbyte/actions/runs/4932259119
No Python unittests run

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: not found in the config.
SKIPPED [2] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [2] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:578: The previous and actual discovered catalogs are identical.
=================== 68 passed, 5 skipped in 79.72s (0:01:19) ===================

…e into augustin/debug-postgres

This reverts commit bae1c14.

…e into augustin/debug-postgres

alafanechere · 2023-05-10T03:54:01Z

🎉Achieved both successful /test above and successful Dagger pipeline run here. Some CDC testing about WAL and Data type look flaky locally / on Dagger pipeline. I'm going to re-run the Dagger pipeline a couple of time for sanity.

alafanechere · 2023-05-10T11:20:55Z

Confirmed the flakyness of CdcInitialSnapshotPostgresSourceDatatypeTest > testDataContent() (failing in this run)

…e into augustin/debug-postgres

alafanechere · 2023-05-10T12:32:13Z

Consequents pipeline runs to check flakyness after modifying INITIAL_WAITING_SECONDS:

alafanechere · 2023-05-10T13:22:12Z

/test connector=connectors/source-postgres

alafanechere · 2023-05-10T19:21:19Z

/test connector=connectors/source-postgres

evantahler

I'm going to punt on this review... I don't really know enough about what's going on in Java to be useful

bnchrch

One comment about a comment.

Outside of that I have no objections.

bnchrch · 2023-05-10T19:56:07Z

tools/ci_connector_ops/ci_connector_ops/pipelines/actions/environments.py

        .with_exec(["mkdir", "/airbyte"])
        .with_mounted_directory("/airbyte", context.get_repo_dir(".", include=include))
        .with_mounted_cache("/airbyte/.gradle", airbyte_gradle_cache, sharing=CacheSharingMode.LOCKED)
        .with_workdir("/airbyte")
    )
+    if context.is_ci:


@alafanechere Can we leave a comment around this that explains

What we are doing

Why

When we should remove it and whos responsible

rodireich · 2023-05-11T16:36:32Z

...ntegrations/bases/base-java/src/main/java/io/airbyte/integrations/util/HostPortResolver.java

-    return System.getProperty("os.name").toLowerCase().startsWith("mac")
-        ? getIpAddress(container)
-        : container.getHost();
+    return getIpAddress(container);


This is going to break the clickhouse integration tests on Mac I think.
The original change was #14701 back in July last year

source-clickhouse that is

rodireich · 2023-05-11T16:41:31Z

...io/airbyte/integration_tests/sources/AbstractPostgresSourceSSLCertificateAcceptanceTest.java

    config = Jsons.jsonNode(ImmutableMap.builder()
-        .put("host", containerInnerAddress.left)
-        .put("port", containerInnerAddress.right)
+        .put("host", HostPortResolver.resolveHost(container))


Is this going to return the inner address and port of a container? This was the reason tests were originally failing when run on Mac.
Can you validate that running integration test on your Mac is still passing.
For example:

./gradlew --info :airbyte-integrations:connectors:source-postgres:integrationTestJava --tests "io.airbyte.integrations.io.airbyte.integration_tests.sources.PostgresSourceSSLCaCertificateAcceptanceTest"

Yes it passed, this is why I made the change...

thanks
I'm just making sure as I don't know dagger 😅

rodireich · 2023-05-11T16:53:52Z

...tions/io/airbyte/integration_tests/sources/CdcInitialSnapshotPostgresSourceDatatypeTest.java

@@ -23,7 +23,7 @@ public class CdcInitialSnapshotPostgresSourceDatatypeTest extends AbstractPostgr
  private static final String SCHEMA_NAME = "test";
  private static final String SLOT_NAME_BASE = "debezium_slot";
  private static final String PUBLICATION = "publication";
-  private static final int INITIAL_WAITING_SECONDS = 5;
+  private static final int INITIAL_WAITING_SECONDS = 30;


Is the difference on dagger expected?

I faced flaky results on my local machine too and increasing the waiting time mitigate these problems on both local and dagger. I found a PR from Sergio on another connector that did it for the same reasons.

rodireich · 2023-05-11T18:44:13Z

/test connector=connectors/source-clickhouse

🕑 connectors/source-clickhouse https://github.com/airbytehq/airbyte/actions/runs/4951414309
✅ connectors/source-clickhouse https://github.com/airbytehq/airbyte/actions/runs/4951414309
No Python unittests run

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestConnection.test_check: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestDiscovery.test_discover: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestBasicRead.test_read: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestFullRefresh.test_sequential_reads: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: not found in the config.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
================= 22 passed, 6 skipped, 30 warnings in 16.44s ==================

rodireich · 2023-05-11T19:17:54Z

@alafanechere I see source-clickhouse failing locally on my Mac with and without your change (I wonder if this is happening to all or just on my setup).
I'm ok with merging this change since it doesn't seem to break anything that wasn't already broken.

alafanechere · 2023-05-11T19:19:15Z

@alafanechere I see source-clickhouse failing locally on my Mac with and without your change (I wonder if this is happening to all or just on my setup).
I'm ok with merging this change since it doesn't seem to break anything that wasn't already broken.

🙏

connectors-ci: fix postgres integration testing

bd5be9b

octavia-squidington-iii added the area/connectors Connector related issues label May 10, 2023

octavia-squidington-iii added the connectors/source/postgres label May 10, 2023

alafanechere and others added 4 commits May 10, 2023 03:59

Merge branch 'master' into augustin/debug-postgres

8eedd50

fix mysql

bae1c14

Merge branch 'augustin/debug-postgres' of github.com:airbytehq/airbyt…

70a16b0

…e into augustin/debug-postgres

fix conflicts

bbab004

octavia-squidington-iii added the connectors/source/mysql label May 10, 2023

alafanechere added 3 commits May 10, 2023 04:16

revert java connectors pipeline changes

6199f35

Revert "fix mysql"

36087e7

This reverts commit bae1c14.

use retry plugin to handle flaky tests

56e6eee

alafanechere force-pushed the augustin/debug-postgres branch from 6faf3f4 to 56e6eee Compare May 10, 2023 02:40

alafanechere and others added 4 commits May 10, 2023 03:03

Automated Change

25d3456

use S3 cache

ebc4665

Merge branch 'augustin/debug-postgres' of github.com:airbytehq/airbyt…

bda7e31

…e into augustin/debug-postgres

use S3 cache

b3e5e10

alafanechere and others added 2 commits May 10, 2023 05:56

clean HostPortResolver

93597e7

Automated Commit - Formatting Changes

1a1721c

airbytehq deleted a comment from github-actions bot May 10, 2023

alafanechere added 4 commits May 10, 2023 13:37

increase INITIAL_WAITING_SECONDS on CDC tests to reduce flakyness

761796f

Merge branch 'augustin/debug-postgres' of github.com:airbytehq/airbyt…

a8a788c

…e into augustin/debug-postgres

remove secrets from run-dagger-pipeline/action.yml

279b461

set CI env var on gradle container

c03db6b

alafanechere force-pushed the augustin/debug-postgres branch from f2bddce to c03db6b Compare May 10, 2023 12:28

airbytehq deleted a comment from github-actions bot May 10, 2023

remove retry from build.gradle

275e477

airbytehq deleted a comment from github-actions bot May 10, 2023

alafanechere marked this pull request as ready for review May 10, 2023 13:22

alafanechere requested review from rodireich, evantahler and a team May 10, 2023 13:24

evantahler reviewed May 10, 2023

View reviewed changes

evantahler requested a review from a team May 10, 2023 19:53

bnchrch approved these changes May 10, 2023

View reviewed changes

rodireich reviewed May 11, 2023

View reviewed changes

alafanechere merged commit 5c5eab0 into master May 11, 2023

alafanechere deleted the augustin/debug-postgres branch May 11, 2023 19:19

marcosmarxm pushed a commit to natalia-miinto/airbyte that referenced this pull request Jun 8, 2023

connectors-ci: fix postgres integration testing (airbytehq#25942)

4060b6c

alafanechere mentioned this pull request Jun 28, 2023

Java DB Source connector licenses to Elv2 #27737

Merged

bnchrch mentioned this pull request Aug 16, 2023

source-mysql and source-mssql failing due to missing arm64 image #29463

Closed

alafanechere mentioned this pull request Aug 31, 2023

source-mysql: do not use testcontainers in integration tests #30032

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

connectors-ci: fix postgres integration testing #25942

connectors-ci: fix postgres integration testing #25942

alafanechere commented May 10, 2023 •

edited

Loading

github-actions bot commented May 10, 2023 •

edited by alafanechere

Loading

alafanechere commented May 10, 2023 •

edited by github-actions bot

Loading

alafanechere commented May 10, 2023

alafanechere commented May 10, 2023

alafanechere commented May 10, 2023 •

edited

Loading

alafanechere commented May 10, 2023

alafanechere commented May 10, 2023

evantahler left a comment

bnchrch left a comment

bnchrch May 10, 2023

rodireich May 11, 2023

rodireich May 11, 2023

rodireich May 11, 2023

alafanechere May 11, 2023

rodireich May 11, 2023

rodireich May 11, 2023

alafanechere May 11, 2023 •

edited

Loading

rodireich commented May 11, 2023 •

edited by github-actions bot

Loading

rodireich commented May 11, 2023

alafanechere commented May 11, 2023

connectors-ci: fix postgres integration testing #25942

connectors-ci: fix postgres integration testing #25942

Conversation

alafanechere commented May 10, 2023 • edited Loading

What

How

Fixing networking issues

In practice:

Fixing CDC test flakyness

Improving Gradle performance in Dagger Pipeline

🚨 User Impact 🚨

github-actions bot commented May 10, 2023 • edited by alafanechere Loading

Before Merging a Connector Pull Request

alafanechere commented May 10, 2023 • edited by github-actions bot Loading

Build Passed

alafanechere commented May 10, 2023

alafanechere commented May 10, 2023

alafanechere commented May 10, 2023 • edited Loading

alafanechere commented May 10, 2023

alafanechere commented May 10, 2023

evantahler left a comment

Choose a reason for hiding this comment

bnchrch left a comment

Choose a reason for hiding this comment

bnchrch May 10, 2023

Choose a reason for hiding this comment

rodireich May 11, 2023

Choose a reason for hiding this comment

rodireich May 11, 2023

Choose a reason for hiding this comment

rodireich May 11, 2023

Choose a reason for hiding this comment

alafanechere May 11, 2023

Choose a reason for hiding this comment

rodireich May 11, 2023

Choose a reason for hiding this comment

rodireich May 11, 2023

Choose a reason for hiding this comment

alafanechere May 11, 2023 • edited Loading

Choose a reason for hiding this comment

rodireich commented May 11, 2023 • edited by github-actions bot Loading

Build Passed

rodireich commented May 11, 2023

alafanechere commented May 11, 2023

alafanechere commented May 10, 2023 •

edited

Loading

github-actions bot commented May 10, 2023 •

edited by alafanechere

Loading

alafanechere commented May 10, 2023 •

edited by github-actions bot

Loading

alafanechere commented May 10, 2023 •

edited

Loading

alafanechere May 11, 2023 •

edited

Loading

rodireich commented May 11, 2023 •

edited by github-actions bot

Loading