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

Lua object class support #7338

Merged
merged 2 commits into from Sep 22, 2016
Merged

Lua object class support #7338

merged 2 commits into from Sep 22, 2016

Conversation

@dotnwat
Copy link
Member

@dotnwat dotnwat commented Jan 24, 2016

No description provided.

@yuyuyu101
Copy link
Member

@yuyuyu101 yuyuyu101 commented Jan 24, 2016

COOL!

Not sure we need to contains lua source in ceph repo. Specify a external directory may better?

@cxwshawn
Copy link
Contributor

@cxwshawn cxwshawn commented Jan 24, 2016

cool! looking forward to being merged !

@liewegas
Copy link
Member

@liewegas liewegas commented Jan 25, 2016

Does it makes sense to use a submodule instead of an extracted tarball for lua?

@dotnwat
Copy link
Member Author

@dotnwat dotnwat commented Jan 26, 2016

@yuyuyu101 @liewegas I have no issue with Lua being in a submodule (what's the motivation?) The only important thing is that we control that submodule because we may want to customize the VM in the future, but I'm assuming I target a ceph/lua repo?

@liewegas
Copy link
Member

@liewegas liewegas commented Jan 26, 2016

@dotnwat
Copy link
Member Author

@dotnwat dotnwat commented Jan 26, 2016

sounds good. i'll need to have someone create ceph/lua repo.. looks like I don't have permission.

@liewegas
Copy link
Member

@liewegas liewegas commented Jan 26, 2016

@dotnwat
Copy link
Member Author

@dotnwat dotnwat commented Jan 27, 2016

Thanks. I need push access to that repo. It isn't a vanilla Lua distribution, and its build configuration has been modified to assume cmake, so we'll need to hack it a bit to play nice with automake too.

@batrick
Copy link
Member

@batrick batrick commented Jan 28, 2016

Hi @noahdesu, this is an exciting feature! I'm still looking through the code and have yet to try this out but I was wondering why you include Roberto's Lua struct module when Lua 5.3 now has a pack function (which is loosely based on struct).

@dotnwat
Copy link
Member Author

@dotnwat dotnwat commented Jan 28, 2016

Hey @batrick thanks for pointing that out! I didn't realize that Lua 5.3 had this function. I wanted to include some useful serialization modules out-of-the-box so people could immediately start using this. It sounds like it might be OK to nuke the struct module for now. Let me know if you have any other questions or issues using cls_lua.

target_include_directories(cephlua
PUBLIC lua/lua-5.3.2/src/
)

This comment has been minimized.

@cbodley

cbodley Jan 28, 2016
Contributor

could we move the cmake bits into a lua/CMakeLists.txt please?

This comment has been minimized.

@dotnwat

dotnwat Jan 28, 2016
Author Member

Absolutely. I'm in the process of stuffing Lua into a git submodule and I'll add this to the todo list.

@dotnwat
Copy link
Member Author

@dotnwat dotnwat commented Jan 30, 2016

@liewegas mind adding me to ceph/lua so I can push there?

@yehudasa
Copy link
Member

@yehudasa yehudasa commented Jan 30, 2016

@noahdesu I updated the permissions there

@dotnwat dotnwat force-pushed the cls-lua branch from f4a9527 to 0ee49be Feb 1, 2016
@dotnwat
Copy link
Member Author

@dotnwat dotnwat commented Feb 1, 2016

Thanks @yehudasa that worked. @liewegas ok got it all submodule-ified. @cbodley could you take a look at the new cmake files?

@cbodley
Copy link
Contributor

@cbodley cbodley commented Feb 1, 2016

@noahdesu the cmake looks good, thanks. i'll check it out and make sure it builds for me too

@dotnwat dotnwat force-pushed the cls-lua branch from 0ee49be to a375f00 Feb 7, 2016
@dotnwat
Copy link
Member Author

@dotnwat dotnwat commented Feb 7, 2016

Rebased to remove merge recent merge conflicts.

@dotnwat dotnwat force-pushed the cls-lua branch from a375f00 to 628ffda Feb 12, 2016
cls_method_context_t hctx = clslua_get_hctx(L);
const char *key = luaL_checkstring(L, 1);
bufferlist *bl = clslua_pushbufferlist(L, NULL);
int ret = cls_cxx_map_get_val(hctx, key, bl);

This comment has been minimized.

@batrick

batrick Feb 14, 2016
Member

I believe ret needs to be negated on error right? In my tests, it returns a negative value which gives us:

Unknown error -2

in the logs.

This comment has been minimized.

@dotnwat

dotnwat Feb 14, 2016
Author Member

Do you mean negated to resolve to the correct errno string via strerror, or negated to change the logic?

This comment has been minimized.

@batrick

batrick Feb 14, 2016
Member

To get the correct errno string I believe.

This comment has been minimized.

@dotnwat

dotnwat Feb 15, 2016
Author Member

Thanks for catching this Patrick. I pushed a fix for it that seems to work (note that I also rebased to fix a merge conflict in master) 50739d7

@athanatos
Copy link
Contributor

@athanatos athanatos commented May 4, 2016

There was some discussion of this at one of the recent CDM's. I think one of the main concerns was that we need a mechanism to whitelist classes for users?

@dotnwat dotnwat force-pushed the cls-lua branch from 3f14c5f to 705a474 May 28, 2016
@dotnwat dotnwat force-pushed the cls-lua branch 2 times, most recently from 223aa95 to 91c7c43 Jun 26, 2016
@dotnwat
Copy link
Member Author

@dotnwat dotnwat commented Jun 27, 2016

@athanatos yes. i just created PR #9972 for review, that adds the loading and caps whitelist changes for object classes.

@batrick
Copy link
Member

@batrick batrick commented Aug 4, 2016

Does RHEL7 have Lua 5.3? Optionally building against the system Lua 5.3 sounds reasonable to me.

Also, Lua's source is pretty small. On my humble laptop a parallel make builds in 2.5 seconds.

@dotnwat
Copy link
Member Author

@dotnwat dotnwat commented Aug 4, 2016

I'm not opposed to linking against a system library, but it seems the precedent is to avoid this when embedding the VM mostly due to compatibility and customization issues. For instance Redis disables byte-code execution with a patch to the VM source, and other things are possible like switching out the native numeric data type.

From the Lua documentation: "Different versions are really different. The API is likely to be a little different (but with compatibility switches), and there is no ABI compatibility: applications that embed Lua and C libraries for Lua must be recompiled. The virtual machine is also very likely to be different in a new version: Lua programs that have been precompiled for one version will not load in a different version."

That might be manageable when creating releases, but strikes me as a headache waiting to happen especially for people mixing Lua versions. Thoughts?

@ktdreyer
Copy link
Member

@ktdreyer ktdreyer commented Aug 4, 2016

There are pros and cons to bundling, and I wanted to at least have the discussion so I understand why we're doing it in each case. As long as the team is ok maintaining it when security fixes come up, I'm ok with it.

@dotnwat
Copy link
Member Author

@dotnwat dotnwat commented Aug 4, 2016

Sounds good. I'm happy to do any tests or research on the issue for everyone to agree on an approach.

@dotnwat dotnwat force-pushed the cls-lua branch from abc439f to 7e3458c Aug 8, 2016
@dotnwat
Copy link
Member Author

@dotnwat dotnwat commented Aug 9, 2016

the libgmock_main.so problem was caused by some cmake stuff that the lua submodule was doing that was causing various other libraries to be built as shared libraries rather than the expected static variety.

@yuriw anyway, let's give this a shot again. i was able to install from packages on deb/rhel and things are working now.

@dotnwat dotnwat added the needs-qa label Aug 9, 2016
@tchaikov
Copy link
Contributor

@tchaikov tchaikov commented Aug 9, 2016

the libgmock_main.so problem was caused by some cmake stuff that the lua submodule was doing that was causing various other libraries to be built as shared libraries rather than the expected static variety.

@noahdesu in case you didn't find it, see https://github.com/ceph/lua/blob/lua-5.1/cmake/dist.cmake#L77 . i suffered from this when working with gmock also. we need to reset this variable after including the lua subdirectory, or save and restore it before and after including the lua subdirectory.

@dotnwat
Copy link
Member Author

@dotnwat dotnwat commented Aug 9, 2016

@tchaikov nice thanks for the reference, that was a tough one to track down. is there any reason to patch the lua variable? when i was reading through dist.cmake i didn't see anything in there we care about. it was packaging and installation stuff.

@tchaikov
Copy link
Contributor

@tchaikov tchaikov commented Aug 9, 2016

Noah, see https://cmake.org/cmake/help/v3.0/variable/BUILD_SHARED_LIBS.html . this variable changes the default behavior of add_library(). and as we are including lua, not building it as a separated module, this variable actually impacts the whole project, including the gmock, which is also included by us.

@dotnwat dotnwat removed the needs-qa label Aug 9, 2016
@dotnwat
Copy link
Member Author

@dotnwat dotnwat commented Aug 9, 2016

@tchaikov cool thanks. i'll get that fixed up tomorrow.

@dotnwat
Copy link
Member Author

@dotnwat dotnwat commented Aug 9, 2016

@tchaikov I think I misread your comment. I understand the global impact of the BUILD_SHARED_LIBS variable. I was asking about patching it indirectly: that variable is set in lua/cmake/dist.cmake which contains stuff related to installation and packaging---cmake boilerplate that we do not depend on, including shared library building which doesn't matter since we build lua as a static library. So I disabled the entire inclusion of lua/cmake/dist.cmake, including the BUILD_SHARED_LIBS global variable. This file also sets numerous cmake variables, so we don't need to worry about them clobbering other settings either.

@dotnwat dotnwat added the needs-qa label Aug 9, 2016
@batrick
Copy link
Member

@batrick batrick commented Aug 9, 2016

@noahdesu, have you seen the thread on ceph-devel concerning Lua packaging? I'd link to it but gmane appears down (maybe permanently).

In particular, I'm concerned about including Ceph libraries in the lua submodule. Can those be moved src/cls/lua?

@dotnwat dotnwat removed the needs-qa label Aug 9, 2016
@dotnwat
Copy link
Member Author

@dotnwat dotnwat commented Aug 9, 2016

@batrick thanks for the pointer, i'll chime in on the list. about your question here: you are referring to cmsgpack and the bufferlist bindings? absolutely, they can be moved to src/cls/lua. lua was originally moved to a submodule and the reason people wanted that was that it 1) screwed up LOC stats and 2) ostensibly was easier to manage. Since I didn't author cmsgpack it seemed more appropriate there, but its easy to move. The reasoning behind leaving bufferlist bindings there was because it was general purpose.

@tchaikov
Copy link
Contributor

@tchaikov tchaikov commented Aug 10, 2016

I was asking about patching it indirectly: that variable is set in lua/cmake/dist.cmake which contains stuff related to installation and packaging---cmake boilerplate that we do not depend on, including shared library building which doesn't matter since we build lua as a static library. So I disabled the entire inclusion of lua/cmake/dist.cmake, including the BUILD_SHARED_LIBS global variable.

@noahdesu sorry, i failed to understand your question. ic. i thought a less intrusive way would be easier for maintenance. but when i am looking at https://github.com/ceph/lua/commits/lua-5.3-ceph: well, we have already added a handful of patches on top of lua-5.3 branch, yet another one won't hurt. =)

@michaelsevilla
Copy link
Contributor

@michaelsevilla michaelsevilla commented Sep 2, 2016

What is the status on this?

@dotnwat
Copy link
Member Author

@dotnwat dotnwat commented Sep 2, 2016

I need to take another look at the thread on ceph-devel to make sure nothing is missing, but I think we just need to slap needs-qa back on this.

dotnwat added 2 commits Jan 31, 2016
Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
Introduces cls_lua that allows object classes to be created dynamically
using the Lua language. Each request is processed in an empty Lua VM
instance, and scripts can be submitted using bufferlist or JSON
encoding.

Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
@dotnwat dotnwat force-pushed the cls-lua branch from 8aa7d90 to cb33c5c Sep 13, 2016
@dotnwat dotnwat added the needs-qa label Sep 14, 2016
@yuriw yuriw merged commit ab42f60 into master Sep 22, 2016
2 checks passed
2 checks passed
Signed-off-by all commits in this PR are signed
Details
default Build finished.
Details
@batrick batrick deleted the cls-lua branch Sep 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.