MicroCeph Remote Replication (2/3): RBD Mirroring by UtkarshBhatthere · Pull Request #398 · canonical/microceph

UtkarshBhatthere · 2024-08-08T11:08:50Z

This Pr is being closed and moved to #437 where it has been rebased over MicroCluster LTS changes.

sabaini

Hi Utkarsh, did a quick once-over review -- please find some comments inline

sabaini · 2024-08-26T13:27:00Z

+	Get:  rest.EndpointAction{Handler: cmdClusterGet, ProxyTarget: false},
+}
+
+// cmdClusterGet returns a json dump of microceph configs with a new key patched in for the remote.


Feel this doc comment could elaborate some more e.g.

// cmdClusterGet returns a json dump of microceph configs including credentials for a new remote. Credentials returned as a newly created keyring.

sabaini · 2024-08-26T13:32:21Z

+		[]string{"mon", "allow *"},
+		[]string{"osd", "allow *"},
+		[]string{"mds", "allow *"},
+		[]string{"mgr", "allow *"},


These are full caps, I wonder if we really need all? Also I seem to recall some description from the spec where the key should only be initialized with minimal caps?

Yes, I believe we should use the bare minimum caps needed. If possible, this should be more fine grained.

sabaini · 2024-08-26T13:34:43Z

+	}
+
+	return handleRbdRepRequest(r.Context(), req)
+}


There seems to be code duplication with the cmdOpsReplication* funcs, possibly could unify them

sabaini · 2024-08-26T13:47:50Z

+/*****************HELPER FUNCTIONS**************************/
+
+// RenderConfAndKeyringFiles generates the $cluster.conf and $cluster.keyring files on the host.
+var RenderConfAndKeyringFiles = func(remoteName string, localName string, configs map[string]string) error {


I see there's several func variables defined here. Do you need to stub them out for unit testing? (Don't see any unit tests referencing them now).

IMHO adding stubs for testing like this is most useful if you need to override a low-level function that won't easily be run in a test env.

I'd also advise to wrap stubs in an interface, this allows to generate mocks with stretchr and mockery

https://github.com/stretchr/testify
https://github.com/vektra/mockery

F.e. take a look at the ceph.processExec var which in a deploymend runs shell commands but for unit testing is mocked out.

sabaini · 2024-08-26T13:54:55Z

+	Delete: rest.EndpointAction{Handler: CmdRemoteDelete, ProxyTarget: false},
+}
+
+var CmdRemotePut = func(state *state.State, r *http.Request) response.Response {


Please add doc comments at least for the main funcs

sabaini · 2024-08-26T13:55:48Z

@@ -0,0 +1,29 @@
+package types
+
+type Remote struct {


This type deserves some docs I believe

sabaini · 2024-08-26T14:00:50Z

 }
+
+// CreateKey creates a client key and returns the said key hash without saving it as a file.
+var CreateKey = func(clientName string, caps ...[]string) (string, error) {


See above re: mocking. The cephRun() func you're using here to perform system commands can be mocked already, see var processExec Runner = RunnerImpl{}

sabaini · 2024-08-26T14:04:32Z

 }

-func genericServiceInit(s interfaces.StateInterface, name string) error {
+func genericServiceInit(s interfaces.StateInterface, name string, isClientService bool) error {


I wonder if instead of adding a flag to the genericServiceInit the rbd-mirror service could subclass GenericServicePlacement, like the RGW service placement does? This would feel cleaner

lmlg · 2024-08-28T23:06:28Z

+	if err != nil || !isOk {
+		err := fmt.Errorf("cluster names can only have [a-z] or [0-9] characters: %w", err)
+		logger.Error(err.Error())
+		return response.InternalError(err)


This should be a BadRequest, since InternalError doesn't represent a user error, imo.

lmlg · 2024-08-28T23:06:35Z

+	if err != nil {
+		err := fmt.Errorf("failed to get config db: %w", err)
+		logger.Error(err.Error())
+		return response.InternalError(err)


Same as above.

lmlg · 2024-08-28T23:07:50Z

+		[]string{"mon", "allow *"},
+		[]string{"osd", "allow *"},
+		[]string{"mds", "allow *"},
+		[]string{"mgr", "allow *"},


Yes, I believe we should use the bare minimum caps needed. If possible, this should be more fine grained.

lmlg · 2024-08-28T23:11:12Z

+
+	if !req.RenderOnly {
+		// Asynchronously persist this on db and send request to other cluster members.
+		go func() {


I'm not sure if we should return before this call completes. Why not make this a synchronous call?

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

UtkarshBhatthere · 2024-10-03T19:38:02Z

This Pr is being closed and moved to #437

UtkarshBhatthere force-pushed the feature/remoteRBDReplication branch from b33b75f to ef9c3af Compare August 9, 2024 12:30

UtkarshBhatthere force-pushed the main branch from 3204396 to 0863c8b Compare August 13, 2024 19:26

UtkarshBhatthere force-pushed the feature/remoteRBDReplication branch 2 times, most recently from 1ff1849 to 08e99d4 Compare August 20, 2024 12:54

sabaini requested changes Aug 26, 2024

View reviewed changes

UtkarshBhatthere changed the title ~~Enable RBD Mirroring~~ MicroCeph Remote Replication (2/3): RBD Mirroring Aug 27, 2024

lmlg requested changes Aug 28, 2024

View reviewed changes

UtkarshBhatthere added 23 commits September 3, 2024 17:38

Made newCephConfig configurable for clstr.conf

c33efdb

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Added rbd-mirror to MicroCeph

6b0a2fd

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Added cluster export command

cd795e2

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Added remote import command

662f354

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Added rbd-mirror to service placement interface

964742d

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Dropped remote_config table

5cc8380

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Fix mon service cleanup

d6378cf

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Add remote import request forwarding to cluster

1155049

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Add cluster name conformance test

76c63a9

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Added CI test for Remote cluster CRUD

24420f7

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Test path unescape

bcd3acd

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

fragmented remote cmds

de9b88d

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Added remote list cmd

1051792

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Added check for remote list output

6e2620b

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Fix documentation and housekeeping.

5e5fc4f

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Added remote remove

8094526

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Test Implementation

ebafad0

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Added FSM skeleton

ffc3c30

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Added logging

b2c08c6

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Enabled verbose

3543f77

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Ported the FSM to qmuntal

6f7d6a9

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Enable RBD Mirroring

6bfb63d

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Patched FSM

47912db

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

UtkarshBhatthere added 2 commits September 3, 2024 17:38

Added rbd_mirror ceph commands

ef848a8

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Added glue code

6061e6c

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

UtkarshBhatthere force-pushed the feature/remoteRBDReplication branch from ba4ba0f to 76dafd2 Compare September 3, 2024 12:08

UtkarshBhatthere added 4 commits September 4, 2024 01:45

Pass state as interface

cfae1f4

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Fixed inverted local/remote names

50d241b

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Fixed peer token dir creation

a01d444

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

peer remove fixed

c31d4bc

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

UtkarshBhatthere force-pushed the feature/remoteRBDReplication branch from 76dafd2 to c31d4bc Compare September 4, 2024 21:54

UtkarshBhatthere added 4 commits September 5, 2024 06:24

Modified prefill to use json

c1e1da9

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Fixed Disable event trigger

69102ed

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Added rbd list command response

31c08da

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Added rbd configure cmd

acf4b68

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

UtkarshBhatthere force-pushed the feature/remoteRBDReplication branch from 78c9c5d to acf4b68 Compare September 5, 2024 12:58

UtkarshBhatthere added 2 commits September 12, 2024 17:45

Added fix for list command scan

32371ba

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

Added Tabular output to replication cmds

1861554

Signed-off-by: Utkarsh Bhatt <utkarsh.bhatt@canonical.com>

UtkarshBhatthere force-pushed the feature/remoteRBDReplication branch from 57da944 to 1861554 Compare October 3, 2024 18:25

UtkarshBhatthere closed this Oct 3, 2024

Conversation

UtkarshBhatthere commented Aug 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sabaini left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

UtkarshBhatthere commented Oct 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

UtkarshBhatthere commented Aug 8, 2024 •

edited

Loading