Register ControllerFactory bean definition as opposite to bean instance #1516

asavov · 2021-01-30T20:04:33Z

With this change KubernetesReconcilerProcessor conforms to BeanFactoryPostProcessor
contract to register BeanDefinitions and not to manipulate bean instances.
As a result Reconciler beans are:

not created at all by postProcessBeanFactory (which was the core issue)
participate in Spring container management hooks, and hence @Autowired

k8s-ci-robot · 2021-01-30T20:04:35Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2021-01-30T20:04:40Z

Welcome @asavov!

It looks like this is your first PR to kubernetes-client/java 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/java has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

yue9944882

/lgtm

@asavov can you remove @Autowired from the commit message, otherwise it will lead to spammy github references on the page

brendandburns · 2021-02-01T16:50:37Z

Looks like you need to run mvn spotless:apply to fix formatting.

asavov · 2021-02-02T07:31:30Z

/lgtm

@asavov can you remove @Autowired from the commit message, otherwise it will lead to spammy github references on the page

Done with updated commit.

asavov · 2021-02-02T07:35:43Z

Looks like you need to run mvn spotless:apply to fix formatting.

Done. And now seems ok.

asavov · 2021-02-02T07:40:56Z

@dsyer @brendanburns https://github.com/kubernetes-client/java/pull/1516/checks has failed. Not sure it's related to this PR. How to Re-run it through UI? Or should I upload new "dummy" commit?

asavov · 2021-02-02T08:05:59Z

You just signed CNCF Individual Contributor License Agreement. Seems this one is also checked.

dsyer · 2021-02-02T08:18:05Z

Looking good. Sorry for the CLA hassle (at least you only have to do it once).

I would make a couple of small (but could be important) changes. I don’t see a need to inject the SharedInformerFactory and it will cause the same kind of instantiation cascade as the getBean() calls we were trying to avoid. Also we might need to be more defensive about the downcast of the BeanFactory.

asavov · 2021-02-02T08:25:40Z

Looking good. Sorry for the CLA hassle (at least you only have to do it once).

No worries at all. It's done.

I don’t see a need to inject the SharedInformerFactory and it will cause the same kind of instantiation cascade as the getBean() calls we were trying to avoid.

  static class TestReconciler implements Reconciler {

    @Autowired private SharedInformer<V1Pod> informerToBeInjected;

Do you refer this snippet? The whole idea is to be able to do exactly this logic: autowire other beans, benefiting from Spring container. I just used SharedInformer, cause it's natural fit, still it might be Env, or AppCtx or @value(${some.prop}). Would provided more details, cause maybe I miss something.

Also we might need to be more defensive about the downcast of the BeanFactory.

The way to go is to extend from BeanDefinitionRegistryPostProcessor and implement postProcessBeanDefinitionRegistry. Then the cast is not needed any more. I'm ok to do it, if you think that's the right approach.

dsyer · 2021-02-02T08:30:26Z

BeanDefinitionRegistryPostProcessor would probably be better. The informer factory I mentioned wasn’t in a test though, I thought I saw it being injected directly into the post processor.

asavov · 2021-02-02T08:36:10Z

BeanDefinitionRegistryPostProcessor would probably be better. The informer factory I mentioned wasn’t in a test though, I thought I saw it being injected directly into the post processor.

public class KubernetesReconcilerProcessor implements BeanFactoryPostProcessor, Ordered {

  private final SharedInformerFactory sharedInformerFactory;

  public KubernetesReconcilerProcessor(SharedInformerFactory sharedInformerFactory) {
    this.sharedInformerFactory = sharedInformerFactory;
  }

You are absolutely right. Hmm. What's the Spring/right way for a BeanFactoryPostProcessor to get this information? A pointer to valid example within a Spring component would definitely help.

dsyer · 2021-02-02T09:46:05Z

One way would be to set a constructor arg ref (not value) if you can get the bean name by type for the SharedInformerFactory from the ListableBeanFactory. If you look at callers of BeanDefinitionBuilder.addConstructorArgReference() you will find existing examples. Some of them work by using a naming convention - that's the simplest - and some have more elaborate ways of looking up the bean definition name for the reference. Or you can use a Supplier to defer the computation. I think this is safe (to create a Bar from an existing Foo):

	@Bean
	public static BeanDefinitionRegistryPostProcessor processor() {
		return new BeanDefinitionRegistryPostProcessor() {

			private ConfigurableListableBeanFactory beanFactory;

			@Override
			public void postProcessBeanFactory(
					ConfigurableListableBeanFactory beanFactory) throws BeansException {
				this.beanFactory = beanFactory;
			}

			@Override
			public void postProcessBeanDefinitionRegistry(BeanDefinitionRegistry registry)
					throws BeansException {
				BeanDefinitionBuilder definition = BeanDefinitionBuilder
						.genericBeanDefinition(Bar.class,
								() -> new Bar(beanFactory.getBean(Foo.class)));
				registry.registerBeanDefinition("bar", definition.getBeanDefinition());
			}
		};
	}

asavov · 2021-02-02T16:13:13Z

Looking at the code of KubernetesInformerFactoryProcessor and KubernetesReconcilerProcessor I came to a conclusion (maybe I'm wrong) that something very generic is mis-implemented. Those classes are both BeanDefinitionRegistryPostProcessor, which implies they work/manipulate on BeanDefinitions or refs or lazy suppliers, still they both accept ready Bean Instances as constr params (ApiClient and SharedInformerFactory).
How about having some live chat to further discuss this or ... cause if we want to impl it right it seems like a breaking change. Or maybe I lack enough knowledge on the subject.

dsyer · 2021-02-02T16:25:05Z

Those two post processors are unrelated to this change though, right? They need to be fixed in probably almost identical ways, if we decide we want to keep them, but I don't think we have to interrupt this PR process to deal with that.

asavov · 2021-02-02T16:38:14Z

Those two post processors are unrelated to this change though, right? They need to be fixed in probably almost identical ways, if we decide we want to keep them, but I don't think we have to interrupt this PR process to deal with that.

I miss that, cause the focus of the change is KubernetesReconcilerProcessor. Or? The idea is to introduce a new player ?

dsyer · 2021-02-02T16:48:01Z

I'm sorry, you are right. You fixed one of them in this PR, though? So we can leave the other one for another effort?

asavov · 2021-02-03T07:34:49Z

I'm sorry, you are right. You fixed one of them in this PR, though? So we can leave the other one for another effort?

Should I go with current version (if approved) OR remove

  public KubernetesReconcilerProcessor(SharedInformerFactory sharedInformerFactory) {
    this.sharedInformerFactory = sharedInformerFactory;
  }

(causing SharedInformerFactory to be pre-instantiated) and default to the SINGLETON SharedInformerFactory instance to be used/selected by KubernetesReconcilerProcessor#postProcessBeanFactory. Thus no bean instantiation at least by KubernetesReconcilerProcessor, based on some defaults, let's say one-and-only-one instance of SharedInformerFactory.

  @Override
  public void postProcessBeanFactory(ConfigurableListableBeanFactory beanFactory) {
    for (String reconcilerName : beanFactory.getBeanNamesForType(Reconciler.class)) {
      BeanDefinition controllerFactoryBeanDefinition =
          BeanDefinitionBuilder.genericBeanDefinition(KubernetesControllerFactory.class)
              .addConstructorArgReference(<the name of singleton shared informer factory>)
              .addConstructorArgReference(reconcilerName)
              .getBeanDefinition();

      // Replace this with call to BDRegistry passed by BeanDefinitionRegistryPostProcessor
      ((DefaultListableBeanFactory) beanFactory)
          .registerBeanDefinition(reconcilerName + "Controller", controllerFactoryBeanDefinition);
    }
  }

dsyer · 2021-02-03T07:38:49Z

That would work. We'd have to be sure about the bean name. The lazy instantiation alternative with a Supplier is better though IMO (no reflection).

asavov · 2021-02-03T08:50:20Z

@dsyer Does this version satisfy-them-all ? :)

public class KubernetesReconcilerProcessor implements BeanDefinitionRegistryPostProcessor, Ordered {

  public static final String DEFAULT_SHARED_INFORMER_FACTORY_BEAN_NAME = "sharedInformerFactory";

  private final SharedInformerFactory sharedInformerFactory;

  private final String sharedInformerFactoryBeanName;

  private ConfigurableListableBeanFactory beanFactory;

  @Deprecated
  // TODO: Should we keep this for backward compatibility?
  public KubernetesReconcilerProcessor(SharedInformerFactory sharedInformerFactory) {
    this.sharedInformerFactory = sharedInformerFactory;
    this.sharedInformerFactoryBeanName = null;
  }

  public KubernetesReconcilerProcessor() {
    this(DEFAULT_SHARED_INFORMER_FACTORY_BEAN_NAME);
  }

  public KubernetesReconcilerProcessor(String sharedInformerFactoryBeanName) {
    notNull(sharedInformerFactoryBeanName, "SharedInformerFactory bean name is required");
    this.sharedInformerFactory = null;
    this.sharedInformerFactoryBeanName = DEFAULT_SHARED_INFORMER_FACTORY_BEAN_NAME;
  }

  @Override
  public int getOrder() {
    return KubernetesInformerFactoryProcessor.ORDER + 1;
  }

  @Override
  public void postProcessBeanFactory(ConfigurableListableBeanFactory beanFactory) {
    this.beanFactory = beanFactory;
  }

  @Override
  public void postProcessBeanDefinitionRegistry(BeanDefinitionRegistry registry) {
    for (String reconcilerName : beanFactory.getBeanNamesForType(Reconciler.class)) {

      Supplier<KubernetesControllerFactory> kubernetesControllerFactorySupplier;

      if (sharedInformerFactory != null) {
        kubernetesControllerFactorySupplier =
            () ->
                new KubernetesControllerFactory(
                    sharedInformerFactory, beanFactory.getBean(reconcilerName, Reconciler.class));
      } else {
        kubernetesControllerFactorySupplier =
            () ->
                new KubernetesControllerFactory(
                    beanFactory.getBean(sharedInformerFactoryBeanName, SharedInformerFactory.class),
                    beanFactory.getBean(reconcilerName, Reconciler.class));
      }

      BeanDefinition controllerFactoryBeanDefinition =
          BeanDefinitionBuilder.genericBeanDefinition(
                  KubernetesControllerFactory.class, kubernetesControllerFactorySupplier)
              .getBeanDefinition();

      registry.registerBeanDefinition(
          reconcilerName + "Controller", controllerFactoryBeanDefinition);
    }
  }
}

dsyer · 2021-02-03T08:52:39Z

That would work, but I don't see why we need to keep the old constructor, or the code path with if (sharedInformerFactory != null).

asavov · 2021-02-03T08:54:03Z

That would work, but I don't see why we need to keep the old constructor, or the code path with if (sharedInformerFactory != null).

If backward compatibility is not an issue - my pleasure to simplify it :)

dsyer · 2021-02-03T08:56:45Z

IMO no-one is using this class directly - it's all via autoconfiguration. Plus although it still says 11.0.1-SNAPSHOT in the top level POM, I know the next release will be 12.0.0 because of the Joda time changes. So I don't think we should care about backwards compatibility here. @yue9944882 would have to say for sure, but she also said that other day that this whole module is considered a prototype still, so breaking changes are expected.

asavov · 2021-02-03T09:17:06Z

IMO no-one is using this class directly - it's all via autoconfiguration. Plus although it still says 11.0.1-SNAPSHOT in the top level POM, I know the next release will be 12.0.0 because of the Joda time changes. So I don't think we should care about backwards compatibility here. @yue9944882 would have to say for sure, but she also said that other day that this whole module is considered a prototype still, so breaking changes are expected.

PR update with new code as discussed.

With this change KubernetesReconcilerProcessor conforms to BeanFactoryPostProcessor contract to register BeanDefinitions and not to manipulate bean instances. As a result Reconciler beans are: 1) not created at all by postProcessBeanFactory (which was the core issue) 2) participate in Spring container management hooks, and hence autowired

dsyer · 2021-02-03T12:33:41Z

...main/java/io/kubernetes/client/spring/extended/controller/KubernetesReconcilerProcessor.java

+  public void postProcessBeanFactory(ConfigurableListableBeanFactory beanFactory) {
+    for (String reconcilerName : beanFactory.getBeanNamesForType(Reconciler.class)) {
+
+      Supplier<KubernetesControllerFactory> kubernetesControllerFactorySupplier =


I guess this can be inlined. It's a matter of taste though.

Agree it's opinionated. I prefer readability vs. compact code.
By the way the PR is finally green... :)

dsyer · 2021-02-03T15:26:56Z

LGTM

asavov · 2021-02-03T16:04:24Z

LGTM

Nice! and thank you.

This is my first contribution, so the PR would be eventually merged by someone on kubernetes-client team with write access. No more actions on my site?

dsyer · 2021-02-04T11:15:01Z

Yes, we have to wait for @brendanburns and @yue9944882 . If they have no more changes to request they can merge it without you doing anything else.

yue9944882

/lgtm
/approve

thanks @dsyer @asavov!

k8s-ci-robot · 2021-02-07T09:01:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: asavov, yue9944882

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [yue9944882]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dsyer · 2021-02-08T09:30:38Z

@asavov are you going to do the same thing for KubernetesInformerFactoryProcessor?

asavov · 2021-02-08T12:57:04Z

KubernetesInformerFactoryProcessor

@dsyer I'd like to. Still at the moment not enough bandwidth.

dsyer · 2021-02-08T16:38:28Z

See #1532 for my solution

k8s-ci-robot added the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Jan 30, 2021

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jan 30, 2021

k8s-ci-robot requested review from brendandburns and yue9944882 January 30, 2021 20:04

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 30, 2021

asavov mentioned this pull request Jan 30, 2021

KubernetesReconcilerProcessor incorrectly instantiate beans #1511

Closed

yue9944882 reviewed Feb 1, 2021

View reviewed changes

k8s-ci-robot assigned yue9944882 Feb 1, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2021

asavov force-pushed the controllerFactory branch from 6fb7950 to aa27bb0 Compare February 2, 2021 07:24

k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. labels Feb 2, 2021

asavov force-pushed the controllerFactory branch from aa27bb0 to bf5e2f8 Compare February 2, 2021 07:49

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 2, 2021

asavov force-pushed the controllerFactory branch from bf5e2f8 to 62aadea Compare February 3, 2021 09:15

asavov force-pushed the controllerFactory branch 2 times, most recently from c48a82b to c7ab7d6 Compare February 3, 2021 11:04

asavov force-pushed the controllerFactory branch from c7ab7d6 to ad7f1b6 Compare February 3, 2021 11:30

dsyer reviewed Feb 3, 2021

View reviewed changes

asavov requested a review from yue9944882 February 3, 2021 14:24

yue9944882 reviewed Feb 7, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2021

k8s-ci-robot merged commit 4681f03 into kubernetes-client:master Feb 7, 2021

dsyer mentioned this pull request Feb 8, 2021

Ensure KubernetesInformerFactoryProcessor does not access any beans #1532

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Register ControllerFactory bean definition as opposite to bean instance #1516

Register ControllerFactory bean definition as opposite to bean instance #1516

asavov commented Jan 30, 2021 •

edited

k8s-ci-robot commented Jan 30, 2021

k8s-ci-robot commented Jan 30, 2021

yue9944882 left a comment

brendandburns commented Feb 1, 2021

asavov commented Feb 2, 2021

asavov commented Feb 2, 2021

asavov commented Feb 2, 2021

asavov commented Feb 2, 2021

dsyer commented Feb 2, 2021

asavov commented Feb 2, 2021

dsyer commented Feb 2, 2021

asavov commented Feb 2, 2021

dsyer commented Feb 2, 2021

asavov commented Feb 2, 2021

dsyer commented Feb 2, 2021

asavov commented Feb 2, 2021 •

edited

dsyer commented Feb 2, 2021

asavov commented Feb 3, 2021 •

edited

dsyer commented Feb 3, 2021

asavov commented Feb 3, 2021

dsyer commented Feb 3, 2021

asavov commented Feb 3, 2021

dsyer commented Feb 3, 2021

asavov commented Feb 3, 2021

dsyer Feb 3, 2021

asavov Feb 3, 2021

dsyer commented Feb 3, 2021

asavov commented Feb 3, 2021 •

edited

dsyer commented Feb 4, 2021

yue9944882 left a comment

k8s-ci-robot commented Feb 7, 2021

dsyer commented Feb 8, 2021

asavov commented Feb 8, 2021

dsyer commented Feb 8, 2021

Register ControllerFactory bean definition as opposite to bean instance #1516

Register ControllerFactory bean definition as opposite to bean instance #1516

Conversation

asavov commented Jan 30, 2021 • edited

k8s-ci-robot commented Jan 30, 2021

k8s-ci-robot commented Jan 30, 2021

yue9944882 left a comment

Choose a reason for hiding this comment

brendandburns commented Feb 1, 2021

asavov commented Feb 2, 2021

asavov commented Feb 2, 2021

asavov commented Feb 2, 2021

asavov commented Feb 2, 2021

dsyer commented Feb 2, 2021

asavov commented Feb 2, 2021

dsyer commented Feb 2, 2021

asavov commented Feb 2, 2021

dsyer commented Feb 2, 2021

asavov commented Feb 2, 2021

dsyer commented Feb 2, 2021

asavov commented Feb 2, 2021 • edited

dsyer commented Feb 2, 2021

asavov commented Feb 3, 2021 • edited

dsyer commented Feb 3, 2021

asavov commented Feb 3, 2021

dsyer commented Feb 3, 2021

asavov commented Feb 3, 2021

dsyer commented Feb 3, 2021

asavov commented Feb 3, 2021

dsyer Feb 3, 2021

Choose a reason for hiding this comment

asavov Feb 3, 2021

Choose a reason for hiding this comment

dsyer commented Feb 3, 2021

asavov commented Feb 3, 2021 • edited

dsyer commented Feb 4, 2021

yue9944882 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 7, 2021

dsyer commented Feb 8, 2021

asavov commented Feb 8, 2021

dsyer commented Feb 8, 2021

asavov commented Jan 30, 2021 •

edited

asavov commented Feb 2, 2021 •

edited

asavov commented Feb 3, 2021 •

edited

asavov commented Feb 3, 2021 •

edited