[#5] Add container zone names support #6

KurlesHS · 2025-01-22T16:38:11Z

KurlesHS commented

2025-01-22 16:38:11 +00:00

Added the ability to explicitly specify the container zone in its name. For this, the syntax "<cnr-name>.<zone-name>" is used, where <cnr-name> is the name of a container and <zone-name> is the name of the zone. If no zone is explicitly specified, the default zone will be used.

A new parameter has been added for the frostfs backend: "default_container_zone", which is responsible for the zone where new containers are created and existing containers are resolved if no zone is specified in the container's name.

close #5

Added the ability to explicitly specify the container zone in its name. For this, the syntax "`<cnr-name>.<zone-name>`" is used, where `<cnr-name>` is the name of a container and `<zone-name>` is the name of the zone. If no zone is explicitly specified, the default zone will be used. A new parameter has been added for the frostfs backend: "`default_container_zone`", which is responsible for the zone where new containers are created and existing containers are resolved if no zone is specified in the container's name. close #5

KurlesHS added 2 commits 2025-01-22 16:38:12 +00:00

[#5 ] Add container zone names support. 2833ac991c

Signed-off-by: Aleksey Kravchenko <al.kravchenko@yadro.com>

[#5 ] Update frostfs backend docs

/ DCO (pull_request) Successful in 43s

Details

/ Builds (pull_request) Successful in 2m0s

Details

/ Lint (pull_request) Successful in 4m20s

Details

/ Test (pull_request) Successful in 3m0s

Details

787f164997

Signed-off-by: Aleksey Kravchenko <al.kravchenko@yadro.com>

potyarkin commented

2025-01-23 07:16:49 +00:00

It's possible that I'm wrong, but I've seen/heard in demos the opposite naming convention: <cnr-name>.<zone-name>. I think we should stick to one approach everywhere.

It's possible that I'm wrong, but I've seen/heard in demos the opposite naming convention: `<cnr-name>.<zone-name>`. I think we should stick to one approach everywhere.

👍 2

KurlesHS was assigned by dkirillov

2025-01-23 08:04:51 +00:00

requested reviews from storage-services-committers, storage-services-developers

2025-01-23 08:05:04 +00:00

KurlesHS force-pushed feature/add-container-zones-support from 787f164997 to 0889e3075f

2025-01-23 08:35:08 +00:00

Compare

KurlesHS commented

2025-01-23 08:51:58 +00:00

It's possible that I'm wrong, but I've seen/heard in demos the opposite naming convention: <cnr-name>.<zone-name>. I think we should stick to one approach everywhere.

Yes, you're not the only one who noticed this. I changed the format of the zone assignment to the expected one.

> It's possible that I'm wrong, but I've seen/heard in demos the opposite naming convention: `<cnr-name>.<zone-name>`. I think we should stick to one approach everywhere. Yes, you're not the only one who noticed this. I changed the format of the zone assignment to the expected one.

dkirillov reviewed 2025-01-23 09:18:30 +00:00

backend/frostfs/frostfs.go Outdated

					
				@ -818,3 +828,3 @@

					container.SetCreationTime(&cnr, time.Now())

					container.SetName(&cnr, containerName)

					container.SetName(&cnr, containerStr)

dkirillov commented

2025-01-23 08:51:16 +00:00

Probably we can set to this just name rather than name+zone

KurlesHS commented

2025-01-23 10:24:57 +00:00

For the internal logic of the application, it does not matter what value is in this property. But if we suddenly have to look at containers not using rclone, but using frostfs-cli, it seems to me that it is better to store in this property the value that the user explicitly entered when specifying the name of the root directory. Besides, in most cases there will be a name without a zone anyway.

For the internal logic of the application, it does not matter what value is in this property. But if we suddenly have to look at containers not using `rclone`, but using `frostfs-cli`, it seems to me that it is better to store in this property the value that the user explicitly entered when specifying the name of the root directory. Besides, in most cases there will be a name without a zone anyway.

dkirillov marked this conversation as resolved

backend/frostfs/frostfs.go Outdated

					
				@ -946,3 +958,2 @@

				func getContainerIDByName(dirEntry fs.DirEntry, containerName string) (ok bool, cnrID cid.ID, err error) {

					if dirEntry.Remote() != containerName {

				func getContainerIDByZoneAndName(dirEntry fs.DirEntry, cnrZone, cnrName, defaultZone string) (ok bool, cnrID cid.ID, err error) {

dkirillov commented

2025-01-23 09:07:39 +00:00

Maybe it's matter of taste, but It's more common to see ok bool after value (cnrID cid.ID)

Maybe it's matter of taste, but It's more common to see `ok bool` after value (`cnrID cid.ID`)

👍 1

dkirillov marked this conversation as resolved

backend/frostfs/frostfs.go Outdated

					
				@ -957,3 +970,3 @@

				}

				func resolveContainerIDWithNNS(resolver *resolver.NNS, containerName string) (cid.ID, error) {

				func resolveContainerIDWithNNS(resolver *resolver.NNS, cnrZone, cnrName, containerStr string) (cid.ID, error) {

dkirillov commented

2025-01-23 09:10:22 +00:00

I'm not sure if we need containerStr string parameter here

I'm not sure if we need `containerStr string` parameter here

👍 1

dkirillov marked this conversation as resolved

backend/frostfs/frostfs.go Outdated

					
				@ -968,2 +982,2 @@

				func (f *Fs) resolveContainerIDHelper(ctx context.Context, containerName string) (cid.ID, error) {

					cnrIDStr, ok := f.containerIDCache[containerName]

				func (f *Fs) resolveContainerIDHelper(ctx context.Context, containerStr string) (cid.ID, error) {

					cnrIDStr, ok := f.containerIDCache[containerStr]

dkirillov commented

2025-01-23 09:04:20 +00:00

Shouldn't we protect f.containerIDCache more granular? Now we hold lock when create/delete container. I'm not sure if it's necessary

Shouldn't we protect `f.containerIDCache` more granular? Now we hold lock when create/delete container. I'm not sure if it's necessary

KurlesHS commented

2025-01-23 10:38:56 +00:00

Due to the fact that PUT operations are performed in parallel in separate goroutines, if a requested container is missing, and we do not use a global lock, all these goroutines will attempt to create their own containers, which could lead to conflicts.

Due to the fact that `PUT` operations are performed in parallel in separate goroutines, if a requested container is missing, and we do not use a global lock, all these goroutines will attempt to create their own containers, which could lead to conflicts.

dkirillov commented

2025-01-27 09:28:06 +00:00

Well, let's add comment about this then.

By the way I'm a little confused by not seeing using mutex in function where we work with resource that should be protected by it (f.containerIDCache). Maybe we should not extract resolveContainerIDHelper into separate function but make a little duplication in resolveContaienrID and resolveOrCreateContainer?

@alexvanin

Well, let's add comment about this then. By the way I'm a little confused by not seeing using mutex in function where we work with resource that should be protected by it (`f.containerIDCache`). Maybe we should not extract `resolveContainerIDHelper` into separate function but make a little duplication in `resolveContaienrID` and `resolveOrCreateContainer`? @alexvanin

alexvanin commented

2025-01-29 13:09:29 +00:00

#9

https://git.frostfs.info/TrueCloudLab/rclone/issues/9

backend/frostfs/frostfs.go Outdated

					
				@ -976,2 +994,2 @@

						if cnrID, err = resolveContainerIDWithNNS(f.resolver, containerName); err == nil {

							f.containerIDCache[containerName] = cnrID.String()

						if cnrID, err = resolveContainerIDWithNNS(f.resolver, cnrZone, cnrName, containerStr); err == nil {

							f.containerIDCache[containerStr] = cnrID.String()

dkirillov commented

2025-01-23 09:13:40 +00:00

Why don't we update cache inside resolveContainerIDWithNNS?
Then we can simplify code here to

if f.resolver != nil {
    return resolveContainerIDWithNNS(f.resolver, cnrZone, cnrName, containerStr)
}

Why don't we update cache inside `resolveContainerIDWithNNS`? Then we can simplify code here to ```golang if f.resolver != nil { return resolveContainerIDWithNNS(f.resolver, cnrZone, cnrName, containerStr) } ```

dkirillov commented

2025-01-23 09:17:39 +00:00

Or we can rewrite resolveContainerIDHelper so it looks like:

cnrIDStr, ok := f.containerIDCache[containerStr]
if ok {
	return parseContainerID(cnrIDStr)
}

cnrID, err := funcToResolveCID()
if err !=nil {
    return cid.ID{}, err 
}

f.containerIDCache[containerStr] = cnrID.String()
return cnrID, nil

Or we can rewrite `resolveContainerIDHelper` so it looks like: ```golang cnrIDStr, ok := f.containerIDCache[containerStr] if ok { return parseContainerID(cnrIDStr) } cnrID, err := funcToResolveCID() if err !=nil { return cid.ID{}, err } f.containerIDCache[containerStr] = cnrID.String() return cnrID, nil ```

👍 1

dkirillov marked this conversation as resolved

backend/frostfs/util.go Outdated

					
				@ -302,3 +302,3 @@

					if domain := container.ReadDomain(cnr); domain.Name() != "" {

						remote = domain.Name()

						if defaultZone != domain.Zone() {

dkirillov commented

2025-01-23 08:59:12 +00:00

Why do we need such difference?

KurlesHS commented

2025-01-23 10:13:48 +00:00

Why do we need such difference?

Without this, the root directories in the default zone will be resolved as "<cnr-name>.<default-zone>", and many integration tests will fail. If we leave only the container name as "Remote", then when listing all container files, the root directories "root-dir.z1" and "root-dir.z2" will have the same name "root-dir", which will lead to ambiguity.

> Why do we need such difference? Without this, the root directories in the default zone will be resolved as `"<cnr-name>.<default-zone>"`, and many integration tests will fail. If we leave only the container name as "Remote", then when listing all container files, the root directories "root-dir.z1" and "root-dir.z2" will have the same name "root-dir", which will lead to ambiguity.

dkirillov marked this conversation as resolved

backend/frostfs/util.go Outdated

					
				@ -310,2 +314,4 @@

					return dir

				}

				func getZoneAndContainerNames(containerStr, defaultZone string) (cnrZone string, cnrName string) {

dkirillov commented

2025-01-23 08:53:28 +00:00

Can we return values in opposite order (name, zone)?

👍 1

dkirillov marked this conversation as resolved

backend/frostfs/util.go Outdated

					
				@ -312,0 +317,4 @@

				func getZoneAndContainerNames(containerStr, defaultZone string) (cnrZone string, cnrName string) {

					defer func() {

						if len(cnrZone) == 0 {

							cnrZone = "container"

dkirillov commented

2025-01-23 08:56:32 +00:00

I woudn't allow empty cnrZone. By the way why we use here "container" rather than defaultZone?

I woudn't allow empty `cnrZone`. By the way why we use here `"container"` rather than `defaultZone`?

KurlesHS commented

2025-01-23 10:50:18 +00:00

Here I mean the following logic: if the user enters the name of the root directory as "<cnr-name>", the zone name will be resolved to the one specified in the restic configuration in the "default_container_zone" parameter. And if the name of root directory is specified in "<cnr-name>." format (with a dot at the end), then zone will resolve to standard one for FrostFS, which is "container".

Here I mean the following logic: if the user enters the name of the root directory as `"<cnr-name>"`, the zone name will be resolved to the one specified in the restic configuration in the "`default_container_zone`" parameter. And if the name of root directory is specified in `"<cnr-name>."` format (with a dot at the end), then zone will resolve to standard one for FrostFS, which is "`container`".

dkirillov commented

2025-01-27 09:45:21 +00:00

Maybe it's better, if user want to enter name of root directory that isn't in default zone make he enter full name (with zone) even if it's container?

Maybe it's better, if user want to enter name of root directory that isn't in default zone make he enter full name (with zone) even if it's `container`?

👍 1

KurlesHS force-pushed feature/add-container-zones-support from 0889e3075f to 5ad64af106

2025-01-23 12:22:19 +00:00

Compare

dkirillov reviewed 2025-01-27 09:36:29 +00:00

backend/frostfs/frostfs.go Outdated

					
				@ -997,0 +1005,4 @@

						return parseContainerID(cnrIDStr)

					}

					cnrID, err := f.resolveCIDByRootDirName(ctx, rootDirName)

					if err == nil {

dkirillov commented

2025-01-27 09:32:30 +00:00

Can we use opposite condition if err != nil ? It's more common

Can we use opposite condition ` if err != nil` ? It's more common

👍 1

backend/frostfs/frostfs.go Outdated

					
				@ -997,0 +1008,4 @@

					if err == nil {

						f.containerIDCache[rootDirName] = cnrID.String()

					}

					return cnrID, err

dkirillov commented

2025-01-27 09:36:17 +00:00

If we know that value doesn't matter because of err != nil, let's write

return cid.ID{}, err

to highlight the fact that we aren't interested in value

If we know that value doesn't matter because of `err != nil`, let's write ```golang return cid.ID{}, err ``` to highlight the fact that we aren't interested in value

👍 1

backend/frostfs/frostfs.go Outdated

					
				@ -1005,2 +1021,4 @@

				func (f *Fs) parseContainer(ctx context.Context, rootDirName string) (cid.ID, error) {

					cnrID, err := parseContainerID(rootDirName)

					if err == nil {

						return cnrID, err