spring-cloud / spring-cloud-deployer-kubernetes

The Spring Cloud Deployer implementation for Kubernetes
Apache License 2.0
157 stars 97 forks source link

Unable to utilize spring-cloud-dataflow instanceIndex when reading from namedDestination #513

Closed michaelchehreh-ccri closed 1 year ago

michaelchehreh-ccri commented 1 year ago

My team and I are attempting to make a set of Streams in spring-cloud-dataflow defined as:

MySource1 > :topic
MySource2 > :topic
:topic > MySink

and we'd like to run 1 of each MySource and 4 instances of MySink. The key detail is that we need to avoid any potential partition assignment instability/inconsistency, so we're attempting to enable static partition assignments on MySink. To do so, we set autoReblanceEnabled=false. We've managed to build two streams with MySource writing to topic with 4 partitions (confirmed data on all partitions) and build a stream with MySink that reads from topic. The problem is that all instances of MySink pull from partition 0 of topic instead of pulling the partition equal to its index.

We've confirmed that the instances of MySink have /config/application.properties files where INSTANCE_INDEX and spring.application.index are set to 0-3 in accordance with the id of the kubernetes pod name. However, all of the Sinks still pull from partition 0 instead of their index. We've done our best to trace through the various spring repos and make sense of why it's not being set correctly. spring.application.index seems like it may not matter here, but INSTANCE_INDEX looks like it should be pulled in by BindingServiceProperties in spring-cloud-stream. We were able to attach a debugger to the Sink and confirm that it is always 0. We also confirmed that the partition successfully changes when we set INSTANCE_INDEX as an environment variable. Is there something we're missing in terms of getting the index to set correctly?

We're using 3.2.4 of spring-cloud-stream/kafka-binder and spring-cloud-dataflow 2.9.6 (effective POM shows the others should be 3.1.6, but I can't imagine that'd be the issue).

https://github.com/spring-cloud/spring-cloud-deployer-kubernetes/blob/0a96e5dbdb259f0276b9c65a276a2e5609893a35/src/main/java/org/springframework/cloud/deployer/spi/kubernetes/KubernetesAppDeployer.java#L494-L495

onobc commented 1 year ago

Similar to previous issue: https://github.com/spring-cloud/spring-cloud-dataflow/issues/3703

michaelchehreh-ccri commented 1 year ago

Some additional context - we're using bitnami charts to spin up SCDF (and kafka/zk/mariadb) and deploying to K8S. When I deploy the stream I am setting deployer.MySink.count=4 and that gets us most what we expect. We get 4 instances of MySink that are named with the index, and a /config mount with the application.properties. The final piece seems to be the actual loading of that config. I would expect that instanceIndex would get set but I'm not seeing that happen

https://github.com/spring-cloud/spring-cloud-stream/blob/v3.2.6/core/spring-cloud-stream/src/main/java/org/springframework/cloud/stream/config/BindingServiceProperties.java#L91-L92

onobc commented 1 year ago

Thanks for that detail @michaelchehreh-ccri ,

We reviewed this issue at our team meeting today and @corneil will be digging into it a bit more. There is question around the spring.application.index property being deprecated/removed as well as the instanceIndex not being populated as expected.

We are on it. Thanks again for the great details - we will get to the bottom of it and echo back what we find here.

corneil commented 1 year ago

As @michaelchehreh-ccri said: org.springframework.cloud.stream.config.BindingServiceProperties#instanceIndex is populated from ${INSTANCE_INDEX:${CF_INSTANCE_INDEX:0}} or spring.cloud.stream.instanceIndex

At the moment there is code in Spring Cloud Kubernetes Deployer sets up the execution of the following:

echo INSTANCE_INDEX="$(expr $HOSTNAME | grep -o "[[:digit:]]*$")" >> /config/application.properties && echo spring.application.index="$(expr $HOSTNAME | grep -o "[[:digit:]]*$")" >> /config/application.properties

The regular expression will return the last number of the host name. If the name is app-1-3 it will use 3 or if it is transform-2-11 is will use 11

We should be setting spring.cloud.stream.instanceIndex instead of spring.application.index in Spring Cloud Kubernetes Deployer.

corneil commented 1 year ago

Some additional context - we're using bitnami charts to spin up SCDF (and kafka/zk/mariadb) and deploying to K8S. When I deploy the stream I am setting deployer.MySink.count=4 and that gets us most what we expect. We get 4 instances of MySink that are named with the index, and a /config mount with the application.properties. The final piece seems to be the actual loading of that config. I would expect that instanceIndex would get set but I'm not seeing that happen

https://github.com/spring-cloud/spring-cloud-stream/blob/v3.2.6/core/spring-cloud-stream/src/main/java/org/springframework/cloud/stream/config/BindingServiceProperties.java#L91-L92 @michaelchehreh-ccri Does your application have a configmap? Does your application have any application.properties|yaml files in resources? Try with app.logging.level.org.springframework=DEBUG in the deployment properties and let's see which properties files are being loaded.

corneil commented 1 year ago

I have updated the streamPartitioning acceptance test to handle more partitions. It works as expected with 2 and 3.

michaelchehreh-ccri commented 1 year ago

There aren't any ConfigMaps on the pods, just the /config mount for the index-provider init container, and as far as I can tell we aren't adding any application.properties/yaml files either. However, even after setting all logging to DEBUG I was unable to locate any reference to application.properties nor any other properties file loading

onobc commented 1 year ago

@michaelchehreh-ccri do you mind uploading your startup logs so we can take a look (if you do, please double check for obfuscation around anything in the logs - I am pretty sure Spring Boot has that covered - but I always check before I do).

michaelchehreh-ccri commented 1 year ago

I'm unfortunately unable to upload logs due to restrictions from our customer, nor have an easy way to replicate the environment to produce the logs.

onobc commented 1 year ago

No worries @michaelchehreh-ccri . If I get a chance today I will ask for a specific search in the startup logs etc..

corneil commented 1 year ago

I added a test that used a named topic and it works as expected.

corneil commented 1 year ago

I'm unfortunately unable to upload logs due to restrictions from our customer, nor have an easy way to replicate the environment to produce the logs.

@michaelchehreh-ccri You need to look at the startup logs to determine if /config/application.properties is loaded during startup.

michaelchehreh-ccri commented 1 year ago

There's nothing in the logs at the moment that I can find suggesting any properties files are being loaded. Is there anything I could look for to determine if it's being used? I'm still relatively new to Spring and am unsure if there's an expected "properties loader" class or a place where I could hook up a debugger to determine if /config/application.properties is being loaded on startup. A little digging suggests it may be a PropertySource or the DeploymentPropertiesResolver?

corneil commented 1 year ago

@michaelchehreh-ccri I tested with log-sink 3.2.1 and it works as expected. When I use log-sink-4.0.0-SNAPSHOT that is based on Boot 3 it doesn't work. Adding the property spring.config.import=file:///config/application.properties to the specific application that has multiple paritions will ensure that Spring Boot 3 application reads that properties file as well. In your case app.MySink.spring.config.import=file:///config/application.properties

michaelchehreh-ccri commented 1 year ago

That seems to have done it. I get a log from ConfigurationPropertySourcesPropertyResolver's DefaultResolver about finding INSTANCE_INDEX in /config/application.properties and subsequently see each instance subscribe to the correct partition(s). Thanks for the support!

corneil commented 1 year ago

Resolved