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-13886 test: Fix system name validation #12570

Merged
merged 7 commits into from
Jul 12, 2023
Merged
6 changes: 6 additions & 0 deletions src/control/cmd/dmg/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,12 @@ and access control settings, along with system wide operations.`
logCmd.SetLog(log)
}

switch cmd.(type) {
case *versionCmd:
// this command don't need the rest of the setup
return cmd.Execute(args)
}

ctlCfg, err := control.LoadConfig(opts.ConfigPath)
if err != nil {
if errors.Cause(err) != control.ErrNoConfigFile {
Expand Down
2 changes: 1 addition & 1 deletion src/tests/ftest/control/daos_agent_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ def test_daos_agent_config_basic(self):
:avocado: tags=all,daily_regression
:avocado: tags=vm
:avocado: tags=control,basic
:avocado: tags=agent_start,daos_agent_config_test,test_daos_agent_config_basic
:avocado: tags=DaosAgentConfigTest,test_daos_agent_config_basic
"""
# Setup the agents
self.add_agent_manager()
Expand Down
2 changes: 1 addition & 1 deletion src/tests/ftest/control/daos_agent_config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ agent_config_val: !mux
config_val:
- "name"
- "! @#$%^&*()_+{}|:<>?-=[];',./"
- "PASS"
- "FAIL"
name_alphanumeric:
config_val:
- "name"
Expand Down
2 changes: 1 addition & 1 deletion src/tests/ftest/control/daos_control_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ def test_daos_control_config_basic(self):
:avocado: tags=all,daily_regression
:avocado: tags=vm
:avocado: tags=control,basic
:avocado: tags=control_start,test_daos_control_config_basic
:avocado: tags=DaosControlConfigTest,test_daos_control_config_basic
"""
# Get the input to verify
c_val = self.params.get("config_val", "/run/control_config_val/*/")
Expand Down
2 changes: 1 addition & 1 deletion src/tests/ftest/control/daos_control_config.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ control_config_val: !mux
config_val:
- "name"
- "! @#$%^&*()_+{}|:<>?-=[];',./"
- "PASS"
- "FAIL"
Copy link
Contributor

Choose a reason for hiding this comment

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

We should expand the coverage to cover special characters and over length, the reason this now fails is because there is a new length requirement introduced by 0b5cdc0 which limits to 15 chars.

I suggest reducing the value in this test case to 15 chars (which should pass even with special characters) and leaving the expectation as "PASS". Then add another name_links_exceeded test case with the original value which should result in a failure.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that would be better.

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 agree that would be better.

@tanabarr pushed the commit ce83191 to update the unit tests in this way 😸

name_numeric:
config_val:
- "name"
Expand Down
78 changes: 61 additions & 17 deletions src/tests/ftest/control/version.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,32 +22,69 @@ class DAOSVersion(TestWithServers):
def __init__(self, *args, **kwargs):
"""Initialize a DAOSVersion object."""
super().__init__(*args, **kwargs)

# Don't waste time starting servers and agents.
self.setup_start_servers = False
self.setup_start_agents = False

self.errors = []

def report_result(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

The tearDown() and setUp() methods provide markers to help find the beginning and end of the test - a sequence of 80 = characters. We typically just end each test_* method with self.log.info("Test passed") to indicate test passing (in addition to avocado's own messaging) and rely on self.fail() or assertions to indicate the test failure before calling tearDown(). If we need more than this it should be standardized so we can use the same search string for all test job.log results. That said, it can be addressed in a different PR.

Copy link
Contributor Author

@knard-intel knard-intel Jul 12, 2023

Choose a reason for hiding this comment

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

In fact, I have done it this way, to keep the same behavior as it was originally done in the PR.
However, if there is some standard to respect I will fix this in a follow up PR as this one has been already merged.

  • Use standardized test result 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.

Fixed with PR #12615

"""Helper printing test result"""
self.log.info("###### Test Result ######")
report_errors(test=self, errors=self.errors)
self.log.info("#########################")

def append_error(self, title, details):
"""Helper adding an error to the list of errors

Args:
title (str): Error message title
details (list): List of string of the error details
"""
msg = title
if details:
msg += "\n\t" + "\n\t".join(details)
self.errors.append(msg)

def test_version(self):
"""Verify version number for dmg, daos, daos_server, and daos_agent against RPM.

:avocado: tags=all,full_regression
:avocado: tags=vm
:avocado: tags=control,daos_cmd
:avocado: tags=version_number,test_version
:avocado: tags=DAOSVersion,test_version
"""
errors = []

# Get RPM version.
rpm_command = "rpm -qa|grep daos-server"
output = run_pcmd(hosts=self.hostlist_servers, command=rpm_command)
self.log.info("RPM output = %s", output)
stdout = output[0]["stdout"][0]
self.log.info("RPM stdout = %s", stdout)
result = re.findall(r"daos-server-[tests-|tests_openmpi-]*([\d.]+)", stdout)
self.log.debug("RPM output = %s", output)
rc = output[0]["exit_status"]
stdout = output[0]["stdout"]
if rc != 0:
msg = "DAOS RPMs not properly installed: rc={}".format(rc)
self.append_error(msg, stdout)
self.report_result()
rpm_version = None
for rpm in stdout:
result = re.findall(r"daos-server-[tests-|tests_openmpi-]*([\d.]+)", rpm)
if result:
rpm_version = result[0]
break
if not result:
errors.append("RPM version is not in the output! {}".format(output))
else:
rpm_version = result[0]
self.log.info("RPM version = %s", rpm_version)
msg = "RPM version could not be defined"
self.append_error(msg, stdout)
self.report_result()
self.log.info("RPM version = %s", rpm_version)
knard-intel marked this conversation as resolved.
Show resolved Hide resolved

# Remove configuration files
cleanup_cmds = [
"sudo find /etc/daos/certs -type f -delete -print",
"sudo rm -fv /etc/daos/daos_server.yml /etc/daos/daos_control.yml"
" /etc/daos/daos_agent.yml",
]
for cmd in cleanup_cmds:
run_pcmd(hosts=self.hostlist_servers, command=cmd)
Comment on lines +80 to +87
Copy link
Contributor

Choose a reason for hiding this comment

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

If we need to run this test w/o certificates, the preferred method is to use the following test yaml entries:

server_config:
  transport_config:
    allow_insecure: true
agent_config:
  transport_config:
    allow_insecure: true
dmg:
  transport_config:
    allow_insecure: true

Copy link
Contributor Author

@knard-intel knard-intel Jul 12, 2023

Choose a reason for hiding this comment

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

I will add these config parameters in a follow up PR as this one has been already merged.
Otherwise, I would prefer to keep the cleanup cmds as it seems that the /etc/daos is not properly cleanned between test runs.

  • Add insecure yaml config

Copy link
Contributor Author

@knard-intel knard-intel Jul 13, 2023

Choose a reason for hiding this comment

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

Fixed with PR #12615


# Get dmg version.
dmg_version = self.get_dmg_command().version()["response"]["version"]
Expand All @@ -58,10 +95,19 @@ def test_version(self):
self.log.info("daos version = %s", daos_version)

# Get daos_agent version.
daos_agent_version = None
daos_agent_cmd = "daos_agent --json version"
output = run_pcmd(hosts=self.hostlist_servers, command=daos_agent_cmd)
daos_agent_version = json.loads("".join(output[0]["stdout"]))["response"]["version"]
self.log.info("daos_agent version = %s", daos_agent_version)
self.log.debug("DAOS Agent output = %s", output)
rc = output[0]["exit_status"]
stdout = output[0]["stdout"]
if rc != 0:
msg = "DAOS Agent not properly installed: rc={}".format(rc)
self.append_error(msg, stdout)
else:
self.log.info("DAOS Agent stdout = %s", "".join(stdout))
daos_agent_version = json.loads("".join(stdout))["response"]["version"]
self.log.info("daos_agent version = %s", daos_agent_version)

# Get daos_server version
daos_server_cmd = DaosServerCommandRunner(path=self.bin)
Expand All @@ -82,8 +128,6 @@ def test_version(self):
if version != rpm_version:
msg = "Unexpected version! {} = {}, RPM = {}".format(
tool, version, rpm_version)
errors.append(msg)
self.errors.append(msg)

self.log.info("###### Test Result ######")
report_errors(test=self, errors=errors)
self.log.info("#########################")
self.report_result()