MGMT-19930: upgrade PostgreSQL stack to pgx v5 and ocm-sdk-go v0.1.499#10232
Closed
andrej1991 wants to merge 1 commit intoopenshift:masterfrom
Closed
MGMT-19930: upgrade PostgreSQL stack to pgx v5 and ocm-sdk-go v0.1.499#10232andrej1991 wants to merge 1 commit intoopenshift:masterfrom
andrej1991 wants to merge 1 commit intoopenshift:masterfrom
Conversation
JUST TESTING IF ALL THE CHANGES ARE NEEDED!!!
This branch moves assisted-service from jackc/pgx/v4 (via older GORM
postgres driver and transitive pgtype/pgconn) to the stack required by
newer openshift-online/ocm-sdk-go, and aligns GORM, DSN construction,
custom DB types, OCM error handling, and subsystem WireMock with that
reality.
Dependency upgrades (go.mod / go.sum + vendor)
-----------------------------------------------
- github.com/openshift-online/ocm-sdk-go: v0.1.205 -> v0.1.499
Brings in github.com/openshift-online/ocm-api-model/{clientapi,model}
v0.0.453 and other transitive updates (e.g. skratchdot/open-golang).
v0.1.499 is built for pgx v5; keeping ocm-sdk-go on v0.1.205 while
moving the rest of the tree to pgx v5 is not viable.
- PostgreSQL driver stack:
- gorm.io/driver/postgres: v1.3.5 -> v1.6.0 (uses pgx v5 under the hood)
- gorm.io/gorm: v1.25.8 -> v1.25.10
- Remove jackc/pgx/v4, pgconn, pgtype, pgio, chunkreader, pgproto3.
- Add jackc/pgx/v5 v5.6.0 and jackc/puddle/v2 (pool support).
- Bump jackc/pgservicefile to a release compatible with pgx v5.
- Drop replace github.com/jackc/pgx/v4 => v4.18.3 (no longer used).
- golang.org/x/sys minor bump on the branch (toolchain alignment).
Submodule go.mod hygiene (api/, client/, models/)
------------------------------------------------
- Remove unused indirect jackc/pgio and jackc/pgtype from api/go.mod and
client/go.mod.
- Remove github.com/jackc/pgtype and pgio from models/go.mod now that
models/custom.go no longer references pgtype.
Why ocm-sdk-go v0.1.499 matters for assisted-service
----------------------------------------------------
assisted-service uses pkg/ocm against accountsmgmt/v1 for:
- ClusterAuthorizations().Post() — create AMS subscription at register.
- Subscriptions().Subscription(id).Get() — read subscription.
- Subscriptions().Subscription(id).Update().Body(sub).SendContext() —
PATCH display name, console URL, external_cluster_id (OpenShift
cluster ID), and status Active.
- Subscriptions().Subscription(id).Delete().SendContext() — delete
subscription on cluster deregister.
Compared to v0.1.205, the newer SDK + ocm-api-model clientapi layer:
1) JSON encoding of API types (e.g. Subscription) is generated in
ocm-api-model/clientapi (jsoniter, explicit field-set bitmaps).
PATCH bodies always include a leading "kind" field and only the
fields the builder set. That affects HTTP fixtures (subsystem
WireMock) that matched only minimal JSON.
2) Subscription PATCH response handling: SendContext wraps the body in
a bufio.Reader and uses Peek(1). If the server returns an HTTP error
with an empty body, Peek hits io.EOF; the client returns with err == nil
while result.status is still 4xx/5xx, and never unmarshals an
errors.Error. assisted-service previously assumed "err != nil" for
failures. pkg/ocm.HandleOCMResponse now treats response.Status() >= 400
as failure even when err is nil (mirrors the intent of the generated
error path when a body exists).
3) Error responses are unmarshalled into github.com/openshift-online/ocm-sdk-go/errors
when the body is valid OCM "Error" JSON. Subsystem stubs that returned
a subscription-shaped JSON body with HTTP 401 caused unmarshalling /
downstream UTF-8 issues; stubs now return proper Error JSON for PATCH
failures (see wiremock_stubs.go).
Per-file / functional changes
-------------------------------
pkg/db/db.go
- Add LibpqDSN(host, port, user, password, database) and Config.LibpqDSN().
- Build a libpq keyword connection string with escaped user/password for
single quotes and backslashes.
- Append client_encoding=UTF8 so pgx v5 / GORM simple-query paths behave
predictably regardless of server client_encoding defaults.
cmd/main.go
- setupDB: use Options.DBConfig.LibpqDSN() instead of hand-rolled
host=... DSN (same semantics, safer quoting, UTF8).
internal/common/common_unitest_db.go
- getDBDSN: delegate to pkg/db.LibpqDSN for unit-test PostgreSQL so tests
use the same DSN rules as production.
models/custom.go
- Remove dependency on github.com/jackc/pgtype (pgx v4 era).
- IP/Subnet driver.Valuer: encode inet/cidr using netip and return textual
prefix strings PostgreSQL accepts.
- IP/Subnet sql.Scanner: accept netip.Prefix / *netip.Prefix as returned
by pgx v5 for inet/cidr, in addition to legacy []byte, string, net.IP,
*net.IPNet.
internal/bminventory/inventory.go
- InstallClusterInternal async goroutine: call storeOpenshiftClusterID
with asyncCtx instead of the outer request ctx so S3/object-store work
is not tied to an HTTP request deadline that can fire before AMS
integration completes.
internal/cluster/cluster.go
- HandlePreInstallError: sanitize installErr.Error() with
strings.ToValidUTF8 before persisting LastInstallationPreparation.Reason
and emitting events. OCM error bodies / parse paths can produce strings
that are not valid UTF-8; PostgreSQL text columns reject those writes.
pkg/ocm/utils.go
- HandleOCMResponse: after the existing err != nil handling, if response is
non-nil and Status() >= 400, return InfraError (401 mapping) for 4xx
and ApiError (503) for 5xx. Covers empty-body HTTP errors from newer
SendContext implementations.
subsystem/utils_test/wiremock_stubs.go
- subscriptionPatchWiremockResponse: for resStatus >= 400, return OCM
Error JSON so the SDK can UnmarshalErrorStatus; avoids subscription-
shaped 401 bodies and invalid UTF-8 in DB reasons.
- Subscription PATCH external_cluster_id: use matchesJsonPath on
$.external_cluster_id instead of equalToJson on a minimal template, so
stubs stay stable across SDK JSON field order/extra fields and reduce
ambiguous matching when many mappings exist.
- Add WireMock stubs for soft-timeouts org capability review and an
accounts_mgmt stub for the cluster-editor test user so authorization
flows match current API expectations.
openshift-ci-robot
added
the
jira/valid-reference
|
@andrej1991: This pull request references MGMT-19930 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
Contributor
Author
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (292)
📒 Files selected for processing (8)
💤 Files with no reviewable changes (1)
WalkthroughTwo indirect Go module dependencies ( Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~2 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
size/XL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: andrej1991 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
api-review
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10232 +/- ##
==========================================
- Coverage 44.36% 44.34% -0.02%
==========================================
Files 416 417 +1
Lines 72793 72824 +31
==========================================
+ Hits 32292 32293 +1
- Misses 37579 37603 +24
- Partials 2922 2928 +6
🚀 New features to boost your workflow:
|
|
@andrej1991: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
openshift-ci
Bot
added
do-not-merge/work-in-progress
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
JUST TESTING IF ALL THE CHANGES ARE NEEDED!!!
This branch moves assisted-service from jackc/pgx/v4 (via older GORM postgres driver and transitive pgtype/pgconn) to the stack required by newer openshift-online/ocm-sdk-go, and aligns GORM, DSN construction, custom DB types, OCM error handling, and subsystem WireMock with that reality.
Dependency upgrades (go.mod / go.sum + vendor)
github.com/openshift-online/ocm-sdk-go: v0.1.205 -> v0.1.499 Brings in github.com/openshift-online/ocm-api-model/{clientapi,model} v0.0.453 and other transitive updates (e.g. skratchdot/open-golang). v0.1.499 is built for pgx v5; keeping ocm-sdk-go on v0.1.205 while moving the rest of the tree to pgx v5 is not viable.
PostgreSQL driver stack:
golang.org/x/sys minor bump on the branch (toolchain alignment).
Submodule go.mod hygiene (api/, client/, models/)
Why ocm-sdk-go v0.1.499 matters for assisted-service ---------------------------------------------------- assisted-service uses pkg/ocm against accountsmgmt/v1 for:
Compared to v0.1.205, the newer SDK + ocm-api-model clientapi layer:
JSON encoding of API types (e.g. Subscription) is generated in
ocm-api-model/clientapi (jsoniter, explicit field-set bitmaps).
PATCH bodies always include a leading "kind" field and only the
fields the builder set. That affects HTTP fixtures (subsystem
WireMock) that matched only minimal JSON.
Subscription PATCH response handling: SendContext wraps the body in
a bufio.Reader and uses Peek(1). If the server returns an HTTP error
with an empty body, Peek hits io.EOF; the client returns with err == nil
while result.status is still 4xx/5xx, and never unmarshals an
errors.Error. assisted-service previously assumed "err != nil" for
failures. pkg/ocm.HandleOCMResponse now treats response.Status() >= 400
as failure even when err is nil (mirrors the intent of the generated
error path when a body exists).
Error responses are unmarshalled into github.com/openshift-online/ocm-sdk-go/errors
when the body is valid OCM "Error" JSON. Subsystem stubs that returned
a subscription-shaped JSON body with HTTP 401 caused unmarshalling /
downstream UTF-8 issues; stubs now return proper Error JSON for PATCH
failures (see wiremock_stubs.go).
Per-file / functional changes
pkg/db/db.go
cmd/main.go
internal/common/common_unitest_db.go
models/custom.go
internal/bminventory/inventory.go
internal/cluster/cluster.go
pkg/ocm/utils.go
subsystem/utils_test/wiremock_stubs.go
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs, README, etc)Reviewers Checklist
Summary by CodeRabbit