juju icon indicating copy to clipboard operation
juju copied to clipboard

[SQLair] Replace slice queries in domain/externalcontroller/state

Open Aflynn50 opened this issue 1 year ago • 6 comments

Switch queries using slices to SQLair queries along with surrounding queries in the same transaction. This change does not add a huge amount of value here but once SQLair supports bulk inserts this will be a good use case.

Checklist

  • [x] Code style: imports ordered, good names, simple structure, etc
  • [x] Comments saying why design decisions were made
  • [x] Go unit tests, with comments saying what you're testing
  • [x] Integration tests, with comments saying what you're testing ~- [ ] doc.go added or updated in changed packages~

QA steps

Unit tests

TEST_PACKAGES="./domain/externalcontroller/... -count=1 -race" TEST_FILTER="Suite" make run-go-tests

Testing cross model relations

#! /bin/bash

for c in "cns" "src" "dst"; do juju bootstrap lxd $c; done
juju switch src; juju add-model default; juju deploy ./testcharms/charms/dummy-source --config token=INITIAL; juju offer dummy-source:sink
juju switch cns; juju add-model default; juju consume src:admin/default.dummy-source; juju deploy ./testcharms/charms/dummy-sink; juju relate dummy-source dummy-sink
juju switch dst; juju destroy-model default -y
juju switch src
watch -c juju status -m default --relations --storage --color

Testing migrations

First, let's create two controllers (one with influxdb, other with grafana) and integrate them:

juju bootstrap localhost ctrl-src1
juju add-model model-src1
juju deploy influxdb
juju offer influxdb:grafana-source  
---
juju bootstrap localhost ctrl-src2
juju add-model model-src2
juju deploy grafana
juju integrate grafana ctrl-src1:admin/model-src1.influxdb

Now you can check the status with --relations to validate that the integration worked.

Also, let's check that the external controller have been stored in dqlite and not in mongodb: (still on ctrl-src2) connect to controller machine and run dqlite:

lxc exec juju-xxx bash
apt install go-dqlite
dqlite -s 127.0.0.1:17666 controller
SELECT * FROM external_controller; // Here you should see one external controller:
dqlite> SELECT * FROM external_controller;
b350dd86-cff7-4e42-8979-f8b40d5907e0|ctrl-src1|-----BEGIN CERTIFICATE-----
MIIEEzCCAnugAwIBAgIVALivOnMNMuM2SgZNOFE1dgDwKoZ3MA0GCSqGSIb3DQEB
...
-----END CERTIFICATE-----

// If the SELECT * FROM external_controller is not working, you may need to add
// the tls certificates to interact with DQLite directly. This can be done with:
- sudo snap install yq
- sudo cat /var/lib/juju/agents/machine-0/agent.conf | yq '.controllercert' | xargs -I% echo % > dqlite.cert
- sudo cat /var/lib/juju/agents/machine-0/agent.conf | yq '.controllerkey' | xargs -I% echo % > dqlite.key
- sudo dqlite -s file:///var/lib/juju/dqlite/cluster.yaml -c ./dqlite.cert -k ./dqlite.key controller

And mongodb:

juju:PRIMARY> db.externalControllers.count()
0

Now let's bootstrap a new controller and migrate model-src2 (grafana integrated with influxdb) to it:

juju bootstrap localhost ctrl-target
juju switch ctrl-src2
juju migrate model-src2 ctrl-target

Let's check that everything has been migrated correctly and stored in dqlite and not in mogodb:

juju switch ctrl-target
juju switch model-src2

lxc exec juju-xxx bash

apt install go-dqlite
dqlite -s 127.0.0.1:17666 controller

dqlite> SELECT * FROM external_controller;
b350dd86-cff7-4e42-8979-f8b40d5907e0|ctrl-src1|-----BEGIN CERTIFICATE-----
MIIEEzCCAnugAwIBAgIVALivOnMNMuM2SgZNOFE1dgDwKoZ3MA0GCSqGSIb3DQEB
...
-----END CERTIFICATE-----

and mongodb:

juju:PRIMARY> db.externalControllers.count()
0

There is also some migration QA that can be tried here: https://github.com/juju/juju/pull/15940 This works except the last step, the external controller does not appear to be registered in the database.

Documentation changes

N/A

Links

Jira card: JUJU-5374

Aflynn50 avatar Jan 31 '24 10:01 Aflynn50

@Aflynn50 can you update the QA steps on performing a migration with CMR. So we can ensure that it's working.

You should be able to see what @nvinuesa did when he originally implemented it.

SimonRichardson avatar Feb 09 '24 10:02 SimonRichardson

@Aflynn50 can you update the QA steps on performing a migration with CMR. So we can ensure that it's working.

You should be able to see what @nvinuesa did when he originally implemented it.

Wasn't able to find any from previous PRs, but got a script from Joe to check CMR. Have put it in the QA.

Aflynn50 avatar Feb 15 '24 16:02 Aflynn50

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

jujubot avatar Feb 20 '24 10:02 jujubot

Thanks for opening a pull request! Please follow the instructions here to ensure your pull request is ready for review. Then, a maintainer will review your patch.

@hpidcock @anvial

jujubot avatar Feb 20 '24 10:02 jujubot

@Aflynn50 I think @SimonRichardson was referring to the QA steps in https://github.com/juju/juju/pull/15940, although these won't help much because this domain is not yet fully wired up. Worth playing with that scenario for later though

nvinuesa avatar Feb 20 '24 10:02 nvinuesa

@Aflynn50 I think @SimonRichardson was referring to the QA steps in #15940, although these won't help much because this domain is not yet fully wired up. Worth playing with that scenario for later though

Done the QA, all worked well, I've added it to the PR desc.

Aflynn50 avatar Feb 22 '24 17:02 Aflynn50

/merge

Aflynn50 avatar Feb 28 '24 08:02 Aflynn50