-
Couldn't load subscription status.
- Fork 580
util: enhance tracevol.py script to work with volumesnapshots #5049
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
TestingScript outputRBD snapshot inside cephCephFS snapshot inside cephRegards |
5ff5ab6 to
a85bfca
Compare
can we have different table for cephfs and rbd just like we have for PVC? |
a85bfca to
a82f7ab
Compare
Done. I have updated the test results with the most recent output :) |
troubleshooting/tools/tracevol.py
Outdated
| pv_data = kube_client("get", "pv", pvc['spec']['volumeName']) | ||
|
|
||
| # Find the type of the PVC (RBD or CephFS) | ||
| fs_type = "CephFS" if "fsName" in pv_data['spec']['csi']['volumeAttributes'] else "RBD" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what happens if there are non-csi pvc or pvc's belongs to other storage provider?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated it like: If the encountered PVC is not a CSI volume, we skip it.
belongs to other storage provider?
The existing checks in the script only contain cases for either CephFS or RBD. I can refactor the detection logic to be more precise in the upcoming PR?
troubleshooting/tools/tracevol.py
Outdated
| # Alternatively, we can use the toolbox pod to fetch these values from ceph directly | ||
| snap_content_name = snapshot['status']['boundVolumeSnapshotContentName'] | ||
| snap_content = kube_client("get", "volumesnapshotcontent", snap_content_name) | ||
| snap_id = "csi-snap-" + get_image_uuid(snap_content['status']['snapshotHandle']) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
csi-snap- is default but you can change the snap prefix in the volumesnapshotclass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
|
what about the column for omap and if the snapshots is present on the cluster or not? |
Since the backing subvolume/image inside ceph is same to that of the PVC, is it necessary to duplicate that info? |
a82f7ab to
f7c24f7
Compare
troubleshooting/tools/tracevol.py
Outdated
| # The snapshot prefix can be optionally defined in the volumesnapshotclass paramaters | ||
| snap_handle = snap_content['status']['snapshotHandle'] | ||
| snap_uuid = get_image_uuid(snap_handle) | ||
| snap_prefix = snap_class['parameters'].get('snapshotNamePrefix') or 'csi-snap-' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to make it more flexible to fetch it from omap rather from the snap class as snap class can be deleted post creation and recreated post snapshot creation with different prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack. On it.
e6bb877 to
c2adb42
Compare
troubleshooting/tools/tracevol.py
Outdated
| Retrieve and parse the list of volume snapshots from the cluster. | ||
| """ | ||
| if ARGS.namespace: | ||
| volumesnapshots = kube_client("get", "volumesnapshots", "-n", ARGS.namespace) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this required? why not directly use ARGS.namespace in kube_client method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kube_client is supposed to be a simple wrapper around kubectl or oc. Doing it like this allows us to fetch both namespaced and clusterscoped resources and also keep the syntax similar to kc/oc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then why is this code in kube_client? mistake?
cmd += ["--namespace", arg.namespace]t
cmd += list(commands)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, missed it. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ARGS.namespace:
volumesnapshots = kube_client("get", "volumesnapshots", "-n", ARGS.namespace)🤔 But still this conditioning will not work as per your requirement.
if ARGS.namespace is not provided, default value will be default namespace and if condition will always be true?
295bfdb to
b9f9e8b
Compare
|
@Mergifyio queue |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at ec5fefc |
This patch adds the functionality to map the k8s volumesnapshots to the cephfs/rbd snapshots. This patch also adds a wrapper around oc/kubectl called `kube_client` which will help get rid of code duplication. Closes: ceph#3344 Signed-off-by: Niraj Yadav <[email protected]>
b9f9e8b to
dcd7adf
Compare
|
/test ci/centos/k8s-e2e-external-storage/1.30 |
|
/test ci/centos/upgrade-tests-cephfs |
|
/test ci/centos/mini-e2e-helm/k8s-1.30 |
|
/test ci/centos/upgrade-tests-rbd |
|
/test ci/centos/mini-e2e/k8s-1.30 |
|
/test ci/centos/k8s-e2e-external-storage/1.32 |
|
/test ci/centos/k8s-e2e-external-storage/1.31 |
|
/test ci/centos/mini-e2e-helm/k8s-1.32 |
|
/test ci/centos/mini-e2e-helm/k8s-1.31 |
|
/test ci/centos/mini-e2e/k8s-1.32 |
|
/test ci/centos/mini-e2e/k8s-1.31 |
Describe what this PR does
This patch adds the functionality to map the k8s volumesnapshots to the cephfs/rbd snapshots.
This patch also adds a wrapper around oc/kubectl called
kube_clientwhich will help get rid of code duplication.Closes: #3344
Future concerns
I will follow up the refactors around
kube_clientin a separate PR