THREESCALE-8404: Bump redis to 5.2.0 #384

jlledom · 2024-05-08T16:13:54Z

This comes from #350. That PR is too big, it has a lot of outdated info and so many comments and commits it could lead to confusion. I decided to split it into smaller PRs easier to review and understand.

This PRs updates the redis gem to 5.2.0 but it doesn't include any new functionality compared to master.

Commits:

5125eed: It updates the Gemfile and fixes some breaking changes.
9fe39fe: Updates some calls to the redis API that changed in redis 5. In particular :sadd?, :srem?, :exists? and :pipelined.
d806123: The new API requires some major changes in the async client. For instance, a new implementation of the pipeline logic.
640a5e4: Update tests.

jlledom · 2024-05-14T09:27:15Z

This comes from #350 (review)

IIRC, when @fetch_timeout is reached, the method will return nil, not raise. A read timeout on the other hand is a low-level TCP timeout that shouldn't happen regularly.

On the other hand, we don't need to be very much bothered by a connection timeout, if we can just reconnect. It's good to be resilient in case of network glitches.

I would suggest though to log the error, can be without a trace, just dome text like:

Que redis (ip/hostname of server if possible) connection timeout at #{FILE}:#{LINE}

In this way we can have an indication of network stability and see whether particular servers or locations are more problematic. This will help debugging and improving the cluster setup.

@akostadinov You're right, the docs say it should return nil, however it raises an exception when connecting to sentinels. I opened an issue upstream to fix it: redis/redis-rb#1279

In the meantime we must capture the exception. And I don't think we need to log it because it would flood the logs with this error happening all the time, since it's the expected behavior.

jlledom · 2024-05-14T11:04:17Z

From #350 (comment):

Why can provider key be nil? Should we conditionally add or should we raise an error?

@akostadinov I honestly don't remember why I did this, and it indeed looks wrong. I'm removing this line for now, let's see if something breaks.

lib/3scale/backend/job_fetcher.rb

lib/3scale/backend/storage_async/pipeline.rb

akostadinov · 2024-05-14T11:31:10Z

lib/3scale/backend/storage_async/methods.rb

+        # All of this might be simplified a bit in the future using the
+        # "methods" in async-redis
+        # https://github.com/socketry/async-redis/tree/master/lib/async/redis/methods
+        # but there are some commands missing, so for now, that's not an option.


seems like these were removed and likely no plan to bring them back, perhaps remove that comment

The comment is outdated. redis-async now relies on https://github.com/socketry/protocol-redis to implement its methods. And if I'm not wrong, we could use that gem for redis-rb too. I'll take a look at some point in the future to make this simpler.

For now, I updated the comment on 054d005

akostadinov · 2024-05-14T11:53:33Z

lib/3scale/backend/storage_helpers.rb

@@ -241,6 +246,38 @@ def cfg_sentinels_handler(options)
            options
          end


Should we make sure that here sentinels are not empty?

Up there we have next if sentinel.empty? and then end.compact so basically we can end up with an empty options[:sentinels]

Are we not already doing it?

apisonator/lib/3scale/backend/storage_helpers.rb

Line 219 in 5125eed

return options if sentinels.nil? || sentinels.empty?

I saw that but it is before the compat call. So we may still end up with empty sentinels list.

You're right, I made some changes here: 9f85a48

lib/3scale/backend/storage_helpers.rb

akostadinov · 2024-05-14T12:06:35Z

lib/3scale/backend/worker.rb

@@ -74,11 +74,11 @@ def perform(job)
      end

      def register_worker
-        redis.sadd(:workers, self)
+        redis.sadd(:workers, self.class.name)


🤔 wouldn't this register all workers under the same name? Isn't the point here to be able to identify registered workers? For example self.to_s may result in worker-backend-pod-10-10... while self.class.name would be all the same for any pod.

Right, my bad. Fixed at c1048fb

jlledom · 2024-05-14T12:13:11Z

From #350 (comment):

Why can provider key be nil? Should we conditionally add or should we raise an error?

@akostadinov I honestly don't remember why I did this, and it indeed looks wrong. I'm removing this line for now, let's see if something breaks.

@akostadinov I found why I did it. It's because some tests fail. redis-rb 5 doesn't accept nils as parameters for it's redis calls, only strings. However, I think it's the tests what were wrong, not the code. I pushed some changes (b4c0c87)

akostadinov · 2024-05-14T12:24:25Z

test/unit/storage_sync_test.rb

-  def test_redis_url_without_scheme
-    assert_nothing_raised do
-      StorageSync.send :new, url('foo')
-    end
-  end


we don't support urls without a schema anymore, correct?

Apparently we do. I don't know why I removed that test. Fixed in 461a712

akostadinov · 2024-05-14T12:27:43Z

test/unit/storage_sync_test.rb

-    assert_client_config(conn, url: "redis://#{config_obj[:url]}",
-                               sentinels: [{ host: '127.0.0.1', port: 26_379 }])
+    assert_sentinel_config(conn, url: "redis://#{config_obj[:url]}",
+                           sentinels: [{ host: '127.0.0.1', port: 26_379 }])


I hope we at least log any a_malformed_url detected. This is to inform users about typos.

It raises an exception that gets printed to stdout, so it'll be logged.

akostadinov · 2024-05-14T15:05:56Z

lib/3scale/backend/storage_async/methods.rb

+        #      which accepts a hash of options in redis-rb.
+        #
+        # All of this might be simplified a bit in the future using
+        # https://github.com/socketry/protocol-redis


I don't understand how it would help but at least link opens 😄

So protocol-redis implements a ruby method for each redis command, which ends up calling call [COMMAND], [ARGS]. AFAIK Both async-redis and redis-rb implement the :call method so theoretically it would be possible to put protocol-redis in the middle and make apisonator use the procol-redis API rather than the redis-rb API. Which would allow us to remove this class entirely. However I didn't investigate enough to be sure it's possible.

Thanks for the explanation @jlledom By looking at the README of that repo I also didn't understand...
But now I understand.
Another link for reference (NOT saying that it needs to be added to the comment). https://www.rubydoc.info/gems/protocol-redis/0.2.0/Protocol/Redis/Methods/Hashes

mayorova · 2024-05-16T13:14:56Z

lib/3scale/backend/alert_limit.rb

@@ -9,7 +9,7 @@ class AlertLimit
      attr_accessor :service_id, :value

      def save
-        storage.sadd(key_allowed_set(service_id), value.to_i) if valid?
+        storage.sadd?(key_allowed_set(service_id), value.to_i) if valid?


@jlledom How did you figure out which calls needed to be updated (from sadd to sadd?) and which are not? Failing tests or smth else?

Cause there are still a lot of occurrences of sadd (for example). I guess it's fine as long as we're not expecting a boolean from them. But how can we make sure?

How did you figure out which calls needed to be updated (from sadd to sadd?) and which are not?

By reading the Changelog, from 4.1.3 to 5.2.0.

For :pipelined and :exists? it's easier because I replaced all occurrences. In the case of :sadd and :sadd? (same for :srem[?]). What I did is check all calls, and add the ? to those where the return value is being used and needs to be a boolean. Then passing tests and making a lot of manual testing in different scenarios. There are not many occurrences of those, so it's doable.

mayorova · 2024-05-16T14:16:10Z

test/unit/storage_sync_test.rb

  def test_redis_url_without_scheme
    assert_nothing_raised do
-      StorageSync.send :new, url('foo')
+      StorageSync.send :new, url('backend-redis:6379')


Suggested change

StorageSync.send :new, url('backend-redis:6379')

storage = StorageSync.send :new, url('backend-redis')

assert_client_config(storage, url: URI('redis://backend-redis:6379'))

Just a suggestion... not sure if a good one (obviously, assert_nothing_raised is not needed in this case)

I was just surprised that the port was deleted, I think this actually modifies the test. And also, there is another test, test_redis_host_and_port which already tests host+port.

I applied this: de82ec2

mayorova · 2024-05-16T14:18:54Z

test/unit/storage_sync_test.rb

-    client.set('foo', 'bar')
-    assert_equal 'bar', client.get('foo')
-  end
+  def assert_client_config(conn, **conf)


Following the convention that typically the assert_ methods follow, I would place the expected config (**conf in this case as a second argument, and the actual one (which will get derived from conn) as the 2nd argument.

That probably means that a normal hash should be used as the expected param instead of kwargs, but for me this order is a bit confusing...

Same for assert_sentinel_config.

But maybe that's fine...

Sound reasonable. I did it in 65d05ef

mayorova · 2024-05-16T16:08:11Z

spec/unit/service_spec.rb

@@ -368,44 +368,44 @@ module Backend
      describe '.active?' do
        it 'returns true when service is active' do
          [
-            { state: :active, id: '8001' },
-            { state: 'active', id: '8001' },
+            { state: :active, id: '8001', provider_key: 'foo' },


I don't understand this change with adding provider_key to all Service.save! calls...

is this required now for some reason? I think it shouldn't?

Why were the tests passing without the provider key before?

See this: #384 (comment)
Then this: #384 (comment)

Do you think the tests were right and it should be possible that persist_sets actually receives an empty provider hey?

I've been checking, and it seems that indeed provider_key is an important part of the Service model on apisonator. I thought it wasn't the case, because service_token is used for most operations on the Service Management API, but it seems that when Service is created or updated through the internal API, theprovider_key is indeed provided 😬 https://github.com/3scale/porta/blob/4b2eb62c3acea37ec32b0f1339a2d63f6b8ddef1/app/lib/backend/model_extensions/service.rb#L20

So, I guess it's fine to add it to tests.

mayorova · 2024-05-16T16:13:12Z

Looks good @jlledom great job! 👏

I'm just concerned about that change with the provider_key in tests. Otherwise, all looks nice.

To match redis-rb API

jlledom mentioned this pull request May 8, 2024

build(deps): bump resque from 1.27.4 to 2.6.0 #374

Closed

jlledom requested review from akostadinov, mayorova and lvillen May 14, 2024 11:02

jlledom mentioned this pull request May 14, 2024

THREESCALE-8404: Add ACL and TLS support for Redis #350

Closed

8 tasks

akostadinov reviewed May 14, 2024

View reviewed changes

lib/3scale/backend/job_fetcher.rb Outdated Show resolved Hide resolved

akostadinov reviewed May 14, 2024

View reviewed changes

lib/3scale/backend/storage_async/pipeline.rb Show resolved Hide resolved

akostadinov reviewed May 14, 2024

View reviewed changes

lib/3scale/backend/storage_helpers.rb Show resolved Hide resolved

akostadinov reviewed May 14, 2024

View reviewed changes

jlledom requested a review from akostadinov May 16, 2024 06:57

mayorova reviewed May 16, 2024

View reviewed changes

mayorova approved these changes May 17, 2024

View reviewed changes

akostadinov approved these changes May 17, 2024

View reviewed changes

jlledom added 4 commits May 20, 2024 12:05

Bump redis to 5.2.0

7449148

Use redis 5.2.0 API

9d00930

Adapt the async client to the new API

189cd3d

To match redis-rb API

Update tests

9b13453

jlledom force-pushed the THREESCALE-8404-redis-5 branch from 65d05ef to 9b13453 Compare May 20, 2024 10:06

jlledom merged commit 312aa10 into 3scale:master May 20, 2024
2 checks passed

jlledom self-assigned this May 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THREESCALE-8404: Bump redis to 5.2.0 #384

THREESCALE-8404: Bump redis to 5.2.0 #384

jlledom commented May 8, 2024

jlledom commented May 14, 2024

jlledom commented May 14, 2024

akostadinov May 14, 2024

jlledom May 14, 2024

akostadinov May 14, 2024

jlledom May 14, 2024

akostadinov May 14, 2024

jlledom May 16, 2024

akostadinov May 14, 2024

jlledom May 15, 2024

jlledom commented May 14, 2024

akostadinov May 14, 2024

jlledom May 15, 2024

akostadinov May 14, 2024

jlledom May 15, 2024

akostadinov May 14, 2024

jlledom May 15, 2024

mayorova May 16, 2024

mayorova May 16, 2024

jlledom May 16, 2024

mayorova May 16, 2024

jlledom May 16, 2024

mayorova May 16, 2024

jlledom May 16, 2024

mayorova May 16, 2024

jlledom May 16, 2024

mayorova May 17, 2024

mayorova commented May 16, 2024

		@@ -241,6 +246,38 @@ def cfg_sentinels_handler(options)
		options
		end

	StorageSync.send :new, url('backend-redis:6379')
	storage = StorageSync.send :new, url('backend-redis')
	assert_client_config(storage, url: URI('redis://backend-redis:6379'))

THREESCALE-8404: Bump redis to 5.2.0 #384

THREESCALE-8404: Bump redis to 5.2.0 #384

Conversation

jlledom commented May 8, 2024

jlledom commented May 14, 2024

jlledom commented May 14, 2024

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

jlledom commented May 14, 2024

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

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

mayorova commented May 16, 2024