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

Infinispan deployment #426

Merged
merged 2 commits into from
Jul 20, 2023
Merged

Infinispan deployment #426

merged 2 commits into from
Jul 20, 2023

Conversation

pruivo
Copy link
Contributor

@pruivo pruivo commented Jul 12, 2023

And Keycloak deployment connecting to the Infinispan server.

Closes #417

# MAIN #
########

if [ "${CLUSTER_1}" == "${CLUSTER_2}" ]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

To save having to set both variables, should we just check if CLUSTER_2 is unset/empty?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CLUSTER_2 and NS_2 are now optional.

EOF
}

function deploy_cache_cr_without_cross_site() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't worry, I'm not going to suggest you change it now you've written everything in bash ... but did you consider writing a custom helm chart like the keycloak approach for the CRs? I'm just curious if there were any technical reasons why you couldn't do it that way, or if it was just personal preference.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just because it was quicker. I need to convert the bash to Taskfiles so they can be imported/includes with other Taskfiles. And the helm chart would save some copy-paste too.

@mhajas mhajas self-requested a review July 13, 2023 12:47
@pruivo pruivo force-pushed the t_ispn_cluster branch 3 times, most recently from 272734b to 515c809 Compare July 13, 2023 16:30
@pruivo pruivo marked this pull request as ready for review July 13, 2023 16:31
@ryanemerson ryanemerson self-requested a review July 14, 2023 10:26
Copy link
Contributor

@ryanemerson ryanemerson left a comment

Choose a reason for hiding this comment

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

LGTM, just one nitpick.

provision/infinispan/commons.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@mhajas mhajas left a comment

Choose a reason for hiding this comment

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

Thank you very much for this. This looks great!

Unfortunately, I wasn't able to try everything, but I see Keycloak connected to ISPN in a different namespace and Infinispan clustered together withing different namespaces.

I added a few comments to the changes.

Copy link
Contributor

Choose a reason for hiding this comment

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

This can be moved to doc folder

provision/infinispan/README.adoc Outdated Show resolved Hide resolved

== Provision Keycloak

The Taskfile in `provistion/openshift` introduced 4 more variables:
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want to convert the bash to taskfiles so it can be imported and merged with the other taskfiles. I'll add docs when everything is in place.

Copy link
Contributor

Choose a reason for hiding this comment

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

@pruivo was there a GH issue created for this task ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kami619 no, I haven't created one. Let me do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

thanks Pedro

Copy link
Contributor

Choose a reason for hiding this comment

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

@kami619 @pruivo sorry for jumping into your conversation, as far as I know Michal also created a task for this purposes - #450 . @mhajas please correct me if I am wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looks pretty similar. Thanks @andyuk1986
I've closed mine.

provision/infinispan/create_ispn_clusters.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@mhajas mhajas Jul 14, 2023

Choose a reason for hiding this comment

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

How did you achieve database replication? Or do we wait for @ryanemerson to make aurora work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I only tested KC -> ISPN connection and ISPN <-> ISPN connection. Yes we need aurora or something similar for database replication.

@ryanemerson
Copy link
Contributor

I just tested with two ROSA clusters and the xsite view is forming as intended:

15:54:20,041 INFO  (jgroups-8,infinispan-0-31854) [org.infinispan.XSITE] ISPN000439: Received new cross-site view: [gh-keycloak, gh-ryanemerson]

@ahus1 ahus1 self-requested a review July 20, 2023 08:31
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.

As agreed in our weekly meeting, we'll merge it once it has been reviewed as a first step. Thank you to the author @pruivo and all reviewers @ryanemerson @mhajas @andyuk1986!

@ahus1 ahus1 merged commit 32e408a into keycloak:main Jul 20, 2023
2 checks passed
local cluster="${2}"

# if file exists, assume oc login is done
[ -f "${kubecfg}" ] || KUBECONFIG="${kubecfg}" CLUSTER_NAME="${cluster}" ${WD}/../aws/rosa_oc_login.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

This causes problems if ./kubecfg_1 and/or ./kubecfg_2 exist from the previous day as we'll attempt to contact ROSA cluster endpoints that no longer exist. A potential solution is to use /tmp here, alongside a timestamp in the filename to avoid conflicts.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ryanemerson - in the OpenShift taskfile, we use `KC_HOSTNAME_SUFFIX´ to see if the cluster has changed. Maybe this opens some possibilities to auto-detect this.

@pruivo pruivo deleted the t_ispn_cluster branch July 25, 2023 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deploy Infinispan cluster
6 participants