[feat] tools-v2: add bs status volume snapshot command #2745

saicaca · 2023-09-12T17:44:10Z

What problem does this PR solve?

Issue Number: #2583

Problem Summary: Add bs status volume snapshot command

What is changed and how it works?

What's Changed: Implement the bs status volume snapshot command ,which is equivalent to the snapshot-status command in snaptool.py.

How it Works: It acquires snapshot status data through HTTP requests, mirroring the behavior in the Python version.

Side effects(Breaking backward compatibility? Performance regression?):

Check List

Relevant documentation/comments is changed or added
I acknowledge that all my contributions will be made under the project's license

Cyber-SiKu

This pr is encapsulated, and you can add your calls on this basis.

Cyber-SiKu · 2023-09-21T06:15:49Z

tools-v2/pkg/cli/command/curvebs/status/volume/snapshot/snapshot.go

+		sCmd.health = cobrautil.HEALTH_WARN
+	}
+
+	count := []int{0, 0, 0, 0, 0, 0}


map is better ?

Fixed in the latest commit.

saicaca · 2023-09-24T15:12:52Z

This pr is encapsulated, and you can add your calls on this basis.

I have rewritten this using the new snapshot utils.

Cyber-SiKu · 2023-09-26T12:59:02Z

tools-v2/pkg/cli/command/curvebs/status/volume/snapshot/snapshot.go

+		cobrautil.QueryAction: cobrautil.ActionGetFileSnapshotList,
+		cobrautil.QueryUser:   sCmd.user,
+		cobrautil.QueryFile:   sCmd.filename,
+		cobrautil.QueryLimit:  100,


defined as constant

Cyber-SiKu · 2023-09-26T12:59:09Z

tools-v2/pkg/cli/command/curvebs/status/volume/snapshot/snapshot.go

+		cobrautil.QueryUser:   sCmd.user,
+		cobrautil.QueryFile:   sCmd.filename,
+		cobrautil.QueryLimit:  100,
+		cobrautil.QueryOffset: 0,


Cyber-SiKu · 2023-09-26T12:59:28Z

tools-v2/pkg/cli/command/curvebs/status/volume/snapshot/snapshot.go

+	for _, s := range statusList {
+		count[s] = 0
+	}


Cyber-SiKu · 2023-09-26T12:59:41Z

tools-v2/pkg/cli/command/curvebs/status/volume/snapshot/snapshot.go

+		if err := json.Unmarshal([]byte(result), &resp); err != nil {
+			return err
+		}
+		if resp.Code != "0" {


defined as constant

Cyber-SiKu · 2023-09-26T12:59:46Z

tools-v2/pkg/cli/command/curvebs/status/volume/snapshot/snapshot.go

+		if resp.Code != "0" {
+			return fmt.Errorf("get clone list fail, error code: %s", resp.Code)
+		}
+		if len(resp.Snapshots) == 0 {


defined as constant

Cyber-SiKu · 2023-10-10T07:02:16Z

Please resolve the conflict and rebase your commit into one.

saicaca · 2023-10-10T14:45:08Z

Please resolve the conflict and rebase your commit into one.

Conflicts have been addressed and I've squashed the commits. Please have a look.

Cyber-SiKu · 2023-10-11T02:17:36Z

cicheck

Cyber-SiKu · 2023-10-11T02:19:55Z

cicheck

caoxianfei1 · 2023-10-12T10:50:34Z

@saicaca

saicaca · 2023-10-12T14:52:09Z

@caoxianfei1
I think this is because NewSnapshotQuerySubUri in tools-v2/internal/utils/snapshot.go incorrectly uses %s instead of %v for params of any type. This causes non-string parameters to be converted into strings like %!s(int=100). The % in the string results in the appearance of (MISSING) in the final URL because of this. The same issue should also occur in other commands where NewSnapshotQuerySubUri is used.

I noticed that #2793 will address this issue so maybe we don't have to fix it in this PR?

caoxianfei1 · 2023-10-13T01:46:34Z

@caoxianfei1 I think this is because NewSnapshotQuerySubUri in tools-v2/internal/utils/snapshot.go incorrectly uses %s instead of %v for params of any type. This causes non-string parameters to be converted into strings like %!s(int=100). The % in the string results in the appearance of (MISSING) in the final URL because of this. The same issue should also occur in other commands where NewSnapshotQuerySubUri is used.

I noticed that #2793 will address this issue so maybe we don't have to fix it in this PR?

I think we need to modify this issue to ensure that every pull request is correct and not rely on a code that has not yet been merged.

caoxianfei1 · 2023-10-13T01:49:36Z

@saicaca it works after modify the function. And it's better that you fix it.

saicaca · 2023-10-13T03:29:43Z

@saicaca it works after modify the function. And it's better that you fix it.

I will do it this noon.

saicaca · 2023-10-13T07:06:09Z

@caoxianfei1 NewSnapshotQuerySubUri is fixed in the latest commit.

saicaca · 2023-10-13T07:20:34Z

cicheck

saicaca · 2023-10-13T07:49:52Z

cicheck

Cyber-SiKu · 2023-10-23T06:07:00Z

cicheck

caoxianfei1 · 2023-10-24T09:48:41Z

tools-v2/pkg/cli/command/curvebs/status/volume/snapshot/snapshot.go

+	snapshotAddrs []string
+	timeout       time.Duration
+	user          string
+	filename      string


It means that we can query one volume snapshot status? can you give an exmaple in README.md.

And i think it may be error.

This argument also exists in the old version tools.

curve/tools/snaptool/querysnap.py

Lines 32 to 33 in ba010be

def snapshot_status(args):

totalCount, records = curltool.get_snapshot_list_all(args.user, args.filename)

According to the implementation of the backend service, only the snapshots with the matching filename will be queried. It seems that the filename doesn't need to be unique so it can return multiple snapshots.

curve/src/snapshotcloneserver/common/snapshotclone_meta_store_etcd.cpp

Lines 149 to 163 in ba010be

int SnapshotCloneMetaStoreEtcd::GetSnapshotList(const std::string &filename,

std::vector<SnapshotInfo> *v) {

ReadLockGuard guard(snapInfos_mutex);

for (auto it = snapInfos_.begin();

it != snapInfos_.end();

it++) {

if (filename == it->second.GetFileName()) {

v->push_back(it->second);

}

}

if (v->size() != 0) {

return 0;

}

return -1;

}

The filename is set when creating the snapshot. (#2772) (It is the same in the old version tools)

curve bs create volume snapshot --user root --filename test --snapshotname snap-test

So, I think this argument can be used like

curve bs status volume snapshot --filename test --snapshotaddr 127.0.0.1:5550,127.0.0.1:5551,127.0.0.1:5552

I will add it to the README.

saicaca · 2023-10-25T18:10:37Z

cicheck

caoxianfei1 · 2023-10-27T06:13:44Z

need to rebase and fix the conflict.

saicaca · 2023-10-27T08:09:33Z

cicheck

saicaca · 2023-10-27T09:36:08Z

need to rebase and fix the conflict.

@caoxianfei1 I think it's ready to merge.

baytan0720

#2793 is merged, it fix NewSnapshotQuerySubUri did not work properly. you can rebase and fix conflict.

baytan0720 · 2023-10-11T09:04:15Z

tools-v2/pkg/cli/command/curvebs/status/volume/snapshot/snapshot.go

+	"github.com/spf13/cobra"
+	"strconv"
+	"time"
+)


sort import libs as

import ( standard lib ... external lib ... )

Fixed in the latest commit.

baytan0720 · 2023-11-01T16:59:34Z

tools-v2/pkg/cli/command/curvebs/status/volume/snapshot/snapshot.go

+	STATUS_ERROR_DELETING: "errorDeleting",
+	STATUS_CANCELING:      "canceling",
+	STATUS_ERROR:          "error",
+}


these structs and variables were defined in snapshotutil, you can use it directly.

Fixed in the latest commit.

baytan0720 · 2023-11-01T17:03:35Z

tools-v2/pkg/cli/command/curvebs/status/volume/snapshot/snapshot.go

+			return err.ToError()
+		}
+
+		var resp QueryResponse


you can declare a variable

var resp struct { snapshotutil.Response SnapshotInfos snapshotutil.SnapshotInfos `json:"SnapshotInfos"` }

Fixed in the latest commit.

Cyber-SiKu · 2023-11-02T02:36:42Z

pls fix conflicts

saicaca · 2023-11-03T15:33:22Z

pls fix conflicts

Conflicts fixed.

saicaca · 2023-11-03T15:33:40Z

cicheck

Cyber-SiKu · 2023-11-06T03:35:29Z

cicheck

caoxianfei1 · 2023-12-25T10:44:40Z

cicheck

montaguelhz · 2024-01-05T09:21:40Z

Are you free to move on, fix the conflict and complete it.

Signed-off-by: saicaca <[email protected]>

saicaca · 2024-01-06T07:13:39Z

Are you free to move on, fix the conflict and complete it.

Yes, I have fixed the conflict.

saicaca force-pushed the dev branch 2 times, most recently from 3514dcb to deb32ef Compare September 18, 2023 05:09

Cyber-SiKu reviewed Sep 21, 2023

View reviewed changes

saicaca force-pushed the dev branch from ee49db4 to 449d662 Compare September 24, 2023 15:16

saicaca requested a review from Cyber-SiKu September 25, 2023 13:55

Cyber-SiKu reviewed Oct 8, 2023

View reviewed changes

saicaca force-pushed the dev branch from b3cde96 to 0eb6213 Compare October 8, 2023 15:03

saicaca requested a review from Cyber-SiKu October 8, 2023 17:36

Cyber-SiKu approved these changes Oct 10, 2023

View reviewed changes

Cyber-SiKu mentioned this pull request Oct 10, 2023

Adaptor Openeuler 22.03-lts-sp2 #2710

Closed

2 tasks

saicaca force-pushed the dev branch from 0eb6213 to 707257c Compare October 10, 2023 14:41

saicaca force-pushed the dev branch from 707257c to 307298d Compare October 13, 2023 07:03

caoxianfei1 reviewed Oct 24, 2023

View reviewed changes

saicaca force-pushed the dev branch from 63018db to edc186c Compare October 25, 2023 18:08

saicaca requested a review from caoxianfei1 October 25, 2023 18:09

saicaca force-pushed the dev branch 2 times, most recently from e5fa7e4 to 7cadf7a Compare October 27, 2023 07:44

baytan0720 reviewed Nov 1, 2023

View reviewed changes

saicaca force-pushed the dev branch 4 times, most recently from 020541c to 7b58dc5 Compare November 3, 2023 15:26

saicaca requested a review from baytan0720 November 3, 2023 15:33

saicaca force-pushed the dev branch from 7b58dc5 to 790a9e1 Compare November 27, 2023 07:01

saicaca force-pushed the dev branch from 790a9e1 to 8a9cb5c Compare December 26, 2023 13:10

[feat] tools-v2: add bs status volume snapshot command

3e23feb

Signed-off-by: saicaca <[email protected]>

saicaca force-pushed the dev branch from 8a9cb5c to 3e23feb Compare January 6, 2024 07:12

	def snapshot_status(args):
	totalCount, records = curltool.get_snapshot_list_all(args.user, args.filename)

	int SnapshotCloneMetaStoreEtcd::GetSnapshotList(const std::string &filename,
	std::vector<SnapshotInfo> *v) {
	ReadLockGuard guard(snapInfos_mutex);
	for (auto it = snapInfos_.begin();
	it != snapInfos_.end();
	it++) {
	if (filename == it->second.GetFileName()) {
	v->push_back(it->second);
	}
	}
	if (v->size() != 0) {
	return 0;
	}
	return -1;
	}

[feat] tools-v2: add bs status volume snapshot command #2745

Are you sure you want to change the base?

[feat] tools-v2: add bs status volume snapshot command #2745

Conversation

saicaca commented Sep 12, 2023 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

Cyber-SiKu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saicaca commented Sep 24, 2023

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

Cyber-SiKu commented Oct 10, 2023

saicaca commented Oct 10, 2023

Cyber-SiKu commented Oct 11, 2023

Cyber-SiKu commented Oct 11, 2023

caoxianfei1 commented Oct 12, 2023

saicaca commented Oct 12, 2023 • edited Loading

caoxianfei1 commented Oct 13, 2023

caoxianfei1 commented Oct 13, 2023

saicaca commented Oct 13, 2023

saicaca commented Oct 13, 2023

saicaca commented Oct 13, 2023

saicaca commented Oct 13, 2023

Cyber-SiKu commented Oct 23, 2023

caoxianfei1 Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

saicaca Oct 24, 2023 • edited Loading

Choose a reason for hiding this comment

saicaca commented Oct 25, 2023

caoxianfei1 commented Oct 27, 2023

saicaca commented Oct 27, 2023

saicaca commented Oct 27, 2023

baytan0720 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

baytan0720 Nov 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Cyber-SiKu commented Nov 2, 2023

saicaca commented Nov 3, 2023

saicaca commented Nov 3, 2023

Cyber-SiKu commented Nov 6, 2023

caoxianfei1 commented Dec 25, 2023

montaguelhz commented Jan 5, 2024

saicaca commented Jan 6, 2024

saicaca commented Sep 12, 2023 •

edited

Loading

saicaca commented Oct 12, 2023 •

edited

Loading

caoxianfei1 Oct 24, 2023 •

edited

Loading

saicaca Oct 24, 2023 •

edited

Loading

baytan0720 Nov 1, 2023 •

edited

Loading