Add verify functionality to snapshot-metadata-lister tool #117

Rakshith-R · 2025-01-29T11:00:00Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it:

This commit adds verify functionality which when
enabled copies changed blocks from source device to target device and finally verifies contents of
both the devices to be same.

Which issue(s) this PR fixes:

Fixes #108

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2025-01-29T11:00:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Rakshith-R

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Rakshith-R]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

carlbraganza

As discussed, a better approach would be to have a separate CLI that invokes the iterator with an emitter that handles the difference check.

Rakshith-R · 2025-02-05T16:17:39Z

As discussed, a better approach would be to have a separate CLI that invokes the iterator with an emitter that handles the difference check.

I added a new tool snapshot-metadata-verifier as discussed.
Please take a look

carlbraganza

I think we're almost aligned. Please see my comments in the code.

I definitely don't approve of the Done method controlling the iterator return error. If you really need it called even in error situations then that's okay.

Apart from that, I suggested the following:

Define an Iterator interface in the interator package that contains a Run operation.
Rename or provide an alternative contructor called New that will return the Iterator interface.
Rename iterator.run to interator.Run
Do not embed the iterator in the verifier package

carlbraganza · 2025-02-10T21:35:40Z

pkg/iterator/iter.go

+type Iterator struct {
 	Args


I don't think you really want make the iterator data structure visible, because if you did then Args would become visible and you'd have to it from being an embedded field into a private field to a private variable:

type Iterator struct { args Args ...

and that has a lot of consequences.

I suggest instead, that the New constructor return an interface with the appropriate functionality. In this way the modularity is preserved with a clean, unpenetratable external interface:

type Iterator interface { Run(ctx context.Context) error } func New(args Args) Iterator { return newIterator(args) } // rename iter.run to iter.Run and the rest of the logic does not change

carlbraganza · 2025-02-10T21:50:05Z

pkg/iterator/iter.go

+	if err != nil {
+		return err
 	}

-	return err
+	return iter.Emitter.SnapshotMetadataIteratorDone(iter.recordNum)


It breaks the iterator abstraction to allow Done to modify the error!

Why do you even need to change the behavior here? The iterator is returning to your verifier and it has full control after it returns.

If you absolutely must invoke Done even in cases of error, then it should be passed the iterator error value in case it is needed, but under no circumstances can it be allowed to modify the iterator return value.

i.e. Re-define Done to be something like this:

// SnapshotMetadataIteratorDone is called prior to termination. // It is passed the error that the iterator would return. SnapshotMetadataIteratorDone(numberRecords int, iterErr error)

and use it thus in the Run method:

iter.Emitter.SnapshotMetadataIteratorDone(iter.recordNum, err) return err

If you absolutely must invoke Done even in cases of error, then it should be passed the iterator error value in case it is needed, but under no circumstances can it be allowed to modify the iterator return value.

I think there is some confusion here.

Done is invoked only when there is no error.

if err != nil { return err } return iter.Emitter.SnapshotMetadataIteratorDone(iter.recordNum)

Hopefully this should be fine ?

I can live with it!

carlbraganza · 2025-02-10T21:54:49Z

pkg/iterator/iter.go

@@ -175,8 +172,8 @@ type iteratorHelpers interface {
 	getChangedBlocks(ctx context.Context, grpcClient api.SnapshotMetadataClient, securityToken string) error
 }

-func newIterator(args Args) *iterator {
-	iter := &iterator{}
+func NewIterator(args Args) *Iterator {


NewIterator stutters! I suggest calling it New so that the external usage is

iter := iterator.New(args)

instead of

iter := iterator.NewIterator(args)

carlbraganza · 2025-02-10T22:17:58Z

examples/snapshot-metadata-verifier/main.go

+	if err := verifier.VerifySnapshotMetadata(ctx, args); err != nil {
+		fmt.Fprintf(os.Stderr, "Error: %v\n", err)
+		os.Exit(1)


I'm not sure why you feel the need to embed the iterator in the verifier package! Would the following invocation not be the same? (Assuming that New is the exposed constructor in the iterator package).

if err := iterator.New(args).Run(ctx); err != nil { ...

i.e. the verifier package has supplied an Emitter that is capable of verifying source/target while the traversal is done by the iterator package.

carlbraganza · 2025-02-10T22:28:16Z

examples/snapshot-metadata-verifier/main.go

+	usageFmt = `This command displays metadata on the content of a VolumeSnapshot object.
+If a previous VolumeSnapshot object is also specified then the metadata
+describes the content changes between the two snapshots, which must both
+be from the same PersistentVolume.
+
+The command is usually invoked in a Pod in the cluster, as the gRPC client
+needs to resolve the DNS address in the SnapshotMetadataService CR.
+


Shouldn't this talk about the verification being done?

carlbraganza · 2025-02-10T22:30:30Z

pkg/verifier/verifier.go

+	if err = a.Clients.Validate(); err != nil {
+		return err
+	}


This is redundant as a.Args.Validate() already checks this.

carlbraganza · 2025-02-10T22:34:00Z

pkg/verifier/verifier.go

+	// SourceDevicePath is optional, and if specified SourceDevice will be used to copy
+	// changed blocks to the TargetDevice.
+	SourceDevicePath string
+
+	// TargetDevice is optional, and if specified changed blocks from the SourceDevice
+	// will be copied to it.
+	TargetDevicePath string


You are passing the device files to VerifyEmitter. Why do you need these paths here? The device files are opened and checked in main.go already!

carlbraganza · 2025-02-10T22:36:54Z

examples/snapshot-metadata-verifier/main.go

+	targetDevice, err := os.OpenFile(args.TargetDevicePath, os.O_RDWR, 0644)
+	if err != nil {


The usage doesn't make it clear whether this target file should exist or not. Is os.O_CREAT needed?

carlbraganza · 2025-02-10T22:37:31Z

examples/snapshot-metadata-verifier/main.go

+	stringFlag(&args.SourceDevicePath, "source-device-path", "src", "", "The source device to use for verification.")
+	stringFlag(&args.TargetDevicePath, "target-device-path", "tgt", "", "The target device to use for verification.")


What does "verification" entail" ? Please describe in the usage.

This commit adds verify functionality which when enabled copies changed blocks from source device to target device and finally verifies contents of both the devices to be same. Signed-off-by: Rakshith R <[email protected]>

Rakshith-R · 2025-02-11T11:32:25Z

Thanks for the insight !

I've modified the pr to further make very minimal changes:

add a new verifier emitter
emitter methods now return a error
main takes 3 more arguments verify, source-device-path and target-device-path (added more info in usage)
- if verify is true, verifier is set as the emitter and iterator is run again.

I definitely don't approve of the Done method controlling the iterator return error. If you really need it called even in error situations then that's okay.

Done method is called only when there is no error. #117 (comment)

Please let me know what you think.

carlbraganza

Some questions:

Why 2 passes in the CLI?
Location of the emitter - in the iterator package or in the cli package?
Update doc comments.

carlbraganza · 2025-02-11T17:24:24Z

examples/snapshot-metadata-lister/main.go

@@ -155,6 +168,34 @@ func main() {
 		os.Exit(1)
 	}

+	if !verify {


I kind-of missed the fact that this verify is happening as a secondary step of the existing lister command. Nothing wrong with that, but why make 2 passes? Why not just use the verify emitter when the --verify flag is set?

carlbraganza · 2025-02-11T17:25:21Z

pkg/iterator/emitters.go

+	return nil
+}
+
+// VerifierEmitter formats the metadata as a table.


Please change the comment description!

Anything in pkg/iterator is part of our external interface: I don't think this functionality should be here!

It has very specific semantics which are not part of the published KEP. I suggest it be in the CLI command package as a sibling of main.go.

carlbraganza · 2025-02-11T17:26:47Z

pkg/iterator/iter.go

+	if err != nil {
+		return err
 	}

-	return err
+	return iter.Emitter.SnapshotMetadataIteratorDone(iter.recordNum)


I can live with it!

carlbraganza · 2025-02-11T17:41:34Z

pkg/iterator/emitters.go

+
+func (verifierEmitter *VerifierEmitter) SnapshotMetadataIteratorRecord(_ int, metadata IteratorMetadata) error {
+	for _, bmd := range metadata.BlockMetadata {
+		buffer := make([]byte, bmd.SizeBytes)


Why allocate the buffer in the inner loop? This is essentially serialized code and the buffer can be reused.

In fact, you could even stash the buffer in the VerifyEmitter struct itself, as a private variable that gets initialized before use if nil.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. kind/feature Categorizes issue or PR as related to a new feature. labels Jan 29, 2025

k8s-ci-robot requested review from PrasadG193 and saad-ali January 29, 2025 11:00

k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 29, 2025

carlbraganza suggested changes Jan 29, 2025

View reviewed changes

Rakshith-R force-pushed the verify-tool branch from a0bb384 to 3ba3934 Compare February 5, 2025 16:16

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 5, 2025

Rakshith-R requested a review from carlbraganza February 5, 2025 16:17

carlbraganza suggested changes Feb 10, 2025

View reviewed changes

carlbraganza reviewed Feb 10, 2025

View reviewed changes

Rakshith-R force-pushed the verify-tool branch from 3ba3934 to 19b3041 Compare February 11, 2025 10:48

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 11, 2025

Add verify functionality to snapshot-metadata-lister tool

fbcbded

This commit adds verify functionality which when enabled copies changed blocks from source device to target device and finally verifies contents of both the devices to be same. Signed-off-by: Rakshith R <[email protected]>

Rakshith-R force-pushed the verify-tool branch from 19b3041 to fbcbded Compare February 11, 2025 11:23

Rakshith-R requested a review from carlbraganza February 11, 2025 11:32

carlbraganza suggested changes Feb 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add verify functionality to snapshot-metadata-lister tool #117

Add verify functionality to snapshot-metadata-lister tool #117

Rakshith-R commented Jan 29, 2025

k8s-ci-robot commented Jan 29, 2025

carlbraganza left a comment

Rakshith-R commented Feb 5, 2025

carlbraganza left a comment •

edited

Loading

carlbraganza Feb 10, 2025

carlbraganza Feb 10, 2025

Rakshith-R Feb 11, 2025

carlbraganza Feb 11, 2025

carlbraganza Feb 10, 2025

carlbraganza Feb 10, 2025 •

edited

Loading

carlbraganza Feb 10, 2025

carlbraganza Feb 10, 2025

carlbraganza Feb 10, 2025 •

edited

Loading

carlbraganza Feb 10, 2025

carlbraganza Feb 10, 2025

Rakshith-R commented Feb 11, 2025

carlbraganza left a comment

carlbraganza Feb 11, 2025

carlbraganza Feb 11, 2025

carlbraganza Feb 11, 2025

carlbraganza Feb 11, 2025

		targetDevice, err := os.OpenFile(args.TargetDevicePath, os.O_RDWR, 0644)
		if err != nil {

		stringFlag(&args.SourceDevicePath, "source-device-path", "src", "", "The source device to use for verification.")
		stringFlag(&args.TargetDevicePath, "target-device-path", "tgt", "", "The target device to use for verification.")

Add verify functionality to snapshot-metadata-lister tool #117

Are you sure you want to change the base?

Add verify functionality to snapshot-metadata-lister tool #117

Conversation

Rakshith-R commented Jan 29, 2025

k8s-ci-robot commented Jan 29, 2025

carlbraganza left a comment

Choose a reason for hiding this comment

Rakshith-R commented Feb 5, 2025

carlbraganza left a comment • edited Loading

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

carlbraganza Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carlbraganza Feb 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rakshith-R commented Feb 11, 2025

carlbraganza left a 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

carlbraganza left a comment •

edited

Loading

carlbraganza Feb 10, 2025 •

edited

Loading

carlbraganza Feb 10, 2025 •

edited

Loading