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".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
