rgw/lua: support reloading lua packages on all RGWs #52326

yuvalif · 2023-07-05T20:17:23Z

test instructions:
https://gist.github.com/yuvalif/95b8ed9ea73ab4591c59644a050e01e2

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

yuvalif · 2023-07-05T20:18:55Z

src/rgw/driver/rados/rgw_sal_rados.cc

@@ -3482,6 +3477,101 @@ int RadosLuaManager::list_packages(const DoutPrefixProvider *dpp, optional_yield
  return 0;
 }

+int RadosLuaManager::watch_reload(const DoutPrefixProvider *dpp, uint64_t* watch_handle, librados::WatchCtx2* watcher)


could not make that a sal API since the callbacks are expected to belong to a class deriving from WatchCtx2 which is RASDOS specific

yuvalif · 2023-07-05T20:21:46Z

src/rgw/rgw_lua_background.cc

@@ -177,5 +189,46 @@ void Background::create_background_metatable(lua_State* L) {
  create_metatable<rgw::lua::RGWTable>(L, true, &rgw_map, &table_mutex);
 }

+#ifdef WITH_RADOSGW_LUA_PACKAGES
+int Background::PackagesWatcher::start() {
+  return static_cast<rgw::sal::RadosLuaManager*>(parent->lua_manager.get())->watch_reload(&parent->dp, &watch_handle, this);


this is ugly...
see comment on: https://github.com/ceph/ceph/pull/52326/files#diff-8d5ab77cf6b24e95c2c7b352c52d1e254781f6c9e7fb3fd148bddc8a3f4321e7R3480

Ceph code, like comedy, is not pretty ;)

github-actions · 2023-07-11T18:59:32Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

anthonyeleven · 2023-07-18T13:32:04Z

doc/radosgw/lua-scripting.rst

@@ -114,6 +115,13 @@ To print the list of packages in the allowlist:
  # radosgw-admin script-package list


+To apply all changes from the allowlist to all RGWs:


I would remove the first "all"

anthonyeleven · 2023-07-18T13:32:25Z

src/rgw/driver/rados/rgw_sal_rados.cc

+  ceph::encode(reload_status, reply);
+  auto pool = store->getRados()->get_lc_pool_ctx();
+  if (!pool->is_valid()) {
+    ldpp_dout(_dpp, 1) << "ERROR: failed to ack reload of " << PACKAGE_LIST_OBJECT_NAME


anthonyeleven · 2023-07-18T13:33:10Z

src/rgw/driver/rados/rgw_sal_rados.cc

+      auto iter = ack.payload_bl.cbegin();
+      ceph::decode(r, iter);
+    } catch (buffer::error& err) {
+      ldpp_dout(_dpp, 1) << "ERROR: couldn't decode Lua packages reload status. error: " << 


nit: lua here and below should be Lua

anthonyeleven · 2023-07-18T13:34:32Z

src/rgw/rgw_lua.cc

+  auto r = install_packages(lua_manager->dpp(), lua_manager->driver(), 
+      y, lua_manager->luarocks_path(), failed_packages);
+  if (r < 0) {
+    ldpp_dout(lua_manager->dpp(), 1) << "WARNING: failed to install lua packages from allowlist. error: " << r


WARNING and errror seem at odds. Perhaps WARNING: failed to install Lua packages from allowlist: " << r << dendl; ?

it says "WARNING" because failing to install packages does not necesarily impact service
only if there is a script that want to use these packages the service may be impacted.
mayebe will change to "error code" istead of "error"?

maybe "return code"?

anthonyeleven · 2023-07-18T13:35:04Z

src/rgw/rgw_lua_background.cc

@@ -177,5 +189,46 @@ void Background::create_background_metatable(lua_State* L) {
  create_metatable<rgw::lua::RGWTable>(L, true, &rgw_map, &table_mutex);
 }

+#ifdef WITH_RADOSGW_LUA_PACKAGES
+int Background::PackagesWatcher::start() {
+  return static_cast<rgw::sal::RadosLuaManager*>(parent->lua_manager.get())->watch_reload(&parent->dp, &watch_handle, this);


Ceph code, like comedy, is not pretty ;)

yuvalif · 2023-07-19T11:25:52Z

jenkins test api

yuvalif · 2023-07-23T15:11:19Z

jenkins test make check

yuvalif · 2023-07-24T08:39:26Z

teuthology results: http://pulpito.ceph.com/yuvalif-2023-07-23_21:15:40-rgw-wip-yuval-lua-test-distro-default-smithi/

cbodley

i'm hoping we can clean up the sal interface

cbodley · 2023-07-31T22:40:36Z

src/rgw/rgw_sal.h

+  /** Add a callback to watch packages reload **/
+  virtual int watch_reload() = 0;
+  /** Stop watching the packages reload **/
+  virtual int unwatch_reload() = 0;
+  /** Send a reply to the reloader of the packages indicating if the reload was successfull **/
+  virtual void ack_reload(uint64_t notify_id, uint64_t cookie, int reload_status) = 0;
+  /** Get the watch handle which should be shared between watch_reload() and unwatch_reload() **/
+  virtual uint64_t watch_handle() const = 0;


these are all internals of rados' watch/notify protocol that don't belong in the sal interface

can the RadosLuaManager just initialize the watch on construction and drop it on shutdown/destruction to avoid exposing the watch_reload() and unwatch_reload() APIs? we shouldn't need an abstraction for ack_reload(), either - the rados implementation would just provide a callback that calls install_packages() from rgw_lua.cc

the only thing i think we actually need in the sal API is reload_packages() to trigger the reload. for rados, that would just send a notify message and wait for acks. that would (indirectly) invoke our callback to do the actual reload

donne in fe641a9

@cbodley can you please re-review?

src/rgw/rgw_lua.cc

cbodley · 2023-07-31T22:54:15Z

src/rgw/rgw_sal.h

+  /** Get dout prefix provider **/
+  virtual const DoutPrefixProvider* dpp() const = 0;


i see that RadosLuaManager needs a dpp for its watch/notify stuff, but that shouldn't need to be exposed in the interface

src/rgw/rgw_sal.h

src/rgw/rgw_admin.cc

cbodley · 2023-07-31T23:27:45Z

src/rgw/rgw_sal.h

-    /** Get a Lua script manager for running lua scripts */
-    virtual std::unique_ptr<LuaManager> get_lua_manager() = 0;
+    /** Get a Lua script manager for running lua scripts and reloading packages */
+    virtual std::unique_ptr<LuaManager> get_lua_manager(const DoutPrefixProvider* dpp = nullptr, const std::string& luarocks_path = "") = 0;


please avoid using default arguments for virtual functions

github-actions · 2023-08-09T14:20:36Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

yuvalif · 2023-09-10T15:24:02Z

jenkins test api

github-actions · 2023-09-13T15:07:54Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

and move the rest of the functionality to sal::Driver this is to prevent the cases where the lua manager was allocated only to invoke a single functions, and did not have any real lifetime. see discussion here: ceph#52326 (comment) Signed-off-by: Yuval Lifshitz <ylifshit@redhat.com>

see PR comments: ceph#52326 (comment) Signed-off-by: Yuval Lifshitz <ylifshit@redhat.com>

yuvalif · 2023-09-18T16:15:11Z

@dang could you please review the latest changes, as I moved most APIs from the Lua manager to the sal Driver?

dang

I don't have a strong opinion about this either way. On the one hand, it's nice to break up the API into chunks. On the other, objects without well defined lifecycles can be problematic. My very slight preference is to have a LuaManager to separate out the API, but it's not a big deal to me.

cbodley

will need to apply the sal changes to driver/posix also

cbodley · 2023-09-20T17:17:22Z

src/rgw/rgw_appmain.cc

    lua_background->start();
    env.lua.background = lua_background.get();
+    static_cast<rgw::sal::RadosLuaManager*>(env.lua.manager.get())->watch_reload(dpp);


what do you mean? i don't think AppMain or the realm reloader should have to manage this watch. RadosLuaManager can call watch/unwatch in its own constructor/destructor without needing anything else

cbodley · 2023-09-20T17:19:10Z

src/rgw/rgw_realm_reloader.cc

-  env.lua.manager = env.driver->get_lua_manager();
+  env.lua.manager = env.driver->get_lua_manager(env.lua.manager->get_luarocks_path());
+  if (env.lua.background) {
+    env.lua.background->set_manager(env.lua.manager.get());


even if this updates the LuaManager pointer, it leaves Background::driver as a dangling pointer to the Driver that we destroyed with DriverManager::close_storage() above. the RadosLuaManager would also hold a handling pointer to RadosStore* const store

we really should shut down this background thread before close_storage(), and only restart it once we've recreated the new driver and lua manager

regarding the background thread. it is paused during reload (and the driver is set to null), and resumed only after the driver is updated to the new one. why do you think there is an issue?

regarding the lua package manager, it does not have any thread, just getting notifications from the rados thread (that should be paused during the reload). so, i'm not sure when the issue can happen?

why do you think there is an issue?

env.lua.manager extends the lifetime of RadosLuaManager past the point of its driver's deletion. this means that the ~RadosLuaManager() dtor would be making calls on a librados::IoCtx whose librados::Rados client was already destroyed. we should free env.lua.manager before calling close_storage()

yuvalif · 2023-09-21T14:03:21Z

will need to apply the sal changes to driver/posix also

the posix driver derives from FilterDriver, so, the API are already implemented

and move the rest of the functionality to sal::Driver this is to prevent the cases where the lua manager was allocated only to invoke a single functions, and did not have any real lifetime. see discussion here: ceph#52326 (comment) Signed-off-by: Yuval Lifshitz <ylifshit@redhat.com>

see PR comments: ceph#52326 (comment) Signed-off-by: Yuval Lifshitz <ylifshit@redhat.com>

cbodley

the Background pause/resume logic does look like it would avoid dangling pointer issues, just a question about locking there

cbodley · 2023-09-21T18:02:39Z

src/rgw/rgw_lua_background.cc

+      std::unique_lock cond_lock(pause_mutex);
+      if (!paused) {
+        set_package_path(L, lua_manager->get_luarocks_path());
+        std::string no_tenant;
+        const auto rc = rgw::lua::read_script(&dp, driver, no_tenant, 
+            null_yield, rgw::lua::context::background, script);


why is it necessary to hold this lock over the call to read_script()?

looking at the lock above:

if (paused) { ldpp_dout(dpp, 10) << "Lua background thread paused" << dendl; std::unique_lock cond_lock(cond_mutex); cond.wait(cond_lock, [this]{return !paused || stopped;});

paused isn't atomic - shouldn't we acquire the lock before checking it? if we take the lock there, we should be able to call set_package_path() under that same lock instead of reacquiring it here

why is it necessary to hold this lock over the call to read_script()?

what if I'm reading the script (= RADOS object), and then real reload is triggered?
I want the function that tries to call Background::pause() to be blocked until the read ends

looking at the lock above:

if (paused) { ldpp_dout(dpp, 10) << "Lua background thread paused" << dendl; std::unique_lock cond_lock(cond_mutex); cond.wait(cond_lock, [this]{return !paused || stopped;});

paused isn't atomic - shouldn't we acquire the lock before checking it? if we take the lock there, we should be able to call set_package_path() under that same lock instead of reacquiring it here

paused is just a boolean, it is being set atomically (although there could be a race condition when one thread sets it and another one reads it but still doe not see the new value).
this means that:

puase() was called, but we still see "pause=false", but in this case, we would see the right value in line 157, won't read the script and won't run it. and just sleep until the next iteration

resume() was called, but we still see "pause=true", in this case we would get into the "if" statement in line 142, and wait. agree there is an issue here, we may get stuck on wait :-(

paused is just a boolean, it is being set atomically (although there could be a race condition when one thread sets it and another one reads it but still doe not see the new value).

there's really no need to be clever about this, just to avoid a lock every 5 seconds

and i still think this would be simpler if we took out this pause/resume logic and just stop/restart the background thread during realm reload

what if I'm reading the script (= RADOS object), and then real reload is triggered?

this wouldn't be a problem if we shutdown/join the thread before destroying the rados store

ok. agree. its is going to be simpler with paus/resume == shutdown/start

fixed here: 0beafd6

@cbodley now background is explicitly shutdown+start in the realm reloader.
this also makes the lua background independent of RADOS

cbodley · 2023-09-21T18:09:51Z

src/rgw/rgw_realm_reloader.cc

-  env.lua.manager = env.driver->get_lua_manager();
+  env.lua.manager = env.driver->get_lua_manager(env.lua.manager->get_luarocks_path());
+  if (env.lua.background) {
+    env.lua.background->set_manager(env.lua.manager.get());


why do you think there is an issue?

env.lua.manager extends the lifetime of RadosLuaManager past the point of its driver's deletion. this means that the ~RadosLuaManager() dtor would be making calls on a librados::IoCtx whose librados::Rados client was already destroyed. we should free env.lua.manager before calling close_storage()

github-actions · 2023-10-05T08:15:01Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

Signed-off-by: Yuval Lifshitz <ylifshit@ibm.com>

and switch to it only once installation is done. this is needed for cases where installation can happen while RGW is running Signed-off-by: Yuval Lifshitz <ylifshit@ibm.com>

cbodley

looks great! we'll eventually want some reload coverage in teuthology

without requiring a restart of the RGWs test instructions: https://gist.github.com/yuvalif/95b8ed9ea73ab4591c59644a050e01e2 also use capitalized "Lua" in logs/doc Signed-off-by: Yuval Lifshitz <ylifshit@ibm.com>

yuvalif · 2023-10-06T10:03:03Z

looks great! we'll eventually want some reload coverage in teuthology

thanks!
the initil drive for this PR is to be able to add lua automation more easily - will do that next.

yuvalif · 2023-10-09T12:46:48Z

teuthology results: http://pulpito.ceph.com/yuvalif-2023-10-08_12:46:18-rgw-wip-yuval-lua-reload-distro-default-smithi/

multisite crash:

 "backtrace": [
        "/lib/x86_64-linux-gnu/libpthread.so.0(+0x14420) [0x7f4273e57420]",
        "(std::_Rb_tree<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, RGWSyncShardMarkerTrack<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >::marker_entry>, std::_Select1st<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, RGWSyncShardMarkerTrack<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >::marker_entry> >, std::less<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, RGWSyncShardMarkerTrack<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >::marker_entry> > >::find(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)+0x5d) [0x56317242ee2d]",
        "(RGWSyncShardMarkerTrack<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >::finish(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)+0x69) [0x563172431e69]",
        "(RGWDataFullSyncSingleEntryCR::operate(DoutPrefixProvider const*)+0xaff) [0x5631726ed8df]",
        "(RGWCoroutinesStack::operate(DoutPrefixProvider const*, RGWCoroutinesEnv*)+0x17c) [0x5631720c86dc]",
        "(RGWCoroutinesManager::run(DoutPrefixProvider const*, std::__cxx11::list<RGWCoroutinesStack*, std::allocator<RGWCoroutinesStack*> >&)+0x35d) [0x5631720c9cad]",
        "(RGWCoroutinesManager::run(DoutPrefixProvider const*, RGWCoroutine*)+0x91) [0x5631720cb111]",
        "(RGWRemoteDataLog::run_sync(DoutPrefixProvider const*, int)+0x351) [0x563172696f51]",
        "(RGWDataSyncProcessorThread::process(DoutPrefixProvider const*)+0x58) [0x56317239da78]",
        "(RGWRadosThread::Worker::entry()+0xbd) [0x563172364abd]",
        "/lib/x86_64-linux-gnu/libpthread.so.0(+0x8609) [0x7f4273e4b609]",
        "clone()"

test issue:

teuthology.exceptions.CommandFailedError: Command failed on smithi081 with status 100: 'DEBIAN_FRONTEND=noninteractive sudo -E apt-get -y --force-yes install python-dev'

ragweed failure:

self = <ragweed.tests.tests.r_test_multipart_index_versioning object at 0x7fb923508a30>
    def check(self):
        for rb in self.get_buckets():
            break
        index_check_result = rgwa().check_bucket_index(rb.name)
        print(index_check_result)

       assert len(index_check_result.invalid_multipart_entries) == 0
       AttributeError: 'list' object has no attribute 'invalid_multipart_entries'

ragweed/tests/tests.py:299: AttributeError

yuvalif · 2023-10-09T14:52:27Z

same teuthology results as "main":
https://pulpito.ceph.com/yuriw-2023-10-07_14:02:30-rgw-main-distro-default-smithi/

yuvalif requested review from dang, mattbenjamin and cbodley July 5, 2023 20:17

github-actions bot added documentation rgw labels Jul 5, 2023

yuvalif commented Jul 5, 2023

View reviewed changes

github-actions bot added the needs-rebase label Jul 11, 2023

yuvalif force-pushed the wip-yuval-lua-reload branch from 6e3f80e to 6349290 Compare July 17, 2023 17:47

github-actions bot added tests and removed needs-rebase labels Jul 17, 2023

yuvalif force-pushed the wip-yuval-lua-reload branch from 6349290 to 3efd14c Compare July 18, 2023 10:28

yuvalif marked this pull request as ready for review July 18, 2023 10:29

yuvalif requested review from a team as code owners July 18, 2023 10:29

yuvalif force-pushed the wip-yuval-lua-reload branch from 3efd14c to bbe831c Compare July 18, 2023 10:38

dang approved these changes Jul 18, 2023

View reviewed changes

anthonyeleven reviewed Jul 18, 2023

View reviewed changes

yuvalif added the wip-yuval-testing label Jul 24, 2023

cbodley reviewed Jul 31, 2023

View reviewed changes

yuvalif force-pushed the wip-yuval-lua-reload branch from 7946bc5 to fe641a9 Compare August 9, 2023 10:57

github-actions bot added the common label Aug 9, 2023

yuvalif requested a review from cbodley August 9, 2023 11:00

github-actions bot added the needs-rebase label Aug 9, 2023

yuvalif force-pushed the wip-yuval-lua-reload branch from fe641a9 to 809cdc8 Compare August 10, 2023 10:21

github-actions bot removed the needs-rebase label Sep 7, 2023

github-actions bot added the needs-rebase label Sep 13, 2023

yuvalif added a commit to yuvalif/ceph that referenced this pull request Sep 14, 2023

rgw/lua: make luarocks path thread safe

89b3a6b

see PR comments: ceph#52326 (comment) Signed-off-by: Yuval Lifshitz <ylifshit@redhat.com>

yuvalif force-pushed the wip-yuval-lua-reload branch from ca066e5 to e018fc3 Compare September 14, 2023 09:27

github-actions bot removed the needs-rebase label Sep 14, 2023

dang approved these changes Sep 19, 2023

View reviewed changes

cbodley reviewed Sep 20, 2023

View reviewed changes

yuvalif added a commit to yuvalif/ceph that referenced this pull request Sep 21, 2023

rgw/lua: make luarocks path thread safe

fce931e

see PR comments: ceph#52326 (comment) Signed-off-by: Yuval Lifshitz <ylifshit@redhat.com>

yuvalif force-pushed the wip-yuval-lua-reload branch from e018fc3 to 889290a Compare September 21, 2023 14:05

cbodley reviewed Sep 21, 2023

View reviewed changes

yuvalif requested a review from cbodley September 26, 2023 16:21

yuvalif force-pushed the wip-yuval-lua-reload branch from 0beafd6 to 9bb458e Compare October 3, 2023 18:32

github-actions bot added the needs-rebase label Oct 5, 2023

yuvalif added 2 commits October 5, 2023 17:21

rgw/lua/doc: fix luajson example

84944ab

Signed-off-by: Yuval Lifshitz <ylifshit@ibm.com>

rgw/lua: install lua packages in temp directory

c217fba

and switch to it only once installation is done. this is needed for cases where installation can happen while RGW is running Signed-off-by: Yuval Lifshitz <ylifshit@ibm.com>

cbodley approved these changes Oct 5, 2023

View reviewed changes

rgw/lua/doc: support reloading lua packages on all RGWs

7a11f1d

without requiring a restart of the RGWs test instructions: https://gist.github.com/yuvalif/95b8ed9ea73ab4591c59644a050e01e2 also use capitalized "Lua" in logs/doc Signed-off-by: Yuval Lifshitz <ylifshit@ibm.com>

yuvalif force-pushed the wip-yuval-lua-reload branch from 9bb458e to 7a11f1d Compare October 6, 2023 09:55

github-actions bot removed the needs-rebase label Oct 6, 2023

yuvalif merged commit 7b774c4 into ceph:main Oct 9, 2023
10 of 11 checks passed

		@@ -114,6 +115,13 @@ To print the list of packages in the allowlist:
		# radosgw-admin script-package list


		To apply all changes from the allowlist to all RGWs:

		/ Get dout prefix provider /
		virtual const DoutPrefixProvider* dpp() const = 0;

rgw/lua: support reloading lua packages on all RGWs #52326

rgw/lua: support reloading lua packages on all RGWs #52326

Conversation

yuvalif commented Jul 5, 2023 • edited

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 11, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuvalif commented Jul 19, 2023

yuvalif commented Jul 23, 2023

yuvalif commented Jul 24, 2023 • edited

cbodley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 9, 2023

yuvalif commented Sep 10, 2023

github-actions bot commented Sep 13, 2023

yuvalif commented Sep 18, 2023

dang left a comment

Choose a reason for hiding this comment

cbodley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuvalif commented Sep 21, 2023

cbodley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 5, 2023

cbodley left a comment

Choose a reason for hiding this comment

yuvalif commented Oct 6, 2023

yuvalif commented Oct 9, 2023 • edited

yuvalif commented Oct 9, 2023

yuvalif commented Jul 5, 2023 •

edited

yuvalif commented Jul 24, 2023 •

edited

yuvalif commented Oct 9, 2023 •

edited