[WIP] update gorm and related libraries #1637

miguelsorianod · 2023-03-14T13:10:50Z

Description

Continuation of #1314. Full context there.

Verification Steps

Checklist (Definition of Done)

machi1990 · 2023-03-14T13:16:43Z

Can't wait for this update to land! Thanks @miguelsorianod @manstis for making this possible

miguelsorianod · 2023-03-16T08:17:49Z

Some tests are still failing. Both kafka and connector ones.
I have been working on reviewing the kafka related ones. After investigating the issues I found that there are a couple of issues: The way the Scan method is implemented for api.JSON with some edge cases and the api.MarshalJSON with some edge cases too.
I got some local changes implemented that made the tests pass but at the same time I found out there are newer gorm and db drivers versions. After locally updating them I found out that that makes the tests pass so it seems to be a bug introduced in the current versions of the libraries in the PR.
I would have updated and pushed the changes but when updating gorm and related libraries the way they are defined point to an arbitrary commit in gorm instead of a tag. Because of this I am going to wait until they update the dependencies to point to a git tag, to provide us with higher stability guarantees.

I observed the connector tests are still failing but let's wait until we update the gorm libraries to see if they still fail.

go.mod

internal/kafka/internal/services/clusters_test.go

miguelsorianod · 2023-04-11T10:22:50Z

I updated the gorm and gorm postgres dependencies to the versions show below:

  gorm.io/driver/postgres v1.5.0
  gorm.io/gorm v1.25.0

miguelsorianod · 2023-04-11T10:23:23Z

After performing the upgrades mentioned in #1637 (comment) the unit tests pass, the kafka integration tests also pass but the connectors tests fail.

It seems all connectors tests are failing:

2023-04-11T10:08:23.3263810Z DONE 33 tests, 33 failures in 46.126s

miguelsorianod · 2023-04-11T10:29:08Z

After performing the upgrades mentioned in #1637 (comment) the unit tests pass, the kafka integration tests also pass but the connectors tests fail.

It seems all connectors tests are failing:
2023-04-11T10:08:23.3263810Z DONE 33 tests, 33 failures in 46.126s

cc @manstis could you take a look? I suspect it might have to do with the migrations again as all the tests are failing with "unknown" state.

manstis · 2023-04-12T09:28:48Z

@miguelsorianod I suspect it is caused by go-gorm/playground#571

Newer (latest) versions of gorm's postgres driver fail to migrate XXXserial columns.

manstis · 2023-04-12T09:29:05Z

@miguelsorianod I am doing some more investigations locally and will report back.

miguelsorianod · 2023-04-12T09:48:05Z

@miguelsorianod I am doing some more investigations locally and will report back.

Thanks 👍

manstis · 2023-04-12T14:34:29Z

Hi @miguelsorianod

I've made the applicable changes to my fork: manstis@57abdc7

You could include them in this PR and it should be OK.

miguelsorianod · 2023-04-12T14:36:54Z

Hi @miguelsorianod

I've made the applicable changes to my fork: manstis@57abdc7

You could include them in this PR and it should be OK.

Can your changes be merged to main through a PR before this PR? or they depend on the changes of this PR being available? If not I will include the change as you said

manstis · 2023-04-12T14:46:57Z

@miguelsorianod Since the changes are only required due to the gorm upgrade I think they should be part of this PR (for the same reason as to why there are kafka changes in this PR already too and they have not first been applied to main).

miguelsorianod · 2023-04-12T15:55:30Z

@miguelsorianod Since the changes are only required due to the gorm upgrade I think they should be part of this PR (for the same reason as to why there are kafka changes in this PR already too and they have not first been applied to main).

It seems reasonable. I will add it into the PR. The authorship will be lost though.

miguelsorianod · 2023-04-12T15:59:52Z

@miguelsorianod Since the changes are only required due to the gorm upgrade I think they should be part of this PR (for the same reason as to why there are kafka changes in this PR already too and they have not first been applied to main).

It seems reasonable. I will add it into the PR. The authorship will be lost though.

I have been able to keep the authorship by getting your fork's branch and cherry-picking the commit. Could you verify that the changes have been included correctly in this PR?

codecov · 2023-04-12T16:23:57Z

Codecov Report

Merging #1637 (04641f3) into main (ce2958d) will decrease coverage by 0.11%.
The diff coverage is 26.92%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1637      +/-   ##
==========================================
- Coverage   82.26%   82.15%   -0.11%     
==========================================
  Files         161      161              
  Lines       14989    15009      +20     
==========================================
+ Hits        12330    12331       +1     
- Misses       2223     2242      +19     
  Partials      436      436

Flag	Coverage Δ
unittests	`82.15% <26.92%> (-0.11%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/db/config.go	`77.90% <26.92%> (-22.10%)`	⬇️

miguelsorianod · 2023-04-12T17:18:25Z

All the checks are passing now. I will do a re-review and start planning the rollout of this changes for KAS Fleet Manager, as merging it will rollout those changes to KAS Fleet Manager Stage and Production.

Bumps [gorm.io/driver/postgres](https://github.com/go-gorm/postgres) from 1.0.8 to 1.3.10. - [Release notes](https://github.com/go-gorm/postgres/releases) - [Commits](go-gorm/postgres@v1.0.8...v1.3.10) --- updated-dependencies: - dependency-name: gorm.io/driver/postgres dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>

When updating gorm from 1.21.7 to 1.23.10 kafka migrations start to return an error: Could not migrate: ERROR: cached plan must not change result type (SQLSTATE 0A000) This error happens due to in the kafka migrations we perform database queries and schema table modifications in an interleaved way and prepared statements are cached. This leads scenarios where a specific table is queried and thus some prepared statements cached, then some table schemas are modified and then when queries are performed again against those queries are outdated cached prepared statements which causes the observed error. This behavior is likely a bug in gorm related to prepared statement caching due to only updating gorm minor version should not break changes. To workaround this for now this commit updates code to disable prepared statements during kafka migrations. This includes the kafka migrations when integration tests are run too. During serve command execution, for connectors migrations and for the integration tests (excluding the migrations part of them) the prepared statements are enabled being the behavior the same as before this change.

gorm.io/gorm has been updated to v1.24.1 gorm.io/driver/postgres has been updated to v1.4.5

the bug related to db prepared statements has been fixed on gorm.io/driver/postgres v1.4.5. We are now using that version so this reenables db prepared statements for kafka migrations

gorm.io/gorm has been updated to v1.24.3 gorm.io/driver/postgres has been updated to v1.4.6

gorm.io/gorm has been updated to v1.25.0 gorm.io/driver/postgres has been updated to v1.5.0

After gorm version update to 1.25.0 the Save method does not return values.

manstis

LGTM 👍

miguelsorianod · 2023-04-18T11:06:30Z

I've made the following manual tests in a development environment:

Performing Kafkas DB migration twice works correctly when using the new gorm version
Performing Connectors DB migration twice works correctly when using the gorm version
Performing Kafka creation before gorm update, then migrating to new gorm version including reapplying db migration works correctly. The kafka intsances remain ready and no errors are observed
Creating Kafka instances after gorm update works correctly
Kafka instances can be listed after gorm updates for both newly created instances and also for previously created instances
Kafka instances can be deleted after form updates for both newly created instances and also for previously created instances
Instances created before the gorm update and instances created after the gorm update look the same. This includes empty and null values
OCP cluster entry pre and post migration looks the same. This includes empty and null values

I am currently waiting for some midstream pipelines issues to be fixed. Once that is done I will proceed to merge this and release it to stage

miguelsorianod added the do-not-merge A Pull Request with this label should not be merged: it might be due to pending questions, feedback label Mar 14, 2023

miguelsorianod requested review from a team as code owners March 14, 2023 13:10

github-actions bot added common kafka labels Mar 14, 2023

miguelsorianod force-pushed the gorm-dependabot-update-1 branch from 38bad7e to 1487dbc Compare March 14, 2023 13:18

machi1990 reviewed Mar 31, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

machi1990 reviewed Mar 31, 2023

View reviewed changes

go.mod Outdated Show resolved Hide resolved

miguelsorianod mentioned this pull request Apr 3, 2023

build(deps): bump gorm.io/driver/postgres from 1.0.8 to 1.5.0 #1693

Closed

ziccardi reviewed Apr 4, 2023

View reviewed changes

internal/kafka/internal/services/clusters_test.go Outdated Show resolved Hide resolved

miguelsorianod force-pushed the gorm-dependabot-update-1 branch from 1487dbc to 5b17562 Compare April 11, 2023 09:47

github-actions bot added the connector label Apr 12, 2023

manstis mentioned this pull request Apr 14, 2023

[DRAFT] MGDCTRS-2034: Remove dbapi.ConnectorDeployment.Annotations #1565

Closed

6 tasks

miguelsorianod mentioned this pull request Apr 17, 2023

build(deps): bump gorm.io/gorm from 1.21.7 to 1.25.0 #1708

Closed

miguelsorianod force-pushed the gorm-dependabot-update-1 branch from 0157c5c to 9217fb9 Compare April 17, 2023 09:44

dependabot bot and others added 16 commits April 18, 2023 10:46

feat: update gorm.io/gorm from 1.23.7 to 1.27.10

c246769

feat: update gormigrate/v2 from 2.0.0 to 2.0.2

85d0f7e

chore: apply go mod tidy

c74176a

fix: fix unit tests after gorm dependencies update

401802d

feat: update gorm.io/gorm from 1.23.10 to 1.24.0

28d64fa

feat: update gorm.io/driver/postgres from 1.3.10 to 1.4.4

f663469

feat: update gorm and gorm postgres drivers versions

d13f771

gorm.io/gorm has been updated to v1.24.1 gorm.io/driver/postgres has been updated to v1.4.5

feat: reenable db prepared statements for kafka migrations

dc4ad88

the bug related to db prepared statements has been fixed on gorm.io/driver/postgres v1.4.5. We are now using that version so this reenables db prepared statements for kafka migrations

feat: update gorm.io/gorm from v1.24.1 to v1.24.2

77366af

feat: update gorm and gorm postgres drivers versions

b4b65d4

gorm.io/gorm has been updated to v1.24.3 gorm.io/driver/postgres has been updated to v1.4.6

feat: update gorm and gorm postgres drivers versions

5c608f2

gorm.io/gorm has been updated to v1.25.0 gorm.io/driver/postgres has been updated to v1.5.0

chore: apply go mod tidy

3a93bdd

fix: change cluster database error test to not expect returned values

7b0f8ed

After gorm version update to 1.25.0 the Save method does not return values.

Changes to support newer version of gorm.

04641f3

miguelsorianod force-pushed the gorm-dependabot-update-1 branch from 9217fb9 to 04641f3 Compare April 18, 2023 08:46

machi1990 approved these changes Apr 18, 2023

View reviewed changes

manstis approved these changes Apr 18, 2023

View reviewed changes

manstis mentioned this pull request Apr 22, 2023

Connectors flaky tests fix #1717

Open

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] update gorm and related libraries #1637

[WIP] update gorm and related libraries #1637

miguelsorianod commented Mar 14, 2023

machi1990 commented Mar 14, 2023

miguelsorianod commented Mar 16, 2023 •

edited

miguelsorianod commented Apr 11, 2023

miguelsorianod commented Apr 11, 2023 •

edited

miguelsorianod commented Apr 11, 2023

manstis commented Apr 12, 2023

manstis commented Apr 12, 2023

miguelsorianod commented Apr 12, 2023

manstis commented Apr 12, 2023

miguelsorianod commented Apr 12, 2023 •

edited

manstis commented Apr 12, 2023

miguelsorianod commented Apr 12, 2023

miguelsorianod commented Apr 12, 2023 •

edited

codecov bot commented Apr 12, 2023 •

edited

miguelsorianod commented Apr 12, 2023 •

edited

manstis left a comment

miguelsorianod commented Apr 18, 2023

[WIP] update gorm and related libraries #1637

Are you sure you want to change the base?

[WIP] update gorm and related libraries #1637

Conversation

miguelsorianod commented Mar 14, 2023

Description

Verification Steps

Checklist (Definition of Done)

machi1990 commented Mar 14, 2023

miguelsorianod commented Mar 16, 2023 • edited

miguelsorianod commented Apr 11, 2023

miguelsorianod commented Apr 11, 2023 • edited

miguelsorianod commented Apr 11, 2023

manstis commented Apr 12, 2023

manstis commented Apr 12, 2023

miguelsorianod commented Apr 12, 2023

manstis commented Apr 12, 2023

miguelsorianod commented Apr 12, 2023 • edited

manstis commented Apr 12, 2023

miguelsorianod commented Apr 12, 2023

miguelsorianod commented Apr 12, 2023 • edited

codecov bot commented Apr 12, 2023 • edited

Codecov Report

miguelsorianod commented Apr 12, 2023 • edited

manstis left a comment

Choose a reason for hiding this comment

miguelsorianod commented Apr 18, 2023

miguelsorianod commented Mar 16, 2023 •

edited

miguelsorianod commented Apr 11, 2023 •

edited

miguelsorianod commented Apr 12, 2023 •

edited

miguelsorianod commented Apr 12, 2023 •

edited

codecov bot commented Apr 12, 2023 •

edited

miguelsorianod commented Apr 12, 2023 •

edited