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-15079 test: VMD Hot Plug - Support rpc.py in spdk-tools package #61

Merged
merged 20 commits into from
Aug 6, 2024

Conversation

shimizukko
Copy link
Contributor

We need to support rpc.py from spdk-tools package (RPM) so that we can use the script in CI.

Make the changes in spdk.spec so that rpc.py is included in the RPM. After installing the RPM, we should be able to use the script by calling:
/usr/share/spdk/scripts/rpc.py

Skip-test: true
Skip-unit-tests: true
Required-githooks: true

@grom72 grom72 force-pushed the makito/DAOS-15079 branch 3 times, most recently from 3d4c895 to 3167ba8 Compare February 21, 2024 09:01
grom72 and others added 4 commits February 22, 2024 13:34
        Skip-test: true
        Skip-unit-tests: true
        Required-githooks: true

Signed-off-by: Tomasz Gromadzki <[email protected]>
We need to support rpc.py from spdk-tools package (RPM) so that
we can use the script in CI.

Make the changes in spdk.spec so that rpc.py is included in the
RPM. After installing the RPM, we should be able to use the
script by calling:
/usr/share/spdk/scripts/rpc.py

Skip-test: true
Skip-unit-tests: true
Required-githooks: true

Signed-off-by: Makito Kano <[email protected]>
Skip-test: true
Skip-unit-tests: true
Required-githooks: true

Signed-off-by: Makito Kano <[email protected]>
Required-githooks: true

Signed-off-by: Tomasz Gromadzki <[email protected]>
    Required-githooks: true

Signed-off-by: Tomasz Gromadzki <[email protected]>
@grom72 grom72 force-pushed the makito/DAOS-15079 branch 2 times, most recently from ff7cb7f to 7c31a3e Compare February 29, 2024 12:49
@grom72 grom72 marked this pull request as ready for review February 29, 2024 15:35
@grom72 grom72 requested a review from a team as a code owner February 29, 2024 15:35
Copy link
Contributor

@tanabarr tanabarr left a comment

Choose a reason for hiding this comment

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

LGTM bar a couple of points:

  • should probably update the changelog date
  • debian changelog also needs to be updated, this doesn't always get done but it's good practice
  • the same file copy should also be done in debian, @brianjmurrell can probably tell you where to update

@tanabarr
Copy link
Contributor

tanabarr commented Mar 1, 2024

@brianjmurrell do we need associated updates to the Debian build to copy across the RPC script?

Copy link
Contributor

@brianjmurrell brianjmurrell left a comment

Choose a reason for hiding this comment

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

Skip-test: true
Skip-unit-tests: true

What's the reason for skipping testing?

@@ -41,5 +41,5 @@
//@Library(value="pipeline-lib@your_branch") _

/* groovylint-disable-next-line CompileStatic */
packageBuildingPipelineDAOSTest(['distros': ['centos7', 'el8', 'el9', 'leap15', 'ubuntu20.04'],
packageBuildingPipelineDAOSTest(['distros': ['el8', 'el9', 'leap15', 'ubuntu20.04'],
'test-tag': 'pr'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know of any tests in DAOS' src/tests/ftest that would exercise this change? If so, we should add it's/their tags to this tag list. Maybe @tanabarr might know of some tests that would be applicable?

Maybe there are no tests yet because this change needs to be made in order to write some tests that would use this new addition?

Copy link
Contributor

@grom72 grom72 Mar 5, 2024

Choose a reason for hiding this comment

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

This PR is required for new tests to be implemented by @shimizukko.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a PR open for those (@shimizukko)? If not, do we have any idea of what the test tags will be for those tests so that we can preemptively add those tags here so they are not forgotten about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test that will use this PR is: daos-stack/daos#13620
It assumes /usr/share/spdk/scripts/rpc.py exists in the environment. The test tag is test_no_activity

spdk.spec Show resolved Hide resolved
@grom72
Copy link
Contributor

grom72 commented Mar 5, 2024

Skip-test: true
Skip-unit-tests: true

What's the reason for skipping testing?

@brianjmurrell do we need any tests if we only add new scripts that are not used at all by SPDK binaries?

@@ -41,5 +41,5 @@
//@Library(value="pipeline-lib@your_branch") _

/* groovylint-disable-next-line CompileStatic */
packageBuildingPipelineDAOSTest(['distros': ['centos7', 'el8', 'el9', 'leap15', 'ubuntu20.04'],
packageBuildingPipelineDAOSTest(['distros': ['el8', 'el9', 'leap15', 'ubuntu20.04'],
'test-tag': 'pr'])
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a PR open for those (@shimizukko)? If not, do we have any idea of what the test tags will be for those tests so that we can preemptively add those tags here so they are not forgotten about?

spdk.spec Show resolved Hide resolved
Skip-test: true
Skip-unit-tests: true
Required-githooks: true

Signed-off-by: Tomasz Gromadzki <[email protected]>
Skip-test: true
Skip-unit-tests: true
Required-githooks: true

Signed-off-by: Tomasz Gromadzki <[email protected]>
    Skip-test: true
    Skip-unit-tests: true
    Required-githooks: true

Signed-off-by: Tomasz Gromadzki <[email protected]>
Skip-test: true
Skip-unit-tests: true
Required-githooks: true

Signed-off-by: Tomasz Gromadzki <[email protected]>
@grom72
Copy link
Contributor

grom72 commented Mar 11, 2024

grom72 (Tomasz Gromadzki) force-pushed the makito/DAOS-15079 branch from 9b4f113 to 3657a60
grom72 (Tomasz Gromadzki) force-pushed the makito/DAOS-15079 branch from 3657a60 to 57473fb

@grom72 We generally disallow force pushes once reviewing has started as the force push voids your reviewers' ability to incrementally review from their last review and requires your reviewers to look at the entire changes again.

These force pushes are done only for commits that have not been yet reviewed.

Skip-test: true
Skip-unit-tests: true
Required-githooks: true

Signed-off-by: Tomasz Gromadzki <[email protected]>
@brianjmurrell
Copy link
Contributor

These force pushes are done only for commits that have not been yet reviewed.

They must have been forced over the commits for last reviews that you had requested since trying to review new commits since my last review did not work. It said it could not find the new commits. That generally happens when you force push over the most recently reviewed commits.

So yes, while you can technically force-push after reviews have started you have to be very careful only to force-push for commits since all of your reviewers' most recent commits. Unless you are very careful to do that, it's easiest just to not force-push once you have started requesting reviews.

What I will sometimes do if I want to work out a problem in a PR and know I will be pushing a bunch of trial-and-error pushes is to create a new PR based on the one I am working on, do all of my testing there and when I finally have that working as I want, I just merge the new PR into the old one, squashing all of the commits as I do the merge.

@grom72
Copy link
Contributor

grom72 commented Mar 12, 2024

These force pushes are done only for commits that have not been yet reviewed.

They must have been forced over the commits for last reviews that you had requested since trying to review new commits since my last review did not work. It said it could not find the new commits. That generally happens when you force push over the most recently reviewed commits.

So yes, while you can technically force-push after reviews have started you have to be very careful only to force-push for commits since all of your reviewers' most recent commits. Unless you are very careful to do that, it's easiest just to not force-push once you have started requesting reviews.

What I will sometimes do if I want to work out a problem in a PR and know I will be pushing a bunch of trial-and-error pushes is to create a new PR based on the one I am working on, do all of my testing there and when I finally have that working as I want, I just merge the new PR into the old one, squashing all of the commits as I do the merge.

OK, next time I will follow your way.

brianjmurrell
brianjmurrell previously approved these changes Mar 12, 2024
Copy link
Contributor

@brianjmurrell brianjmurrell left a comment

Choose a reason for hiding this comment

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

This is a great set of changes @grom72.

I do want to see a test run though so that we are sure we are not landing something that will break everyone's work in flight.

Since you have to push a new commit to remove the Skip-test pragma it would be worthwhile just updating the date stamps in the debian/changelog and spdk.spec files.

Required-githooks: true

Signed-off-by: Tomasz Gromadzki <[email protected]>
tanabarr
tanabarr previously approved these changes Mar 13, 2024
Priority: 2
PR-repos: daos@PR-13602
Test-tag: pr,hw,medium,hot_plug
Required-githooks: true

Signed-off-by: Gromadzki, Tomasz <[email protected]>
(fix PR reference)

PR-repos: daos@PR-13620
Test-tag: pr,hw,medium,hot_plug
Required-githooks: true

Signed-off-by: Gromadzki, Tomasz <[email protected]>
PR-repos: daos@PR-13620:10
Test-tag: pr,hw,medium,hot_plug
Required-githooks: true

Skip-coverity-test: true
Skip-fault-injection-test: true
Skip-func-test-el8: true
Skip-nlt: true
Skip-python-bandit: true
Skip-test-el-8.6-rpms: true
Skip-unit-test-memcheck: true
Skip-unit-tests: true

Skip-func-hw-test-medium: true
Skip-func-hw-test-medium-verbs-provider: true
Skip-func-hw-test-medium-md-on-ssd: false
Skip-func-hw-test-medium-verbs-provider-md-on-ssd: false
Skip-func-hw-test-medium-ucx-provider: true
Skip-func-hw-test-large: true

Signed-off-by: Gromadzki, Tomasz <[email protected]>
@brianjmurrell
Copy link
Contributor

@grom72 You cannot add PR-repos: daos@… to dependency PRs like this. The purpose of the downstream (i.e. daos) testing stages is to use the version of daos from master and release/* to validate that if we land this PR it will not break those two branches (or other folks' PRs based on those branches).

What is it that you are trying to achieve with specifying PR-repos: daos@PR-13620:10 here? Are you trying to test that this PR also works with some changes in daos-stack/daos#13620? If so, the way to test that is to add PR-repos: spdk@PR-61[:{<build-num>,lastSuccessfulBuild|lastBuild|etc.}] to that PR's commit message so that Jenkins builds and tests that PR with this PR as it's spdk.

Required-githooks: true

Signed-off-by: Gromadzki, Tomasz <[email protected]>
@janekmi
Copy link
Contributor

janekmi commented Jul 9, 2024

Outdated

  • master:
    • test_daos_rebuild_ec:DAOS-14982
    • test_daos_degraded_ec:DAOS-14982
  • release/2.4:
    • Dropped from the validation.
Skip-list-master: test_daos_rebuild_ec:DAOS-14982 test_daos_degraded_ec:DAOS-14982

Skip-list-master: test_daos_rebuild_ec:DAOS-14982 test_daos_degraded_ec:DAOS-14982

Allow-unstable-test: true
Skip-func-hw-test-large: false

Priority: 2

Signed-off-by: Jan Michalski <[email protected]>
Skip-list-master: test_daos_rebuild_ec:DAOS-14982 test_daos_degraded_ec:DAOS-14982

Allow-unstable-test: true
Skip-func-hw-test-large: false

Priority: 2

Signed-off-by: Jan Michalski <[email protected]>
@janekmi
Copy link
Contributor

janekmi commented Jul 11, 2024

Outdated

  • Skip-list-master:

    • test_daos_rebuild_ec:DAOS-14982
    • test_daos_degraded_ec:DAOS-14982
  • master:

    • test_ms_failover:DAOS-16103
  • release/2.6:

    • test_dfuse_daos_build_wb:DAOS-16168
    • test_osa_offline_reintegration_without_checksum:DAOS-15608
    • test_daos_extend_simple:DAOS-15640

Updated skip-lists:

Skip-list-master: test_daos_rebuild_ec:DAOS-14982 test_daos_degraded_ec:DAOS-14982 test_ms_failover:DAOS-16103
Skip-list-release/2.6: test_dfuse_daos_build_wb:DAOS-16168 test_osa_offline_reintegration_without_checksum:DAOS-15608 test_daos_extend_simple:DAOS-15640

Skip-list-master: test_daos_rebuild_ec:DAOS-14982 test_daos_degraded_ec:DAOS-14982 test_ms_failover:DAOS-16103
Skip-list-release/2.6: test_dfuse_daos_build_wb:DAOS-16168 test_osa_offline_reintegration_without_checksum:DAOS-15608 test_daos_extend_simple:DAOS-15640

Allow-unstable-test: true
Skip-func-hw-test-large: false

Priority: 2

Signed-off-by: Jan Michalski <[email protected]>
@janekmi
Copy link
Contributor

janekmi commented Jul 12, 2024

Outdated

  • Skip-list-master:

    • test_daos_rebuild_ec:DAOS-14982
    • test_daos_degraded_ec:DAOS-14982
    • test_ms_failover:DAOS-16103
  • Skip-list-release/2.6:

    • test_dfuse_daos_build_wb:DAOS-16168
    • test_osa_offline_reintegration_without_checksum:DAOS-15608
    • test_daos_extend_simple:DAOS-15640
  • master:

    • test_cart_rpc:DAOS-15989
  • release/2.6:

    • PASS

Updated skip lists:

Skip-list-master: test_daos_rebuild_ec:DAOS-14982 test_daos_degraded_ec:DAOS-14982 test_ms_failover:DAOS-16103 test_cart_rpc:DAOS-15989
Skip-list-release/2.6: test_dfuse_daos_build_wb:DAOS-16168 test_osa_offline_reintegration_without_checksum:DAOS-15608 test_daos_extend_simple:DAOS-15640

Skip-list-master: test_daos_rebuild_ec:DAOS-14982 test_daos_degraded_ec:DAOS-14982 test_ms_failover:DAOS-16103 test_cart_rpc:DAOS-15989
Skip-list-release/2.6: test_dfuse_daos_build_wb:DAOS-16168 test_osa_offline_reintegration_without_checksum:DAOS-15608 test_daos_extend_simple:DAOS-15640

Allow-unstable-test: true
Skip-func-hw-test-large: false

Priority: 2

Signed-off-by: Jan Michalski <[email protected]>
@janekmi
Copy link
Contributor

janekmi commented Jul 18, 2024

Up-to-date

  • Skip-list-master:

    • test_daos_rebuild_ec:DAOS-14982 (awaiting backport)
    • test_daos_degraded_ec:DAOS-14982 (awaiting backport)
    • test_ms_failover:DAOS-16103
    • test_cart_rpc:DAOS-15989
  • Skip-list-release/2.6:

    • test_dfuse_daos_build_wb:DAOS-16168 (resolved)
    • test_osa_offline_reintegration_without_checksum:DAOS-15608
    • test_daos_extend_simple:DAOS-15640 (resolved)
  • main:

    • DaosCoreTest:DAOS-16008 (26 failures)
  • release/2.6:

    • test_daos_single_rdg_tx:DAOS-14982

@janekmi
Copy link
Contributor

janekmi commented Jul 18, 2024

@brianjmurrell The same as here: daos-stack/pmdk#37 (comment). Now this PR is supposedly the most extensively tested PR in history. 🙃 I see no point in continuing this pursuit.

I think for now we are forced to apply the current "golden" standard and just accept that a not successful build accompanied by documentation explaining the causes of all the failures is sufficient to land the PR.

Hopefully, the landing build which excludes the "Test Hardware" stage will be green also for this landing.

Please consider landing just after the DAOS 2.6 GA.

@brianjmurrell
Copy link
Contributor

Hopefully, the landing build which excludes the "Test Hardware" stage will be green also for this landing.

The downstream test builds of dependencies always include the same stages, even on landing, unlike the landing runs of daos itself. This is because even ideally, daos landings would include hardware testing but due to their frequency, we just don't have the capacity to do that. Dependency landings are much less frequent so we can still afford to do the hardware testing there and trying to skip it is simply not worth the extra logic contortion to do it (and maintain it).

But yes, I suppose we are just going to have to cave and force-land broken PRs due to the testing of daos being so unstable. Unfortunately.

Please add @daos-stack/release-engineering to the reviewer list when you are ready for this to land.

@janekmi janekmi requested a review from a team August 6, 2024 14:56
@brianjmurrell brianjmurrell merged commit b7381a5 into master Aug 6, 2024
0 of 3 checks passed
@brianjmurrell brianjmurrell deleted the makito/DAOS-15079 branch August 6, 2024 16:00
@brianjmurrell
Copy link
Contributor

brianjmurrell commented Aug 7, 2024

@janekmi Please be sure to watch the downstream testing from the landing and delete any branches (i.e. from GitHub) it may have left behind due to test failures (non-failing downstream testing cleans up it's branches automatically, failed downstream testing does not).

You should also remove any left-over branches from the downstream testing from the PR (#61).

This is an unfortunate side-effect of landing PRs with known failures.

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

Successfully merging this pull request may close these issues.

5 participants