[
https://issues.apache.org/jira/browse/KNOX-2900?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18113348#comment-18113348
]
Sandor Molnar commented on KNOX-2900:
-------------------------------------
Claude input:
h2. Verification
I reviewed the current code against all five points. First, some context on the
current state: KNOX-2899 disabled the per-service filter in
{{ClouderaManagerServiceDiscovery.discoverService}} (lines 297-300 are now
commented out), while the {{shouldSkipServiceDiscovery}} helper is retained but
marked {{@SuppressWarnings("PMD.UnusedPrivateMethod")}}. "The service filter"
refers to skipping services whose model generators aren't in the topology's
{{includedServices}} set, which originates from the descriptor in
{{SimpleDescriptorHandler}} (lines 265-266).
All five points are valid:
1) Invalid config + restart re-triggers autodiscovery — CONFIRMED
In {{PollingConfigurationAnalyzer.hasConfigChanged}} (lines 341-362), the
previously recorded config is looked up by service type. If a service was in an
invalid config state, discovery never produced a {{ServiceModel}}, so
{{addServiceConfiguration}} never stored a {{ServiceConfigurationModel}} for
that type. Therefore the lookup returns {{null}}, the code hits the {{else}}
branch, logs {{serviceEnabled}}, and sets {{configHasChanged = true}}. Every
subsequent restart of the still-invalid service repeats this.
2) PCA uses a different workflow than CM discovery — CONFIRMED
{{getCurrentServiceConfiguration}} (lines 600-635) builds a raw
{{ServiceConfigurationModel}} (service props + role props) directly from the CM
API. It never runs {{ServiceModelGenerator.handles()}} / {{generateService()}}.
So PCA compares raw config maps and cannot observe whether the config is valid
(would actually yield a model), nor compare against the discovery output stored
in {{ClusterConfigurationCache}}.
3) Discovery overwrites the global cache/.ver instead of merging — CONFIRMED
(core blocker)
{{ClusterConfigurationCache.addServiceConfiguration}} does
{{clusterMap.put(cluster, configs)}} (line 57) — full replace.
{{ClusterConfigurationFileStore.store}} writes a fresh
{{ServiceConfigurationRecord}} with only the passed configs (lines 57-72) —
full overwrite of the {{.ver}} file. With the filter off (today), every
discovery returns all services, so the cache stays complete and PCA works. With
the filter on, a descriptor referencing only a subset produces a {{scpMap}}
with only that subset and clobbers the cache/{{.ver}} for the whole {{(address,
cluster)}}. This is precisely why the filter was disabled.
4) {{excluded.service.types}} only applied in autodiscovery, not PCA — CONFIRMED
Excluded service types are filtered in {{getClusterServices}} (lines 404-410).
PCA's {{isStartEvent}} (lines 525-538) only checks
{{serviceModelGeneratorExists}}, not the excluded set. An excluded type that
still has a model generator will trigger rediscovery on start/restart.
5) {{excluded.role.types}} not respected by PCA scale events — CONFIRMED
Excluded role types are applied via {{TypeNameFilter}} in
{{ServiceRoleCollectorBuilder}} (lines 80-81) during discovery. PCA's
{{isScaleEvent}} (lines 540-549) only checks {{serviceModelGeneratorExists}}
for the service type and the event code — it never checks the event's
{{ROLE_TYPE}} against the excluded role types.
h2. Plan
Root cause chain: re-enabling the filter makes each discovery return only a
topology's subset of services -> the overwrite semantics (#3) corrupt the
shared cache -> PCA (which reads that cache and uses a divergent workflow, #2)
then makes
wrong rediscovery decisions (#1, #4, #5). So #3 and #2 are the foundation;
#1/#4/#5 are correctness fixes oter is the last step.
Step 1 — Merge instead of overwrite the cache and .ver file (issue #3)
- {{ClusterConfigurationCache.addServiceConfiguration}}: merge the incoming
configs into the existing per-cluster {{Map<serviceType,
ServiceConfigurationModel>}} (merge by service type) instead of
{{put}}-replacing the whole map.
- {{ClusterConfigurationFileStore.store}}: read the existing {{.ver}} record,
merge the new service types imerge}} method invoked from
{{ClouderaManagerClusterConfigurationMonitor.addServiceConfiguration}}).
- Nuance: merging alone never evicts a genuinely removed service type. Removal
must stay driven by {{clearCache}}/{{stopMonitoring}} when no descriptor
references the cluster, and by role-delete scale events. Document this so merge
doesn't
mask real removals.
Step 2 — Make PCA reuse the discovery workflow and compare against the cache
(issue #2)
- Change {{getCurrentServiceConfiguration}} (or add a helper) to run the same
model-building path as {{ClouderaManagerServiceDiscovery}}
({{ServiceModelGenerator.handles()}} -> {{generateService()}}) rather than
assembling a raw
{{ServiceConfigurationModel}}. Practically, refactor
{{discoverService}}/{{generateServiceModels}} into a rl.
- Compare the freshly-built models against what's in
{{ClusterConfigurationCache}} for that {{(address, cluster)}}. Trigger
rediscovery only when the model output differs — not when raw config bytes
differ.
Step 3 — Suppress rediscovery for services stuck in invalid config (issue #1)
- In {{hasConfigChanged}}, the {{serviceConfig == null}} -> {{serviceEnabled}}
-> {{configHasChanged = trueWith Step 2 in place, gate this on whether the
service now produces a valid {{ServiceModel}}. If discoverystill yields no
model (config still invalid), do not trigger rediscovery. Only flip to changed
when an actual model appears where there was none.
Step 4 — Honor {{excluded.service.types}} in PCA (issue #4)
- Load {{getClouderaManagerServiceDiscoveryExcludedServiceTypes()}} into PCA
(lowercased, mirroring {{Cloud}}).
- In {{isStartEvent}}, treat start/restart events whose {{SERVICE_TYPE}} is
excluded as non-relevant so they're never added to {{relevantEvents}}.
Step 5 — Honor {{excluded.role.types}} in PCA scale events (issue #5)
- Load {{getClouderaManagerServiceDiscoveryExcludedRoleTypes()}} into PCA and
reuse {{TypeNameFilter}}.
- In {{isScaleEvent}}, read the event's {{ROLE_TYPE}} (already captured as
{{RelevantEvent.role}}) and return false when that role type is excluded.
Step 6 — Re-enable the filter
- Uncomment the skip logic in {{discoverService}} (lines 297-300), restore the
{{log.skipServiceDiscovery}}pressWarnings("PMD.UnusedPrivateMethod")}} on
{{shouldSkipServiceDiscovery}}.
Step 7 — Tests
- {{PollingConfigurationAnalyzerTest}}: invalid-config-restart produces no
rediscovery (#1); excluded service start event ignored (#4); excluded role-type
scale event ignored (#5); rediscovery fires only on real model-output change
(#2).
- New cache/store test: two descriptors referencing disjoint service subsets of
one cluster both survive insequential discovery (#3).
- Existing CM discovery tests still pass with the filter re-enabled (#6).
Sequencing: #3 -> #2 -> #1 -> #4/#5 (independent) -> #6 -> tests throughout. #3
and #2 are the hard parts; #4 and #5 are small and self-contained.
> KNOX-2899 followup - reenable service based discovery filter
> ------------------------------------------------------------
>
> Key: KNOX-2900
> URL: https://issues.apache.org/jira/browse/KNOX-2900
> Project: Apache Knox
> Issue Type: Task
> Reporter: Attila Magyar
> Priority: Major
>
> In KNOX-2899 the service based discovery filter was temporary disabled
> because it interferes with the polling configurator analyzer.
> * Either a permanent fix is needed that works well with the polling config
> analyizer
> * Or the filter should be permanently removed, if the performance gain would
> be negligible
> cc: [~smolnar]
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
