Add support for JGroups JDBC_PING mode in clustered mode #139

danifr · 2020-06-23T12:47:35Z

Closes #138

treydock · 2020-06-24T13:18:52Z

spec/acceptance/1_class_spec.rb

+    end
+
+    describe port(7600) do
+      it { is_expected.to be_listening.on('0.0.0.0').with('tcp') }


This is failing. I am guessing because in the config template you set to ::ipaddress and here you are looking for 0.0.0.0 so the values don't match.

treydock · 2020-06-24T13:19:53Z

templates/config.cli.erb

+/subsystem=jgroups/channel=ee:write-attribute(name=stack, value="tcp")
+/subsystem=jgroups/stack=udp: remove()
+/socket-binding-group=standard-sockets/socket-binding=jgroups-udp:remove()
+/interface=private:write-attribute(name=inet-address, value=${jboss.bind.address.private:<%= scope['::ipaddress'] %>})


This and the public value should either be 0.0.0.0 or a parameter to define the addresses that defaults to $facts['networking']['ip'].

treydock · 2020-06-24T13:32:12Z

The template for modifying the tcp stack looks really redundant. There are several things that are removed and later added with no difference in their configuration. I would recommend taking the block under subsystem urn:jboss:domain:jgroups:7.0 and doing a diff before and after the jboss CLI runs to ensure the minimum amount of changes are being made in this pull request. Just looking at the changes it looks like it could be simplified to this:

/subsystem=jgroups/stack=tcp/protocol=MPING: remove()
/socket-binding-group=standard-sockets/socket-binding=jgroups-mping:remove()
/subsystem=jgroups/stack=tcp/protocol=pbcast.GMS: remove()
/subsystem=jgroups/stack=tcp/protocol=JDBC_PING: add(data-source="KeycloakDS", properties=[initialize_sql="CREATE TABLE IF NOT EXISTS JGROUPSPING (own_addr varchar(200) NOT NULL, cluster_name varchar(200) NOT NULL, updated TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, ping_data varbinary(5000) DEFAULT NULL, PRIMARY KEY (own_addr, cluster_name)) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin"])
/subsystem=jgroups/stack=tcp/protocol=pbcast.GMS: add(properties=[join_timeout=30000, print_local_addr=true, print_physical_addrs=true])
/subsystem=jgroups/channel=ee:write-attribute(name=stack, value="tcp")
/subsystem=jgroups/stack=udp: remove()
/socket-binding-group=standard-sockets/socket-binding=jgroups-udp:remove()

Also in my configs MPING shows as a socket-protocol not protocol. Also things like changes to pbcast.GMS could be simplified to use write-attribute rather than doing a remove + add.

Another thing is if someone says enable_jdbc_ping => true and then later switches to false the template needs to undo the changes so there needs to be the necessary changes to ensure the values are enforced when enable_jdbc_ping is false.

danifr · 2020-06-25T15:03:36Z

@treydock

after waaay too many hours trying to change the config.cli template without success, I finally understand why it was originally done in a redundant way.

TL;DR: Order matters. We need to delete them first so they are added in a certain order later on. Otherwise it does not work.

Are you OK leaving this part as it is? I will address your other comments.

treydock · 2020-06-25T15:52:52Z

Verified this works, but it can't be run twice, you have to add guard clauses before removing or adding otherwise the second time Puppet runs it will fail. If you take the code from that web article and run it 2 times, the second time will give errors because you can not add something if it already exists and can't remove something if it's already removed.

This is simplified logic, removed unnecessary add/remove logic and ensured can be run more than once without error:

if (outcome == success) of /subsystem=jgroups/stack=tcp/protocol=MPING:read-resource
/subsystem=jgroups/stack=tcp/protocol=MPING: remove()
end-if
if (outcome == success) of /subsystem=jgroups/stack=tcp/protocol=FD_SOCK:read-resource
/subsystem=jgroups/stack=tcp/protocol=FD_SOCK: remove()
/subsystem=jgroups/stack=tcp/protocol=FD_SOCK: add()
end-if
if (outcome == success) of /subsystem=jgroups/stack=tcp/protocol=pbcast.GMS:read-resource
/subsystem=jgroups/stack=tcp/protocol=pbcast.GMS: remove()
/subsystem=jgroups/stack=tcp/protocol=pbcast.GMS: add(properties=[join_timeout=30000, print_local_addr=true, print_physical_addrs=true])
end-if
if (outcome == success) of /subsystem=jgroups/stack=tcp/protocol=FRAG3:read-resource
/subsystem=jgroups/stack=tcp/protocol=FRAG3: remove()
end-if
if (outcome != success) of /subsystem=jgroups/stack=tcp/protocol=FRAG2:read-resource
/subsystem=jgroups/stack=tcp/protocol=FRAG2: add()
end-if
if (outcome != success) of /subsystem=jgroups/stack=tcp/protocol=JDBC_PING:read-resource
/subsystem=jgroups/stack=tcp/protocol=JDBC_PING: add(data-source="KeycloakDS", properties=[initialize_sql="CREATE TABLE IF NOT EXISTS JGROUPSPING (own_addr varchar(200) NOT NULL, cluster_name varchar(200) NOT NULL, updated TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, ping_data varbinary(5000) DEFAULT NULL, PRIMARY KEY (own_addr, cluster_name)) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin"])
end-if
/subsystem=jgroups/channel=ee:write-attribute(name=stack, value="tcp")
if (outcome == success) of /subsystem=jgroups/stack=udp:read-resource
/subsystem=jgroups/stack=udp: remove()
end-if
if (outcome == success) of /socket-binding-group=standard-sockets/socket-binding=jgroups-udp:read-resource
/socket-binding-group=standard-sockets/socket-binding=jgroups-udp:remove()
end-if
if (outcome == success) of /socket-binding-group=standard-sockets/socket-binding=jgroups-mping:read-resource
/socket-binding-group=standard-sockets/socket-binding=jgroups-mping:remove()
end-if
/interface=private:write-attribute(name=inet-address, value=${jboss.bind.address.private:0.0.0.0})
/interface=public:write-attribute(name=inet-address, value=${jboss.bind.address:0.0.0.0})

danifr · 2020-06-26T12:35:29Z

I made a few changes, apart from the most obvious ones.

Your code was still not working for me so I used add-index=0 to preserve the order https://github.com/keycloak/keycloak-containers/blob/b4a1fa301114ec08b70861d1cc6241ac8c85ef4a/server/tools/cli/jgroups/discovery/default.cli#L4
I improved the postgresql query by copying the one keycloak-container is using https://github.com/keycloak/keycloak-containers/blob/master/docker-compose-examples/keycloak-postgres-jdbc-ping.yml#L49
I realized FRAG3 works fine so I removed the fragment that was replacing it by FRAG2.

treydock · 2020-06-30T13:24:02Z

I think I know why there are many remove/add...that's a very odd way to make changes that can run multiple times since you can't do adds twice without either removing first or checking for existence.

Did you try the code below? If you had issues then what version of Keycloak and what errors? I tested in Vagrant using 8.0.1 and it works just fine. This removes the unnecessary removals. The PR diff I see still shows FRAG removal/add. The example below keeps it but can omit that part of keeping FRAG3 works.

I did a diff of standalone-ha.xml with your changes and then with changes below and diff looks similar if not identical.

if (outcome == success) of /subsystem=jgroups/stack=tcp/protocol=MPING:read-resource
/subsystem=jgroups/stack=tcp/protocol=MPING: remove()
end-if
/subsystem=jgroups/stack=tcp/protocol=FD_SOCK: remove()
/subsystem=jgroups/stack=tcp/protocol=FD_SOCK: add()
/subsystem=jgroups/stack=tcp/protocol=pbcast.GMS: remove()
/subsystem=jgroups/stack=tcp/protocol=pbcast.GMS: add(properties=[join_timeout=30000, print_local_addr=true, print_physical_addrs=true])
if (outcome == success) of /subsystem=jgroups/stack=tcp/protocol=FRAG3:read-resource
/subsystem=jgroups/stack=tcp/protocol=FRAG3: remove()
end-if
if (outcome != success) of /subsystem=jgroups/stack=tcp/protocol=FRAG2:read-resource
/subsystem=jgroups/stack=tcp/protocol=FRAG2: add()
end-if
if (outcome != success) of /subsystem=jgroups/stack=tcp/protocol=JDBC_PING:read-resource
/subsystem=jgroups/stack=tcp/protocol=JDBC_PING: add(data-source="KeycloakDS", properties=[initialize_sql="CREATE TABLE IF NOT EXISTS JGROUPSPING (own_addr varchar(200) NOT NULL, cluster_name varchar(200) NOT NULL, updated TIMESTAMP DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP, ping_data varbinary(5000) DEFAULT NULL, PRIMARY KEY (own_addr, cluster_name)) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_bin"])
end-if
/subsystem=jgroups/channel=ee:write-attribute(name=stack, value="tcp")
if (outcome == success) of /subsystem=jgroups/stack=udp:read-resource
/subsystem=jgroups/stack=udp: remove()
end-if
if (outcome == success) of /socket-binding-group=standard-sockets/socket-binding=jgroups-udp:read-resource
/socket-binding-group=standard-sockets/socket-binding=jgroups-udp:remove()
end-if
if (outcome == success) of /socket-binding-group=standard-sockets/socket-binding=jgroups-mping:read-resource
/socket-binding-group=standard-sockets/socket-binding=jgroups-mping:remove()
end-if
/interface=private:write-attribute(name=inet-address, value=${jboss.bind.address.private:0.0.0.0})
/interface=public:write-attribute(name=inet-address, value=${jboss.bind.address:0.0.0.0})

danifr · 2020-06-30T14:22:32Z

Sorry it seems I forgot to add the latest version of my config.cli in my latest commit last Friday.
I am running the test locally and if they pass I will push them to my feature branch

danifr · 2020-06-30T15:34:21Z

I don't think those failures are related to my change:

Scheduling refresh of Class[Systemd::Systemctl::Daemon_reload]
  Info: Systemd::Unit_file[keycloak.service]: Scheduling refresh of Service[keycloak]
  Info: Class[Systemd::Systemctl::Daemon_reload]: Scheduling refresh of Exec[systemctl-daemon-reload]
  Notice: /Stage[main]/Systemd::Systemctl::Daemon_reload/Exec[systemctl-daemon-reload]: Triggered 'refresh' from 1 event

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#build-times-out-because-no-output-was-received

The build has been terminated

treydock · 2020-07-01T14:20:25Z

templates/config.cli.erb

+if (outcome == success) of /subsystem=jgroups/stack=tcp/protocol=MPING:read-resource
+/subsystem=jgroups/stack=tcp/protocol=MPING: remove()
+end-if
+if (outcome == success) of /subsystem=jgroups/stack=tcp/protocol=FD_SOCK:read-resource


This specific condition can be removed. The remove + add makes the condition unnecessary.

Done it. I rebase -i and fixup the commit. Let me know if that works for you

treydock · 2020-07-01T14:24:06Z

I re-ran previous passing tests on master branch and they now hang, not sure what changed to break this, will have to investigate. I think only one minor change left for the config.cli template and this is good to go.

treydock reviewed Jun 24, 2020

View reviewed changes

danifr force-pushed the jdbc_ping branch 2 times, most recently from 6b294f4 to 9c7c3b2 Compare June 26, 2020 15:04

danifr force-pushed the jdbc_ping branch from 9c7c3b2 to b28addc Compare June 30, 2020 14:50

treydock reviewed Jul 1, 2020

View reviewed changes

treydock added the enhancement label Jul 1, 2020

danifr added 4 commits July 1, 2020 22:56

Add support for JGroups JDBC_PING mode in clustered mode

bab8c08

Add parameters for public and private jboss bind address

8a7c482

Fix acceptance tests for jdbc_ping

0016b0e

Add clustered JDBC_PING documentation

c36cf76

danifr force-pushed the jdbc_ping branch from b28addc to c36cf76 Compare July 1, 2020 20:58

treydock merged commit ee6894d into treydock:master Jul 2, 2020

danifr deleted the jdbc_ping branch July 6, 2020 06:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for JGroups JDBC_PING mode in clustered mode #139

Add support for JGroups JDBC_PING mode in clustered mode #139

danifr commented Jun 23, 2020

treydock Jun 24, 2020

treydock Jun 24, 2020

treydock commented Jun 24, 2020

danifr commented Jun 25, 2020

treydock commented Jun 25, 2020

danifr commented Jun 26, 2020

treydock commented Jun 30, 2020

danifr commented Jun 30, 2020

danifr commented Jun 30, 2020

treydock Jul 1, 2020

danifr Jul 1, 2020

treydock commented Jul 1, 2020

Add support for JGroups JDBC_PING mode in clustered mode #139

Add support for JGroups JDBC_PING mode in clustered mode #139

Conversation

danifr commented Jun 23, 2020

treydock Jun 24, 2020

Choose a reason for hiding this comment

treydock Jun 24, 2020

Choose a reason for hiding this comment

treydock commented Jun 24, 2020

danifr commented Jun 25, 2020

treydock commented Jun 25, 2020

danifr commented Jun 26, 2020

treydock commented Jun 30, 2020

danifr commented Jun 30, 2020

danifr commented Jun 30, 2020

treydock Jul 1, 2020

Choose a reason for hiding this comment

danifr Jul 1, 2020

Choose a reason for hiding this comment

treydock commented Jul 1, 2020