MariaDB HA via Galera and tests #23

jcpowermac · 2018-02-16T21:53:57Z

Adds an HA plan which is provided using Galera.
Adds testing for HA only (communication to the mariadb service using the other plans does not function needs additional investigation)

jcpowermac · 2018-02-20T15:15:30Z

@jmontleon can I get a review

jmontleon · 2018-02-20T15:40:59Z

@jcpowermac @djzager @jwmatthews @dymurray Overall this looks like a good start to me. I'll try to give it a test drive today.

One concern I have right now this repo serves as the source for both the upstream and downstream apb role packages but this diverges pretty significantly from what we are shipping downstream (and maybe even what we are able to?)

For the mediawiki where we have a different downstream/upstream image we deploy we created a patch file.
https://raw.githubusercontent.com/ansibleplaybookbundle/mediawiki-apb/master/downstream.patch

This gets applied whenever we don't build in the upstream copr:
https://github.com/ansibleplaybookbundle/mediawiki-apb/blob/master/mediawiki-apb-role.spec#L22-L24

I don't have a better idea at the moment as to how we handle this, but it also feels like the diff could get out of hand and become a pain to manage rather quickly in a more advanced case where we're ripping out entire plans as well.

Does anyone have a better idea?

djzager

I agree with @jmontleon that this is a good start. I had a few comments just from looking it over.

I am a fan of the testing included, although slightly hard to follow between test_tasks.yml and separately including (ha|dev|prod).yml tasks conditionally on the plan & action variables. I believe we can do better than that.

This is the first example of a test playbook that I have seen that actually does testing instead of simple verifications. @dymurray would be proud.

I'm curious about two things:

Could/would/should this be a separate APB entirely with development and production plans for mariadb-ha? (I'm not suggesting this is the route we should take, just trying to give possibilities based on @jmontleon 's comments)
Is it possible for ha to be a parameter instead of a separate plan? We currently have dev|prod plans for mariadb (not ha), why not dev|prod plans for mariadb-ha?

djzager · 2018-02-20T15:52:23Z

playbooks/test.yml

+      vars:
+        plan: "{{ outer_item }}"
+      with_items:
+        - ha


Why only ha? I see you've added dev, ha, prod to playbooks/vars/, but it only looks like you are using the one.

@djzager I kept having problems with the tests failing for dev and prod. Specifically when trying to the mysql commands to create the table via either the service or pod ip address. Its something I figured after I got this PR merged that I could look into more.

That is fair to me. Adding a todo for the remainder would be a good idea then.

djzager · 2018-02-20T15:53:14Z

playbooks/vars/dev.yml

+---
+_apb_plan_id: "dev"
+service_name: "rhscl-mariadb"
+namespace: "test-{{ _apb_plan_id }}-{{ service_name }}"


Are we not able to push most of these variables down into the role vars/defaults?

I don't believe this is as practical as I originally thought when making the comment, however I do believe the simpler our Ansible code, the better.

djzager · 2018-02-20T16:06:31Z

roles/rhscl-mariadb-apb-openshift/tasks/test.yml

+- set_fact:
+    podobj: "{{ pods.stdout | from_json }}"
+
+# Create table using service


Look @dymurray it's a test.yml and I like it.

jcpowermac · 2018-02-20T17:29:24Z

@djzager

I am ok with the ha being in another repo but if that is the case should we do it for the other database apbs as well right? I am asking this because next up for me is postgresql ha.
I was testing with a "dev" like instance for most of my testing so an ephemeral version of HA is certainly doable - the statefulset template would just need to be changed to support that.

djzager · 2018-02-20T18:00:10Z

@jcpowermac

If we can avoid creating new projects to support HA in our database APBs, we should absolutely do that. I know I had mentioned it as a possibility, but your comment about moving onto postgresql made it clear how bad of an idea having 10 database APBs 5 for non-ha and 5 for ha 🤒 .
I believe it would be best if this (and future) APBs could take the HA as a non-updateable parameter instead of as a plan (it feels like plan vs. parameter is a distinction without a difference).
One thing that we need for our APBs, and the Ansible code it is made of, is for them to be as simple and consumable as possible. I notice that we rely heavily on the when clause to handle everything from including tasks to deploying k8s services. It is outside the scope of this PR to fix those problems. I would ask though that you find a way not to have when: action != "test" throughout the main task file.

djzager

Putting my earlier comments into a review.

djzager · 2018-02-20T18:01:29Z

roles/rhscl-mariadb-apb-openshift/tasks/main.yml

@@ -32,12 +34,24 @@
      protocol: TCP
      target_port: 3306
  register: mariadb_service
+  when:
+    - _apb_plan_id != "ha"
+    - action != "test"


I would like it if we could move away from our reliance on conditionals.

djzager · 2018-02-20T18:02:11Z

apb.yml

@@ -61,6 +65,15 @@ plans:
    parameters: *_params
    updates_to:
      - prod
+  - name: ha


Would like for this to be a parameter instead of a plan.

jcpowermac · 2018-02-20T18:31:42Z

@djzager I can start making the changes from a plan -> parameter but that doesn't seem to resolve @jmontleon concerns.

djzager · 2018-02-20T18:45:38Z

😎 @jcpowermac I knew I was forgetting something.

@jmontleon I have a few questions that I'm not sure how to answer.

What is the difference between the redhat and centos images? Why do we need the centos images in order to support ha?

    - 'registry.access.redhat.com/rhscl/mariadb-100-rhel7'
    - 'registry.access.redhat.com/rhscl/mariadb-101-rhel7'
    - 'registry.access.redhat.com/rhscl/mariadb-102-rhel7'
    - 'registry.centos.org/centos/mariadb-100-centos7'
    - 'registry.centos.org/centos/mariadb-101-centos7'
    - 'registry.centos.org/centos/mariadb-102-centos7'

Even if the rhel7 images were compatible, I'm not sure what we would do about https://github.com/jcpowermac/mariadb-apb/blob/9cd265a209c763da32f8a82ea9bdc5dfff8ff898/roles/rhscl-mariadb-apb-openshift/templates/bc-ha.yaml.j2#L11-L16 downstream.

If we are not able to support HA downstream, this appears to require a much more significant patch to the downstream environment than what we have in mediawiki. Maybe we should schedule a meeting to discuss this in detail?

jcpowermac · 2018-02-20T18:55:02Z

@djzager / @jmontleon both are software collection images the only difference is RHEL vs CentOS. The problem with both is they don't provide the galera packages or which. Also the image needs to be modified so I can template the galera configuration. That is why there is two buildconfigs. There is an outstanding PR sclorg/mariadb-container#67 that has gone nowhere fast but that only resolves the first buildconfig.

jcpowermac · 2018-02-20T23:26:47Z

@djzager here is an example of using a dictionary to determine state to reduce the amount of conditionals: https://github.com/jcpowermac/ansible-statemachine-test. I am not sure this really reduces any complexity. If this isn't the direction you are thinking it might help if you create an example.

The problem is the database APBs are not simple (dev/prod +ha +update +testing) and the ansible to support all those options is complex.

djzager · 2018-02-21T15:16:19Z

@jcpowermac Thank you for the example. I need to spend some more time thinking about this PR ahead of our call tomorrow.

So that you are aware, I have been working on an APB reference example and one notable omission is a test playbook. After playing around with .travis.yml and doing some research, we think it would be great to have a single source of truth for testing APBs (much like this ansible role test shim and it being used to test other ansible roles ).

With those things in mind and your excellent use of the test playbook, I need to do some work on the the APB reference example.

jcpowermac force-pushed the galera branch from 73325b9 to aea7ced Compare February 20, 2018 14:19

Adding Galera plan and testing

61dac73

jcpowermac force-pushed the galera branch from aea7ced to 61dac73 Compare February 20, 2018 14:19

jcpowermac changed the title ~~[WIP] MariaDB HA via Galera and tests~~ MariaDB HA via Galera and tests Feb 20, 2018

djzager reviewed Feb 20, 2018

View reviewed changes

cleanup, todo

9cd265a

djzager requested changes Feb 20, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MariaDB HA via Galera and tests #23

MariaDB HA via Galera and tests #23

jcpowermac commented Feb 16, 2018 •

edited

Loading

jcpowermac commented Feb 20, 2018

jmontleon commented Feb 20, 2018 •

edited

Loading

djzager left a comment •

edited

Loading

djzager Feb 20, 2018

jcpowermac Feb 20, 2018 •

edited

Loading

djzager Feb 20, 2018

djzager Feb 20, 2018

djzager Feb 20, 2018

djzager Feb 20, 2018

jcpowermac commented Feb 20, 2018 •

edited

Loading

djzager commented Feb 20, 2018

djzager left a comment

djzager Feb 20, 2018

djzager Feb 20, 2018

jcpowermac commented Feb 20, 2018

djzager commented Feb 20, 2018

jcpowermac commented Feb 20, 2018

jcpowermac commented Feb 20, 2018

djzager commented Feb 21, 2018 •

edited

Loading

MariaDB HA via Galera and tests #23

Are you sure you want to change the base?

MariaDB HA via Galera and tests #23

Conversation

jcpowermac commented Feb 16, 2018 • edited Loading

jcpowermac commented Feb 20, 2018

jmontleon commented Feb 20, 2018 • edited Loading

djzager left a comment • edited Loading

Choose a reason for hiding this comment

djzager Feb 20, 2018

Choose a reason for hiding this comment

jcpowermac Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

djzager Feb 20, 2018

Choose a reason for hiding this comment

djzager Feb 20, 2018

Choose a reason for hiding this comment

djzager Feb 20, 2018

Choose a reason for hiding this comment

djzager Feb 20, 2018

Choose a reason for hiding this comment

jcpowermac commented Feb 20, 2018 • edited Loading

djzager commented Feb 20, 2018

djzager left a comment

Choose a reason for hiding this comment

djzager Feb 20, 2018

Choose a reason for hiding this comment

djzager Feb 20, 2018

Choose a reason for hiding this comment

jcpowermac commented Feb 20, 2018

djzager commented Feb 20, 2018

jcpowermac commented Feb 20, 2018

jcpowermac commented Feb 20, 2018

djzager commented Feb 21, 2018 • edited Loading

jcpowermac commented Feb 16, 2018 •

edited

Loading

jmontleon commented Feb 20, 2018 •

edited

Loading

djzager left a comment •

edited

Loading

jcpowermac Feb 20, 2018 •

edited

Loading

jcpowermac commented Feb 20, 2018 •

edited

Loading

djzager commented Feb 21, 2018 •

edited

Loading