Skip to content

fix(admin): back k8s client/scaler beans off their own types and guard the missing client (#7912) - #7917

Merged
Aias00 merged 3 commits into
apache:masterfrom
konlue:fix/kubernetes-scaler-bean-conditions
Oct 27, 2026
Merged

Aias00 merged 3 commits into
apache:masterfrom
konlue:fix/kubernetes-scaler-bean-conditions

Conversation

@konlue

@konlue konlue commented Oct 21, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #7912.

Summary

A deployment that registers its own DeploymentProperties bean silently loses the Kubernetes scaling client: apiClient was guarded by @ConditionalOnMissingBean(DeploymentProperties.class), so the presence of that bean — which the issue's configuration has — suppresses client creation, leaving the component-scanned KubernetesScaler with a null client whose first scale call NPEs (scaleByNum only catches ApiException). On the plain binding path the old code did create the client; the regression is specific to a pre-registered properties bean. The kubernetesScaler @Bean method was dead code (its condition could not fire once apiClient was gone) and is removed.

Modifications

  • The properties are now a conditional @Bean method (@ConditionalOnMissingBean) instead of @EnableConfigurationProperties, so a user-supplied DeploymentProperties cleanly wins instead of competing with a second candidate — the previous arrangement made apiClient ambiguous and failed startup with NoUniqueBeanDefinitionException once the client was actually created. The prefix stays on the DeploymentProperties class: repeating @ConfigurationProperties(prefix = "shenyu.k8s.deployment") on the factory method is a duplicate definition that spring-boot-configuration-processor rejects at compile time.
  • apiClient backs off on @ConditionalOnMissingBean(AppsV1Api.class); the shenyu.k8s.scale.enabled gate is unchanged.
  • scaleByNum / scaleByAction log a warning and no-op when the client is absent, and scaleByAction now returns the outcome so ScaleObserver can report a scale that was not applied. Client construction failures (unreachable cluster, unusable CA) are caught and degrade to the same logged no-op rather than failing startup.
  • TLS verification is enabled for the token client (Config.fromToken(..., true)) and the CA is handed to the client as a re-readable copy instead of a stream that was already closed.

Verifying this change

  • KubernetesConfigurationTest: standard binding creates exactly one AppsV1Api; a pre-registered DeploymentProperties (under a name unrelated to the factory parameter) no longer suppresses it — fails with the reported NoUniqueBeanDefinitionException on the previous configuration; a user client wins the back-off; the user properties bean is the one used; disabled creates no client. shouldBindDeploymentPropertiesFromTheConfiguredPrefix pins that the prefix is still bound with the annotation on the class alone (name / apiServer / token read from shenyu.k8s.deployment.*) — removing the class-level annotation fails it with expected: <shenyu-bootstrap> but was: <null> and drops the client bean count to zero.

  • KubernetesScalerTest: Optional.empty() makes both scale entry points safe no-ops.

  • All related tests and checkstyle pass:

    KubernetesConfigurationTest: 6/6
    KubernetesScalerTest: 4/4
    ScaleServiceTest: 11/11
    TaskSchedulerManagerTest: 3/3
    checkstyle: clean
    

Known follow-up

@ConditionalOnMissingBean on a component-scanned @Configuration is ordering-dependent: a user @Bean AppsV1Api in a configuration processed later would still produce two candidates. Moving this configuration to @AutoConfiguration (or injecting ObjectProvider<AppsV1Api>) is the deterministic fix and is left for a follow-up rather than widened into this PR. Pre-existing behavior, unchanged here.

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: #7917 fix(admin): back k8s client/scaler beans off their own types and guard the missing client (#7912)

Verdict: changes requested — one concrete, reproduced startup regression in the exact configuration the linked issue reports, plus a test that hides it.

First, the parts I checked and agree with:

  • Deleting the kubernetesScaler @Bean is right. Its @ConditionalOnMissingBean({AppsV1Api.class, DeploymentProperties.class}) could not fire once apiClient was gone, and the live scaler is the component-scanned @Component. KubernetesScaler does keep this.appsV1Api = appsV1Api.orElse(null);, so the new Objects.isNull(appsV1Api) guards are effective, and ScaleService.applyTarget already treats a false return as "not applied", so the no-op path does not corrupt appliedTarget.
  • The guards are needed, not defensive noise: ScaleTaskInitializer is an unconditional CommandLineRunner that calls scaleService.executeScaling(), and shenyu.k8s.scale.enabled only gates the client, not the scaler, so an active policy row NPE'd the startup path before this change.
  • TestPropertyValues does accept key:value — TestPropertyValues.Pair.getSeparatorIndex takes Math.min(indexOf(':'), indexOf('=')) — so the ca-cert-path: entry is not a bug (see nit 5 below), and pr_build really did execute KubernetesConfigurationTest (4/4) and KubernetesScalerTest (4/4) on 6ef826af.

Blocking: a pre-registered DeploymentProperties now fails startup, and the new test passes by accident

@EnableConfigurationProperties(DeploymentProperties.class) stays on KubernetesConfiguration, so it always registers its own properties bean (shenyu.k8s.deployment-…DeploymentProperties). Once apiClient is actually created, it needs a DeploymentProperties argument — and with a user-supplied properties bean there are two candidates.

I reproduced this against the real KubernetesConfiguration/DeploymentProperties sources on Spring Boot 3.3.1 / Spring 6.1.10 (same as the root pom), only stubbing Config/ApiClient/AppsV1Api:

T1 new | enabled + standard binding                        -> containsBean(apiClient)=true;  AppsV1Api beans=[apiClient]
T2 new | enabled + props pre-registered (as written)       -> containsBean(apiClient)=true;  AppsV1Api beans=[apiClient]
T6 new | enabled + props pre-registered (explicit name)    -> STARTUP FAILED: NoUniqueBeanDefinitionException :
     expected single matching bean but found 2: preRegisteredK8sProperties,
     shenyu.k8s.deployment-org.apache.shenyu.admin.config.properties.DeploymentProperties
O1 old | enabled + standard binding                        -> containsBean(apiClient)=true;  AppsV1Api beans=[apiClient]
O2 old | enabled + props pre-registered                    -> containsBean(apiClient)=false; AppsV1Api beans=[]   (context starts)

Two things follow.

  1. shouldCreateClientEvenWhenPropertiesBeanIsPreRegistered only passes because of a bean-name coincidence. withBean(DeploymentProperties.class, this::deploymentProperties) registers the extra bean under the name deploymentProperties, which is exactly the apiClient factory parameter name (<arg>-parameters</arg> is enabled in the root pom), so determineAutowireCandidate resolves the ambiguity by name. Give the same bean any other name — which is what a real user would write, e.g. @Bean DeploymentProperties k8sProperties() — and the context fails with NoUniqueBeanDefinitionException. The test therefore does not pin the behaviour it claims; rename it explicitly (.withBean("preRegisteredProperties", DeploymentProperties.class, …)) and it goes red.
  2. That failure is a behaviour change for the reported configuration. O2 (old code, pre-registered properties) starts and merely leaves appsV1Api empty; T6 (this PR) refuses to start. Turning "scaling silently does nothing" into "Admin does not boot" in the scenario the issue is about is worse than the bug being fixed, and the failure is not limited to apiClient — any other injection point of DeploymentProperties becomes ambiguous too.

Suggested repair: stop having two sources for the same properties bean. Either

  • register the properties from KubernetesConfiguration itself, so a user bean can cleanly win, e.g. replace @EnableConfigurationProperties(DeploymentProperties.class) with

    @Bean
    @ConditionalOnMissingBean
    @ConfigurationProperties(prefix = "shenyu.k8s.deployment")
    public DeploymentProperties deploymentProperties() {
        return new DeploymentProperties();
    }

    which keeps the issue's requested "custom properties override" working and makes @ConditionalOnMissingBean(AppsV1Api.class) on apiClient meaningful; or

  • keep @EnableConfigurationProperties and qualify the parameter so the @EnableConfigurationProperties bean is always the one injected, documenting that overriding the properties bean is unsupported.

Either way, the test should use an explicitly named pre-registered bean so it exercises the real case.

Non-blocking

  1. @ConditionalOnMissingBean on a scanned @Configuration is ordering-dependent. shouldBackOffWhenACustomClientIsProvided passes because withBean registers the custom client before refresh(). In a real deployment a user @Bean AppsV1Api in their own scanned @Configuration that is processed after KubernetesConfiguration produces two AppsV1Api beans, and KubernetesScaler(Optional<AppsV1Api> …) then fails with NoUniqueBeanDefinitionException (Optional is not one of indicatesMultipleBeans, so this is a hard failure, not an empty Optional). I reproduced it:

    ours imported first    -> Client beans=[apiClient, customClient]
        Optional<Client> injection -> NoUniqueBeanDefinitionException (startup would FAIL)
    custom client imported first -> Client beans=[customClient], Optional<Client> -> one client
    

    The old code had the same latent problem, so this is not a regression, but Spring Boot documents this condition as auto-configuration-only. Moving this configuration to @AutoConfiguration (or injecting ObjectProvider<AppsV1Api> and picking the first candidate) would make the back-off deterministic, and it deserves a line in the PR description rather than a test that only proves harness ordering.

  2. containsBean("apiClient") cannot distinguish "client created" from "@Bean returned null". containsBean is satisfied by the bean definition, which exists whenever @ConditionalOnProperty matched — apiClient still return null on IOException, which Spring registers as a NullBean. The two containsBean assertions therefore only prove the condition outcome, not client creation, while shouldNotCreateClientWhenScalingIsDisabled uses getBeansOfType(AppsV1Api.class). Use the same getBeansOfType/hasSingleBean assertion in all four tests.

  3. The PR description overstates the bug. It says "the condition can never pass and the AppsV1Api bean is never created, no matter what the user configures". My O1/C runs show the old code does create the client on the plain binding path, because the @EnableConfigurationProperties registrar of the same configuration class runs after its @Bean methods, so DeploymentProperties is not yet visible to OnBeanCondition. The verified regression is exactly the pre-registered-properties case — which is the case in issue #7912 and which that issue itself hedges. Worth correcting, since the description becomes the changelog entry.

  4. scaleByAction drops the new false. scaleByAction calls scaleByNum(newReplicas) and ignores the result, so "client unavailable" and "ApiException" are both invisible to callers; the only signal is the WARN. Consider returning boolean (or logging once at the caller) so policy reconciliation can react.

  5. Config.fromToken(apiServer, token, false) disables TLS verification and then sets a CA cert on the client, so the cert is never used for verification. That was harmless while the bean was never created; now this path runs for the first time, so please confirm the intent — true is almost certainly what "trust this CA" means. Also note setSslCaCert receives a try-with-resources stream that is closed immediately after the call, i.e. the client keeps a stream it cannot re-read.

  6. Mixed property separators in enabledProps(). "shenyu.k8s.deployment.ca-cert-path:" + certFile() works, but getSeparatorIndex takes the first : or = of the whole string, so a value that itself contains a colon before the = would silently truncate the key. Use = for readability and safety, as the other entries do.

  7. testScaleByNumWithoutClientIsSafeNoOp / testScaleByActionWithoutClientIsSafeNoOp build the scaler with Optional.empty() — good, that pins the exact NPE from the issue. No notes there.

CI

build (shenyu-integrated-test-http) is red on this head, but it is the pre-existing master regression from #7903 (CryptorResponsePluginTest.testWhenDoesntExistFiled still asserts the old 10001 contract while CryptorResponsePlugin.fieldErrorParse now throws ResponsiveException(-116)). Not caused by this PR — it needs a separate fix on master.

@konlue

konlue commented Oct 21, 2026

Copy link
Copy Markdown
Contributor Author

Fixed, and thanks for the reproduction — you were right that the test only passed on the bean-name coincidence.

I went with the first repair route:

  • @EnableConfigurationProperties is gone. The properties are now a conditional @Bean (@ConditionalOnMissingBean + @ConfigurationProperties(prefix = "shenyu.k8s.deployment")), so a user bean cleanly wins and the apiClient argument has exactly one candidate.
  • The pre-registration test now registers the bean under preRegisteredProperties. Against the previous configuration, it fails with the exact NoUniqueBeanDefinitionException you reported (two candidates: preRegisteredProperties and shenyu.k8s.deployment-…).
  • Added a second test asserting the client is built from the override.

On the non-blocking points:

  • The assertions now use getBeansOfType.
  • scaleByAction returns the result, and ScaleObserver warns when it is false.
  • Config.fromToken(..., true) with the CA read into a ByteArrayInputStream (construction failures are caught so a bad CA still degrades to a logged no-op rather than failing startup).
  • The property entries use =.

I also corrected the description below — you are right that the old code created the client on the plain binding path; the regression was the pre-registered-properties case. The ordering caveat for @ConditionalOnMissingBean on a scanned @Configuration is noted as a follow-up rather than refactoring to @AutoConfiguration here.

@wy471x wy471x left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirmation: I independently reviewed this change and agree with @Aias00's blocking conclusion — the DeploymentProperties registered by @EnableConfigurationProperties and a user-supplied bean with an explicit name create two candidates, so injecting apiClient fails with NoUniqueBeanDefinitionException; the existing withBean(DeploymentProperties.class, ...) only resolves the ambiguity by the parameter name deploymentProperties by coincidence. I suggest removing the duplicate source as described above and changing the test to use an explicitly named bean.

Also, the CI failure in build (shenyu-integrated-test-http) is the pre-existing master issue from #7903, not introduced by this PR.

@Aias00

Aias00 commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Friendly nudge — the branch is still at 6ef826af, so nothing has changed since my review.

The one blocking item is small and self-contained: the PR makes a pre-registered DeploymentProperties bean fatal.

T6 new | props pre-registered (explicit name) -> STARTUP FAILED:
  NoUniqueBeanDefinitionException : expected single matching bean but found 2:
  preRegisteredK8sProperties,
  shenyu.k8s.deployment-org.apache.shenyu.admin.config.properties.DeploymentProperties

Today's shouldCreateClientEvenWhenPropertiesBeanIsPreRegistered passes only because the harness registers the bean under the name deploymentProperties, which happens to equal the factory method's parameter name — with -parameters enabled, Spring resolves the injection by name and the test goes green while a real user's explicitly-named properties bean fails the context. Renaming the pre-registered bean reproduces the startup failure.

The fix I suggested still holds: declare the properties bean yourself instead of relying on @EnableConfigurationProperties ordering,

@Bean
@ConditionalOnMissingBean
@ConfigurationProperties(prefix = shenyu.k8s.deployment)
public DeploymentProperties deploymentProperties() {
    return new DeploymentProperties();
}

and swap the containsBean(apiClient) assertion for getBeansOfType(AppsV1Api.class), since containsBean cannot tell a created bean from a NullBean produced by return null.

The non-blocking notes from the same review (PR-body wording about the condition never passing, scaleByAction dropping scaleByNum's boolean, Config.fromToken(..., false) disabling TLS verification while also setting a CA, and the :/= mix in enabledProps()) can be taken or left — but the description claim about the condition is worth correcting since it is what the reviewer will check first.

@konlue

konlue commented Oct 22, 2026

Copy link
Copy Markdown
Contributor Author

Apologies for the confusion — my previous reply went out before the branch did. The fixes are now pushed (head 6ef826af → 36ff34f):

  • Conditional @Bean for the properties.
  • The pre-registration test renamed to preRegisteredProperties (it fails with your exact NoUniqueBeanDefinitionException against the old configuration).
  • getBeansOfType assertions.
  • scaleByAction returning the outcome.
  • TLS verification enabled with the CA as a re-readable stream.
  • = separators.

The description is also updated.

Local run:

KubernetesConfigurationTest: 5/5
KubernetesScalerTest: 4/4
ScaleServiceTest: 11/11
TaskSchedulerManagerTest: 3/3
checkstyle: clean

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-review: #7917 fix(admin): back k8s client/scaler beans off their own types and guard the missing client (#7912)

Verdict: approved. The blocking regression I reported on 6ef826af is fixed, and the test now pins the real scenario instead of passing by bean-name coincidence.

The blocking issue is resolved

I had reproduced NoUniqueBeanDefinitionException with a DeploymentProperties bean registered under a name other than the factory parameter (T6 in my previous review), while the old test only passed because withBean(DeploymentProperties.class, …) registers the bean as deploymentProperties, matching the apiClient parameter name.

Both halves of that are now addressed:

  • @EnableConfigurationProperties is gone; the properties are registered by a @Bean @ConditionalOnMissingBean @ConfigurationProperties(prefix = "shenyu.k8s.deployment") method, so there is no second candidate to compete with apiClient's argument.
  • shouldCreateClientEvenWhenPropertiesBeanIsPreRegistered now registers the bean explicitly as preRegisteredProperties, and the test asserts a single AppsV1Api bean rather than containsBean-style presence. That is exactly the case that used to fail.
  • shouldUseThePreRegisteredProperties goes further and asserts assertSame(custom, context.getBean(DeploymentProperties.class)), i.e. the user bean is not merely tolerated but is the one the client is built from.

The rest checks out

  • @ConditionalOnMissingBean(AppsV1Api.class) on apiClient is the right key now that the client is actually created. A @Bean returning null is registered as a NullBean, which does not match AppsV1Api by type, so the condition still fires and Optional<AppsV1Api> still resolves to empty — shouldNotCreateClientWhenScalingIsDisabled and the two Optional.empty() scaler tests cover both ends of that.
  • KubernetesScaler is still the component-scanned @Component, so removing the dead kubernetesScaler @Bean leaves the injection point intact, and ScaleObserver now logs a warning when scaleByAction reports false instead of silently doing nothing.
  • The two real latent bugs folded in here are worth having: Config.fromToken(..., true) actually verifies the API server certificate, and new ByteArrayInputStream(caCertStream.readAllBytes()) replaces a FileInputStream that the try-with-resources closed before the client ever read it. Both are called out in the PR description, which is what I asked for.
  • catch (IOException | RuntimeException e) around client construction is broad, but it is the correct trade-off for a component whose failure mode should be "logged no-op", not "Admin does not boot".

Non-blocking

  1. The ordering caveat is still there, and now it applies to the properties bean too. @ConditionalOnMissingBean on a component-scanned @Configuration is evaluated in registration order, so a user @Bean DeploymentProperties in a configuration parsed after KubernetesConfiguration produces two candidates and apiClient fails with NoUniqueBeanDefinitionException again — the same class of failure, only narrower. You have already documented this as a known follow-up and named @AutoConfiguration / ObjectProvider as the deterministic fix, so I am not blocking on it; please keep the follow-up open rather than letting the "Known follow-up" section age out of the description.
  2. testScaleByActionWithoutClientIsSafeNoOp asserts assertDoesNotThrow but not the false return, which is the contract ScaleObserver now relies on. assertFalse would pin it more tightly.

Nit: script/shenyu_checkstyle.xml has CustomImportOrder commented out, so the assertEquals / assertFalse / assertNotNull / assertDoesNotThrow / assertTrue ordering in KubernetesScalerTest is not a build problem — just unusual to read.

@Aias00

Aias00 commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Thanks for this PR — I reviewed it against #7912 and previously approved it, but the current head (36ff34) does not compile, so I have to switch this to request changes.

What breaks

pr_build fails in shenyu-admin:

[ERROR] .../shenyu-admin/src/main/java/org/apache/shenyu/admin/config/KubernetesConfiguration.java:[54,33] Duplicate @ConfigurationProperties definition for prefix 'shenyu.k8s.deployment'
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.7.0:compile (default-compile) on project shenyu-admin: Compilation failure

This is a hard javac error, not a flaky test. DeploymentProperties already carries @ConfigurationProperties(prefix = "shenyu.k8s.deployment") at the class level, and the new @Bean deploymentProperties() method (lines 51-56) repeats the same prefix. spring-boot-configuration-processor's ConfigurationMetadataAnnotationProcessor rejects the duplicate registration during compilation.

I reproduced this in isolation against Spring Boot 3.3.1 (the version this repo pins) and got the identical diagnostic, then confirmed the fix below.

Suggested fix

Drop the @ConfigurationProperties(prefix = ...) annotation (and its now-unused import) from the @Bean method and rely on the class-level annotation:

@Bean
@ConditionalOnMissingBean
public DeploymentProperties deploymentProperties() {
    return new DeploymentProperties();
}

I verified locally that this still compiles and that the prefix is bound correctly (apiServer / token are populated from shenyu.k8s.deployment.*), so the behaviour you wanted in #7912 — a user-supplied DeploymentProperties bean winning via @ConditionalOnMissingBean while still being bound — is preserved.

The other red checks are cascade

e2e, sql-matrix, list-contract, registry_dependencies, build-docker-images and the shenyu-integrated-test-* jobs are all red because of this same compile failure: list-contract stops at -rf :shenyu-admin, and sql-matrix fails only on AssertionError: List contract failed or was canceled/skipped. They should clear once shenyu-admin compiles.

Happy to re-review as soon as a fix is pushed.

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes: the current head does not compile.

pr_build fails with KubernetesConfiguration.java:[54,33] Duplicate @ConfigurationProperties definition for prefix 'shenyu.k8s.deployment' — DeploymentProperties is already annotated with that prefix at the class level, and the new @Bean deploymentProperties() method repeats it, which spring-boot-configuration-processor rejects as a hard javac error.

Please drop the method-level @ConfigurationProperties annotation (keep the class-level one); that compiles and still binds shenyu.k8s.deployment.*, so the #7912 override behaviour is preserved. Details in the comment above. I will re-review once fixed.

@konlue

konlue commented Oct 23, 2026

Copy link
Copy Markdown
Contributor Author

Fixed — the method-level @ConfigurationProperties and its import are gone, so the prefix lives on DeploymentProperties alone, exactly as you sketched; @ConditionalOnMissingBean still lets a user-supplied bean win the method.

I also added shouldBindDeploymentPropertiesFromTheConfiguredPrefix, which asserts name / apiServer / token are bound from shenyu.k8s.deployment.*: with only the class-level annotation left, nothing in the suite pinned the binding itself, and dropping that annotation makes the new test fail with expected: <shenyu-bootstrap> but was: <null> while the client bean count drops to zero.

One honest caveat: a local -pl shenyu-admin -Prelease compile on my JDK does not reproduce the processor diagnostic (the metadata is generated without complaint), so CI on the new head is the real confirmation for that half.

2 files, +21/-5. Local status:

KubernetesConfigurationTest: 6/6
KubernetesScalerTest: 4/4
ScaleServiceTest: 11/11
TaskSchedulerManagerTest: 3/3
checkstyle: clean

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good hardening pass on the Kubernetes scaler wiring. I checked the bean graph specifically because the kubernetesScaler @Bean is removed:

  • KubernetesScaler is already annotated @Component, so it is still discovered; its Optional<AppsV1Api> + DeploymentProperties constructor arguments resolve (the new @ConditionalOnMissingBean DeploymentProperties bean supplies the latter, and Optional tolerates a null AppsV1Api). No missing-bean startup failure.

The individual fixes are all correct:

  • client.setSslCaCert(new ByteArrayInputStream(caCertStream.readAllBytes())) fixes a real bug: FileInputStream was closed by try-with-resources while ApiClient reads the CA lazily when it builds the HTTP client, so the cert could be unreadable at that point. A re-readable in-memory copy is the right answer.
  • Config.fromToken(..., true) turns on certificate verification (previously false). That is the correct security posture; combined with the catch of RuntimeException, an unreachable cluster or unusable CA now degrades to a logged no-op instead of failing Admin startup.
  • @ConditionalOnMissingBean(AppsV1Api.class) is the right condition for the client bean, and @ConditionalOnMissingBean on deploymentProperties() lets a user-supplied bean win.
  • scaleByAction returning boolean with null-client guards, plus ScaleObserver logging a warning when an action was not applied, makes silent no-ops observable.

One note: with TLS verification now on, clusters using a CA that does not match the API server certificate will stop scaling (by design) but only log an error. Worth mentioning in the deployment docs so the behaviour change is discoverable.

LGTM.

@Aias00

Aias00 commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

Root cause found for the build (shenyu-integrated-test-http) failure, and it is not in your branch — no code change needed on your side.

The job fails on:

CachePluginTest.testMemoryModle:54 expected: <true> but was: <false>
CachePluginTest.testRedisModle:73  expected: <true> but was: <false>

I traced this across three unrelated PRs (#7917, #7895, #7888) and all three fail on exactly the same two assertions, so it is not specific to your change. CachePluginTest.testPass() fetches /http/test/cache twice with a 2s gap and asserts the two response bodies match; the rule handle sets timeoutSeconds=3000, so a cache hit is expected. The logs show every response carrying a fresh timestamp ("msg":"Fri Oct 23 13:07:54 UTC 2026", then 13:07:55, then 13:07:56, then 13:07:58) — the cache never serves the stored entry. The test itself has not changed since 2022, so this is a cache-plugin regression introduced by the 10-23 / 10-24 cache work (#7969 coalesce concurrent misses, then #8079 Vary replay).

It is already fixed on master. The same job passes on newer heads:

Your branch is behind master (this PR: 58 commits), which is why it still carries the broken cache code.

What to do: rebase onto current master and let CI re-run. That should clear this job without touching your diff. If it still fails after the rebase, the regression is genuinely yours and I will dig in again.


Additional note for this PR: the e2e and e2e-case (...) failures are a separate infrastructure issue, not the cache bug. The log ends with:

@konlue
konlue force-pushed the fix/kubernetes-scaler-bean-conditions branch from 091ae16 to ec3f685 Compare October 26, 2026 16:28

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional validation of the already-approved head ec3f685: KubernetesConfigurationTest and KubernetesScalerTest passed all 10 tests, zero failures/errors/skips. Rechecked the earlier named-properties-bean ambiguity and duplicate ConfigurationProperties prefix concerns: the current conditional properties bean and class-only prefix address them. No new blocking finding; my existing approval still applies. No live Kubernetes API server was exercised. Validation baseline: PR head plus a clean, uncommitted merge of master 52339b2 in the isolated worktree. No source was manually patched or pushed.

Used isolated worktrees and the Maven wrapper with JDK 21/project compiler settings. Checkstyle passed for the targeted Maven runs; RAT and JaCoCo were skipped. No full reactor/e2e pass is claimed.

@Aias00
Aias00 merged commit e1ffee3 into apache:master Oct 27, 2026
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Existing deployment properties suppress the enabled Kubernetes scaling client bean

3 participants