Skip to content
This repository has been archived by the owner on Jan 29, 2024. It is now read-only.

Configure JGroups like DB, in entrypoint #100

Closed
wants to merge 10 commits into from

Conversation

solsson
Copy link

@solsson solsson commented Dec 15, 2017

This PR is based on my interpretation of the discussion in #92, in particular the remarks from @stianst:

  • Add support for configuration at startup, triggered by environment variables and applied through jboss-cli
  • Do everything in the jboss/keycloak image
  • Add a kubernetes example in a separate folder

The structure for JGroups is heavily inspired by #96. Thanks @rayscunningham, I think the two PRs could easily be merged.

Now, my chief insight from #92 and #94 was that DNS_PING is significantly less complex than both JDBC_PING and KUBE_PING. Here I've actually implemented something similar with JGroups 3 in a couple of lines of bash. To use it:

  1. Set up a DNS record that points to your keycloak instances. In Kubernetes that would be a headless service.
  2. Set JGROUPS_SETUP env to TCPPING which basically only triggers the execution of getent hosts $JGROUPS_DNS_NAME.

I seem to get faster cluster joins with this strategy, and no records left behind at instance shutdown.

For those who want to stick with JDBC_PING, set JGROUPS_SETUP env to JDBC_PING. It's sligtly improved compared to #92, with less waiting for JGroups to ping exited instances, thanks to clear_table_on_view_change.

Note that #95 hasn't been merged, so logging to console in thits PR is always at or above INFO level.

fails to load database driver. Might require a module update.
http://www.jgroups.org/manual/index.html#_jdbc_ping
cautions against this because it increases database traffic
but JGroups is designed for thousands of replicas and that'll
rarely be the case with something like keycloak.
as it won't be worth the trouble of adding module jars,
and maintaining duplicate connection strings.

JNDI reuse is probably quite robust now despite the
failing shutdown hook, thanks to clear_table_on_view_change
@@ -25,5 +25,10 @@ else
echo "[KEYCLOAK DOCKER IMAGE] Using the embedded H2 database"
fi

if [ "$JGROUPS_SETUP" != "" ]; then
echo "[KEYCLOAK DOCKER IMAGE] Using custom JGroups setup $JGROUPS_SETUP"
/bin/sh /opt/jboss/keycloak/bin/change-jgroups.sh $JGROUPS_SETUP
Copy link

Choose a reason for hiding this comment

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

This won't forward OS signals: I gave it a try and didn't configure the DNS properly, and the Docker container got stuck in the "Looking up initial hosts for DNS name" loop. It couldn't be stopped with a SIGTERM, since bash doesn't forward signals to child processes.

I exchanged this line with the following lines and was able to stop the Docker image properly when stuck in the initial discovery phase.

trap 'kill -TERM $CHILD_PID; exit 1' TERM INT
/opt/jboss/keycloak/bin/change-jgroups.sh "$JGROUPS_SETUP" &
CHILD_PID=$!
wait $CHILD_PID
trap - TERM INT
wait $CHILD_PID

Copy link
Author

Choose a reason for hiding this comment

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

Good point. I think I just copied this from how change-database.sh is invoked. I'd suggest a switch to /bin/bash, but I would prioritize similarity with change-database.

@solsson
Copy link
Author

solsson commented Feb 19, 2018

Running this solution in production since a month or two, on average three keycloak instances, without any issue.

@hannesrohde
Copy link

Hello Staffan,
thank you for your work on this PR, it was a big help for me getting our Keycloak cluster running with JGROUPS_JDBC!

Is is intentional that you only set the cache owners attribute for the caches

  • sessions, authenticationSessions, offlineSessions, loginFailures
    but not for
  • clientSessions, offlineClientSessions, actionTokens

Also, I found that we did not have to expose ports 9090 (management), 4712 (txn-recovery-ev) and 4713 (txn-status-mgr) to get the cluster working, is that correct?

@solsson
Copy link
Author

solsson commented Apr 5, 2018

@hannesrohde

Is is intentional that you only set the cache owners attribute for the caches

Got this from #92 (comment). I haven't investigated further.

Also, I found that we did not have to expose ports 9090 (management), 4712 (txn-recovery-ev) and 4713 (txn-status-mgr) to get the cluster working, is that correct?

Regarding 9090 it depends on if you want to do management from other pods. For the two txn ports I don't know. I started out exposing every port to eliminate that as a source of jgroups errors, so it's quite likely that those are not needed.

@hannesrohde
Copy link

In the manual http://www.keycloak.org/docs/3.4/server_installation/index.html#cache it sounds like the caches should all be replicated (and have multiple owners).
I have now changed cache-owners.cli to use the same setting "value=${env.CACHE_OWNERS:1}" for all caches. We will be running a test setup for the next few weeks and I will check if there are any issues.

@MCalverley
Copy link

MCalverley commented Apr 22, 2018

Thanks a lot for this! This has been working great for me for months (upgraded to Keycloak 3.4.3 of course). Using the following Deployment yaml on GKE with the nginx ingress controller handling HTTPS:

apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  labels:
    app: keycloak
    service: keycloak
  name: keycloak
spec:
  replicas: 2
  selector:
    matchLabels:
      app: keycloak
  strategy:
    type: RollingUpdate
  revisionHistoryLimit: 2
  template:
    metadata:
      name: keycloak
      labels:
        app: keycloak
        service: keycloak
    spec:
      containers:
      - name: keycloak
        image: # Docker image built from pull request here
        imagePullPolicy: IfNotPresent
        args:
        - -b=0.0.0.0
        - --server-config=standalone-ha.xml
        env:
        - name: JGROUPS_SETUP
          value: TCPPING
        - name: CACHE_OWNERS
          value: "2"
        - name: POD_IP
          valueFrom:
            fieldRef:
              fieldPath: status.podIP
        - name: DB_VENDOR
          value: postgres
        - name: POSTGRES_DATABASE
          value: keycloak
        - name: POSTGRES_USER
          valueFrom:
            secretKeyRef:
              name: environment
              key: postgres_username
        - name: POSTGRES_PORT_5432_TCP_ADDR
          valueFrom:
            secretKeyRef:
              name: environment
              key: postgres_server
        - name: POSTGRES_PASSWORD
          valueFrom:
            secretKeyRef:
              name: environment
              key: postgres_password
        - name: PROXY_ADDRESS_FORWARDING
          value: "true"
        - name: KEYCLOAK_USER
          valueFrom:
            secretKeyRef:
              name: environment
              key: keycloak_user
        - name: KEYCLOAK_PASSWORD
          valueFrom:
            secretKeyRef:
              name: environment
              key: keycloak_password
        ports:
        - name: keycloak-http
          containerPort: 8080
        - name: jgroups-tcp
          containerPort: 7600
        readinessProbe:
          httpGet:
            path: /auth/
            port: keycloak-http
        livenessProbe:
          exec:
            command:
            - sh
            - -c
            - "curl -f -s -o/dev/null http://localhost:8080/auth/ && ! jstack -l $(ps aux | grep [j]ava | awk '{ print $2 }') | grep -A6 'default task' | grep -q apache"
          initialDelaySeconds: 120
          periodSeconds: 60
          timeoutSeconds: 3
        resources:
          requests:
            memory: 512Mi
          limits:
            memory: 1Gi
        securityContext:
          privileged: false
        terminationMessagePath: /dev/termination-log
      dnsPolicy: ClusterFirst
      restartPolicy: Always
      securityContext: {}
      terminationGracePeriodSeconds: 30
---
apiVersion: v1
kind: Service
metadata:
  name: keycloak
  labels:
    app: keycloak
    service: keycloak
spec:
  ports:
  - name: auth
    port: 8080
    targetPort: keycloak-http
    protocol: TCP
  selector:
    app: keycloak
---
apiVersion: v1
kind: Service
metadata:
  name: jgroups-dns-ping
  labels:
    app: keycloak
    service: keycloak
  annotations:
    service.alpha.kubernetes.io/tolerate-unready-endpoints: "true"
spec:
  publishNotReadyAddresses: true
  clusterIP: None
  selector:
    app: keycloak
  ports:
  - protocol: TCP
    port: 7600
    targetPort: jgroups-tcp

The liveness probe looks for a deadlock we've been having about once a week. Haven't been able to find the cause of it yet, but it seems to occur in server-to-server OIDC communication as we're using one realm as an identity provider for another realm on the same Keycloak server.

@solsson
Copy link
Author

solsson commented Apr 23, 2018

@MCalverley Do you run with the additional cache_owners=2 attribute that @hannesrohde suggests?

We're also in production since a couple of months. Good that you've added memory limits and reduced the number of ports to expose.

In strategy: we've found the following to have a more pleasant rolling update behavior, though admittedly causing a memory use spike:

    rollingUpdate:
      maxSurge: 1
      maxUnavailable: 0

@MCalverley
Copy link

@solsson, yes, I set cache owners as part of the environment variables:

        - name: CACHE_OWNERS
          value: "2"

I think it works correctly since I at least see it rebalancing to two cache owners when it restarts because of that deadlock bug I mentioned.
Thanks for the suggestion of changing the update strategy! I'll have to do some tests next time there's a Keycloak update to install to see how much of a difference there is.

@stianst
Copy link
Contributor

stianst commented Jun 8, 2018

A bit of an update from the Keycloak team. We finally have managed to set aside some time to start looking at this issue. I like having both DNS_PING and KUBE_PING available and this is a great starting point for that.

We are also working on upgrading to WildFly 13 and hopefully Keycloak 4.0.0.Final will be upgraded to that. That should provide make it much easier to also provide KUBE_PING support.

Will get around to reviewing this PR in detail soon. Hopefully next week.

@solsson
Copy link
Author

solsson commented Jun 8, 2018

Good news, good work @stianst. I guess this means you'll only need some kind of kubernetes manifest example?

@stianst
Copy link
Contributor

stianst commented Jun 8, 2018

What I'm thinking is to review this PR. Make sure it gets to a mergable state. Merge that. Then add KUBE_PING as well and we can have the option to switch between the two like we do for DBs today. I'd like to await having Keycloak on WF 13 though first so we can check it works properly there. Also, need to make sure the README file covers all the env values for clustering setup.

@pete-woods
Copy link

Very excited about this support, if it gets merged. It's pretty much the only blocker to being able to use this official KeyCloak image in HA mode. Without this, you need to roll your own to enable the *_PING support.

@stianst
Copy link
Contributor

stianst commented Jun 15, 2018

Some more news from the Keycloak team. WildFly 13 did not make it into 4.0.0.Final. It's currently targeted to 4.1.0.Final. Then there is also PTOs coming up soon, so most likely it will take at least another month until we can pick up this work and review it properly.

@pete-woods
Copy link

Oh well. At least it's easy to roll your own image. For what it's worth, I de-conflicted @solsson 's branch here: https://github.com/pete-woods/keycloak/tree/server-jgroups-setup

@stianst
Copy link
Contributor

stianst commented Aug 20, 2018

An update from the Keycloak team with regards to clustering support in the Docker image. Keycloak 4.4.0.Final will be upgraded to WildFly 13 Final which has a number of improvements around discovery mechanisms and will be much simpler to enable clustering with it. We also now have an previous Infinispan team member on our team who will be looking at this after 4.4 is out. The aim will be to have easy support for clustering on standalone Docker, Kubernetes and OpenShift out of the box.

@stianst
Copy link
Contributor

stianst commented Aug 20, 2018

From @slaskawi it looks like DNS_PING will handle both OpenShift and Kubernetes use-cases. What we do for standalone Docker is still to be decided.

@pete-woods
Copy link

Something that works well in AWS's ECS would be ideal. I've been using the JDBC_PING support from this PR, and it works well with basically no extra setup required.

@stianst
Copy link
Contributor

stianst commented Aug 23, 2018

I'm guessing DNS_PING doesn't work in S3 unless you're running on top of Kubernetes or OpenShift?

@pete-woods
Copy link

When I was looking at setting this up originally, it looked like getting DNS_PING would require more work on the AWS side, whereas AWS's database tech (RDS and RDS clusters) worked with JDBC_PING out of the box with no config changes.

@frekele
Copy link

frekele commented Aug 23, 2018

@pete-woods

If you are interested, an alternative is to use S3_PING into aws. It is better to use S3_PING instead of JDBC_PING, because jdbc_ping will overload your DATABASE with many queries.
Best still is to use the DNS_PING that will be available after from keycloak 4.4.0.Final with support at
JGroups 4.x as the @stianst commented.

@pete-woods
Copy link

Thanks for the push in the right direction, folks. Seems like ECS supports DNS based service discovery: https://docs.aws.amazon.com/AmazonECS/latest/developerguide/create-service-discovery.html

@slaskawi
Copy link
Contributor

Thanks a lot for the contribution!

We decided to refactor the HA part a bit. In #151 we proposed a universal mechanism for using different discovery protocols. In the Pull Request we uploaded a test image so that you can play with it. Please have a look at the Pull Request and put your comments there.

@stianst Please close it in favor of #151

@stianst stianst closed this Sep 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants