Skip to content
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.

Sign up for GitHub

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

Jump to bottom

rbd: log sitestatuses and description #4431

Merged
merged 1 commit into from
Feb 19, 2024
Merged

rbd: log sitestatuses and description #4431

merged 1 commit into from
Feb 19, 2024

Conversation

yati1998
Copy link
Contributor

This commit logs sitestatues and description in
GetVolumeReplicationInfo RPC call for better
debuging.

Fixes: #4430

@mergify mergify bot added the component/rbd Issues related to RBD label Feb 13, 2024
Madhu-1
Madhu-1 reviewed Feb 13, 2024
View reviewed changes
internal/csi-addons/rbd/replication.go Outdated
@@ -891,6 +896,9 @@ func RemoteStatus(gmis *librbd.GlobalMirrorImageStatus) (librbd.SiteMirrorImageS
ss librbd.SiteMirrorImageStatus
err error = librbd.ErrNotExist
)
log.UsefulLog(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

%q might not help as you are logging the details, can you please check and provide the log output?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log this as TraceLog log?

Madhu-1
Madhu-1 reviewed Feb 13, 2024
View reviewed changes
internal/csi-addons/rbd/replication.go Outdated
ctx,
"remote status %q:, description=%q",
remoteStatus,
description)
resp, err := getLastSyncInfo(description)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log Description inside the getLastSyncInfo function

@yati1998
Copy link
Contributor Author

Haven't tested the PR yet, Will request for reviw and add the output log once done.
Thanks

@yati1998 yati1998 force-pushed the sitestatus branch 4 times, most recently from 1c045ff to 525d9f6 Compare February 13, 2024 12:54
@nixpanic
Copy link
Member

Marking this as Draft for now, please update the PR description with the results of your testing.

@nixpanic nixpanic marked this pull request as draft February 13, 2024 14:16
@yati1998
Copy link
Contributor Author

Here's the log from rbdplugin pods.

I0215 16:35:19.925403       1 replication.go:804] ID: 24 Req-ID: 0001-0011-openshift-storage-0000000000000001-9dccf869-40a9-4125-9a00-2d2011346d55 GetVolumeReplicationInfo rpc call
I0215 16:35:19.928315       1 omap.go:89] ID: 24 Req-ID: 0001-0011-openshift-storage-0000000000000001-9dccf869-40a9-4125-9a00-2d2011346d55 got omap values: (pool="ocs-storagecluster-cephblockpool", namespace="", name="csi.volume.9dccf869-40a9-4125-9a00-2d2011346d55"): map[csi.imageid:1c8ae58015560 csi.imagename:csi-vol-9dccf869-40a9-4125-9a00-2d2011346d55 csi.volname:pvc-026b0c93-666a-4c14-988c-1a4f013955db csi.volume.owner:appset-busybox-1]

I0215 16:35:19.947049       1 replication.go:920] ID: 22 Req-ID: 0001-0011-openshift-storage-0000000000000001-8d68260e-6e34-4583-aa07-cc57adf8c457 Site Statuses: [{ stopped local image is primary %!s(int64=1708014906) %!s(bool=true)} {741b47ec-4914-4998-98cb-f74c940c9f4e replaying replaying, {"bytes_per_second":60347.73,"bytes_per_snapshot":1839104.0,"last_snapshot_bytes":1810432,"last_snapshot_sync_seconds":0,"local_snapshot_timestamp":1708014905,"remote_snapshot_timestamp":1708014905,"replay_state":"idle"} %!s(int64=1708014914) %!s(bool=true)}]
I0215 16:35:19.947075       1 replication.go:956] ID: 22 Req-ID: 0001-0011-openshift-storage-0000000000000001-8d68260e-6e34-4583-aa07-cc57adf8c457 description="replaying, {\"bytes_per_second\":60347.73,\"bytes_per_snapshot\":1839104.0,\"last_snapshot_bytes\":1810432,\"last_snapshot_sync_seconds\":0,\"local_snapshot_timestamp\":1708014905,\"remote_snapshot_timestamp\":1708014905,\"replay_state\":\"idle\"}"

@yati1998 yati1998 marked this pull request as ready for review February 15, 2024 16:45
@yati1998 yati1998 requested review from Madhu-1 and nixpanic February 15, 2024 16:47
nixpanic
nixpanic requested changes Feb 16, 2024
View reviewed changes
internal/csi-addons/rbd/replication.go Outdated
var (
ss librbd.SiteMirrorImageStatus
err error = librbd.ErrNotExist
)
log.DebugLog(
ctx,
"Site Statuses: %s", gmis.SiteStatuses)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this logging includes %!s in the output, it seems gmis.SiteStatuses can not simply be formatted as a string, does the struct have a String() function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it doesn't have it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to log it, you will either need to log attributes of if, or write a String() function for it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you do the logging at line 898 inside for loop of for i := range gmis.SiteStatuses { but before if check? so that we don't need to log multiple times?

internal/csi-addons/rbd/replication.go Outdated Show resolved Hide resolved
@yati1998 yati1998 requested a review from nixpanic February 16, 2024 11:32
@yati1998
Copy link
Contributor Author

Here's the updated log:

Site Statuses: [{ stopped local image is primary %!s(int64=1708321957) %!s(bool=true)} {741b47ec-4914-4998-98cb-f74c940c9f4e replaying replaying, {"bytes_per_second":0.0,"bytes_per_snapshot":0.0,"last_snapshot_bytes":0,"last_snapshot_sync_seconds":0,"local_snapshot_timestamp":1708321800,"remote_snapshot_timestamp":1708321800,"replay_state":"idle"} %!s(int64=1708321967) %!s(bool=true)}]
I0219 05:52:49.392252       1 replication.go:921] ID: 25 Req-ID: 0001-0011-openshift-storage-0000000000000001-4a99c0b1-5ec1-432e-a659-e72e200e1967 Site Statuse: stopped local image is primary 1708321957 true
I0219 05:52:49.392261       1 replication.go:910] ID: 25 Req-ID: 0001-0011-openshift-storage-0000000000000001-4a99c0b1-5ec1-432e-a659-e72e200e1967 Site Statuse of MirrorUUID:741b47ec-4914-4998-98cb-f74c940c9f4e: replaying replaying, {"bytes_per_second":0.0,"bytes_per_snapshot":0.0,"last_snapshot_bytes":0,"last_snapshot_sync_seconds":0,"local_snapshot_timestamp":1708321800,"remote_snapshot_timestamp":1708321800,"replay_state":"idle"} 1708321967 true

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Feb 19, 2024

%!s(int64=1708321957) %!s(bool=true)}

still log contains %!s

@yati1998
Copy link
Contributor Author

%!s(int64=1708321957) %!s(bool=true)}

still log contains %!s

yeah, that's the older change, kept it to get your reviews. If logging the site statues individually work for you, or it have to be printed as the list too? I will update the PR accordingly.
cc @Madhu-1

@yati1998 yati1998 requested a review from Madhu-1 February 19, 2024 08:55
Madhu-1
Madhu-1 requested changes Feb 19, 2024
View reviewed changes
Copy link
Collaborator

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM @yati1998 CI is failing, please paste the output from the testing.

@yati1998 yati1998 requested a review from a team February 19, 2024 10:56
@nixpanic
Copy link
Member

@Mergifyio rebase

@nixpanic nixpanic added logging The Change is only in logging ci/skip/e2e skip running e2e CI jobs labels Feb 19, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 19, 2024

rebase

✅ Nothing to do for rebase action

@nixpanic
Copy link
Member

@Mergifyio queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 19, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at fbaf9d5

@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Feb 19, 2024
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.28

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Feb 19, 2024
@mergify mergify bot merged commit fbaf9d5 into ceph:devel Feb 19, 2024
40 checks passed
@yati1998
Copy link
Contributor Author

/cherry-pick release-4.15

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Feb 20, 2024

/cherry-pick release-4.15

@yati1998 cherr-pick to downstream branch doesn't work in this repo and also please take care of backporting #4429 with this commit

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Feb 20, 2024

@Mergifyio backport release-3.10

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Feb 20, 2024

@Mergifyio backport release-v3.10

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 20, 2024

backport release-3.10

❌ No backport have been created

  • Backport to branch release-3.10 failed

GitHub error: Branch not found

@mergify Mergify
Copy link
Contributor

mergify bot commented Feb 20, 2024 •
edited
Loading

backport release-v3.10

✅ Backports have been created

@mergify mergify bot mentioned this pull request Feb 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Madhu-1 Madhu-1 Madhu-1 approved these changes

@nixpanic nixpanic nixpanic approved these changes

Assignees
No one assigned
Labels
ci/skip/e2e skip running e2e CI jobs component/rbd Issues related to RBD logging The Change is only in logging
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Log/Return SiteStatuses details and description in GetVolumeReplicationInfo RPC call
4 participants
@yati1998 @nixpanic @Madhu-1 @ceph-csi-bot