Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Long Startup Times When Many Configmaps Exist #1681

Open
patrick-shinn-infor opened this issue Jul 26, 2024 · 7 comments · May be fixed by #1711
Open

Long Startup Times When Many Configmaps Exist #1681

patrick-shinn-infor opened this issue Jul 26, 2024 · 7 comments · May be fixed by #1711

Comments

@patrick-shinn-infor
Copy link

Describe the bug
After upgrading to Spring Boot 3.3.0 (spring-cloud-starter-kubernetes-fabric8-config:3.1.2) from Spring Boot 2, I noticed that our application startup times in Kubernetes increased nearly 5-fold.

My team uses Kapp to manage our CI/CD development environment, which results in a large number of configmaps being created as historical changes are tracked. Because of this, we have been experiencing extremely long startup delays while the KubernetesClientEventBasedConfigMapChangeDetector does its initial load of all configmaps in the current namespace. I have tried several configurations including verbosely passing the name of the desired configmap and applying a label selector; however, the result is that all configmaps in the namespace are always read.

I realize that our situation is an extreme case in that we have something close to 2,000 configmaps in our namespace due to how Kapp handles configmap changes, but in my mind, it does not make sense that every configmap in the namespace is loaded when I explicitly specify which configmap or which labels should be used to select the configmaps I wish to pass to the application. I have not dug deeply into why the configmaps are loaded in this way, and I am sure there is a good reason for it, but I wanted to raise awareness that in namespaces that have a large number of configmaps the current implementation results in extremely slow startup times.

Additionally, I was not able to find anything in the documentation that describes this behavior. I believe there should be some clarification as the documentation can be currently interpreted as all other configurations will not be read.

The current implementation appears to be related to 1176.

Sample
There is no sample to provide here, the long load times can be easily recreated by creating a number of configmaps in the namespace of the application.

@wind57
Copy link
Contributor

wind57 commented Jul 26, 2024

I'm not sure what you mean by:

KubernetesClientEventBasedConfigMapChangeDetector does its initial load of all configmaps in the current namespace

We do read all config maps indeed, but not here, not in the "change detector". There, we only create informers and "listen" for changes.

I think your issue is a little elsewhere and to that extent let me ask you a question back: you configure your app to use properties from a certain config map as properties in your app, correct? Something like:

spring:
  application:
    name: my-app
  cloud:
    kubernetes:
      config:
        namespace: spring-k8s
        sources:
            - name: config-map-one

For such a config we read all the configmaps indeed in a certain namespace, and if there are many of them, this might take time.

At the moment, this is the only way we read config-maps and this indeed changed from version 2. The reasoning was that instead of doing many individual requests (for each configmap in your configuration), we do only a single one. In your case (please confirm if you are using the app in this manner), the reverse happens, you might benefit from individual reading of configmaps... and we do not have such a possibility atm.

From what I recall, this is the second time in 2 years that someone mentioned such an issue, so this should be an enhancement.

@patrick-shinn-infor
Copy link
Author

Hello @wind57,

Thanks for the quick turn around 😄Yes, we have used a similar configuration to the one you have provided. Our initial configuration was as follows for our kuberenetes profile. Note that we do set spring.application.name in the base profile that we use, although it is not depicted here.

spring:
  config:
    import: "kubernetes:"
  cloud:
    kubernetes:
      config:
        enabled: true
      reload:
        enabled: true

This should have defaulted to fabric8 searching in the current namespace for a configmap that was named the same as our spring application (which it did, the config was read properly). We then tried to directly specify the configmap by explicitly stating the source just as you have shown in your example. As noted, this did not help with our startup time since all configmaps are read.

I can't imagine there are many users out there that have a plethora of configmaps like we do, but this does identify an interesting edge case. I agree that there should be some enhancement, whether it be in how the configmaps are read or simply to the documentation so that the behavior is understood by the consumer.

Sorry for the confusion with:

I'm not sure what you mean by:
KubernetesClientEventBasedConfigMapChangeDetector does its initial load of all configmaps in the current namespace

As I mentioned, I did not dig too deeply into how the configmaps are read, I just followed the logging to where things started to slow down 😄

@wind57
Copy link
Contributor

wind57 commented Jul 26, 2024

yeah, setting the name there would not help, we still read them all...

I am more inclined to "fix" this as an enhancement, instead of documenting the shortcoming. I have not touched that code in quite some time and I am a bit busy with other things, nevertheless I will eventually get to this too.

@ryanjbaxter can you please add the "enhancement" tag in here please? thank you

@wind57
Copy link
Contributor

wind57 commented Sep 5, 2024

I started working on a prototype to see where that takes me and how easy that would be to implement here.

If that is successful, I still have some bad news: it will only be present in the "next major release". I assume that is going to be in 4.x.x. The issue is that creating changes and make them backwards compatible is far too complicated: I tried that in a separate branch and gave up on the idea.

I'll update this issue as soon as I progress further.

@wind57
Copy link
Contributor

wind57 commented Sep 23, 2024

doable indeed: #1711

I'll keep this PR up to date until we have green light to merge it

@wind57
Copy link
Contributor

wind57 commented Sep 26, 2024

@ryanjbaxter does github allow to link a PR to this issue? it will be easier to close, whenever the time comes.

@ryanjbaxter
Copy link
Contributor

Done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants