rgw/test/lua: add lua integration tests suite#52931
Conversation
|
teuthology passing: http://pulpito.front.sepia.ceph.com/yuvalif-2023-08-10_19:12:31-rgw:lua-wip-yuval-lua-teuthology-distro-default-smithi/ |
561a45a to
083a6eb
Compare
|
teuthology passing with "requests" test: http://pulpito.ceph.com/yuvalif-2023-08-13_17:02:46-rgw:lua-wip-yuval-lua-teuthology-distro-default-smithi/ |
|
jenkins test make check |
qa/tasks/lua_tests.py
Outdated
|
|
||
|
|
||
| @contextlib.contextmanager | ||
| def pre_process(ctx, config): |
There was a problem hiding this comment.
This function was created for AMQP, so I think it's not needed here
src/test/rgw/lua/test_lua.py
Outdated
|
|
||
|
|
||
| ##################### | ||
| # lau scripting tests |
src/test/rgw/lua/bootstrap
Outdated
There was a problem hiding this comment.
@cbodley can we add a symlink for this file as this is exactly same as we use it in bucket notifications?
083a6eb to
c896495
Compare
qa/tasks/lua_tests.py
Outdated
|
|
||
| with contextutil.nested( | ||
| lambda: download(ctx=ctx, config=config), | ||
| lambda: pre_process(ctx=ctx, config=config), |
c896495 to
894e3e3
Compare
src/test/rgw/lua/test_lua.py
Outdated
| return conn | ||
|
|
||
|
|
||
| def connection2(): |
There was a problem hiding this comment.
894e3e3 to
29e43e3
Compare
| parameters in your teuthology-suite command. Example command for this is as follows:: | ||
|
|
||
| teuthology-suite --ceph-repo https://github.com/ceph/ceph-ci.git -s rgw:lua --ceph your_ceph_branch_name -m smithi --suite-repo https://github.com/your_name/ceph.git --suite-branch your_branch_name | ||
|
|
There was a problem hiding this comment.
It'd be great if you can add an example here of how we can use extra_attr in the yaml file.
04a6154 to
595192c
Compare
|
teuthology is passing after latest cleanup commit: https://pulpito.ceph.com/yuvalif-2023-11-14_16:01:15-rgw:lua-wip-yuval-lua-teuthology-distro-default-smithi/ |
qa/tasks/lua_tests.py
Outdated
| attr = [] | ||
|
|
||
| if 'extra_attr' in client_config: | ||
| attr = client_config.get('extra_attr') | ||
|
|
||
| args = ['cd', '{tdir}/ceph/src/test/rgw/lua/'.format(tdir=testdir), run.Raw('&&'), | ||
| 'LUATESTS_CONF=./lua-tests.{client}.conf'.format(client=client), | ||
| 'tox', | ||
| './test_lua.py', | ||
| '-v'] |
There was a problem hiding this comment.
those final two args are for pytest rather than tox, so didn't get used in the teuthology run; you need to put them after a -- to forward them to pytest
attr above is unused, but you might want to use it for filtering the pytest markers
the invocation in qa/tasks/s3tests.py looks like this:
args += ['tox', '--', '-v', '-m', ' and '.join(attrs)]
cbodley
left a comment
There was a problem hiding this comment.
looks great, merge when you're satisfied with the teuthology results
the packaging changes probably need backports, but i'm not sure about the suite itself - what do you think?
|
the
|
c4df11e to
95cbb6a
Compare
it would probably be good to backport everything. in the future, when we fix bugs with lua we would probably add matching tests, so bug backports would be simpler if we backport the test suite |
Signed-off-by: Yuval Lifshitz <ylifshit@redhat.com>
95cbb6a to
eb21bc6
Compare
Signed-off-by: Yuval Lifshitz <ylifshit@redhat.com> Fixes: https://tracker.ceph.com/issues/63616
eb21bc6 to
46500ca
Compare
|
jenkins test api |
|
This broke all testing on RHEL: https://pulpito.ceph.com/rishabh-2023-11-28_13:00:50-fs-rishabh-2023nov26-testing-default-smithi/ "lua-devel" does not exist. |
|
we need the "codeready-builder" to be enabled for rhel8 in order to find that repo. |
|
This adds The If this is only used for test suites, isn't there a better option for these dependencies? E.g., from top of my head the following might be a better fit (depending on how this is used exactly):
|
adding lua-devel and luarocks is not for test suite.
|
|
Thanks for your reply.
Just to be sure: Is this a hard requirement on running ceph (client related) stuff or does it simply fail if one uploads a script without the Lua stack being available? If it is a hard requirement then I just have to ask if requiring Lua, including a whole c/cpp development stack that it pulls in by default, for every ceph client user isn't a bit overkill?
For Debian it's really FWIW, we exposed Ceph Squid on our test repos and users immediately noticed and increased package installations and compiler does mean bigger exposure of systems, so this all is stemming from real user concern, not just pure cosmetic ones; just to give you some context on why I bother with my questions. |
|
@ThomasLamprecht in the RPM world (e.g. https://rhel.pkgs.org/8/okey-x86_64/lua-devel-5.3.4-10.el8.x86_64.rpm.html) I see that it also pulls in but regardless, for the RGW it is a hard requirement, the code won't compile without the lua header and won't load without the lua .so runtime. |
That part would be fine, as Debian can have this as build-dependency but not as (runtime) dependency,
bummer, that isn't ideal for us, FWIW we probably could provide a no-op .so and document how to disable that if one really requires Lua scripted RGW response hooks.
We provide a more general distribution of Ceph, and while most of our users really do not need the RGW some still do, and we do not want to block them out completely.
That might be good to have for us as stop-gap. Out of interest, what are some common use cases for Lua scripting in RGW? And could dynamically loading the Lua library only if required be another alternative here? I did not check how this feature is implemented, so I might miss some rather obvious way this cannot work, but I imagine something along the lines of manually using dlopen and friends only if there is a Lua script is configured might allow having this functionality while not requiring a hard-dependency on the Lua related packages and all they pull in. |
this would be the simplest to implement
we don't do that for other parts of the code, but it sounds possible |
@ThomasLamprecht those rgw python bindings only cover the file-based rgw-nfs interface, which most rgw users won't need. dropping that from the ceph-common package may be the simplest solution |
|
Ah, thanks for that pointer, this was indeed an option that we wanted to evaluate as one of the short-term solutions for us here, so your comment saved us a bit of investigation time. Ps. a colleague pointed out that the Further, it seems that luarock is not called if there isn't an allowlist at all, and if there is one gets an explicit error message logged if luarocks is not available, so this package dependency could be downgraded to a |
One can attach lua scripts as sort of hooks to implement dynamic checks or transformations of RGW requests since Ceph Pacific. Thus, a lua library is now required for base support and optionally one can use the luarocks deployment and management system for Lua modules to use more advanced scripts/modules. With commit 46500ca ("rgw/test/lua: add lua integration tests suite") the dependency relations where cleaned up, as the respective entries where missing completely from debian/control. But that commit is pulling in much more than required due to adding the devel package `liblua5.3-dev` instead of the library-only `liblua5.3-0` one, and having `luarocks` as hard dependency compared to an optional Suggests. Fixing that avoids pulling in a whole build/compiler/autotools/... stack with 65 new packages just when one wants to use librgw2 or python3-rgw for simple RGW requests, or just needs the ceph-common package, which pulls in librgw2 transitively. This was reported by prolific community member Neobin on the Proxmox forum [0], and then discussed on the original PR, adding the dependencies [1]. [0]: https://forum.proxmox.com/threads/156433/post-715148 [1]: ceph#52931 (comment) Fixes: https://tracker.ceph.com/issues/68873 Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
One can attach lua scripts as sort of hooks to implement dynamic checks or transformations of RGW requests since Ceph Pacific. Thus, a lua library is now required for base support and optionally one can use the luarocks deployment and management system for Lua modules to use more advanced scripts/modules. With commit 46500ca ("rgw/test/lua: add lua integration tests suite") the dependency relations got cleaned up, as the respective entries were missing completely from debian/control. But that commit is pulling in much more than required due to adding the devel package `liblua5.3-dev` instead of the library-only `liblua5.3-0` one, and having `luarocks` as hard dependency compared to an optional Suggests. Fixing that avoids pulling in a whole build/compiler/autotools/... stack with 65 new packages just when one wants to use librgw2 or python3-rgw for simple RGW requests, or just needs the ceph-common package, which pulls in librgw2 transitively. This was reported by prolific community member Neobin on the Proxmox forum [0], and then discussed on the original PR, adding the dependencies [1]. [0]: https://forum.proxmox.com/threads/156433/post-715148 [1]: ceph#52931 (comment) Fixes: https://tracker.ceph.com/issues/68873 Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
One can attach lua scripts as sort of hooks to implement dynamic checks or transformations of RGW requests since Ceph Pacific. Thus, a lua library is now required for base support and optionally one can use the luarocks deployment and management system for Lua modules to use more advanced scripts/modules. With commit 46500ca ("rgw/test/lua: add lua integration tests suite") the dependency relations got cleaned up, as the respective entries were missing completely from debian/control. But that commit is pulling in much more than required due to adding the devel package `liblua5.3-dev` instead of the library-only `liblua5.3-0` one, and having `luarocks` as hard dependency compared to an optional Suggests. Fixing that avoids pulling in a whole build/compiler/autotools/... stack with 65 new packages just when one wants to use librgw2 or python3-rgw for simple RGW requests, or just needs the ceph-common package, which pulls in librgw2 transitively. This was reported by prolific community member Neobin on the Proxmox forum [0], and then discussed on the original PR, adding the dependencies [1]. [0]: https://forum.proxmox.com/threads/156433/post-715148 [1]: ceph/ceph#52931 (comment) Fixes: https://tracker.ceph.com/issues/68873 Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
One can attach lua scripts as sort of hooks to implement dynamic checks or transformations of RGW requests since Ceph Pacific. Thus, a lua library is now required for base support and optionally one can use the luarocks deployment and management system for Lua modules to use more advanced scripts/modules. With commit 46500ca ("rgw/test/lua: add lua integration tests suite") the dependency relations got cleaned up, as the respective entries were missing completely from debian/control. But that commit is pulling in much more than required due to adding the devel package `liblua5.3-dev` instead of the library-only `liblua5.3-0` one, and having `luarocks` as hard dependency compared to an optional Suggests. Fixing that avoids pulling in a whole build/compiler/autotools/... stack with 65 new packages just when one wants to use librgw2 or python3-rgw for simple RGW requests, or just needs the ceph-common package, which pulls in librgw2 transitively. This was reported by prolific community member Neobin on the Proxmox forum [0], and then discussed on the original PR, adding the dependencies [1]. [0]: https://forum.proxmox.com/threads/156433/post-715148 [1]: ceph#52931 (comment) Fixes: https://tracker.ceph.com/issues/68873 Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Checklist
Show available Jenkins commands
jenkins retest this pleasejenkins test classic perfjenkins test crimson perfjenkins test signedjenkins test make checkjenkins test make check arm64jenkins test submodulesjenkins test dashboardjenkins test dashboard cephadmjenkins test apijenkins test docsjenkins render docsjenkins test ceph-volume alljenkins test ceph-volume toxjenkins test windows