copr / copr

Clone

#583 [backend] fix default arguments in redis scripts
Closed 5 years ago by praiskup. Opened 5 years ago by praiskup.
Unknown source fix-backend-testsuite  into  master

file modified
+1 -1 
@@ -378,7 +378,7 @@
 

      if hasattr(opts, "redis_port"):
 

          kwargs["port"] = opts.redis_port
 

  

-     return StrictRedis(charset="utf-8", decode_responses=True, **kwargs)
 

+     return StrictRedis(encoding="utf-8", decode_responses=True, **kwargs)
 

  

  

  def format_tb(ex, ex_traceback):

file modified
+2 -2 
@@ -209,7 +209,7 @@
 

          vmd_list = self.get_all_vm_in_group(group)
 

          return [vmd for vmd in vmd_list if vmd.bound_to_user is not None]
 

  

-     def acquire_vm(self, groups, ownername, pid, task_id=None, build_id=None, chroot=None):
 

+     def acquire_vm(self, groups, ownername, pid, task_id="None", build_id="None", chroot="None"):
 

          """
 

          Try to acquire VM from pool.
 

  
@@ -262,7 +262,7 @@
 

          self.log.info("Release vm result `%s`", lua_result)
 

          return lua_result == "OK"
 

  

-     def start_vm_termination(self, vm_name, allowed_pre_state=None):
 

+     def start_vm_termination(self, vm_name, allowed_pre_state="None"):
 

          """
 

          Initiate VM termination process using redis publish.

file modified
+1 
@@ -128,6 +128,7 @@
 

          vmd.store_field(self.rc, "state", VmStates.READY)
 

  

          # undefined both last_health_check and server_start_timestamp
 

+         mc_time.time.return_value = 0.1
 

          with pytest.raises(NoVmAvailable):
 

              self.vmm.acquire_vm([GID1], self.ownername, 42)

Retyping LUA default arguments seem to work differently (or
there's none retyping now?) on redis-py v3, so the None value
isn't automatically converted to 'None' string (ditto for "mocked"
values).

CI green :-)

1 new commit added

  • [backend] fix charset warnings on redis-py v3
5 years ago

Though CI is not restarted after pushing more commits ... still not ideal :-(

Metadata Update from @praiskup:
- Request assigned
5 years ago

Assigning @frostyx, promised to take a look at this.

Thank you for giving me time to look at this.

I am not sure, why do we even need the LUA scripts. It is definitely out of the scope of this PR, but if anyone knows, I would be interested in it.

Retyping LUA default arguments seem to work differently (or
there's none retyping now?) on redis-py v3

https://github.com/andymccurdy/redis-py#encoding-of-user-input

redis-py 3.0 only accepts user data as bytes, strings or numbers (ints, longs and floats). Attempting to specify a key or a value as any other type will raise a DataError exception.

redis-py 2.X attempted to coerce any type of input into a string. While occasionally convenient, this caused all sorts of hidden errors when users passed boolean values (which were coerced to 'True' or 'False'), a None value (which was coerced to 'None') or other values, such as user defined types.

All 2.X users should make sure that the keys and values they pass into redis-py are either bytes, strings or numbers.

+1 to this PR from me

I am not sure, why do we even need the LUA scripts. It is definitely out of the scope of this PR, but if anyone knows, I would be interested in it.

Trust me that I don't love this at all. Some time ago @clime told me that the server-side LUA scripting provides some convenient atomicity which is needed for the VM allocation mechanism (but I haven't done analysis, since it just works).

All 2.X users should make sure that the keys and values they pass into redis-py are either bytes, strings or numbers.

Thanks for the reference -- I'll cite it in the commit message.

I'll push since nobody else planned to take a look at this. Thank you for the review!

48880cc and dcc82cb pushed. Thanks again for having a look.

Pull-Request has been closed by praiskup
5 years ago

Some time ago @clime told me that the server-side LUA scripting provides some convenient atomicity which is needed for the VM allocation mechanism

Sounds right, I've seen also stackoverflow discussion recommending LUA scripting for atomicity.

However, if this is the only reason, I think, that we can utilize Pipelines - https://github.com/andymccurdy/redis-py/blob/master/README.rst#pipelines

but yeah, I am not very eager for rewriting the code, as it is hell to test it.
Changes Summary 3
+1 -1
file changed
backend/backend/helpers.py
+2 -2
file changed
backend/backend/vm_manage/manager.py
+1 -0
file changed
backend/tests/vm_manager/test_manager.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.