rbd: added rbd info to validateRBDImageCount func #4938

OdedViner · 2024-10-31T14:15:04Z

Describe what this PR does

Added rbd info and rbd status on Failure message

bash-5.1$ rbd info --pool=replicapool csi-vol-2b6574ee-9c6f-4a3b-9a96-09060763f7a7
rbd image 'csi-vol-2b6574ee-9c6f-4a3b-9a96-09060763f7a7':
	size 1 GiB in 256 objects
	order 22 (4 MiB objects)
	snapshot_count: 0
	id: 1102e70da18f
	block_name_prefix: rbd_data.1102e70da18f
	format: 2
	features: layering
	op_features: 
	flags: 
	create_timestamp: Thu Oct 31 13:26:32 2024
	access_timestamp: Thu Oct 31 13:26:32 2024
	modify_timestamp: Thu Oct 31 13:26:32 2024

bash-5.1$ rbd status --pool=replicapool csi-vol-2b6574ee-9c6f-4a3b-9a96-09060763f7a7
Watchers: none

Is there anything that requires special attention

Do you have any questions?

Is the change backward compatible?

Are there concerns around backward compatibility?

Provide any external context for the change, if any.

For example:

Kubernetes links that explain why the change is required
CSI spec related changes/catch-up that necessitates this patch
golang related practices that necessitates this change

Related issues

Mention any github issues relevant to this PR. Adding below line
will help to auto close the issue once the PR is merged.

Fixes: #issue_number
#4547

Future concerns

List items that are not part of the PR and do not impact it's
functionality, but are work items that can be taken up subsequently.

Checklist:

Commit Message Formatting: Commit titles and messages follow
guidelines in the developer
guide.
Reviewed the developer guide on Submitting a Pull
Request
Pending release
notes
updated with breaking and/or notable changes for the next major release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

/retest ci/centos/<job-name>: retest the <job-name> after unrelated
failure (please report the failure too!)

nixpanic · 2024-11-01T09:09:00Z

e2e/rbd.go

 		framework.Failf(
 			"backend images not matching kubernetes resource count,image count %d kubernetes resource count %d"+
-				"\nbackend image Info:\n %v",
+				"\nbackend image Info:\n %v\n images information and status%v",


status%v could use a space befor the %v

nixpanic · 2024-11-01T09:12:51Z

e2e/rbd_helper.go

+	}
+	err = json.Unmarshal([]byte(stdOut), &imgStatus)
+	if err != nil {
+		return imgStatus, fmt.Errorf("unmarshal failed: %w. raw buffer response: %s",


don't return the buffer in the error message, as that may get printed many more times when callers log the error. Instead, use framework.Logf() to report details like this.

e2e/rbd_helper.go:1125:3: k8s.io/kubernetes/test/e2e/framework.Logf does not support error-wrapping directive %w

Failed to run container: https://url.corp.redhat.com/cc13cdb

framework.Logf() does not support %w for logging errors. Use %v while logging, but when %w with fmt.Errorf() or errors.New().

nixpanic · 2024-11-01T09:14:51Z

e2e/rbd_helper.go

+		fmt.Sprintf("rbd status %s %s --format json", rbdOptions(poolName), imageName),
+		rookNamespace)
+	if err != nil {
+		return imgStatus, fmt.Errorf("failed to get rbd status: %w", err)


It helps to give this err != nil and the below stdErr != "" errors a slightly different message. When something goes wrong, it can be identified more precisely if the error message is unique.

nixpanic · 2024-11-01T09:15:50Z

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

nixpanic · 2024-11-01T10:03:51Z

e2e/rbd.go

+				return
+			}
+			// Collecting image details for printing
+			imageDetails = append(imageDetails, fmt.Sprintf("Pool Name: %s, Image Name: %s, Img Info: %v, Img Status: %v", pool, image, imgInfo, imgStatus))


golangci-lint complains that this line is too log: https://github.com/ceph/ceph-csi/actions/runs/11613610012/job/32379913964?pr=4938#step:3:820

Madhu-1 · 2024-11-04T07:34:24Z

e2e/rbd.go

+		for _, image := range imageList {
+			imgInfo, err := getImageInfo(f, image, pool)
+			if err != nil {
+				return


we need to log error and continue as this is for debuggin

Madhu-1 · 2024-11-04T07:34:30Z

e2e/rbd.go

+			}
+			imgStatus, err := getImageStatus(f, image, pool)
+			if err != nil {
+				return


we need to log error and continue as this is for debugging

e2e/rbd.go

e2e/rbd_helper.go

Madhu-1 · 2024-11-04T10:31:08Z

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

Madhu-1 · 2024-11-04T13:11:52Z

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

OdedViner · 2024-11-04T14:04:50Z

Output from failure job: https://url.corp.redhat.com/e9aa53d

[38;5;9m[FAILED] backend images not matching kubernetes resource count,image count 1 kubernetes resource count 11
  backend image Info:
   [csi-vol-d2bb1f89-26f7-4c37-a22d-d72f9a7aa30b]
   images information and status Pool: replicapool, Image: csi-vol-d2bb1f89-26f7-4c37-a22d-d72f9a7aa30b, Info: {csi-vol-d2bb1f89-26f7-4c37-a22d-d72f9a7aa30b 0 0 4194304}, Status: {  0 false 0 }�[0m
  �[38;5;9mIn �[1m[It]�[0m�[38;5;9m at: �[1m/go/src/github.com/ceph/ceph-csi/e2e/rbd.go:206�[0m �[38;5;243m@ 11/04/24 13:29:22.803�[0m

@Madhu-1 @nixpanic Is this the desired output? When I ran it manually I got the following results:

bash-5.1$ rbd info --pool=replicapool csi-vol-2b6574ee-9c6f-4a3b-9a96-09060763f7a7
rbd image 'csi-vol-2b6574ee-9c6f-4a3b-9a96-09060763f7a7':
	size 1 GiB in 256 objects
	order 22 (4 MiB objects)
	snapshot_count: 0
	id: 1102e70da18f
	block_name_prefix: rbd_data.1102e70da18f
	format: 2
	features: layering
	op_features: 
	flags: 
	create_timestamp: Thu Oct 31 13:26:32 2024
	access_timestamp: Thu Oct 31 13:26:32 2024
	modify_timestamp: Thu Oct 31 13:26:32 2024
bash-5.1$ rbd status --pool=replicapool csi-vol-2b6574ee-9c6f-4a3b-9a96-09060763f7a7
Watchers: none

Madhu-1 · 2024-11-04T14:09:29Z

Output from failure job: https://url.corp.redhat.com/e9aa53d

[38;5;9m[FAILED] backend images not matching kubernetes resource count,image count 1 kubernetes resource count 11
  backend image Info:
   [csi-vol-d2bb1f89-26f7-4c37-a22d-d72f9a7aa30b]
   images information and status Pool: replicapool, Image: csi-vol-d2bb1f89-26f7-4c37-a22d-d72f9a7aa30b, Info: {csi-vol-d2bb1f89-26f7-4c37-a22d-d72f9a7aa30b 0 0 4194304}, Status: {  0 false 0 }�[0m
  �[38;5;9mIn �[1m[It]�[0m�[38;5;9m at: �[1m/go/src/github.com/ceph/ceph-csi/e2e/rbd.go:206�[0m �[38;5;243m@ 11/04/24 13:29:22.803�[0m

@Madhu-1 @nixpanic Is this the desired output? When I ran it manually I got the following results:

bash-5.1$ rbd info --pool=replicapool csi-vol-2b6574ee-9c6f-4a3b-9a96-09060763f7a7
rbd image 'csi-vol-2b6574ee-9c6f-4a3b-9a96-09060763f7a7':
	size 1 GiB in 256 objects
	order 22 (4 MiB objects)
	snapshot_count: 0
	id: 1102e70da18f
	block_name_prefix: rbd_data.1102e70da18f
	format: 2
	features: layering
	op_features: 
	flags: 
	create_timestamp: Thu Oct 31 13:26:32 2024
	access_timestamp: Thu Oct 31 13:26:32 2024
	modify_timestamp: Thu Oct 31 13:26:32 2024
bash-5.1$ rbd status --pool=replicapool csi-vol-2b6574ee-9c6f-4a3b-9a96-09060763f7a7
Watchers: none

How about if we just return the stdout and log it, i think it will be useful for debugging instead of unmarshal output. i tlooks like the above logs is not much helpful, @nixpanic WDYT?

nixpanic · 2024-11-06T09:42:32Z

How about if we just return the stdout and log it, i think it will be useful for debugging instead of unmarshal output. i tlooks like the above logs is not much helpful, @nixpanic WDYT?

Logging stdout is sufficient for me too. It may also be easier to find the right timestamp/location of errors when the logs contain pretty unique words like block_name_prefix.

Signed-off-by: Oded Viner <[email protected]>

OdedViner · 2024-11-06T18:40:20Z

Output from failure job: https://url.corp.redhat.com/e9aa53d

[38;5;9m[FAILED] backend images not matching kubernetes resource count,image count 1 kubernetes resource count 11
  backend image Info:
   [csi-vol-d2bb1f89-26f7-4c37-a22d-d72f9a7aa30b]
   images information and status Pool: replicapool, Image: csi-vol-d2bb1f89-26f7-4c37-a22d-d72f9a7aa30b, Info: {csi-vol-d2bb1f89-26f7-4c37-a22d-d72f9a7aa30b 0 0 4194304}, Status: {  0 false 0 }�[0m
  �[38;5;9mIn �[1m[It]�[0m�[38;5;9m at: �[1m/go/src/github.com/ceph/ceph-csi/e2e/rbd.go:206�[0m �[38;5;243m@ 11/04/24 13:29:22.803�[0m

@Madhu-1 @nixpanic Is this the desired output? When I ran it manually I got the following results:

bash-5.1$ rbd info --pool=replicapool csi-vol-2b6574ee-9c6f-4a3b-9a96-09060763f7a7
rbd image 'csi-vol-2b6574ee-9c6f-4a3b-9a96-09060763f7a7':
	size 1 GiB in 256 objects
	order 22 (4 MiB objects)
	snapshot_count: 0
	id: 1102e70da18f
	block_name_prefix: rbd_data.1102e70da18f
	format: 2
	features: layering
	op_features: 
	flags: 
	create_timestamp: Thu Oct 31 13:26:32 2024
	access_timestamp: Thu Oct 31 13:26:32 2024
	modify_timestamp: Thu Oct 31 13:26:32 2024
bash-5.1$ rbd status --pool=replicapool csi-vol-2b6574ee-9c6f-4a3b-9a96-09060763f7a7
Watchers: none

How about if we just return the stdout and log it, i think it will be useful for debugging instead of unmarshal output. i tlooks like the above logs is not much helpful, @nixpanic WDYT?

I return imageInfo, string, error because validateStripe function

Madhu-1 · 2024-11-06T18:49:56Z

Info: {csi-vol-d2bb1f89-26f7-4c37-a22d-d72f9a7aa30b 0 0 4194304}, Status: { 0 false 0 }

Info: {csi-vol-d2bb1f89-26f7-4c37-a22d-d72f9a7aa30b 0 0 4194304}, Status: { 0 false 0 } This should log both key and value from the struct without it its difficult to understand and debug it.

It would be good if we just return the stdout and log the stdout in the logs that helps for debugging, actual struct is missing few details.

OdedViner · 2024-11-07T06:59:59Z

Info: {csi-vol-d2bb1f89-26f7-4c37-a22d-d72f9a7aa30b 0 0 4194304}, Status: { 0 false 0 } This should log both key and value from the struct without it its difficult to understand and debug it.

It would be good if we just return the stdout and log the stdout in the logs that helps for debugging, actual struct is missing few details.

images not matching kubernete

Info: {csi-vol-d2bb1f89-26f7-4c37-a22d-d72f9a7aa30b 0 0 4194304}, Status: { 0 false 0 }

Info: {csi-vol-d2bb1f89-26f7-4c37-a22d-d72f9a7aa30b 0 0 4194304}, Status: { 0 false 0 } This should log both key and value from the struct without it its difficult to understand and debug it.

It would be good if we just return the stdout and log the stdout in the logs that helps for debugging, actual struct is missing few details.

And what should be done with this function? It relies on the imgInfo struct.

ceph-csi/e2e/rbd_helper.go

Lines 1132 to 1133 in cea8bf8

    
           imgInfo, err := getImageInfo(f, imageData.imageName, defaultRBDPool) 
        
           if err != nil {

OdedViner · 2024-11-11T08:47:22Z

@Madhu-1 @nixpanic Which method would be better: getImageStatus, which returns a string output, or getImageInfo, which returns a struct with all parameters?

Madhu-1 · 2024-11-11T12:26:01Z

@Madhu-1 @nixpanic Which method would be better: getImageStatus, which returns a string output, or getImageInfo, which returns a struct with all parameters?

we could have a base function that returns the string and getImageStatus can use the base function and later do unmarshal and from the validation we will call the base function that returns the string data and print it.

Signed-off-by: Oded Viner <[email protected]>

mergify bot added the component/rbd Issues related to RBD label Oct 31, 2024

nixpanic requested changes Nov 1, 2024

View reviewed changes

nixpanic reviewed Nov 1, 2024

View reviewed changes

OdedViner force-pushed the add_rbd_info branch from 71d09c0 to 00ee8b1 Compare November 3, 2024 10:04

OdedViner requested a review from nixpanic November 3, 2024 10:05

Madhu-1 reviewed Nov 4, 2024

View reviewed changes

OdedViner force-pushed the add_rbd_info branch 2 times, most recently from 90b18cb to ffae514 Compare November 4, 2024 09:27

iPraveenParihar reviewed Nov 4, 2024

View reviewed changes

e2e/rbd.go Show resolved Hide resolved

iPraveenParihar reviewed Nov 4, 2024

View reviewed changes

e2e/rbd_helper.go Outdated Show resolved Hide resolved

OdedViner force-pushed the add_rbd_info branch from ffae514 to 0e15981 Compare November 4, 2024 12:56

OdedViner force-pushed the add_rbd_info branch from 0e15981 to da3d8b6 Compare November 4, 2024 13:51

rbd: added rbd info to validateRBDImageCount func

e19572a

Signed-off-by: Oded Viner <[email protected]>

OdedViner force-pushed the add_rbd_info branch from da3d8b6 to e19572a Compare November 6, 2024 18:38

add attrubute to imageInfo struct

4fc6960

Signed-off-by: Oded Viner <[email protected]>

OdedViner force-pushed the add_rbd_info branch from 0c69545 to 4fc6960 Compare November 11, 2024 18:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rbd: added rbd info to validateRBDImageCount func #4938

rbd: added rbd info to validateRBDImageCount func #4938

OdedViner commented Oct 31, 2024 •

edited

Loading

nixpanic Nov 1, 2024

OdedViner Nov 3, 2024

nixpanic Nov 1, 2024

OdedViner Nov 3, 2024

OdedViner Nov 4, 2024

nixpanic Nov 5, 2024

nixpanic Nov 1, 2024

OdedViner Nov 3, 2024

nixpanic commented Nov 1, 2024

nixpanic Nov 1, 2024

OdedViner Nov 3, 2024

Madhu-1 Nov 4, 2024

OdedViner Nov 4, 2024

Madhu-1 Nov 4, 2024

OdedViner Nov 4, 2024

Madhu-1 commented Nov 4, 2024

Madhu-1 commented Nov 4, 2024

OdedViner commented Nov 4, 2024

Madhu-1 commented Nov 4, 2024

nixpanic commented Nov 6, 2024

OdedViner commented Nov 6, 2024

Madhu-1 commented Nov 6, 2024

OdedViner commented Nov 7, 2024

OdedViner commented Nov 11, 2024

Madhu-1 commented Nov 11, 2024

rbd: added rbd info to validateRBDImageCount func #4938

Are you sure you want to change the base?

rbd: added rbd info to validateRBDImageCount func #4938

Conversation

OdedViner commented Oct 31, 2024 • edited Loading

Describe what this PR does

Is there anything that requires special attention

Related issues

Future concerns

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nixpanic commented Nov 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Madhu-1 commented Nov 4, 2024

Madhu-1 commented Nov 4, 2024

OdedViner commented Nov 4, 2024

Madhu-1 commented Nov 4, 2024

nixpanic commented Nov 6, 2024

OdedViner commented Nov 6, 2024

Madhu-1 commented Nov 6, 2024

OdedViner commented Nov 7, 2024

OdedViner commented Nov 11, 2024

Madhu-1 commented Nov 11, 2024

OdedViner commented Oct 31, 2024 •

edited

Loading