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

Conversation

knard-intel
Copy link
Contributor

@knard-intel knard-intel commented Jul 3, 2023

Description

Fix test regression with system name introduce with PR #12509:

Fix regression introduce with PR #12508: issue with dmg and configuration files parsing.

Miscellaneous small improvements of the src/tests/ftest/control/version.py test.

Validation

Test-tag is used to limit the tests to the functional ones updated by this PR.

Before requesting gatekeeper:

  • Two review approvals and any prior change requests have been resolved.
  • Testing is complete and all tests passed or there is a reason documented in the PR why it should be force landed and forced-landing tag is set.
  • Features: (or Test-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.
  • Commit messages follows the guidelines outlined here.
  • Any tests skipped by the ticket being addressed have been run and passed in the PR.

Gatekeeper:

  • You are the appropriate gatekeeper to be landing the patch.
  • The PR has 2 reviews by people familiar with the code, including appropriate watchers.
  • Githooks were used. If not, request that user install them and check copyright dates.
  • Checkpatch issues are resolved. Pay particular attention to ones that will show up on future PRs.
  • All builds have passed. Check non-required builds for any new compiler warnings.
  • Sufficient testing is done. Check feature pragmas and test tags and that tests skipped for the ticket are run and now pass with the changes.
  • If applicable, the PR has addressed any potential version compatibility issues.
  • Check the target branch. If it is master branch, should the PR go to a feature branch? If it is a release branch, does it have merge approval in the JIRA ticket.
  • Extra checks if forced landing is requested
    • Review comments are sufficiently resolved, particularly by prior reviewers that requested changes.
    • No new NLT or valgrind warnings. Check the classic view.
    • Quick-build or Quick-functional is not used.
  • Fix the commit message upon landing. Check the standard here. Edit it to create a single commit. If necessary, ask submitter for a new summary.

@github-actions
Copy link

github-actions bot commented Jul 3, 2023

Bug-tracker data:
Ticket title is 'control/daos_agent_config.py:DaosAgentConfigTest.test_daos_agent_config_basic - '
Status is 'In Review'
Labels: 'ci_impact,daily_test,triaged'
https://daosio.atlassian.net/browse/DAOS-13886

Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Functional on EL 8 completed with status UNSTABLE. https://build.hpdd.intel.com/job/daos-stack/job/daos//view/change-requests/job/PR-12570/1/testReport/

@knard-intel knard-intel force-pushed the ckochhof/fix/master/daos-13886 branch from 9fc7233 to 571e013 Compare July 3, 2023 13:09
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@knard-intel knard-intel force-pushed the ckochhof/fix/master/daos-13886 branch from 571e013 to 2ac3633 Compare July 3, 2023 13:11
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@knard-intel knard-intel force-pushed the ckochhof/fix/master/daos-13886 branch from 9790013 to 5601fae Compare July 4, 2023 09:24
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@knard-intel knard-intel marked this pull request as ready for review July 4, 2023 10:40
@knard-intel knard-intel requested a review from a team as a code owner July 4, 2023 10:40
@daosbuild1
Copy link
Collaborator

Test stage Functional on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12570/5/execution/node/1097/log

Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Functional on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12570/6/execution/node/1097/log

@knard-intel knard-intel force-pushed the ckochhof/fix/master/daos-13886 branch from eed5046 to f84e1d5 Compare July 4, 2023 12:38
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Functional on EL 8 completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12570/7/execution/node/1097/log

@knard-intel knard-intel force-pushed the ckochhof/fix/master/daos-13886 branch from f84e1d5 to b17991f Compare July 4, 2023 14:17
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@knard-intel knard-intel force-pushed the ckochhof/fix/master/daos-13886 branch from b17991f to 3041157 Compare July 4, 2023 14:26
…/daos-13886

Features: control
Required-githooks: true
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@knard-intel
Copy link
Contributor Author

knard-intel commented Jul 7, 2023

Restart CI build as it was stuck in functional HW test with the wolf network issue.
However, the VM functional tests were all passed with the last fix :)

shimizukko
shimizukko previously approved these changes Jul 7, 2023
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@daosbuild1
Copy link
Collaborator

Test stage Functional Hardware Medium Verbs Provider completed with status FAILURE. https://build.hpdd.intel.com//job/daos-stack/job/daos/view/change-requests/job/PR-12570/19/execution/node/1242/log

mjmac
mjmac previously approved these changes Jul 10, 2023
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@@ -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 😸

@tanabarr tanabarr dismissed stale reviews from mjmac and shimizukko via 3ba1ce2 July 11, 2023 12:24
@tanabarr tanabarr self-requested a review July 11, 2023 12:25
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

Features: control
Required-githooks: true

Signed-off-by: Tom Nabarro <tom.nabarro@intel.com>
@tanabarr tanabarr force-pushed the ckochhof/fix/master/daos-13886 branch from 3ba1ce2 to ce83191 Compare July 11, 2023 12:27
Copy link
Collaborator

@daosbuild1 daosbuild1 left a comment

Choose a reason for hiding this comment

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

LGTM. No errors found by checkpatch.

@knard-intel knard-intel requested a review from a team July 12, 2023 05:58
@mchaarawi mchaarawi merged commit 34552ff into master Jul 12, 2023
21 checks passed
@mchaarawi mchaarawi deleted the ckochhof/fix/master/daos-13886 branch July 12, 2023 12:38
Comment on lines +80 to +87
# 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)
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

# 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

10 participants