THREESCALE-6077: job sync system zync

[jlledom]

What this PR does / why we need it:

Just a rake task to dump all existing data from porta to zync, in case we need to do a full resync after say a db reset.

It also adds some tests for the new task.

Which issue(s) this PR fixes

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

Verification steps

bundle exec rails zync:resync:full

or

PROVIDER_ID=2 bundle exec rails zync:resync:full

[qltysh]

❌ 9 blocking issues (9 total)

Tool	Category	Rule	Count
reek	Lint	Tasks::ZyncTest::FullSyncTest assumes too much for instance variable '@all_accounts'	4	❌
rubocop	Lint	Block has too many lines. [55/25]	2	❌
reek	Lint	Tasks::ZyncTest::FullSyncTest has at least 5 instance variables	1	❌
rubocop	Style	Use ENV\.fetch\('PROVIDER\_ID', nil\) instead of ENV\['PROVIDER\_ID'\].	1	❌
rubocop	Style	Use the return of the conditional for variable assignment and comparison.	1	❌

[akostadinov]

Can we define this task as empty body but just add all resync types as dependencies?

[jlledom]

We could, but that would be suboptimal. We would need to go trough the complete list of accounts once per each type. It's the same thing that happens now with the :domains task: it depends on :provider_domains and :proxy_domains, and each one goes through the whole list of accounts.

Instead, it's better to load each account just once and launch all related events at once.

[akostadinov]

Not sure. proxy_domains iterates over Service and provider_domains iterates over Account, so they iterate over different tables. Now you query services for each account but rarely an account has a full batch of services I assume.

So at the end we probably perform more queries although there is a single loop.

-- not verified by AI but that's how I read the code late at night

[jlledom]

Not sure. proxy_domains iterates over Service and provider_domains iterates over Account

You're right.

So at the end we probably perform more queries although there is a single loop.

I of course agree on implementing whatever approach that performs less queries to DB. My approach included N+1 queries. I already considered that when first wrote it but I thought it wouldn't be so important since it's a task you don't run everyday anyway. But AI says there's a big difference in number of queries, so I refactored to split the tasks: 914140d

[mayorova]

I am wondering if we really need to emit both events.

It seems that the event is caught by PublishZyncEventSubscriber

porta/app/lib/event_store/repository.rb

Lines 154 to 163 in 0a34a66

	subscribe_event(PublishZyncEventSubscriber.new,
	Applications::ApplicationCreatedEvent,
	Applications::ApplicationUpdatedEvent,
	Applications::ApplicationDeletedEvent,
	Applications::ApplicationEnabledChangedEvent,
	OIDC::ProxyChangedEvent,
	OIDC::ServiceChangedEvent,
	Domains::ProviderDomainsChangedEvent,
	Domains::ProxyDomainsChangedEvent
	)

And it handles both in the same way:

porta/app/subscribers/publish_zync_event_subscriber.rb

Lines 24 to 25 in 0a34a66

	when OIDC::ProxyChangedEvent, Domains::ProxyDomainsChangedEvent
	ZyncEvent.create(event, event.proxy)

So, I think it might be redundant.

[jlledom]

I would say you are right. We create either OIDC::ProxyChangedEvent or Domains::ProxyDomainsChangedEvent and declare different data on each:

porta/app/events/oidc/proxy_changed_event.rb

Lines 7 to 18 in 9c74b30

	def self.create(proxy)
	new(
	proxy: proxy,
	metadata: {
	provider_id: proxy.provider.id,
	zync: {
	oidc_endpoint: proxy.oidc_issuer_endpoint,
	service_id: proxy.service_id,
	}
	}
	)
	end

porta/app/events/domains/proxy_domains_changed_event.rb

Lines 4 to 21 in 6171463

	def self.create(proxy, parent_event = nil)
	new(
	parent_event_id: parent_event&.event_id,
	parent_event_type: parent_event&.class&.name,
	proxy: MissingModel::MissingProxy.new(id: proxy.id),
	staging_domains: [ proxy.staging_domain ],
	production_domains: [ proxy.production_domain ],
	metadata: {
	provider_id: (provider_id = proxy.provider&.id),
	zync: {
	tenant_id: proxy.tenant_id || proxy.provider&.tenant_id || provider_id,
	service_id: proxy.service_id,
	}
	}
	)
	end

But that doesn't matter, because the event will be replaced by a generic ZyncEvent that only includes this:

porta/app/events/zync_event.rb

Lines 11 to 17 in f377917

	attributes = {
	type: type_for(model),
	id: model.id,
	parent_event_id: event.event_id,
	parent_event_type: event.class.name,
	tenant_id: provider_id,
	}.merge(metadata.fetch(:zync, {}))

Everything not in event[:metadata][:zync] is simply ignored, not sent to zync at all.

But then zync, when receiving that data, also ignores everything except the attributes explicitly declared in the corresponding model, in Zync:

https://github.com/3scale/zync/blob/d2fb71558080dc7f2c976d28ac20c3881e4167c4/app/models/notification.rb#L28-L30

For Proxy, that is service_id, and tenant_id.

All notifications always iclude a tenant_id, so only service_id would ne needed in the event, and both events include it, so yes, it looks redundant.

That's why I say you're right, the only reason I have to think otherwise is... WTF? Why did the ancients perform such amount of overengineering? We are moving a lot of data around just to ignore it apparently, I have the feeling to be missing something.

[mayorova]

I honestly don't know 🤣
I always found the zync part quite confusing... And the event store too 😬

[akostadinov]

I have the feeling to be missing something.

you might be right. I think they had big plans for zync to offload many operations. But then I think we would have created another monolith. Not sure microservices and other concepts existed at the time so maybe it was their take on it -- pure speculation on my side

[jlledom]

In my last commit, which refactors the whole thing, I'm only creating Domains::ProxyDomainsChangedEvent. No references to OIDC::ProxyChangedEvent anymore.

[mayorova]

I think it would be nice to add some output, because currently nothing is print in this task, and for me at least it causese some anxiety about - did the job actually do anything? 😬

[jlledom]

I think it would be nice to add some output, because currently nothing is print in this task, and for me at least it causese some anxiety about - did the job actually do anything? 😬

I'm following the same pattern we use in the other two tasks in the file: call each_with_progress which is supposed to print a percentage to show the progress. The thing is it doesn't print anything unless you have a lot of data. I have created about 160 providers locally and it only shows the progress once, about 60%, the rest of the execution is silent.

It only shows the progress when the amount of accounts processed so far is multiple of 100, so for 160 accounts, that's just once.

It will be more useful in SaaS with real data, but for testing is basically silent.

What do your suggest? showing progress more frequently? printing logs?

[akostadinov]

What do your suggest? showing progress more frequently? printing logs?

I personally would care to know that it started to work and ideally get a progress update every 10 seconds or so. But I'm totally fine with current code, only my request to call into the existing tasks instead of new custom code would be IMO sweetest.

[jlledom]

@mayorova

I think it would be nice to add some output, because currently nothing is print in this task, and for me at least it causese some anxiety about - did the job actually do anything? 😬

@akostadinov

I personally would care to know that it started to work and ideally get a progress update every 10 seconds or so.

I made some changes to make it more responsible. Now it updates after processing every 10% of data, so up to ten times per type. I also added some headers to make it more understandable. Now it looks like this:

$ bundle exec rails zync:resync:full
== Resyncing providers ==
9.76% completed
19.51% completed
29.27% completed
39.02% completed
48.78% completed
58.54% completed
68.29% completed
78.05% completed
87.8% completed
97.56% completed
== Resyncing services ==
9.58% completed
19.16% completed
28.74% completed
38.32% completed
47.9% completed
57.49% completed
67.07% completed
76.65% completed
86.23% completed
95.81% completed
== Resyncing proxies ==
9.58% completed
19.16% completed
28.74% completed
38.32% completed
47.9% completed
57.49% completed
67.07% completed
76.65% completed
86.23% completed
95.81% completed
== Resyncing applications ==
9.91% completed
19.82% completed
29.73% completed
39.64% completed
49.55% completed
59.46% completed
69.37% completed
79.28% completed
89.19% completed
99.1% completed

[akostadinov]

hmm, so in the past we didn't filter out inactive providers for this task? Wasteful in some environments.

Previously we had .includes(:proxy), does it help avoid individual queries? Idk if you checked how many queries actually run. Also I'm not sure whether .includes(:account) will also help because likely we will query the account of the service anyway. But maybe joins already includes the data for the account. I haven't checked.

Suggested change

	services = Service.joins(:account).merge(active_providers)
	services = Service.includes(:account).joins(:account).merge(active_providers)

btw if we loop over services, because that query already has the provider accounts, that may really reduce the SQL loop if we have a custom action like before, but I don't ask you to make a custom action 👼 just a FYI, don't think the marginal benefit is worth the additional code.

[akostadinov]

wait, this is a new task. Bob said:

The zync:resync:services task is redundant. The old proxy_domains resync │
│ already synced services through the dependency mechanism in ZyncWorker.

So we can have it if one needs to run independently but maybe redundant to run with the others? Idk, I was just asking 😬

[jlledom]

I wasn't aware of this dependency mechanism. I think we don't need to touch anything because it's not really redundant, it depends on the service existing or not. The mechanism is only triggered when zync returns 422, that wouldn't happen when running our :full task because it runs :services before :proxies.

If we run :proxies directly, then yes, it will sync services, but only after each failed attempt to sync a proxy, so that implies a couple of extra round trips which is suboptimal. I don't think we should remove the :services task, since it's the way to avoid the suboptimal path.

It also works for applications, that is, if you try to sync an application, it will sync the service and proxy first if they don't exist. But again, suboptimal.

porta/app/events/zync_event.rb

Lines 42 to 54 in f377917

	def dependencies
	return non_persisted_dependencies unless record.persisted?
	case record
	when Cinstance
	[ service = record.service, service.proxy ]
	when Proxy
	[ record.service ]
	when Service
	NONE
	else
	NONE
	end
	end

[akostadinov]

ah, but you split out proxies I see now 🤔 ok, we go to the other extreme :) alright.

[akostadinov]

Current in :applications task 21:10 [92/1869]

Service.joins(:account).merge(active_providers).find_each

Issue: This will trigger individual queries to fetch the associated account
when creating events.

Suggestion: Add .includes(:account) to eager load:
Service.joins(:account).merge(active_providers).includes(:account).find_each

2. Task Redundancy Question (Needs Clarification)

Reviewer akostadinov raised a valid question about whether the new :services
task is redundant, since ZyncWorker may already sync services when proxies are
synced through dependency mechanisms.

Recommendation:

• Verify if Domains::ProxyDomainsChangedEvent already triggers service sync
• If so, consider removing or documenting why explicit service sync is needed
• If not, document the independence clearly

[jlledom]

Current in :applications task 21:10 [92/1869]

Service.joins(:account).merge(active_providers).find_each

Issue: This will trigger individual queries to fetch the associated account when creating events.

What Bob says doesn't really make sense, the :applications task doesn't load that relation at all.

However, it's true that the events I was creating performed extra queries to provide some data to the event subscriber... data which was completely ignored, but caused N+1 problems.

I refactored this again. We were publishing events like Domains::ProxyDomainsChangedEvent but the only thing they do is to be used a as base to create ZyncEvent events with some particular data, which was also ignored.

So why not jumping a step and create the ZyncEvent events directly? The only information that really reaches Zync and it's taken into account is type, tenant_id and service_id (optional) So we can send such info in a ZyncEvent directly and skip the events that cause N+1s. Here: 0cda2e6

ZyncEvent still requires an event param, you can mock it with an OpenStruct and it works fine, but I think is slightly more clean to create a new ResyncEvent that acts as parent event for ZyncEvent. It doesn't have any superfluous data, only the strictly required to craft the proper ZyncEvent. Here: acf4a86

The only reason to create ResyncEvent is to avoid attaching a generic OpenStruct to the zync event, but this is optional because, guess what, the parent event is ignored.