[
https://issues.apache.org/jira/browse/KNOX-2900?focusedWorklogId=1041304&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1041304
]
ASF GitHub Bot logged work on KNOX-2900:
----------------------------------------
Author: ASF GitHub Bot
Created on: 14/Sep/26 08:01
Start Date: 14/Sep/26 08:01
Worklog Time Spent: 10m
Work Description: smolnar82 commented on code in PR #1395:
URL: https://github.com/apache/knox/pull/1395#discussion_r4003172738
##########
gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java:
##########
@@ -618,22 +751,33 @@ protected ServiceConfigurationModel
getCurrentServiceConfiguration(final String
ApiRoleConfigList roleConfigList =
roleCollector.getAllServiceRoleConfigurations(clusterName, service);
- for (ApiRoleConfig roleConfig : roleConfigList.getItems()) {
- ApiConfigList configList = roleConfig.getConfig();
-
- String roleName = roleConfig.getName();
- String roleType = roleConfig.getRoleType();
- ApiHostRef hostRef = roleConfig.getHostRef();
- ApiRole role = new
ApiRole().name(roleName).type(roleType).hostRef(hostRef);
- roleConfigs.put(role, configList);
- }
- currentConfig = new ServiceConfigurationModel(svcConfig, roleConfigs);
+ final ApiService apiService = new
ApiService().name(service).type(serviceType);
Review Comment:
To reuse the discovery workflow, `getCurrentServiceConfiguration()`
reconstructs the `ApiService` / `ApiRole` / config objects it feeds into
`ServiceModelFactory.generateServiceModels()`. But this reconstruction doesn't
populate the same fields that real discovery
(`ClouderaManagerServiceDiscovery.discoverService`) hands to generators. Any
`ServiceModelGenerator` that dereferences a field the reconstruction leaves
null throws an NPE inside `generateService()`. This is not an edge case — it
hits the majority of generators:
- **`role.getHostRef().getHostname()`** — used by **34 of 51 generators**
(`OozieServiceModelGenerator`, `SolrServiceModelGenerator`, HBase, Impala, and
most others). NPEs if the reconstructed role's `hostRef` (or its hostname)
isn't populated exactly as discovery populates it.
- **`service.getClusterRef().getClusterName()`** — used by
`YarnUIServiceModelGenerator` and `JobHistoryUIServiceModelGenerator`. NPEs
because the synthetic `ApiService` sets only `name` / `type`, no `clusterRef`.
- Any other field a given generator's `handles()` / `generateService()`
reads that the reconstruction omits.
**Failure path & blast radius:** a start/restart/scale event for almost any
discoverable service (YARN, Oozie, Solr, HBase, …) → `hasConfigChanged` →
`getCurrentServiceConfiguration` → `ServiceModelFactory.generateServiceModels`
→ the generator's `generateService()` → **NPE**. The NPE is not an
`ApiException`, so the method's own try/catch doesn't catch it; it propagates
to `monitorClusterConfigurationChanges`' outer `catch(Exception)`, aborting the
**entire** polling cycle for **all** clusters. Because the triggering event is
never marked processed, the same NPE recurs every polling interval — PCA is
effectively dead for that gateway.
**Root cause / fix direction:** the reconstruction in
`getCurrentServiceConfiguration()` diverges from how
`ClouderaManagerServiceDiscovery` builds these objects. Rather than
hand-rebuilding `ApiService` / `ApiRole` from `readServiceConfig`, PCA should
obtain the model inputs through the **same** code path discovery uses (the
shared component introduced here), so every field a generator may read is
populated identically. A unit test that runs each registered generator against
PCA-reconstructed inputs would have caught this and would guard against
regressions as new generators land.
##########
gateway-discovery-cm/src/main/java/org/apache/knox/gateway/topology/discovery/cm/monitor/PollingConfigurationAnalyzer.java:
##########
@@ -340,25 +344,31 @@ private boolean hasConfigChanged(String address, String
clusterName, List<Releva
// Get the previously-recorded configuration
ServiceConfigurationModel serviceConfig =
serviceConfigurations.get(re.getServiceType());
- if (serviceConfig != null) {
- // Get the current config for the started service, and compare with
the previously-recorded config
- ServiceConfigurationModel currentConfig =
- getCurrentServiceConfiguration(address, clusterName,
re.getService());
-
- if (currentConfig != null) {
- log.analyzingCurrentServiceConfiguration(re.getService());
- try {
- configHasChanged = hasConfigurationChanged(serviceConfig,
currentConfig);
- } catch (Exception e) {
- log.errorAnalyzingCurrentServiceConfiguration(re.getService(),
e);
- }
+ // Get the current (model-derived) config for the started service.
This is null when the service produces no
+ // model (e.g. invalid configuration), just as such a service is
absent from the recorded baseline.
+ ServiceConfigurationModel currentConfig =
+ getCurrentServiceConfiguration(address, clusterName,
re.getService(), re.getServiceType());
+
+ if (serviceConfig == null && currentConfig == null) {
+ // Was and remains in an invalid configuration state (no model
either time): nothing to proxy, no change.
+ log.skippingConfigChangeForInvalidService(re.getService(),
re.getServiceType());
+ } else if (serviceConfig != null && currentConfig != null) {
+ // Valid before and now: compare the recorded and current configs to
detect a change.
+ log.analyzingCurrentServiceConfiguration(re.getService());
+ try {
+ configHasChanged = hasConfigurationChanged(serviceConfig,
currentConfig);
+ } catch (Exception e) {
+ log.errorAnalyzingCurrentServiceConfiguration(re.getService(), e);
}
- } else {
- // A new service (no prior config) represent a config change, since
a descriptor may have referenced
- // the "new" service, but discovery had previously not succeeded
because the service had not been
- // configured (appropriately) at that time.
+ } else if (currentConfig != null) {
+ // No prior config, but the service now produces a model: new /
became valid -> re-discover.
log.serviceEnabled(re.getService());
configHasChanged = true;
+ } else {
+ // Had a prior config but produces no model now: became invalid /
was removed -> re-discover so the
+ // service is dropped from the affected topologies (and the
scoped-replace merge clears its baseline).
+ log.serviceDisabled(re.getService());
+ configHasChanged = true;
Review Comment:
This new branch treats `getCurrentServiceConfiguration() == null` as "the
service is now invalid / disabled" and forces re-discovery. But that method
**also** returns `null` on any `ApiException` - network blip, auth failure,
transient 5xx:
```java
} catch (ApiException e) {
log.clouderaManagerConfigurationAPIError(e);
}
return currentConfig; // still null on API error
```
So a `null` return is ambiguous: it can mean either "config genuinely
produced no model" or "couldn't reach CM". hasConfigChanged's else branch
(prior config present, currentConfig == null) can't tell them apart and
unconditionally sets configHasChanged = true, logs serviceDisabled, and
triggers a full re-discovery.
**Failure path:** CM API has a transient outage while computing the current
config for a service that has a valid prior baseline →
`getCurrentServiceConfiguration` catches the `ApiException` and returns `null`
→ `hasConfigChanged` forces re-discovery → repeats every polling cycle until CM
recovers.
**Regression:** previously an API-error `null` was a harmless no-op
(configHasChanged stayed false). This PR turns a transient CM error into a
repeating full-cluster rediscovery storm — the same failure class KNOX-2900 set
out to eliminate.
**Fix direction:** distinguish "no model produced" from "CM unreachable".
Options: let the `ApiException` propagate (or rethrow) instead of collapsing it
to `null`, or return a tri-state / Optional so the caller can skip the change
decision on API errors and only treat a genuinely empty-but-successful result
as "service invalid".
Issue Time Tracking
-------------------
Worklog Id: (was: 1041304)
Time Spent: 0.5h (was: 20m)
> 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
> Assignee: Tamás Marcinkovics
> Priority: Major
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> 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)
