THREESCALE-11354: Sync application plan name to backend

[jlledom]

What this PR does / why we need it:

This PR is about syncing the application name to backend when the name is updated from porta.
An application plan can be attached to many cinstances, and despite backend providing an endpoint for application batch update, our pisoni gem doesn't provide a method for it, so we have to send a request per cinstance. Due to that, I added a worker to do the sync, so the user request is not blocked for plans with many cinstances.

I considered adding an Application.save_batch method to pisoni, or sending the request any other way, but I don't think any of those is worth it, considering how much demand this change will really have in real world.

Which issue(s) this PR fixes

https://redhat.atlassian.net/browse/THREESCALE-11354

Verification steps

1. Create an application plan and attach it to an application
2. Update the plan name via API at PUT /admin/api/services/<id>/application_plans/<plan_id>
3. Authorize that application to backend via GET /transactions/authorize.xml
4. Backend should return the updated plan name

[qltysh]

All good ✅

[akostadinov]

is it not simpler to add the method to pisoni? PR description sounds like it is harder but adding a background worker with the possibility that it may fail or be delayed seems to me as much more work 🤔

[jlledom]

is it not simpler to add the method to pisoni? PR description sounds like it is harder but adding a background worker with the possibility that it may fail or be delayed seems to me as much more work 🤔

I don't think it'll be specially hard. It's just it requires extra steps like creating a PR to pisoni repo, then launch a new release, upgrade porta side... that's what I wanted to avoid.

Also, there's no other use case for applications batch update in backend. And it's not much important if the job is delayed or fails because It's not really clear why the clients want this in the first place, as this guy commented https://redhat.atlassian.net/browse/THREESCALE-11354?focusedCommentId=11989077

[akostadinov]

I somehow don't like introducing background jobs for no important reason. They are somehow harder to catch errors with and number of background jobs sometimes seem to create troubles. It's more about that, than the actual risk of not having name updated. But if you think this a rare operation I can agree.

[akostadinov]

3scale/pisoni#36 we need to update pisoni anyways

[jlledom]

@akostadinov I made some changes to use the new pisoni release. Now the update is made in batches of 1000 apps.

I know you don't like it, but I decided to keep the background job. I don't think we can avoid it because, tough most of plans have less than 1000 apps and would be updated in one request/batch, there are a few dozens plans with more than 10K apps assigned. There's even one plan with 1.3 millions apps.

In my local machine, updating a plan with 15K apps, takes the background job about 8 seconds to complete. I don't think it's acceptable to block a client's request for so much time. Also, we should consider that unicorn has a timeout of 30 or 40 seconds I think, so there's a top limit on the number of apps that could be updated synchronously. A request to update a plan with about 60-70K apps would keep the client blocked for 30 seconds, only to end up failing with a timeout.

I also refactored a bit to satisfy Qltysh demands.

[akostadinov]

Did updating 15k apps with pisoni in a single request fail?

[jlledom]

Did updating 15k apps with pisoni in a single request fail?

Haven't tried, the batch size is 1000. How big do you think the batch should be?

[akostadinov]

Haven't tried, the batch size is 1000. How big do you think the batch should be?

If there is no issue updating 15k apps, I think it is fine to just do it instead of introducing batches.

[akostadinov]

@akostadinov I made some changes to use the new pisoni release.

What do you mean? Edited the release after release? Wouldn't it better to just release a new patch version?

[jlledom]

@akostadinov I made some changes to use the new pisoni release.

What do you mean? Edited the release after release? Wouldn't it better to just release a new patch version?

I meant I made changes here, in this PR, to call the new pisoni method

[jlledom]

Haven't tried, the batch size is 1000. How big do you think the batch should be?

If there is no issue updating 15k apps, I think it is fine to just do it instead of introducing batches.

But what's the limit? at some point we will need batches. We have a plan with 1.3 million apps.

[akostadinov]

But what's the limit? at some point we will need batches. We have a plan with 1.3 million apps.

If things work in a single call, then why implement a batch? Will 1.5M in a single call work?

[jlledom]

But what's the limit? at some point we will need batches. We have a plan with 1.3 million apps.

If things work in a single call, then why implement a batch? Will 1.5M in a single call work?

Not realistic. The payload size would be huge and would take a lot of time to process. I'm sure we would hit a timeout or some DoS protection on the way. I'd say a more reasonable batch size would be about 10k apps which would take about 1MB per request.

[jlledom]

Rebased to solve conflicts

[akostadinov]

Ok, I see that 1.5M apps results in some 122MB JSON. Processing time is a lot. RSS jumps to 3GB although it includes the client as well but still too much. I'm starting to wonder though whether we really need to update these... Anyway, I think a batch of 10k sounds like a reasonable batch size. See my results table below.

P.S. yet to review the actual implementation but I wanted first to see theoretically what makes most sense.

[akostadinov-bot]

Review: Sync Application Plan Name to Backend

The problem is real and the approach is sound — renaming an ApplicationPlan doesn't trigger cinstance callbacks, so backend keeps the stale plan name. Using after_commit + async worker + save_batch is the right design.

Issues

1. includes(:service, :plan) is wasteful — the plan and service are already known

In update_backend_plan, self IS the plan, and all cinstances share the same plan.name, plan.id, and service.backend_id. The code eagerly loads these associations per-batch even though they're constants. For 1.5M cinstances, Rails generates 3 SQL queries per batch (cinstances + services + plans) when only 1 is needed.

Suggested alternative:

def update_backend_plan
  bid = service.backend_id
  pid = id
  pname = name
  cinstances.select(:id, :application_id, :state, :redirect_url).find_in_batches do |batch|
    apps = batch.map do |ci|
      state = ci.state
      state = :active if ci.live?
      { service_id: bid, id: ci.application_id, state: state,
        plan_id: pid, plan_name: pname, redirect_url: ci.redirect_url }
    end
    ThreeScale::Core::Application.save_batch(bid, apps)
  end
end

This eliminates the eager loads and avoids loading unnecessary columns.

2. No error handling — one failed batch aborts the entire sync

If save_batch raises on batch 5 of 1500, batches 6–1500 are never processed. For a large plan, that's potentially >99% of apps left stale. Suggest rescuing per-batch:

cinstances.find_in_batches do |batch|
  begin
    ThreeScale::Core::Application.save_batch(...)
  rescue => e
    Rails.logger.error("Failed to sync batch for plan #{id}: #{e.message}")
  end
end

3. No concurrency/dedup protection

BackendMetricWorker uses Sidekiq::Throttled::Worker with concurrency: { limit: 1, key_suffix: ... } to prevent duplicate concurrent syncs. This worker has none. If someone renames a plan twice quickly, two workers run in parallel both iterating all cinstances. Safe (idempotent) but wasteful. A throttle keyed on plan_id would collapse them.

4. Default find_in_batches batch_size (1000) means many HTTP calls

For 1.5M cinstances: 1500 sequential HTTP calls to apisonator. I benchmarked the apisonator batch endpoint directly — 10k apps/batch runs fine at ~810KB request size and ~2.6s. Bumping batch_size to 5000–10000 would cut HTTP overhead by 5–10x.

5. PR description contradicts the code

The description says "I considered adding an Application.save_batch method to pisoni [...] but I don't think any of those is worth it". But the code does use save_batch, which already exists in pisoni 1.31.0. Description seems to be from an earlier iteration.

What's correct

• after_commit + async worker avoids blocking the admin request
• saved_change_to_name? guard correctly scopes the trigger
• save_batch is much better than individual saves
• backend_application_attributes extraction is a clean refactor
• Idempotent — re-sending the same plan name is harmless, and the worker reads the current name at execution time
• find_in_batches prevents loading all cinstances into memory

Benchmark data (apisonator batch endpoint)

I wrote a benchmark (bench/internal_api/batch_applications_bench.rb) in apisonator to test the batch endpoint at scale:

Apps	Request Size	Response Size	Time	Throughput
10,000	810 KB	2.1 MB	2.6s	3,803/s
50,000	4.0 MB	10.4 MB	13.5s	3,702/s
100,000	8.0 MB	20.9 MB	29.3s	3,414/s
500,000	40.4 MB	104.8 MB	2m38s	3,162/s
1,000,000	81.0 MB	209.7 MB	5m55s	2,818/s
1,500,000	122 MB	315 MB	9m22s	2,668/s

1.5M in a single call works but uses ~3 GB RSS. The batched approach from porta (1000 per call) avoids this memory spike entirely — each call is tiny. The main cost is HTTP round-trip overhead from many sequential calls, hence the suggestion to increase batch_size.

[akostadinov-bot]

Follow-up: Consider Iterable Jobs

This worker is a textbook case for iterable/interruptible jobs. The current find_in_batches loop inside a single perform call means:

• For 1.5M cinstances, the job runs 6+ minutes — a single long-lived Sidekiq job.
• Pod kills lose all progress — if the pod is terminated at batch 800/1500, all work is lost and the retry starts from scratch. Long-running jobs that die unrecoverably on pod termination are highly discouraged in 3scale's container environment.
• No graceful shutdown — the job doesn't respect Sidekiq's shutdown signals mid-iteration, so deploys can SIGKILL it.

Sidekiq 7.3+ has built-in iterable jobs (Sidekiq::Job::Iterable). Porta is currently on Sidekiq 6.4, so that's not available today, but the sidekiq-iteration gem backports this pattern to Sidekiq 6.x. Alternatively, this could be tracked as a follow-up for when Sidekiq is upgraded.

With iterable jobs the worker would look roughly like:

class BackendUpdateApplicationPlanWorker
  include Sidekiq::Job
  include SidekiqIteration::Iteration

  def build_enumerator(plan_id, cursor:)
    plan = ApplicationPlan.find_by(id: plan_id)
    return unless plan

    active_record_batches_enumerator(
      plan.cinstances.select(:id, :application_id, :state, :redirect_url),
      cursor: cursor,
      batch_size: 5000
    )
  end

  def each_iteration(batch, plan_id)
    plan = ApplicationPlan.find_by(id: plan_id)
    return unless plan

    bid = plan.service.backend_id
    apps = batch.map do |ci|
      state = ci.state
      state = :active if ci.live?
      { service_id: bid, id: ci.application_id, state: state,
        plan_id: plan.id, plan_name: plan.name, redirect_url: ci.redirect_url }
    end
    ThreeScale::Core::Application.save_batch(bid, apps)
  end
end

What this gives you for free:

• Cursor-based resumption — if the pod dies at batch 800/1500, the job resumes at 801 on restart, not from scratch
• Graceful interruption — automatically stops between iterations when Sidekiq receives a shutdown signal (deploys, scaling), then re-enqueues with the cursor
• Per-batch error isolation — one failed batch doesn't abort the remaining 700
• No long-running job — from Sidekiq's perspective, each iteration is a short unit of work; the framework handles re-enqueue between iterations
• Throttling hooks — throttle_on lets you back off if backend is overloaded

Even if adding sidekiq-iteration is out of scope for this PR, it's worth tracking — the current pattern of a multi-minute find_in_batches loop in a single perform is fragile in a containerized environment where pods can be killed at any time.

[akostadinov-bot]

Correction: Sidekiq 7.3 is already on master

My previous comment incorrectly stated porta is on Sidekiq 6.4 — I was looking at a stale local branch. Master has Sidekiq 7.3.2, which means Sidekiq::Job::Iterable is available out of the box, no extra gem needed.

This makes the iterable jobs approach even more straightforward:

class BackendUpdateApplicationPlanWorker
  include Sidekiq::Job
  include Sidekiq::Job::Iterable

  sidekiq_options queue: :backend_sync

  def build_enumerator(plan_id, cursor:)
    plan = ApplicationPlan.find_by(id: plan_id)
    return unless plan

    active_record_batches_enumerator(
      plan.cinstances.select(:id, :application_id, :state, :redirect_url),
      cursor: cursor,
      batch_size: 5000
    )
  end

  def each_iteration(batch, plan_id)
    plan = ApplicationPlan.find_by(id: plan_id)
    return unless plan

    bid = plan.service.backend_id
    apps = batch.map do |ci|
      state = ci.state
      state = :active if ci.live?
      { service_id: bid, id: ci.application_id, state: state,
        plan_id: plan.id, plan_name: plan.name, redirect_url: ci.redirect_url }
    end
    ThreeScale::Core::Application.save_batch(bid, apps)
  end
end

Zero new dependencies. Cursor-based resumption, graceful shutdown on pod kill, per-batch error isolation — all built in.