feat: add support for the "list" command #31
Conversation
Use snapshot_lvm_snapset_name to identify a set of snapshots. The snapset name is appended to the LV name to indicate membership in the set. Using a set name will map more easily to how snapm is being implemented. If/when we decide to back the role with snapm, it will be easier to keep the current interface if we use only a snapset name and drop the prefix. See : https://github.com/snapshotmanager/snapm/ Signed-off-by: Todd Gill <[email protected]>
|
@richm just a draft right now. I need to clean up and provide some examples for how the user should traverse the JSON response from list. |
richm
changed the title
|
Is the In list.yml, you can parse the JSON output by using something like |
yes, ok - will document. FYI - the json will look like: I edited this response - I think if the user requests all, the response should include all LVM volumes (both snapshots and origins, along with mount point information).... updated JSON below. Will update PR
Ok. Will change to: |
trgill
force-pushed
the
snapset_names
branch
2 times, most recently
from
017fab2 to
8c70e8f
Compare
|
[citest] |
|
[citest] |
trgill
force-pushed
the
snapset_names
branch
2 times, most recently
from
19b3352 to
1c26d4b
Compare
|
[citest] |
|
|
[citest] |
|
the failure on centos-7 is related to the new list command - I'm investigating: TASK [linux-system-roles.snapshot : Set snapshot_facts to the JSON results] ******************************************************************************************************************************************************************************* MSG: the field 'args' has an invalid value ({'snapshot_facts': '{{ snapshot_cmd.stdout | from_json }}'}), and could not be converted to an dict.The error was: Expecting value: line 1 column 1 (char 0) The error appears to be in '/home/tgill/workspace/snapshot/tests/roles/linux-system-roles.snapshot/tasks/list.yml': line 9, column 3, but may The offending line appears to be:
|
|
The problem is due to --json being added after centos7... will find work around. findmnt: unrecognized option --json |
trgill
force-pushed
the
snapset_names
branch
2 times, most recently
from
93e4bcf to
213712b
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
[citest] |
tasks/files/snapshot.py
Outdated
|
|
||
| output = output.replace('"', "") | ||
| output = output.replace("\n", "") | ||
| kv = dict(arg.split("=", 1) for arg in output.split(" ") if arg) |
There was a problem hiding this comment.
I think this function could just do
return dict(arg.split("=", 1) for arg in output.split(" ") if arg)
That is, return a dict instead of a string. I think the function that calls lvm_get_fs_mount_points will ultimately do the json string conversion to return to the caller
There was a problem hiding this comment.
it isn't working properly for blockdevices that are mounted in multiple mount points
Ah - then I guess you could do the output.replace('"', ""), then instead of output.replace("\n", "") do lines = output.split("\n"), then iterate over lines and return a list of dict from this function
There was a problem hiding this comment.
I've updated so the output for the case of multiple mounts looks like:
"mounts": {
"/dev/vg3/lv1_vg3": null,
"/dev/vg3/lv1_vg3_snapset2": [
{
"TARGET": "/mnt/database",
"SOURCE": "/dev/mapper/vg3-lv1_vg3_snapset2",
"FSTYPE": "xfs",
"OPTIONS": "rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota"
}
],
"/dev/vg3/lv2_vg3": null,
"/dev/vg3/lv2_vg3_snapset2": null,
"/dev/vg3/lv3_vg3": null,
"/dev/vg3/lv3_vg3_snapset2": null,
"/dev/vg2/lv1_vg2": null,
"/dev/vg2/lv1_vg2_snapset2": [
{
"TARGET": "/mnt/production_mnt",
"SOURCE": "/dev/mapper/vg2-lv1_vg2_snapset2",
"FSTYPE": "xfs",
"OPTIONS": "rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota"
}
],
"/dev/vg2/lv2_vg2": null,
"/dev/vg2/lv2_vg2_snapset2": null,
"/dev/vg1/lv1_vg1": null,
"/dev/vg1/lv1_vg1_snapset2": [
{
"TARGET": "/mnt/new_mountpoint",
"SOURCE": "/dev/mapper/vg1-lv1_vg1_snapset2",
"FSTYPE": "xfs",
"OPTIONS": "rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota"
},
{
"TARGET": "/mnt/other_mp",
"SOURCE": "/dev/mapper/vg1-lv1_vg1_snapset2",
"FSTYPE": "xfs",
"OPTIONS": "rw,relatime,seclabel,attr2,inode64,logbufs=8,logbsize=32k,noquota"
}
],
"/dev/vg1/lv2_vg1": null,
"/dev/vg1/lv2_vg1_snapset2": null
}
I think I followed you suggestion and the result it:
def lvm_get_fs_mount_points(block_path):
find_mnt_command = [
"findmnt",
block_path,
"-P",
]
mount_list = list()
rc, output = run_command(find_mnt_command)
if rc:
return None
output = output.replace('"', "")
for line in output.split("\n"):
if len(line):
mount_list.append(dict(arg.split("=", 1) for arg in line.split(" ") if arg))
return mount_list
tasks/files/snapshot.py
Outdated
| if fs_mount_points: | ||
| fs_list.append(fs_mount_points) | ||
| fs_dict[block_path] = fs_mount_points | ||
| fs_list = list() |
There was a problem hiding this comment.
? the for loop builds fs_list, then you reset it here?
There was a problem hiding this comment.
this is a problem. will fix. it isn't working properly for blockdevices that are mounted in multiple mount points.
|
[citest] |
|
[citest bad] |
|
[citest] |
return a json list of VGs that have include a list of the LVs that are in the snapshot set. Signed-off-by: Todd Gill <[email protected]> Co-authored-by: Richard Megginson <[email protected]>
Enhancement: add support for the "list" command
Reason: allowing the user to list snapshots that are part of a snapset is part of the MVP requirements
Result: users can list snapsets as needed.
Note:
the PR also includes a fix to make sure snapshot size allocation is at least the LVM minimum of 512MiB.
the naming of snapsets has been simplified to just use the snapset name as a suffix - the prefix parameter has been removed. the change will provide better compatibility with how the snapm API works.
Issue Tracker Tickets (Jira or BZ if any):