Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DAOS-14698 crtl: Create container with attributes #15300

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
5fcd159
DAOS-14698 crtl: Create container with attributes
kanard38 Oct 9, 2024
315f440
DAOS-14698 crtl: Create container with attributes
kanard38 Oct 9, 2024
865f2e5
DAOS-14698 crtl: Create container with attributes
kanard38 Oct 9, 2024
4dd51bd
Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…
kanard38 Oct 22, 2024
819b856
DAOS-14698 crtl: Create container with attributes
kanard38 Oct 9, 2024
b922f70
DAOS-14698 crtl: Create container with attributes
kanard38 Oct 9, 2024
825512d
Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…
kanard38 Oct 23, 2024
2e9afbd
DAOS-14698 crtl: Create container with attributes
kanard38 Oct 9, 2024
2d26f4d
Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…
kanard38 Oct 23, 2024
055c3a0
DAOS-14698 crtl: Create container with attributes
kanard38 Oct 9, 2024
a3d0a02
Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…
kanard38 Oct 24, 2024
7f26733
DAOS-14698 crtl: Create container with attributes
kanard38 Oct 9, 2024
5e72550
Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…
kanard38 Oct 25, 2024
5dca3b4
Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…
kanard38 Oct 25, 2024
98bd3c2
Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…
kanard38 Oct 28, 2024
a390e8a
DAOS-14698 crtl: Create container with attributes
kanard38 Oct 9, 2024
9af9320
Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…
kanard38 Oct 29, 2024
d2ee639
DAOS-14698 crtl: Create container with attributes
kanard38 Oct 9, 2024
90186bc
Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…
kanard38 Nov 4, 2024
0561a5b
DAOS-14698 crtl: Create container with attributes
kanard38 Oct 9, 2024
263168f
DAOS-14698 crtl: Create container with attributes
kanard38 Oct 9, 2024
436436f
DAOS-14698 crtl: Create container with attributes
kanard38 Oct 9, 2024
f30184d
Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…
kanard38 Nov 5, 2024
b1aaa8f
Merge remote-tracking branch 'origin/master' into ckochhof/dev/master…
kanard38 Nov 15, 2024
af54e76
DAOS-14698 crtl: Create container with attributes
kanard38 Nov 5, 2024
d230c87
DAOS-14698 crtl: Create container with attributes
kanard38 Nov 5, 2024
e3f510d
DAOS-14698 crtl: Create container with attributes
kanard38 Nov 5, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 32 additions & 13 deletions src/client/dfs/duns.c
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/**
* (C) Copyright 2019-2023 Intel Corporation.
* (C) Copyright 2019-2024 Intel Corporation.
*
* SPDX-License-Identifier: BSD-2-Clause-Patent
*/
Expand Down Expand Up @@ -1131,18 +1131,20 @@ duns_create_path(daos_handle_t poh, const char *path, struct duns_attr_t *attrp)
int
duns_link_cont(daos_handle_t poh, const char *cont, const char *path)
{
daos_handle_t coh;
daos_prop_t *prop;
struct daos_prop_entry *entry;
daos_pool_info_t pinfo = {0};
daos_cont_info_t cinfo = {0};
daos_cont_layout_t type;
char pool_str[DAOS_UUID_STR_SIZE];
char cont_str[DAOS_UUID_STR_SIZE];
int len;
char str[DUNS_MAX_XATTR_LEN];
char type_str[10];
int rc, rc2;
daos_handle_t coh;
daos_prop_t *prop;
struct daos_prop_entry *entry;
daos_pool_info_t pinfo = {0};
daos_cont_info_t cinfo = {0};
daos_cont_layout_t type;
char pool_str[DAOS_UUID_STR_SIZE];
char cont_str[DAOS_UUID_STR_SIZE];
int len;
char str[DUNS_MAX_XATTR_LEN];
char type_str[10];
bool backend_dfuse = false;
int rc2;
int rc;

if (path == NULL) {
D_ERROR("Invalid path\n");
Expand Down Expand Up @@ -1239,6 +1241,7 @@ duns_link_cont(daos_handle_t poh, const char *cont, const char *path)
D_ERROR("Failed to access container: %d (%s)\n", rc, strerror(rc));
D_GOTO(err_link, rc);
}
backend_dfuse = true;
}
} else if (type != DAOS_PROP_CO_LAYOUT_UNKNOWN) {
/** create a new file for other container types */
Expand Down Expand Up @@ -1304,6 +1307,22 @@ duns_link_cont(daos_handle_t poh, const char *cont, const char *path)
}
D_GOTO(err_link, rc);
}
if (backend_dfuse) {
struct stat finfo;
/*
* This next stat will cause dfuse to lookup the entry point and perform a
* container connect, therefore this data will be read from root of the new
* container, not the directory.
*
* TODO: This could call getxattr to verify success.
*/
rc = stat(path, &finfo);
if (rc) {
rc = errno;
D_ERROR("Failed to access new container: %d (%s)\n", rc, strerror(rc));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is moved, not new code but it would be good to use the correct macros here. The S here stands for system, that is it's a errno.

Suggested change
D_ERROR("Failed to access new container: %d (%s)\n", rc, strerror(rc));
DS_ERROR(rc, "Failed to access new container");

Copy link
Contributor Author

@knard-intel knard-intel Nov 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Use up to date logging macros

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Use up to date logging macros

Fixed with commit 0561a5b

goto err_link;
}
}

out_cont:
rc2 = daos_cont_close(coh, NULL);
Expand Down
103 changes: 37 additions & 66 deletions src/control/cmd/daos/container.go
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ type containerCreateCmd struct {
Mode ConsModeFlag `long:"mode" short:"M" description:"DFS consistency mode"`
ACLFile string `long:"acl-file" short:"A" description:"input file containing ACL"`
Group ui.ACLPrincipalFlag `long:"group" short:"g" description:"group who will own the container (group@[domain])"`
Attrs ui.SetPropertiesFlag `long:"attrs" short:"a" description:"user-defined attributes (key:val[,key:val...])"`
Args struct {
Label string `positional-arg-name:"label"`
} `positional-args:"yes"`
Expand Down Expand Up @@ -305,11 +306,7 @@ func (cmd *containerCreateCmd) Execute(_ []string) (err error) {
defer disconnectPool()

var contID string
if cmd.Path != "" {
contID, err = cmd.contCreateUNS()
} else {
contID, err = cmd.contCreate()
}
contID, err = cmd.contCreate()
if err != nil {
return err
}
Expand Down Expand Up @@ -397,77 +394,51 @@ func (cmd *containerCreateCmd) contCreate() (string, error) {
if err != nil {
return "", err
}
contID := cmd.contUUID.String()
cContID := C.CString(contID)
defer freeString(cContID)

var contID string
if cmd.contUUID == uuid.Nil {
contID = cmd.contLabel
} else {
contID = cmd.contUUID.String()
}

cmd.Infof("Successfully created container %s", contID)
return contID, nil
}

func (cmd *containerCreateCmd) contCreateUNS() (string, error) {
var dattr C.struct_duns_attr_t

props, cleanupProps, err := cmd.getCreateProps()
if err != nil {
return "", err
cleanupContainer := func() {
rc := C.daos_cont_destroy(cmd.cPoolHandle, cContID, goBool2int(true), nil)
if err := daosError(rc); err != nil {
cmd.Noticef("Failed to clean-up container %v", err)
}
}
defer cleanupProps()
dattr.da_props = props

if !cmd.Type.Set {
return "", errors.New("container type is required for UNS")
}
dattr.da_type = cmd.Type.Type
if len(cmd.Attrs.ParsedProps) != 0 {
kjacque marked this conversation as resolved.
Show resolved Hide resolved
attrs := make(attrList, 0, len(cmd.Attrs.ParsedProps))
for key, val := range cmd.Attrs.ParsedProps {
attrs = append(attrs, &attribute{
Name: key,
Value: []byte(val),
})
}

if cmd.poolUUID != uuid.Nil {
poolUUIDStr := C.CString(cmd.poolUUID.String())
defer freeString(poolUUIDStr)
C.uuid_parse(poolUUIDStr, &dattr.da_puuid[0])
}
if cmd.contUUID != uuid.Nil {
contUUIDStr := C.CString(cmd.contUUID.String())
defer freeString(contUUIDStr)
C.uuid_parse(contUUIDStr, &dattr.da_cuuid[0])
}
if err := cmd.openContainer(C.DAOS_COO_RW); err != nil {
cleanupContainer()
return "", errors.Wrapf(err, "failed to open new container %s", contID)
}
defer cmd.closeContainer()

if cmd.ChunkSize.Set {
dattr.da_chunk_size = cmd.ChunkSize.Size
}
if cmd.ObjectClass.Set {
dattr.da_oclass_id = cmd.ObjectClass.Class
}
if cmd.DirObjectClass.Set {
dattr.da_dir_oclass_id = cmd.DirObjectClass.Class
}
if cmd.FileObjectClass.Set {
dattr.da_file_oclass_id = cmd.FileObjectClass.Class
}
if cmd.CHints != "" {
hint := C.CString(cmd.CHints)
defer freeString(hint)
C.strncpy(&dattr.da_hints[0], hint, C.DAOS_CONT_HINT_MAX_LEN-1)
if err := setDaosAttributes(cmd.cContHandle, contAttr, attrs); err != nil {
cleanupContainer()
return "", errors.Wrapf(err, "failed to set user attributes on new container %s", contID)
}
}

cPath := C.CString(cmd.Path)
defer freeString(cPath)
if cmd.Path != "" {
cPath := C.CString(cmd.Path)
defer freeString(cPath)

dunsErrno := C.duns_create_path(cmd.cPoolHandle, cPath, &dattr)
rc := C.daos_errno2der(dunsErrno)
if err := daosError(rc); err != nil {
return "", errors.Wrapf(err, "duns_create_path() failed")
dunsErrno := C.duns_link_cont(cmd.cPoolHandle, cContID, cPath)
rc := C.daos_errno2der(dunsErrno)
if err := daosError(rc); err != nil {
cleanupContainer()
return "", errors.Wrapf(err, "duns_link_cont() failed")
}
}

contID := C.GoString(&dattr.da_cont[0])
cmd.contUUID, err = uuid.Parse(contID)
if err != nil {
cmd.contLabel = contID
}
cmd.Infof("Successfully created container %s type %s", contID, cmd.Type.String())
cmd.Infof("Successfully created container %s", contID)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious - Was the type %s intentionally removed? I think it was previously only printed for POSIX, but now it never is.

Copy link
Contributor Author

@knard-intel knard-intel Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just keep the message of the main merged function.
However, I also prefer this log message as it contains more relevant infos.
Thus I will change it.

  • Add container type in the log message

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just keep the message of the main merged function. However, I also prefer this log message as it contains more relevant infos. Thus I will change it.

  • Add container type in the log message

Fixed with commit 7f26733

Copy link
Contributor

@daltonbohning daltonbohning Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know what is expected here, but this now prints unknown when a type is not specified.

Successfully created container e6a21e5a-7161-4a7d-a604-1a7f929eb2b8 type unknown

return contID, nil
}

Expand Down
113 changes: 113 additions & 0 deletions src/tests/ftest/dfuse/container_attrs.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
"""
(C) Copyright 2020-2024 Intel Corporation.

SPDX-License-Identifier: BSD-2-Clause-Patent
"""

import os
import re

from apricot import TestWithServers
from dfuse_utils import get_dfuse, start_dfuse


class DfuseContainerAttrs(TestWithServers):
"""Check if the dfuse attributes of a container are properly managed.

:avocado: recursive
"""

def setUp(self):
"""Set up each test case."""
# obtain separate logs
self.update_log_file_names()

# Start the servers and agents
super().setUp()

def _check_attrs(self, dfuse, container_name):
phender marked this conversation as resolved.
Show resolved Hide resolved
"""Check if the DFuse attributes of a container are loaded

Check in the log file of the dfuse instance if it contains the DFuse attributes of a given
container. It also checks the value of the attributes found.

Args:
dfuse (Dfuse): DFuse instance to check
container_name (str): Name of the container
"""
log_file = os.linesep.join(dfuse.get_log_file_data().output[0].stdout)
attrs = self.params.get("attrs", f"/run/{container_name}/*")
for name, value in [attr.split(':') for attr in attrs.split(",")]:
match = re.findall(
fr"^.+\ssetting\s+'{name}'\s+is\s+(\d+)\s+seconds$",
log_file,
re.MULTILINE)
self.assertEqual(
len(match),
1,
f"Unexpected number setting(s) of attribute {name}: want=1, got={len(match)}")
self.assertEqual(
value,
match[0],
f"Unexpected value for attribute {name}: want={value}, got={match[0]}")

def test_dfuse_container_attrs(self):
"""Jira ID: DAOS-14698.

Test Description:
Create a container with DFuse attributes
Mount a DFuse mount point
Check the output of the DFuse log
ashleypittman marked this conversation as resolved.
Show resolved Hide resolved
:avocado: tags=all,pr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking - do these tests really need to be pr? Or could they be daily_regression or full_regression?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no opinion on this, and do not fill legitimate to have one.
I will let you decide with the other test members.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need your input :) Roughly, it's something like

  • Is the test critical and quick?
    • yes -> pr
    • no -> Is the test fairly important or very quick?
      • yes -> daily_regression
      • no -> weekly_regression

IMO these new tests belong in daily_regression unless they are very critical to base funcionality. @ashleypittman thoughts?

Related, I think we should add the daos_cmd feature tag to these tests since they verify some GO code paths for the daos command

Copy link
Contributor Author

@knard-intel knard-intel Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Add daos_cmd feature tag
  • Change tags from pr to daily_regression

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Add daos_cmd feature tag
  • Change tags from pr to daily_regression

Fixed with commit 7f26733

:avocado: tags=vm
:avocado: tags=dfuse,container
:avocado: tags=DfuseContainerAttrs,test_dfuse_container_attrs
"""
self.log.info("Creating DAOS pool")
pool = self.get_pool()

self.log_step("Creating DAOS container with Dfuse attributes")
container = self.get_container(pool, namespace="/run/container_01/*")

self.log_step("Mounting DFuse mount point")
dfuse = get_dfuse(self, self.hostlist_clients)
dfuse.env["D_LOG_FLUSH"] = "INFO"
start_dfuse(self, dfuse, pool, container)

self.log_step("Checking DFuse log file")
self._check_attrs(dfuse, "container_01")

self.log_step("Test passed")

def test_dfuse_subcontainer_attrs(self):
"""Jira ID: DAOS-14698.

Test Description:
Create a container
Mount a DFuse mount point
Create a sub-container with DFuse attributes
Check the output of the DFuse log
:avocado: tags=all,pr
:avocado: tags=vm
:avocado: tags=dfuse,container
:avocado: tags=DfuseContainerAttrs,test_dfuse_subcontainer_attrs
"""
self.log.info("Creating DAOS pool")
pool = self.get_pool()

self.log_step("Creating DAOS container")
container = self.get_container(pool, namespace="/run/container_02/*")

self.log_step("Mounting DFuse mount point")
dfuse = get_dfuse(self, self.hostlist_clients)
dfuse.env["D_LOG_FLUSH"] = "INFO"
start_dfuse(self, dfuse, pool, container)

self.log_step("Creating DAOS subcontainer with DFuse attributes")
sub_dir = os.path.join(dfuse.mount_dir.value, "foo")
container = self.get_container(pool, namespace="/run/container_03/*", path=sub_dir)

self.log_step("Checking DFuse log file")
self._check_attrs(dfuse, "container_03")

self.log_step("Test passed")
30 changes: 30 additions & 0 deletions src/tests/ftest/dfuse/container_attrs.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
hosts:
test_servers: 1
test_clients: 1

timeout: 200

server_config:
name: daos_server
engines_per_host: 1
engines:
0:
log_file: daos_server0.log
storage: auto

pool:
scm_size: 80%

container_01:
type: POSIX
attrs: dfuse-attr-time:666,dfuse-dentry-time:999
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. This will be useful for some other tests as well. Once this lands I'll write up a ticket to identify those (for the test team to handle)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it would be an idea to have a way of exporting these from dfuse somehow. For now having a python function in ftest would be useful and then if we have a more direct way of querying it then we can plug it in later.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant it's nice to specify attrs on create, not necessarily dfuse though.
If dfuse already has the attrs what would container create need them for?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see what you mean. My mistake.

control_method: daos

container_02:
type: POSIX
control_method: daos

container_03:
type: POSIX
attrs: dfuse-attr-time:42,dfuse-dentry-time:404
control_method: daos
8 changes: 6 additions & 2 deletions src/tests/ftest/util/daos_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def pool_autotest(self, pool):

def container_create(self, pool, sys_name=None, path=None, cont_type=None,
oclass=None, dir_oclass=None, file_oclass=None, chunk_size=None,
properties=None, acl_file=None, label=None):
properties=None, acl_file=None, label=None, attrs=None):
# pylint: disable=too-many-arguments
"""Create a container.

Expand All @@ -96,6 +96,8 @@ def container_create(self, pool, sys_name=None, path=None, cont_type=None,
pairs defining the container properties. Defaults to None
acl_file (str, optional): ACL file. Defaults to None.
label (str, optional): Container label. Defaults to None.
attrs (str, optional): String of comma-separated <name>:<value> pairs defining the
container user attributes. Defaults to None.

Returns:
dict: the daos json command output converted to a python dictionary
Expand All @@ -110,10 +112,12 @@ def container_create(self, pool, sys_name=None, path=None, cont_type=None,
properties += ',rd_lvl:1'
else:
properties = 'rd_lvl:1'

return self._get_json_result(
("container", "create"), pool=pool, sys_name=sys_name, path=path,
type=cont_type, oclass=oclass, dir_oclass=dir_oclass, file_oclass=file_oclass,
chunk_size=chunk_size, properties=properties, acl_file=acl_file, label=label)
chunk_size=chunk_size, properties=properties, acl_file=acl_file, label=label,
attrs=attrs)

def container_clone(self, src, dst):
"""Clone a container to a new container.
Expand Down
3 changes: 3 additions & 0 deletions src/tests/ftest/util/daos_utils_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,9 @@ def __init__(self):
# --acl-file=PATH
# input file containing ACL
self.acl_file = FormattedParameter("--acl-file={}", None)
# --attrs=<name>:<value>[,<name>:<value>,...]
# user-defined attributes
self.attrs = FormattedParameter("--attrs={}", None)

class CreateSnapSubCommand(CommonContainerSubCommand):
"""Defines an object for the daos container create-snap command."""
Expand Down
Loading
Loading