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

refactor(userconfig): new generator for SDKv2 #1417

Merged
merged 3 commits into from
Jan 12, 2024

Conversation

byashimov
Copy link
Contributor

About this change—what it does

  • Add new user config generator for SDKv2
  • Use TypeSet for arrays with scalar elements

@byashimov byashimov force-pushed the byashimov-new-sdk-userconfig-generator branch 2 times, most recently from 69c71e8 to e2de3d2 Compare October 26, 2023 10:00
@byashimov byashimov changed the base branch from main to v4 October 26, 2023 10:01
@byashimov byashimov force-pushed the byashimov-new-sdk-userconfig-generator branch 26 times, most recently from feeccfc to b86c298 Compare November 2, 2023 09:00
@byashimov byashimov force-pushed the byashimov-new-sdk-userconfig-generator branch from b86c298 to 9d01553 Compare November 2, 2023 10:17
@ivan-savciuc
Copy link
Contributor

Results of migration from v3 straight to v4 (this version of user_config).

  1. This one is not related to this PR in particular, since not related to user_config but I still would like to mention it here. Let's say we create a simple PG resource in v3 without user_config at all:
resource "aiven_pg" "pg1" {
  service_name = "xxx"
  project = "xxx"
  plan = "startup-4"
}

After successful creation in v3 and migration to the latest version of user_config we constantly get diff in the plan/apply:

Terraform will perform the following actions:

  # aiven_pg.pg1 will be updated in-place
  ~ resource "aiven_pg" "pg1" {
      - cloud_name              = "google-europe-west1" -> null
        id                      = "aiven-ci-kubernetes-operator/ivans-pg1"
        # (19 unchanged attributes hidden)

        # (2 unchanged blocks hidden)
    }

Even after consecutive terraform apply this diff never goes away, which is a problem.

  1. Now back to migration from v3 causes service recreation:
resource "aiven_pg" "pg2" {
  service_name = "xx"
  project = "xx"
  plan = "startup-4"

  pg_user_config {
    pg_read_replica = true
    pg_service_to_fork_from = aiven_pg.pg1.service_name
  }
}

After migration of the resource from above we get a massive diff and moreover it forces resource recreation, and this issues/diff never disappears even after recreation.

~ pg_user_config {
          - additional_backup_regions = [] -> null
          - backup_hour               = 9 -> null
          - backup_minute             = 42 -> null
          - enable_ipv6               = false -> null
          - ip_filter                 = [
              - "0.0.0.0/0",
            ] -> null
          - ip_filter_string          = [] -> null
          + pg_service_to_fork_from   = "ivans-pg1" # forces replacement
          - pg_stat_monitor_enable    = false -> null
          - pg_version                = "15" -> null
          - service_to_fork_from      = "ivans-pg1" -> null
          - shared_buffers_percentage = 0 -> null
          - static_ips                = false -> null
          - work_mem                  = 0 -> null
            # (1 unchanged attribute hidden)

          - pglookout {
              - max_failover_replication_time_lag = 60 -> null
            }
        }

There are some other issues but I can reproduce them even without need of migration from v3 to v4 and will highlight them in a separate thread.

Copy link
Contributor

@ivan-savciuc ivan-savciuc left a comment

Choose a reason for hiding this comment

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

A bug found related to boolean behaviour 🐞

Lets try something like this:

resource "aiven_kafka" "k1" {
  service_name = "xxx"
  project = "xxx"
  plan = "business-4"
  cloud_name = "google-europe-west1"

  kafka_user_config {
    schema_registry = true
  }
}

After successful service creation lets try to comment // schema_registry = true

resource "aiven_kafka" "k1" {
  service_name = "xxx"
  project = "xxx"
  plan = "business-4"
  cloud_name = "google-europe-west1"

  kafka_user_config {
    // schema_registry = true
  }
}

And the consecutive plan/apply will give us this:

      ~ kafka_user_config {
          ~ schema_registry            = true -> false
...

Which is a first problem, when we remove configuration from the TF manifest it should not go to false but rather to nil/omit empty removes it. Second problem this diff will never go away, regardless how many times we apply a new change.

@byashimov byashimov force-pushed the byashimov-new-sdk-userconfig-generator branch 2 times, most recently from 3c72924 to 58fe1cd Compare December 5, 2023 21:59
@byashimov
Copy link
Contributor Author

A bug found related to boolean behaviour 🐞

I have added a test, and additional cases. All green. Please checkout the test case in files.

Serpentiel
Serpentiel previously approved these changes Dec 7, 2023
Copy link
Contributor

@Serpentiel Serpentiel left a comment

Choose a reason for hiding this comment

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

lgtm

@Serpentiel Serpentiel assigned ivan-savciuc and unassigned Serpentiel Dec 7, 2023
ivan-savciuc
ivan-savciuc previously approved these changes Dec 11, 2023
Copy link
Contributor

@ivan-savciuc ivan-savciuc left a comment

Choose a reason for hiding this comment

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

I can no longer reproduce issues with boolean configuration options; good job fixing it.

The same issues remain during migrating from v3 to v4 (new user config). However, I can reproduce most of these issues with the current user config generator on v4.

@ivan-savciuc
Copy link
Contributor

ivan-savciuc commented Dec 11, 2023

@byashimov please fix the conflicts on this branch

@Serpentiel Serpentiel dismissed stale reviews from ivan-savciuc and themself via 9501ff7 January 4, 2024 12:18
@byashimov byashimov requested a review from a team January 4, 2024 12:18
Serpentiel
Serpentiel previously approved these changes Jan 4, 2024
Copy link
Contributor

@Serpentiel Serpentiel left a comment

Choose a reason for hiding this comment

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

lgtm, DON'T squash, use MERGE

Serpentiel
Serpentiel previously approved these changes Jan 12, 2024
@Serpentiel Serpentiel closed this Jan 12, 2024
@Serpentiel Serpentiel reopened this Jan 12, 2024
@Serpentiel Serpentiel force-pushed the byashimov-new-sdk-userconfig-generator branch from 5dea8cf to 81ca4b6 Compare January 12, 2024 13:32
@Serpentiel Serpentiel merged commit 50ec4df into main Jan 12, 2024
10 checks passed
@Serpentiel Serpentiel deleted the byashimov-new-sdk-userconfig-generator branch January 12, 2024 13:36
@byashimov byashimov restored the byashimov-new-sdk-userconfig-generator branch January 12, 2024 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants