spdk: add a new package by hyoklee · Pull Request #35520 · spack/spack

hyoklee · 2023-02-16T18:22:43Z

The Storage Performance Development Kit (SPDK) provides a set of tools
and libraries for writing high performance, scalable, user-mode storage
applications.

tldahlgren

Confirmed the branch and 14 tags exist. Have a quick question about a possible conflict.

tldahlgren · 2023-02-16T20:14:36Z

var/spack/repos/builtin/packages/spdk/package.py

+    variant("vhost", default=False, description="Build vhost target")
+    variant(
+        "virtio", default=False, description="Build vhost initiator and virtio-pci bdev modules"
+    )


Given the description, should there be a conflict if ~vhost+virtio?

@tldahlgren , that's a good catch!
I thought it will fail but test passed with ~vhost+virtio:

https://github.com/hyoklee/spack/actions/runs/4205727843/

@soumagne wrote the initial code for this package.
Thus, I'd like to defer to @soumagne 's opinion.

@mchaarawi, can you answer my question since @soumagne is not responding?

i am not familiar with this. you better ask the spdk community: https://spdk.io/community/

Thanks, @mchaarawi !

@hyoklee sorry I had missed your earlier question. Same comment here as for the other packages, you want to remove the previous versions first and clean up the options. This package is particularly problematic because earlier versions were lacking install rules etc. The options passed here are based on what daos uses and some of these options only exist in specific spdk versions.

tldahlgren · 2023-02-21T20:26:24Z

Looks like there's an issue with a dependency:

PKG-DIRECTIVES: 1 issue found
[13](https://github.com/spack/spack/actions/runs/4197103773/jobs/7278991698#step:6:14)
1. spdk: dependency on [email protected] cannot be satisfied by known versions of fio
[14](https://github.com/spack/spack/actions/runs/4197103773/jobs/7278991698#step:6:15)
    happening in /home/runner/work/spack/spack/var/spack/repos/builtin/packages/spdk/package.py
[15](https://github.com/spack/spack/actions/runs/4197103773/jobs/7278991698#step:6:16)
    known versions of fio are 3.26, 3.25, 3.19, 3.16, 2.19

hyoklee · 2023-02-21T20:30:53Z

Looks like there's an issue with a dependency:

PKG-DIRECTIVES: 1 issue found
[13](https://github.com/spack/spack/actions/runs/4197103773/jobs/7278991698#step:6:14)
1. spdk: dependency on [email protected] cannot be satisfied by known versions of fio
[14](https://github.com/spack/spack/actions/runs/4197103773/jobs/7278991698#step:6:15)
    happening in /home/runner/work/spack/spack/var/spack/repos/builtin/packages/spdk/package.py
[15](https://github.com/spack/spack/actions/runs/4197103773/jobs/7278991698#step:6:16)
    known versions of fio are 3.26, 3.25, 3.19, 3.16, 2.19

@tldahlgren , I think Action ran before 9f3f4b3 is merged.

@spackbot run pipeline

spackbot-app · 2023-02-21T20:30:55Z

I'm sorry, gitlab does not have your latest revision yet, I can't run that pipeline for you right now.

One likely possibility is that your PR pipeline has been temporarily deferred, in which case, it is awaiting a develop pipeline, and will be run when that finishes.

Please check the gitlab commit status message to see if more information is available.

Details

Unexpected response from gitlab: {'message': '404 Commit Not Found'}

hyoklee · 2023-02-23T17:02:48Z

See #35647 for dpdk dependency error.

soumagne · 2023-03-03T22:01:49Z

var/spack/repos/builtin/packages/spdk/package.py

+        "vtune",
+    )
+
+    depends_on("[email protected]:")


are you using an external dpdk in that case also ? where is it specified in the command line ?

Yes. See DPDK/dpdk#58

soumagne · 2023-03-03T22:05:54Z

var/spack/repos/builtin/packages/spdk/package.py

+            if os.path.isfile(join_path("dpdk", "build", "lib", file)):
+                install(join_path("dpdk", "build", "lib", file), prefix.lib)
+        mkdir(join_path(prefix.include, "dpdk"))
+        install_tree("dpdk/build/include", join_path(prefix.include, "dpdk"))


this part was previously added to install the dpdk module from spdk. So maybe you should make a variant, something like +dpdk that installs all those files as an option ? This way you don't need the dependency on dpdk anymore and daos should be happy with it. You might want to also add a conflict with dpdk if the variant is enabled (just in case someone is using the dpdk package somewhere else).

Thanks again for the review. So, I can delete those lines if I use +dpdk variant. Did I understand your comment correctly?

var/spack/repos/builtin/packages/spdk/package.py

hyoklee · 2023-03-16T16:58:34Z

@tldahlgren would you please merge this?

tldahlgren · 2023-03-16T17:03:37Z

@tldahlgren would you please merge this?

It needs to pass the required CI check. Let me see what I can do.

hyoklee · 2023-03-16T19:35:38Z

I understand but that's not correct. spdk includes its own dpdk as a submodule which is not the version that you are including and daos then uses that same dpdk, not an external one. To be more accurate, the version spdk is using internally is: v21.02-4891-g744e58cca9. By adding an external dpdk into the loop you're risking to use something that's unstable and not tested as daos might pick those headers instead of the ones from spdk. Can you please point me to the exact error that you see? I have never needed to include the dpdk dependency in the past and everything has worked fine so I'm not sure why you are seeing errors. In any case, like I said, the dpdk package should be marked as a conflict and not as a dependency.

DAOS doesn't link dpdk properly if it tires to use spdk-built dpdk. I think it's the nature of Spack Scons package that daos uses. It's similar to failure that Spack-installed hwloc is not being picked up by DAOS.
If you can show me a working GitHub Action that works on Ubuntu-latest with what you suggested, I'll make a patch accordingly.

hyoklee · 2023-03-29T14:45:53Z

@tldahlgren , would you please merge this package since all checks passed? I don't see any further comment from @soumagne .

soumagne · 2023-03-29T15:25:01Z

@hyoklee unfortunately it will be difficult for me to find sufficient time to set GitHub actions and work on that issue. The package as it is right now is incorrect because you are adding a dependency on the wrong dpdk version. I asked you to show me the error that you were getting so that I could potentially help you in fixing it but I have not seen that from you yet. If you want to merge it as is that's fine but don't be surprised if you end up with random issues, I won't be able to help in that case.

hyoklee · 2023-03-29T15:41:55Z

@soumagne , here's the error output:

==> Error: OSError: No such file or directory: '/tmp/runner/spack-stage/spack-stage-spdk-23.01-hjoqrt4ggjfmq6p6crhmwt33dgb4x257/spack-src/dpdk/build/lib/pkgconfig'
[793](https://github.com/hyoklee/daos/actions/runs/4367166850/jobs/7638132171#step:5:794)

[794](https://github.com/hyoklee/daos/actions/runs/4367166850/jobs/7638132171#step:5:795)
/home/runner/work/daos/daos/spack/var/spack/repos/builtin/packages/spdk/package.py:90, in install_additional_files:
[795](https://github.com/hyoklee/daos/actions/runs/4367166850/jobs/7638132171#step:5:796)
         87        prefix = self.prefix
[796](https://github.com/hyoklee/daos/actions/runs/4367166850/jobs/7638132171#step:5:797)
         88
[797](https://github.com/hyoklee/daos/actions/runs/4367166850/jobs/7638132171#step:5:798)
         89        dpdk_build_dir = join_path(self.stage.source_path, "dpdk", "build", "lib")
[798](https://github.com/hyoklee/daos/actions/runs/4367166850/jobs/7638132171#step:5:799)
  >>     90        install_tree(join_path(dpdk_build_dir, "pkgconfig"), join_path(prefix.lib, "pkgconfig"))
[799](https://github.com/hyoklee/daos/actions/runs/4367166850/jobs/7638132171#step:5:800)
         91        for file in os.listdir(dpdk_build_dir):
[800](https://github.com/hyoklee/daos/actions/runs/4367166850/jobs/7638132171#step:5:801)
         92            if os.path.isfile(join_path("dpdk", "build", "lib", file)):
[801](https://github.com/hyoklee/daos/actions/runs/4367166850/jobs/7638132171#step:5:802)
         93                install(join_path("dpdk", "build", "lib", file), prefix.lib)
[802](https://github.com/hyoklee/daos/actions/runs/4367166850/jobs/7638132171#step:5:803)

soumagne · 2023-03-29T16:46:56Z

I see thanks @hyoklee let me have a look, we should be able to fix that.

soumagne

@hyoklee please have a look at the changes I made. I would expect that would allow you to build this time, while conforming to the expected versions. You will have to add the 21.11.1 version of dpdk since it's not there.

soumagne · 2023-03-29T18:49:57Z

var/spack/repos/builtin/packages/spdk/package.py

+    maintainers("hyoklee")
+
+    version("master", branch="master", submodules=True)
+    version("23.01", tag="v23.01", submodules=True)


This is the version used by DAOS 2.2, see: https://github.com/daos-stack/daos/blob/v2.2.0/utils/build.config

Suggested change

version("23.01", tag="v23.01", submodules=True)

version("22.01.1", tag="v22.01.1", submodules=True)

var/spack/repos/builtin/packages/spdk/package.py

soumagne · 2023-03-29T19:04:09Z

var/spack/repos/builtin/packages/spdk/package.py

+    def install_dpdk_files(self):
+        spec = self.spec
+        prefix = self.prefix
+
+        dpdk_build_dir = join_path(spec["dpdk"].prefix, "lib")
+
+        install_tree(join_path(dpdk_build_dir, "pkgconfig"), join_path(prefix.lib, "pkgconfig"))
+
+        for file in os.listdir(dpdk_build_dir):
+            f = join_path(dpdk_build_dir, file)
+            if os.path.isfile(f):
+                install(f, prefix.lib)
+
+        mkdir(join_path(prefix.include, "dpdk"))
+        install_tree(join_path(spec["dpdk"].prefix, "include"), join_path(prefix.include, "dpdk"))
+
+    @run_after("install")
+    def install_additional_files(self):
+        spec = self.spec
+        prefix = self.prefix
+        if "+dpdk" in spec:
+            self.install_dpdk_files()
+        # Copy the config.h file, as some packages might require it.
+        mkdir(prefix.share)
+        mkdir(join_path(prefix.share, "spdk"))
+        install_tree("examples/nvme/fio_plugin", join_path(prefix.share, "spdk", "fio_plugin"))
+        install_tree("include", join_path(prefix.share, "spdk", "include"))
+        install_tree("scripts", join_path(prefix.share, "spdk", "scripts"))


I think we can remove all these if we do the dpdk dependency method, this way there won't be any conflict and that should allow you to build. You'll just need to track the dpdk version used whenever you add a newer version of spdk.

Suggested change

def install_dpdk_files(self):

spec = self.spec

prefix = self.prefix

dpdk_build_dir = join_path(spec["dpdk"].prefix, "lib")

install_tree(join_path(dpdk_build_dir, "pkgconfig"), join_path(prefix.lib, "pkgconfig"))

for file in os.listdir(dpdk_build_dir):

f = join_path(dpdk_build_dir, file)

if os.path.isfile(f):

install(f, prefix.lib)

mkdir(join_path(prefix.include, "dpdk"))

install_tree(join_path(spec["dpdk"].prefix, "include"), join_path(prefix.include, "dpdk"))

@run_after("install")

def install_additional_files(self):

spec = self.spec

prefix = self.prefix

if "+dpdk" in spec:

self.install_dpdk_files()

# Copy the config.h file, as some packages might require it.

mkdir(prefix.share)

mkdir(join_path(prefix.share, "spdk"))

install_tree("examples/nvme/fio_plugin", join_path(prefix.share, "spdk", "fio_plugin"))

install_tree("include", join_path(prefix.share, "spdk", "include"))

install_tree("scripts", join_path(prefix.share, "spdk", "scripts"))

var/spack/repos/builtin/packages/spdk/package.py

hyoklee · 2023-04-05T17:37:05Z

@soumagne , do you have any patch for the errors that I reported?
If not, let's just merge as is.

soumagne · 2023-04-05T17:47:18Z

yes I explicitly made suggestions to fix the package and address the errors. Did you not see them ? Have you included those and addressed my comments?

hyoklee · 2023-04-05T17:49:29Z

yes I explicitly made suggestions to fix the package and address the errors. Did you not see them ? Have you included those and addressed my comments?

I applied them and they did not work. Please see the inline replies that I've already made.

soumagne · 2023-04-05T17:50:51Z

which inline replies ? I do not see anything.

hyoklee · 2023-04-05T17:52:01Z

which inline replies ? I do not see anything.

Sorry, they were pending. I submitted them and I think you can see them now.

hyoklee · 2023-04-05T17:54:26Z

It's weird. My comment is still "Pending"

Both gcc-11 & 12 fail to compile spdk 22.01.1:

2023-03-30T14:58:04.3254028Z   >> 352    env_internal.h:64:41: error: field 'driver' has incomplete type
2023-03-30T14:58:04.3254430Z      353       64 |         struct rte_pci_driver           driver;
2023-03-30T14:58:04.3254741Z      354          |                                         ^~~~~~
2023-03-30T14:58:04.3255237Z   >> 355    make[2]: *** [/tmp/runner/spack-stage/spack-stage-spdk-22.01.1-dpz5

soumagne · 2023-04-05T17:54:33Z

I still cannot see anything except that you marked some of the comments as resolved although they are not.

soumagne · 2023-04-05T17:55:56Z

ok I'll have a look at that compile error.

hyoklee · 2023-04-05T17:58:12Z

Here are more pending comments:

hyoklee

See comments.

hyoklee · 2023-03-03T22:10:38Z

var/spack/repos/builtin/packages/spdk/package.py

+        "vtune",
+    )
+
+    depends_on("[email protected]:")


Yes. See DPDK/dpdk#58

hyoklee · 2023-03-07T14:13:47Z

var/spack/repos/builtin/packages/spdk/package.py

+            if os.path.isfile(join_path("dpdk", "build", "lib", file)):
+                install(join_path("dpdk", "build", "lib", file), prefix.lib)
+        mkdir(join_path(prefix.include, "dpdk"))
+        install_tree("dpdk/build/include", join_path(prefix.include, "dpdk"))


Thanks again for the review. So, I can delete those lines if I use +dpdk variant. Did I understand your comment correctly?

var/spack/repos/builtin/packages/spdk/package.py

hyoklee · 2023-04-22T16:13:59Z

@soumagne , any progress?

soumagne · 2023-04-24T14:55:24Z

no sorry I haven't been able to get to that yet. In the meantime I would suggest you just delete the part that installs the additional files, that should not be needed.

hyoklee · 2023-04-26T23:14:00Z

@tldahlgren , would you please merge this PR?

hyoklee · 2023-04-27T02:48:48Z

Thank you so much, @tldahlgren and @soumagne !

@soumagne

* spdk: add a new package * chore: fix formatting and style * fix: add rdma-core dependency * fix: remove spdk < 23.01 versions per @soumagne review * spdk: add 22.01.1 version * spdk: address @soumagne reviews * spdk: fix fio audit failure * spdk: fix fio version and remove debugging info

hyoklee added 2 commits February 16, 2023 12:21

spdk: add a new package

4585c36

chore: fix formatting

b6edaaa

spackbot-app bot added dependencies new-package new-variant new-version labels Feb 16, 2023

chore: fix style

d0891bd

hyoklee mentioned this pull request Feb 16, 2023

fio: add a new version #35522

Merged

tldahlgren reviewed Feb 16, 2023

View reviewed changes

tldahlgren self-assigned this Feb 16, 2023

fix: add rdma-core dependency

28149c7

hyoklee added 4 commits February 23, 2023 11:09

chore: fix style

49640a6

fix: remove spdk < 23.01 versions per @soumagne review

b2316a2

chore: fix style

485ce91

spdk: add 22.01.1 version

cfb878e

soumagne reviewed Mar 3, 2023

View reviewed changes

hyoklee added 4 commits March 8, 2023 10:33

spdk: address @soumagne review

62a90a3

spdk: fix fio audit failure

3dbe298

spdk: address @soumagne review

b6a53e3

spdk: fix fio version and remove debugging info

d1592e4

soumagne reviewed Mar 10, 2023

View reviewed changes

var/spack/repos/builtin/packages/spdk/package.py Show resolved Hide resolved

soumagne reviewed Mar 10, 2023

View reviewed changes

var/spack/repos/builtin/packages/spdk/package.py Outdated Show resolved Hide resolved

spdk: address @soumagne review

ddc6381

soumagne suggested changes Mar 29, 2023

View reviewed changes

soumagne reviewed Mar 29, 2023

View reviewed changes

var/spack/repos/builtin/packages/spdk/package.py Show resolved Hide resolved

hyoklee closed this Apr 5, 2023

hyoklee reopened this Apr 5, 2023

hyoklee commented Apr 5, 2023

View reviewed changes

fix(spdk): address @soumagne review

c222faf

hyoklee dismissed tldahlgren’s stale review via c222faf April 26, 2023 02:24

soumagne approved these changes Apr 26, 2023

View reviewed changes

tldahlgren approved these changes Apr 26, 2023

View reviewed changes

tldahlgren merged commit 0d7890b into spack:develop Apr 26, 2023

hyoklee deleted the hyoklee-spdk branch April 27, 2023 02:48

	version("23.01", tag="v23.01", submodules=True)
	version("22.01.1", tag="v22.01.1", submodules=True)

Conversation

hyoklee commented Feb 16, 2023

Uh oh!

tldahlgren left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tldahlgren commented Feb 21, 2023

Uh oh!

hyoklee commented Feb 21, 2023

Uh oh!

spackbot-app bot commented Feb 21, 2023

Uh oh!

hyoklee commented Feb 23, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hyoklee commented Mar 16, 2023

Uh oh!

tldahlgren commented Mar 16, 2023

Uh oh!

hyoklee commented Mar 16, 2023

Uh oh!

hyoklee commented Mar 29, 2023

Uh oh!

soumagne commented Mar 29, 2023

Uh oh!

hyoklee commented Mar 29, 2023

Uh oh!

soumagne commented Mar 29, 2023

Uh oh!

soumagne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hyoklee commented Apr 5, 2023

Uh oh!

soumagne commented Apr 5, 2023

Uh oh!

hyoklee commented Apr 5, 2023

Uh oh!

soumagne commented Apr 5, 2023

Uh oh!

hyoklee commented Apr 5, 2023

Uh oh!

hyoklee commented Apr 5, 2023

Uh oh!

soumagne commented Apr 5, 2023

Uh oh!

soumagne commented Apr 5, 2023

Uh oh!

hyoklee commented Apr 5, 2023

Uh oh!

hyoklee left a comment

Choose a reason for hiding this comment

Uh oh!