rbd-mirror: improve status to show additional snapshot details #48582
Conversation
There was a problem hiding this comment.
$ rbd mirror image snapshot ls pool1/test_image --format json --pretty-format
SNAPID NAME SIZE 133 .mirror.primary.4779442d-ac5b-4c19-8abc-b5324dced42b.d19c3e96-9fcc-4f42-9192-fd6d85e0ef72 10 MiB 137 .mirror.primary.4779442d-ac5b-4c19-8abc-b5324dced42b.038921bd-5966-48da-91b0-41067e6c32c0 10 MiB 140 .mirror.primary.4779442d-ac5b-4c19-8abc-b5324dced42b.18a527ea-31e2-4401-acd4-7bd401822da1 10 MiB
I'm not sure I follow the use case here. Mirror snapshots can be listed with rbd snap ls --all and a similarly constrained (when compared to rbd snap ls --all) output is available as part of rbd mirror image status.
@idryomov I was not aware of BTW, since we have a way to create a mirror snapshot with Thoughts? |
|
Although |
No,
We don't have |
The list of mirror snapshots by itself doesn't seem like it would be particularly useful to an average user. Additional context is needed because they wouldn't know that the most recent snapshot being primary means that the image is primary, for example. I think that was the idea behind including the list of mirror snapshots in One such improvement might be to make it list all mirror snapshots instead of just primary mirror snapshots. Another might be to list more metadata with each snapshot, e.g. include the creation timestamp. |
There was a problem hiding this comment.
It seems redundant having two commands do the same thing.
Such as:
mirror image snapshot Create RBD mirroring image snapshot.
mirror image snapshot create Create RBD mirroring image snapshot.
That being said, backwards compatibility is needed and the original one shouldn't be removed. In my opinion, this should be added to something such as rbd mirror image status
Currently `rbd mirror image status` show mirror snapshot details with primary cluster only, with this change it should show mirror snapshots for non-primary too. Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
pkalever
changed the title
There was a problem hiding this comment.
I typed up six or seven comments on various redundancies and inconsistencies in both table and structured outputs before realizing that rbd snap ls --all doesn't suffer from any them. Why not just copy what rbd snap ls --all does as previously discussed -- make the output exactly the same, just with non-mirror snapshots omitted?
The only useful tweak I can immediately think of is we can drop PROTECTED column here since mirror snapshots can't be protected with rbd snap protect.
If a particular piece of information needs to be parsed out, structured output should be used instead. |
output with And the The only extra fields are PRIMARY|DEMOTED|PEER-UUID, are you saying we don't need this information? not sure if I missed something! We are showing information under PRIMARY because now we enabled non-primary too. I thought, having additional details like PRIMARY status instead of NAMESPACE and PROTECTED might be useful, and those were the only diff as compared to Let me know your thoughts, please. Thanks! |
|
I would use json/xml format for full information. For the plain format it is more important to have it readable, so any attempts to "compress" it, like combining PRIMARY|DEMOTED columns to one (e.g. the column could contain just "P" "D" letters) would look good to me. Also thinking, may be we should avoid snapshot names in the plain text output. The names do not look like very useful to me, and one could get them in other format output if needed. |
These are user snapshots whereas we are talking about mirror snapshots. ... along with other (user, trash, etc) snapshots if they are present. What I'm suggesting is to make
This information is contained in NAMESPACE column. Here is the output I initially had in mind: |
Do you see a problem with how mirror snapshots are formatted by
I disagree. Without names matching up mirror snapshots from primary and secondary cluster is pretty hard -- it's what you typically grep for. |
I don't see a problem with it :-)
You could display the needed information in some shorter form. If you just need to know if a snapshot from primary or secondary cluster it is not necessary to display almost 100 character snapshot name. And again, the full information could be obtain from json output, and using Actually, I don't care much. What I was trying to say was that if there were two ways to display the same information (in plane text mode) I would vote for the way that used less space on the screen. I think in the past we just did not think much about how to output data. It is not necessary we need to think now (in this PR), but I just wanted to mention it. E.g. timestamps could use much less space if some other format is used. Just as a good example for me is |
True, but the user submitting a tracker ticket wouldn't know to use
Yeah, you hit the nail there. I feel like we have never really strived towards |
$ rbd mirror --cluster=site-a image status pool1/test_image
test_image:
global_id: 5750c75e-84d1-478b-a086-64c896455abb
state: up+stopped
description: local image is primary
service: admin on localhost.localdomain
last_update: 2022-11-10 12:25:04
peer_sites:
name: site-b
state: up+replaying
description: replaying, {"bytes_per_second":0.0,"bytes_per_snapshot":0.0,"local_snapshot_timestamp":1668063274,"remote_snapshot_timestamp":1668063274,"replay_state":"idle"}
last_update: 2022-11-10 12:24:53
snapshots:
SNAPID NAME DETAILS
19 .mirror.primary.5750c75e-84d1-478b-a086-64c896455abb.5d5c288b-3773-4c0d-8cb0-86434947d3ab (primary peer_uuids:[ea7ff904-a899-4adb-87f8-785e9bb44f33])
20 .mirror.primary.5750c75e-84d1-478b-a086-64c896455abb.0116326a-3753-4eb8-90ae-60f3bffe64c2 (primary peer_uuids:[ea7ff904-a899-4adb-87f8-785e9bb44f33])
22 .mirror.primary.5750c75e-84d1-478b-a086-64c896455abb.453a2985-8ee9-4280-946b-8403d1f445f6 (primary peer_uuids:[ea7ff904-a899-4adb-87f8-785e9bb44f33])
23 .mirror.primary.5750c75e-84d1-478b-a086-64c896455abb.99a96faf-5552-45f1-8524-94fc37723616 (primary peer_uuids:[ea7ff904-a899-4adb-87f8-785e9bb44f33])
$ rbd mirror --cluster=site-a image status pool1/test_image --format=json --pretty-format
{
"name": "test_image",
[...]
"snapshots": [
{
"id": 7,
"name": ".mirror.primary.5750c75e-84d1-478b-a086-64c896455abb.3bf7a5fa-9ac1-496a-896c-036027308380",
"namespace": {
"state": "primary",
"mirror_peer_uuids": [
"ea7ff904-a899-4adb-87f8-785e9bb44f33"
],
"complete": true
}
},
{
"id": 9,
"name": ".mirror.primary.5750c75e-84d1-478b-a086-64c896455abb.a3abfa0b-ec8e-4b2f-953b-c69cc2a1fcb1",
"details": {
"state": "primary",
"mirror_peer_uuids": [
"ea7ff904-a899-4adb-87f8-785e9bb44f33"
],
"complete": true
}
}
]
}
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
pkalever
force-pushed
the
mirror-snap
branch
from
6a033d3
to
7a55fb5
Compare
Align the snapshot details look like `rbd snap ls --all`
$ rbd mirror --cluster=site-b image status pool1/test_image
test_image:
global_id: 5750c75e-84d1-478b-a086-64c896455abb
state: up+replaying
description: replaying, {"bytes_per_second":0.0,"bytes_per_snapshot":0.0,"local_snapshot_timestamp":1668063305,"remote_snapshot_timestamp":1668063305,"replay_state":"idle"}
service: admin on localhost.localdomain
last_update: 2022-11-10 18:44:53
peer_sites:
name: 43f2e3d2-8e58-4c4f-8f2a-c205f3627c1c
state: up+stopped
description: local image is primary
last_update: 2022-11-10 18:44:51
snapshots:
SNAPID NAME SIZE TIMESTAMP DETAILS
25 .mirror.non_primary.5750c75e-84d1-478b-a086-64c896455abb.e879fca5-1f08-4711-bdbb-a07c43ac017c 10 MiB Thu Nov 10 12:25:10 2022 (non-primary peer_uuids:[] 02cabf51-e87e-419f-b767-6c7727db2bd8:23 copied)
$ rbd mirror --cluster=site-b image status pool1/test_image --format=json --pretty-format
{
"name": "test_image",
[...]
"snapshots": [
{
"id": 25,
"name": ".mirror.non_primary.5750c75e-84d1-478b-a086-64c896455abb.e879fca5-1f08-4711-bdbb-a07c43ac017c",
"size": "10 MiB",
"timestamp": "Thu Nov 10 12:25:10 2022",
"details": {
"state": "non-primary",
"mirror_peer_uuids": [],
"complete": true,
"primary_mirror_uuid": "02cabf51-e87e-419f-b767-6c7727db2bd8",
"primary_snap_id": 23,
"last_copied_object_number": 3
}
}
]
}
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
pkalever
force-pushed
the
mirror-snap
branch
from
7a55fb5
to
34111a1
Compare
|
@idryomov could you please provide your review on this PR? |
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
|
This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution! |
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days. |
$ rbd mirror --cluster=site-b image status pool1/test_image
$ rbd mirror --cluster=site-b image status pool1/test_image --format=json --pretty-format
Signed-off-by: Prasanna Kumar Kalever <prasanna.kalever@redhat.com>
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume toxjenkins test windows