8000 MigrationModel duplicate entry by mabartos · Pull Request #39994 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

MigrationModel duplicate entry #39994

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Jun 11, 2025
Merged

MigrationModel duplicate entry #39994

merged 4 commits into from
Jun 11, 2025

Conversation

mabartos
Copy link
Contributor
@mabartos mabartos commented May 27, 2025 •
edited
Loading

Closes: #39866

  • Cleanup for duplicated old records in MIGRATION_MODEL table
  • Unique constraint on the VERSION col in MIGRATION_MODEL table
  • Advanced lock mechanism will be done in a follow-up issues as this might be backported to 26.2.x

I've reproduced the issue before:
Screenshot From 2025-05-27 14-05-52

After these changes, everything should be good:
Screenshot From 2025-05-27 15-43-50

Testing approach

  • Included these changes into distribution and built custom image mabartos/keycloak:26.3.0
  • Having Deployment config with 5 replicas
  • Initial version Keycloak 26.1.3
  • Then upgraded to the mabartos/keycloak/26.3.0
  • Change sets were propagated (cleanup + unique constraint)
  • Table contains only 2 records with versions (26.1.3 and 26.3.0)
Used Testing Deployment 
apiVersion: v1
kind: Service
metadata:
  name: keycloak
  labels:
    app: keycloak
spec:
  ports:
    - protocol: TCP
      port: 8080
      targetPort: http
      name: http
  selector:
    app: keycloak
  type: ClusterIP
---
apiVersion: v1
kind: Service
metadata:
  labels:
    app: keycloak
  # Used to
  name: keycloak-discovery
spec:
  selector:
    app: keycloak
  # Allow not-yet-ready Pods to be visible to ensure the forming of a cluster if Pods come up concurrently
  # publishNotReadyAddresses: true
  clusterIP: None
  type: ClusterIP
---
apiVersion: apps/v1
# Use a stateful setup to ensure that for a rolling update Pods are restarted with a rolling strategy one-by-one.
# This prevents losing in-memory information stored redundantly in two Pods.
#kind: StatefulSet
kind: Deployment
metadata:
  name: keycloak
  labels:
    app: keycloak
spec:
  #serviceName: keycloak-discovery
  # Run with one replica to save resources, or with two replicas to allow for rolling updates for configuration changes
  replicas: 5
  strategy:
    type: Recreate
  selector:
    matchLabels:
      app: keycloak
  template:
    metadata:
      labels:
        app: keycloak
    spec:
      containers:
        - name: keycloak
          image: quay.io/keycloak/keycloak:26.1.3
          args: ["start"]
          env:
            - name: KC_BOOTSTRAP_ADMIN_USERNAME
              value: "admin"
            - name: KC_BOOTSTRAP_ADMIN_PASSWORD
              value: "admin"
            # In a production environment, add a TLS certificate to Keycloak to either end-to-end encrypt the traffic between
            # the client or Keycloak, or to encrypt the traffic between your proxy and Keycloak.
            # Respect the proxy headers forwarded by the reverse proxy
            # In a production environment, verify which proxy type you are using, and restrict access to Keycloak
            # from other sources than your proxy if you continue to use proxy headers.
            - name: KC_PROXY_HEADERS
              value: "xforwarded"
            - name: KC_HTTP_ENABLED
              value: "true"
            # In this explorative setup, no strict hostname is set.
            # For production environments, set a hostname for a secure setup.
            - name: KC_HOSTNAME_STRICT
              value: "false"
            - name: KC_HEALTH_ENABLED
              value: "true"
            - name: 'KC_CACHE'
              value: 'ispn'
            # Use the Kubernetes configuration for distributed caches which is based on DNS
            - name: 'KC_CACHE_STACK'
              value: 'kubernetes'
            # Passing the Pod's IP primary address to the JGroups clustering as this is required in IPv6 only setups
            - name: POD_IP
              valueFrom:
                fieldRef:
                  fieldPath: status.podIP
            # Instruct JGroups which DNS hostname to use to discover other Keycloak nodes
            # Needs to be unique for each Keycloak cluster
            - name: JAVA_OPTS_APPEND
              value: '-Djgroups.dns.query="keycloak-discovery" -Djgroups.bind.address=$(POD_IP)'
            - name: 'KC_DB_URL_DATABASE'
              value: 'keycloak'
            - name: 'KC_DB_URL_HOST'
              value: 'postgres'
            - name: 'KC_DB'
              value: 'postgres'
            # In a production environment, use a secret to store username and password to the database
            - name: 'KC_DB_PASSWORD'
              value: 'keycloak'
            - name: 'KC_DB_USERNAME'
              value: 'keycloak'
          ports:
            - name: http
              containerPort: 8080
            - name: management
              containerPort: 9000  
          startupProbe:
            httpGet:
              path: /health/started
              port: 9000
            failureThreshold: 15
          readinessProbe:
            httpGet:
              path: /health/ready
              port: 9000
          livenessProbe:
            httpGet:
              path: /health/live
              port: 9000
          resources:
            limits:
              cpu: 2000m
              memory: 4000Mi
            requests:
              cpu: 1500m
              memory: 3500Mi
---
# This is deployment of PostgreSQL with an ephemeral storage for testing: Once the Pod stops, the data is lost.
# For a production setup, replace it with a database setup that persists your data.
apiVersion: apps/v1
kind: Deployment
metadata:
  name: postgres
  labels:
    app: postgres
spec:
  
  replicas: 1
  selector:
    matchLabels:
      app: postgres
  template:
    metadata:
      labels:
        app: postgres
    spec:
      containers:
        - name: postgres
          image: mirror.gcr.io/postgres:17
          env:
            - name: POSTGRES_USER
              value: "keycloak"
            - name: POSTGRES_PASSWORD
              value: "keycloak"
            - name: POSTGRES_DB
              value: "keycloak"
            - name: POSTGRES_LOG_STATEMENT
              value: "all"
          ports:
            - name: postgres
              containerPort: 5432
          volumeMounts:
            # Using volume mount for PostgreSQL's data folder as it is otherwise not writable
            - name: postgres-data
              mountPath: /var/lib/postgresql
      volumes:
        - name: postgres-data
          emptyDir: {}
---
apiVersion: v1
kind: Service
metadata:
  labels:
    app: postgres
  name: postgres
spec:
  selector:
    app: postgres
  ports:
    - protocol: TCP
      port: 5432
      targetPort: 5432
  type: ClusterIP

We might probably even update the MigrationModelAdapter to only update fields instead of trying to create a new record.

@mabartos mabartos requested a review from a team as a code owner May 27, 2025 13:49
@mabartos mabartos marked this pull request as draft May 27, 2025 13:51
@ahus1
Copy link
Contributor
ahus1 commented May 27, 2025

We might probably even update the MigrationModelAdapter to only update fields instead of trying to create a new record.

I disagree with this approach as we want to do an insert, as the insert and the unique constraint prevent doing a migration twice. Doing a migration twice on a realm might lead to wrong results as a developer would have never thought of that situation.

@keycloak-github-bot keycloak-github-bot bot mentioned this pull request May 29, 2025
Closed
@mabartos
Copy link
Contributor Author

@ahus1 Thanks for the review! 🌴 I've updated the approach based on your comments and I'll test it and fix the test cases later.

@mabartos mabartos mentioned this pull request May 30, 2025
Closed
@mabartos mabartos marked this pull request as ready for review May 30, 2025 12:19
@mabartos mabartos requested review from a team as code owne 8000 rs May 30, 2025 12:19
@mabartos
Copy link
Contributor Author
mabartos commented May 30, 2025 •
edited
Loading

Tried the approach again with the testing approach described in the PR description and everything works as expected.

image

@mabartos mabartos requested a review from ahus1 May 30, 2025 12:24
@keycloak-github-bot keycloak-github-bot bot mentioned this pull request May 30, 2025
Closed
@mabartos @ahus1
MigrationModel duplicate entry
5f44db7 
Closes keycloak#39866

Signed-off-by: Martin Bartoš <mabartos@redhat.com>
Co-authored-by: Alexander Schwartz <alexander.schwartz@gmx.net>
@ahus1
Review
a37beff 
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
@ahus1
Copy link
Contributor
ahus1 commented Jun 2, 2025

@mabartos - I reviewed this changed locally and tested some variations:

  • When adding a DB Schema, the delete failed as it was duplicating the schema prefix - and if you pass in the schema, you must not add a dot as a suffix, as Liquibase adds it. The safest way is to pass null, and then Liquibase does the right thing.
  • When there are entries in the table with the same version and the same update_time, the select will not find them, and therefore not delete them. I updated the SQL to compare the IDs instead.
  • I see that you pass in a normalized ID column. If we do that, we should do the same for the other columns as well.

I've pushed a commit that fixed it for me when testing it locally. Please let me know if you agree with these changes.

@mabartos
Consider update time and then decide
071c5db 
Signed-off-by: Martin Bartoš <mabartos@redhat.com>
@mabartos
Copy link
Contributor Author
mabartos commented Jun 10, 2025 •
edited
Loading

@ahus1 Thanks for the review and the additional testing!

The safest way is to pass null, and then Liquibase does the right thing.

+1

When there are entries in the table with the same version and the same update_time, the select will not find them, and therefore not delete them. I updated the SQL to compare the IDs instead.

Yes, that is true and your changes kinda resolve this issue. However, as IDs of MigrationModel entity (as PKs) are not auto-incremented and referenced as the resource tags, they are randomly generated and cannot be compared based on time of creation. It can lead to a situation in which a more recent record in a table is deleted even it was added later than the one before for the same version. I've slightly edited the SQL script to consider first the update time and then if those are equal choosing some random one. WDYT?

I see that you pass in a normalized ID column. If we do that, we should do the same for the other columns as well.

+1

Tried the same testing approach and it should work as expected.

@ahus1
Cleanup
47041df 
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
@mabartos
Copy link
Contributor Author

@ahus1 Just an info that I've added another commit to prevent some confusion: 071c5db

Described in: #39994 (comment)

ahus1
ahus1 approved these changes Jun 11, 2025
View reviewed changes
Copy link
Contributor
@ahus1 ahus1 left a comment

Choose a reason for hiding this comment

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

The new SQL is a bit more complicated than I hoped for. I verified it, and also did some manual tests with multiple duplicate entries, and it worked as expected.

Thank you for this change - approving!

@ahus1 ahus1 merged commit ad92af3 into keycloak:main Jun 11, 2025
76 checks passed
@mabartos
Copy link
Contributor Author

The new SQL is a bit more complicated than I hoped for.

@ahus1 Yes, me too, but at least we have it semantically correct. I tried to simplify things for reviews as much as possible.

Thanks!

@mabartos mabartos mentioned this pull request Jun 11, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahus1 ahus1 ahus1 approved these changes

Assignees
No one assigned
Labels
flaky-test team/cloud-native team/core-clients team/core-iam
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

MigrationModel duplicate entry on Recreate Upgrade in Cluster with 2+ nodes
2 participants
@mabartos @ahus1
0