Lvol snapshot management of User Created flag #106

DamiaSan · 2024-02-05T06:47:52Z

Which issue(s) this PR fixes:

Issue #7578

What this PR does / why we need it:

Special notes for your reviewer:

Additional documentation or context

Longhorn 7578 Signed-off-by: Damiano Cipriani <[email protected]>

shuo-wu

The UserCreated flag modification LGTM.
But the vendor update introduced xattrs SnapshotTime as well. We need to add it for ReplicaSnapshotCreate so that one snapshot has the same timestamp among all replicas. Besides, for snapshots, the creation time should be loaded from the xattrs instead.

DamiaSan · 2024-02-05T13:10:28Z

The UserCreated flag modification LGTM. But the vendor update introduced xattrs SnapshotTime as well. We need to add it for ReplicaSnapshotCreate so that one snapshot has the same timestamp among all replicas. Besides, for snapshots, the creation time should be loaded from the xattrs instead.

Yes, I agree with you. We will make the development for SnapshotTimestamp in another PR, now let's merge this one so we can update longhorn-instance-manager and close the UserCreated issue.

DamiaSan · 2024-02-06T06:57:15Z

@innobead @derekbit @shuo-wu I don't have the permission to merge this PR. Who can make it?

innobead · 2024-02-06T07:23:31Z

@innobead @derekbit @shuo-wu I don't have the permission to merge this PR. Who can make it?

You have write permission, so should be able to merge it 🤔 Wait, I will review this later first before merging. Thanks.

innobead · 2024-02-06T07:51:06Z

pkg/spdk/engine.go

 	return err
 }

-func (e *Engine) snapshotOperation(spdkClient *spdkclient.Client, inputSnapshotName string, snapshotOp SnapshotOperationType) (snapshotName string, err error) {
+func (e *Engine) snapshotOperation(spdkClient *spdkclient.Client, inputSnapshotName string, snapshotOp SnapshotOperationType, opts util.SnapshotOptions) (snapshotName string, err error) {


Suggest just use opts *SnapshotOptions to prevent unnecessary obj copy. This suggestion is for all changed methods.

innobead · 2024-02-06T07:55:26Z

pkg/spdk/replica.go

@@ -835,6 +847,9 @@ func (r *Replica) SnapshotCreate(spdkClient *spdkclient.Client, snapshotName str
 		return nil, fmt.Errorf("zero or multiple snap lvols with UUID %s found after lvol snapshot", snapUUID)
 	}

+	if userCreatedFlag {
+		bdevLvolList[0].DriverSpecific.Lvol.Xattrs[spdkclient.UserCreated] = "true"
+	}


Why do we need this? This should be implemented in BdevLvolGet already.

innobead · 2024-02-06T07:58:02Z

pkg/util/util.go

@@ -22,6 +22,10 @@ import (
 	"github.com/longhorn/go-common-libs/proc"
 )

+type SnapshotOptions struct {
+	UserCreated bool
+}


Suggest moving this to pkg/spdk/typges.go instead? Adding this to a common util package seems not intuitive to me.

innobead · 2024-02-06T08:05:16Z

pkg/spdk/server.go

+		UserCreated: req.UserCreated,
+	}
+
+	snapshotName, err := e.SnapshotCreate(spdkClient, req.SnapshotName, opts)


Why don't we use the same way for both replica and engine snapshot call? The engine call is using an option struct, but the replica call is just using a bool parameter.

return r.SnapshotCreate(spdkClient, req.SnapshotName, req.UserCreated)

Can we just make them consistent?

pkg/client/client.go

pkg/spdk/engine.go

shuo-wu · 2024-02-06T11:53:01Z

pkg/spdk/replica.go

@@ -1427,7 +1440,7 @@ func (r *Replica) RebuildingDstSnapshotCreate(spdkClient *spdkclient.Client, sna
 	}()

 	snapLvolName := GetReplicaSnapshotLvolName(r.Name, snapshotName)
-	snapUUID, err := spdkClient.BdevLvolSnapshot(r.rebuildingLvol.UUID, snapLvolName)
+	snapUUID, err := spdkClient.BdevLvolSnapshot(r.rebuildingLvol.UUID, snapLvolName, nil)


We should add opts for this API input then put it here.

Remember to update this.
This API will be invoked by the engine rebuilding here. The engine should check the flag of each snapshot then input the opts correct for RebuildingDstSnapshotCreate invocation

Sorry but I don't understand well what you are asking to me. If this API is invoked by the engine during the rebuild, the snapshot is not a user created snapshot ans so there is no necessity to pass opts here.

This function copies all snapshots from a healthy replica to the new replica. If a snapshot was ever requested by users, this snapshot should contain the attribute userCreated: true for all replicas.
In other words, the tag user created means if the snapshot CR of the volume is requested by the users themselves. It does not stand for the snapshot file in a specific replica being created by API SnapshotCreate or rebuilt from other replicas.

Ok thanks, I will add this

pkg/spdk/replica.go

Longhorn 7578 Signed-off-by: Damiano Cipriani <[email protected]>

DamiaSan · 2024-02-08T06:59:04Z

Codespell is returning the following error:
Error: ./proto/vendor/protobuf/src/google/protobuf/empty.proto:43: re-use ==> reuse
It is a comment, what should I do? Modify this file?

innobead · 2024-02-08T07:27:17Z

Codespell errro is fine. Just skip it.

DamiaSan · 2024-02-08T07:32:24Z

Codespell errro is fine. Just skip it.

So, once you will have resolved the requested changes (I have already addressed all of them), the PR can be merged also with codespell errors?

innobead · 2024-02-08T07:41:33Z

Yes. However, we will fix the codespell setting later.

shuo-wu · 2024-02-09T09:21:17Z

pkg/spdk/replica.go

@@ -1427,7 +1440,7 @@ func (r *Replica) RebuildingDstSnapshotCreate(spdkClient *spdkclient.Client, sna
 	}()

 	snapLvolName := GetReplicaSnapshotLvolName(r.Name, snapshotName)
-	snapUUID, err := spdkClient.BdevLvolSnapshot(r.rebuildingLvol.UUID, snapLvolName)
+	snapUUID, err := spdkClient.BdevLvolSnapshot(r.rebuildingLvol.UUID, snapLvolName, nil)


Remember to update this.
This API will be invoked by the engine rebuilding here. The engine should check the flag of each snapshot then input the opts correct for RebuildingDstSnapshotCreate invocation

innobead · 2024-02-20T05:35:18Z

@DamiaSan is this PR ready?

mergify · 2024-02-21T13:54:27Z

This pull request is now in conflict. Could you fix it @DamiaSan? 🙏

DamiaSan · 2024-02-22T11:49:26Z

I am going to close this PR and will make a new one.

DamiaSan · 2024-02-23T09:00:58Z

#115

proto: add user created flag to snapshot request

7139aff

Longhorn 7578 Signed-off-by: Damiano Cipriani <[email protected]>

DamiaSan requested review from innobead, derekbit and shuo-wu February 5, 2024 06:47

shuo-wu previously approved these changes Feb 5, 2024

View reviewed changes

innobead requested changes Feb 6, 2024

View reviewed changes

DamiaSan dismissed shuo-wu’s stale review via 98f92da February 6, 2024 11:07

DamiaSan force-pushed the main branch from 2cfdb30 to 98f92da Compare February 6, 2024 11:07

shuo-wu reviewed Feb 6, 2024

View reviewed changes

spdk lvol: snapshot management of UserCreated flag

d67b39e

Longhorn 7578 Signed-off-by: Damiano Cipriani <[email protected]>

DamiaSan force-pushed the main branch from 98f92da to d67b39e Compare February 6, 2024 15:39

DamiaSan and others added 2 commits February 8, 2024 07:43

Merge branch 'longhorn:main' into main

3f84c15

vendor: update go-spdk-helper

7bf5881

Longhorn 7578 Signed-off-by: Damiano Cipriani <[email protected]>

shuo-wu requested changes Feb 9, 2024

View reviewed changes

DamiaSan force-pushed the main branch from 70d321e to 7bf5881 Compare February 16, 2024 10:54

DamiaSan closed this Feb 22, 2024

DamiaSan mentioned this pull request Feb 22, 2024

User created flag #115

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lvol snapshot management of User Created flag #106

Lvol snapshot management of User Created flag #106

DamiaSan commented Feb 5, 2024

shuo-wu left a comment

DamiaSan commented Feb 5, 2024

DamiaSan commented Feb 6, 2024

innobead commented Feb 6, 2024

innobead Feb 6, 2024

DamiaSan Feb 6, 2024

innobead Feb 6, 2024

DamiaSan Feb 6, 2024

innobead Feb 6, 2024

DamiaSan Feb 6, 2024

innobead Feb 6, 2024

DamiaSan Feb 6, 2024

shuo-wu Feb 6, 2024

shuo-wu Feb 9, 2024

DamiaSan Feb 9, 2024

shuo-wu Feb 21, 2024 •

edited

Loading

DamiaSan Feb 22, 2024

DamiaSan commented Feb 8, 2024 •

edited

Loading

innobead commented Feb 8, 2024

DamiaSan commented Feb 8, 2024

innobead commented Feb 8, 2024

shuo-wu Feb 9, 2024

innobead commented Feb 20, 2024

mergify bot commented Feb 21, 2024

DamiaSan commented Feb 22, 2024

DamiaSan commented Feb 23, 2024

Lvol snapshot management of User Created flag #106

Lvol snapshot management of User Created flag #106

Conversation

DamiaSan commented Feb 5, 2024

Which issue(s) this PR fixes:

What this PR does / why we need it:

Special notes for your reviewer:

Additional documentation or context

shuo-wu left a comment

Choose a reason for hiding this comment

DamiaSan commented Feb 5, 2024

DamiaSan commented Feb 6, 2024

innobead commented Feb 6, 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

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

shuo-wu Feb 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DamiaSan commented Feb 8, 2024 • edited Loading

innobead commented Feb 8, 2024

DamiaSan commented Feb 8, 2024

innobead commented Feb 8, 2024

Choose a reason for hiding this comment

innobead commented Feb 20, 2024

mergify bot commented Feb 21, 2024

DamiaSan commented Feb 22, 2024

DamiaSan commented Feb 23, 2024

shuo-wu Feb 21, 2024 •

edited

Loading

DamiaSan commented Feb 8, 2024 •

edited

Loading