✨ bump controller-runtime to v0.15.0

[kkkkun]

bump controller-runtime to v0.15.0-beta.0

Partial : #3376

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: kkkkun
Once this PR has been reviewed and has the lgtm label, please assign varshaprasad96 for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[k8s-ci-robot]

Hi @kkkkun. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[camilamacedo86]

Thank you for looking into that.

Following some considerations:

• You need to update your branch with the master. See that when you run make generate, it made updates on the samples reverting changes addressed. It will fail in the CI

• I recommend you pull the PR ⚠️ (go/v4): upgrade controller runtime from 0.14 to 0.15  #3394 and continue the work since it fixed many issues. We have breaking changes to address regards webhooks and their templates. Either these changes will affect the hack scripts that generate the docs samples.

The above PR shows fix the issues, but we are still facing a problem in the e2e tests that need to be fixed

• We either need to bump a commit of the declarative plugin where it is using go 1.20, controller-runtime latest release + k8s 1.27

See: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/declarative/v1/api.go#L35 (we usually contribute with the project to ensure that all is bumped to use the latest k8s version)

[kkkkun]

Thank you for looking into that.

Following some considerations:

• You need to update your branch with the master. See that when you run make generate, it made updates on the samples reverting changes addressed. It will fail in the CI
• I recommend you pull the PR WIP: ⚠️ (go/v4): upgrade controller runtime from 0.14 to 0.15 alpha #3394 and continue the work since it fixed many issues. We have breaking changes to address regards webhooks and their templates. Either these changes will affect the hack scripts that generate the docs samples.

The above PR shows fix the issues, but we are still facing a problem in the e2e tests that need to be fixed

• We either need to bump a commit of the declarative plugin where it is using go 1.20, controller-runtime latest release + k8s 1.27

See: https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/declarative/v1/api.go#L35 (we usually contribute with the project to ensure that all is bumped to use the latest k8s version)

Thanks. I cherry-pick #3394 and update controller-runtime to v0.15.0-beta.0

[camilamacedo86]

/ok-to-test

[camilamacedo86]

We need to check if that would be the best approach but I think we can either to do in a follow up.

[camilamacedo86]

@kkkkun,

See the CI results.
We need either to do the bump https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/declarative/v1/api.go#L35 with the commit which has the bumps either, see kubernetes-sigs/kubebuilder-declarative-pattern#335

However, the e2e should be passing. It should only fail in the pull-kubebuilder-test because of the above.

So, your PR will be good when it has been passing in ALL tests less pull-kubebuilder-test and APIDiff

[kkkkun]

@kkkkun,

See the CI results. We need either to do the bump https://github.com/kubernetes-sigs/kubebuilder/blob/master/pkg/plugins/golang/declarative/v1/api.go#L35 with the commit which has the bumps either, see kubernetes-sigs/kubebuilder-declarative-pattern#335

However, the e2e should be passing. It should only fail in the pull-kubebuilder-test because of the above.

Should we wait PR kubernetes-sigs/kubebuilder-declarative-pattern#335 merge firstly ?

[camilamacedo86]

@kkkkun,

Should we wait PR kubernetes-sigs/kubebuilder-declarative-pattern#335 merge firstly ?

You can comment the test for the declarative here: https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/testdata/test.sh#L36

Then, ALL tests should be passing less APIDiff CI check.
If all tests been passing that that means the upgrade was done successfully

[camilamacedo86]

@kkkkun can you revert ?

[kkkkun]

pull-kubebuilder-test failed because kubebuilder-declarative-pattern need update controller-runtime firstly.

[kkkkun]

pull-kubebuilder-e2e-k8s-1-25-3 pull-kubebuilder-e2e-k8s-1-26-0 and
pull-kubebuilder-e2e-k8s-1-27-1

running: docker rmi -f e2e-test/controller-manager:awpk
  << Timeline
  [FAILED] Timed out after 240.001s.
  Expected success, but got an error:
      <*errors.errorString | 0xc0004f8a40>: 
      curl pod in Running status
      {
          s: "curl pod in Running status",
      }
  In [It] at: /home/prow/go/src/sigs.k8s.io/kubebuilder/test/e2e/v4/plugin_cluster_test.go:95 @ 05/20/23 05:36:33.262

I don't know those failed reason.

[camilamacedo86]

Hi @kkkkun,

You first need to sort out the breaking changes: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kubebuilder/3421/pull-kubebuilder-test/1659791412230623232

See that we need to change the scaffolds to address the changes on the controller-runtime

Then, regards:

running: docker rmi -f e2e-test/controller-manager:awpk
  << Timeline
  [FAILED] Timed out after 240.001s.
  Expected success, but got an error:
      <*errors.errorString | 0xc0004f8a40>: 
      curl pod in Running status
      {
          s: "curl pod in Running status",
      }
  In [It] at: /home/prow/go/src/sigs.k8s.io/kubebuilder/test/e2e/v4/plugin_cluster_test.go:95 @ 05/20/23 05:36:33.262

See that we create a pod with a curl image so that we can test the metrics config and been exported with prometheus enabled. In this case, the error is here :

kubebuilder/test/e2e/v4/plugin_cluster_test.go

Lines 305 to 317 in a6fca17

	By("validating that the curl pod is running as expected")
	verifyCurlUp := func() error {
	// Validate pod status
	status, err := kbc.Kubectl.Get(
	true,
	"pods", "curl", "-o", "jsonpath={.status.phase}")
	ExpectWithOffset(3, err).NotTo(HaveOccurred())
	if status != "Completed" && status != "Succeeded" {
	return fmt.Errorf("curl pod in %s status", status)
	}
	return nil
	}
	EventuallyWithOffset(2, verifyCurlUp, 240*time.Second, time.Second).Should(Succeed())

You can try add either running by replacing if status != "Completed" && status != "Succeeded" { with if status != "Completed" && status != "Succeeded" && status != "Running" {. It seems that when we bump it will also required.

[kkkkun]

Hi @kkkkun,

You first need to sort out the breaking changes: https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kubebuilder/3421/pull-kubebuilder-test/1659791412230623232

In my env, it's normal. I would check again.

You can try add either running by replacing if status != "Completed" && status != "Succeeded" { with if status != "Completed" && status != "Succeeded" && status != "Running" {. It seems that when we bump it will also required.
Fixed. Thanks

[camilamacedo86]

Hi @kkkkun,

See that we still unable to bump. We need to address the breaking changes; https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kubebuilder/3421/pull-kubebuilder-test/1660213955664023552

[kkkkun]

Hi @kkkkun,

See that we still unable to bump. We need to address the breaking changes; https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/kubernetes-sigs_kubebuilder/3421/pull-kubebuilder-test/1660213955664023552

The break changes caused that kubernets is update to 1.27.2, but sigs.k8s.io/kubebuilder-declarative-pattern is used v0.13.0-beta.1.0.20230113160225-18dbaf5fcd85 which relies on k8s.io/client-go v0.26.3. We need to merge kubernetes-sigs/kubebuilder-declarative-pattern#335 firstly and upgrade sigs.k8s.io/kubebuilder-declarative-pattern here.

The code is
https://github.com/kubernetes-sigs/kubebuilder-declarative-pattern/pull/335/files#diff-1ae880bba1fe98d869b10684825cb031b82ef46feb717f22469a8da7c6f6f73bR542

-func (rt recordTrigger) Create(ev event.CreateEvent, _ workqueue.RateLimitingInterface) {
+func (rt recordTrigger) Create(ctx context.Context, ev event.CreateEvent, _ workqueue.RateLimitingInterface) {

Here we just need to upgrade kubebuilder-declarative-pattern when kubernetes-sigs/kubebuilder-declarative-pattern#335 merged. Do nothing except it.

[camilamacedo86]

Hi @kkkkun,

That is great. We have only one small issue to solve now in the tests, see:

running: kubectl delete -f https://github.com/jetstack/cert-manager/releases/download/v1.5.3/cert-manager.yaml
  �[1mSTEP:�[0m removing controller image and working dir �[38;5;243m@ 05/23/23 14:25:07.617�[0m
  running: docker rmi -f e2e-test/controller-manager:odnc
  �[38;5;243m<< Timeline�[0m

  �[38;5;9m[FAILED] Timed out after 10.049s.
  Expected
      <string>:   % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                       Dload  Upload   Total   Spent    Left  Speed
      
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0*   Trying 10.96.33.238:8443...
      * TCP_NODELAY set
      * connect to 10.96.33.238 port 8443 failed: Connection refused
      * Failed to connect to e2e-odnc-controller-manager-metrics-service.e2e-odnc-system.svc port 8443: Connection refused
      
  0     0    0     0    0     0      0      0 --:--:-- --:--:-- --:--:--     0
      * Closing connection 0
      curl: (7) Failed to connect to e2e-odnc-controller-manager-metrics-service.e2e-odnc-system.svc port 8443: Connection refused
      
  to contain substring
      <string>: < HTTP/2 200�[0m
  �[38;5;9mIn �[1m[It]�[0m�[38;5;9m at: �[1m/home/prow/go/src/sigs.k8s.io/kubebuilder/test/e2e/v4/plugin_cluster_test.go:88�[0m �[38;5;243m@ 05/23/23 14:24:08.543�[0m
�[38;5;243m------------------------------�[0m
�[38;5;9m• [FAILED] [259.215 seconds]�[0m

It is failing to connect in to pod to get the metrics.

[Kavinjsir]

The overall updates looks good to me.

Nits: current updates seems NOT passed by make generate-docs:

Generating cronjob tutorial...
INFO[0000] Generating the sample context of Cronjob...
INFO[0000] destroying directory for sample project
  running: docker rmi -f
INFO[0000] refreshing tools and creating directory...
  cleaning up tools
  preparing testing directory: docs/book/src/cronjob-tutorial/testdata/project/
INFO[0000] Initializing the project
  running: kubebuilder init --plugins go/v4 --domain tutorial.kubebuilder.io --repo tutorial.kubebuilder.io/project --license apache2 --owner The Kubernetes authors
INFO[0001] Adding a new config type
  running: kubebuilder create api --group batch --version v1 --kind CronJob --resource --controller
INFO[0005] Implementing admission webhook
  running: kubebuilder create webhook --group batch --version v1 --kind CronJob --defaulting --programmatic-validation
TODO: update tutorial
INFO[0006] 1. update specs
INFO[0006] 2. update webhook
ERRO[0006] error fixing cronjob_webhook.go: unable to find the content to be replaced
exit status 1
make: *** [generate-docs] Error 1

Looks like, update_webhook requires update. Check on other steps might also necessary.

[Kavinjsir]

@camilamacedo86 @kkkkun
From my local, the similar error happens when testing on e2e/v4, where there is the test spec calling GenerateV4ComponentConfig

Passing the config file config/manager/controller_manager_config.yaml would fail. I think the scaffolded code line using struct from controller-runtime does NOT work any more:

options, err = options.AndFrom(ctrl.ConfigFile().AtPath(configFile))

I tried running the controller locally, where the error came:

❯ go run cmd/main.go --config config/manager/controller_manager_config.yaml
2023-05-29T13:41:56-04:00       ERROR   setup   unable to load the config file  {"error": "could not decode file into runtime.Object"}
main.main
        /Users/jintony/pro/kubebuilder-fork/test/e2e/v4/e2e-ycxy/cmd/main.go:70
runtime.main
        /usr/local/Cellar/go/1.20.4/libexec/src/runtime/proc.go:250
exit status 1

This is possibly because controller-runtime@0.15.0 is deprecating Component-config, see:
https://github.com/kubernetes-sigs/controller-runtime/releases/tag/v0.15.0

Guess the following PR brings the breaking change:
https://github.com/kubernetes-sigs/controller-runtime/pull/2149/files?diff=split&w=0

One possible hotfix maybe to change the code by referencing here:
https://github.com/kubernetes-sigs/controller-runtime/blob/595f5695653b7f7d4d022b3ab438a94377799c16/pkg/config/config_test.go#L37

---

However, since such feature is deprecating, I'm wondering if we'd keep the support for it still? Would it be too earlier or good to remove the scaffolding/project of component-config ?

[kkkkun]

make generate-docs

I test my local passed.

make generate-docs 
go run hack/docs/generate_samples.go
Generating documents...
Generating component-config tutorial...
INFO[0000] Generating the sample context of component-config... 
INFO[0000] destroying directory for sample project      
  running: docker rmi -f 
INFO[0000] refreshing tools and creating directory...   
  cleaning up tools
  preparing testing directory: docs/book/src/component-config-tutorial/testdata/project/
INFO[0000] Initializing the project                     
  running: kubebuilder init --domain tutorial.kubebuilder.io --repo tutorial.kubebuilder.io/project --license apache2 --owner The Kubernetes authors --plugins=go/v4 --component-config
INFO[0015] Adding a new config type                     
  running: kubebuilder create api --group config --version v2 --kind ProjectConfig --resource --controller=false --make=false
  running: make build
Generating cronjob tutorial...
INFO[0096] Generating the sample context of Cronjob...  
INFO[0096] destroying directory for sample project      
  running: docker rmi -f 
INFO[0096] refreshing tools and creating directory...   
  cleaning up tools
  preparing testing directory: docs/book/src/cronjob-tutorial/testdata/project/
INFO[0096] Initializing the project                     
  running: kubebuilder init --plugins go/v4 --domain tutorial.kubebuilder.io --repo tutorial.kubebuilder.io/project --license apache2 --owner The Kubernetes authors
INFO[0097] Adding a new config type                     
  running: kubebuilder create api --group batch --version v1 --kind CronJob --resource --controller
INFO[0100] Implementing admission webhook               
  running: kubebuilder create webhook --group batch --version v1 --kind CronJob --defaulting --programmatic-validation
TODO: update tutorial
  running: go get github.com/robfig/cron
  running: make build
  running: go get github.com/robfig/cron
  running: make build

[camilamacedo86]

The @kkkkun,

The above failures are in the e2e tests. Those are that one that we need to solve.

@Kavinjsir,

That #3421 (comment) is great work.

So, we need to change our scaffold to do as suggested.
If we need we can scaffold the file and inform in the options when componet-config flag is used.

[hsyed-dojo]

@Kavinjsir I found a work around for the issue you are seeing, just don't register the custom config type with the scheme. It doesn't seem to be necessary.

[varshaprasad96]

@Kavinjsir @kkkkun @camilamacedo86 @hsyed-dojo @everettraven I just took a look at the code base and the changes made in controller-runtime. When dug deeper, the error looks like:

2023-06-02T14:37:39-04:00       ERROR   setup   unable to load the config file  {"error": "could not decode file into runtime.Object no kind \"ControllerManagerConfig\" is registered for version \"controller-runtime.sigs.k8s.io/v1alpha1\" in scheme \"pkg/runtime/scheme.go:100\""}

This means when ctrl.File is called in cmd/main.go it calls in here which registers the ComponentConfig with the scheme. Though the field has been deprecated, the ComponentConfig type should still be registered with controller-runtime.sigs.k8s.io/v1alpha1 scheme which seems to be missing. This seems to be a bug somewhere in c-r if I'm understanding right (please correct me if I'm wrong and am missing something).

This aside, based on the deprecation mentioned in controller-runtime, it looks like the correct method to pass the config would be in manager.New through manager.Options{Controller: config.Controller{}}. Even though we use the CopmponentConfigSpec looks like the controller.config will accept only specific fields as mentioned here (https://github.com/kubernetes-sigs/controller-runtime/pull/2149/files?diff=split&w=0#diff-1756df02166e2e4f37f79da02f44f8ee4ecd85537673b2be5a91d28a71451c4fR22). Us making changes and keeping the code for reading options through the file would still not translate to the manager struct. Hence the options we have now for moving forward are:

1. To pass the values through config.Controller. For example:

mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), manager.Options{
		Scheme: scheme,
		Controller: config.Controller{
			NeedLeaderElection:      pointer.Bool(true),
			MaxConcurrentReconciles: 2,
		},
	})

2. Remove only the component config tests for now, Deprecate the component config related code and examples in KB, add comments on what are the next steps to move forward based on the decision that will eventually stem from the issue RFC: Deprecation and removal of ComponentConfig controller-runtime#895.

I would vote for (2) as of now, since the decision on removing the component config is not final yet in controller-runtime. If there are folks ready to maintain the code in controller-runtime, then we can support the same here. Till then we can deprecate and mention in our docs stating that component config option is deprecated from KB, is not tested or officially supported. The decision on whether to remove it entirely, will be based on controller-runtime and would not block this PR.

[hsyed-dojo]

I vote for 2 as well. I switched our operators away from component config today.

What I'd really like from the controller runtime is a prescribed solution for watching and reacting to a specific/singleton custom resource under the supervision of the manager. Kubebuilder could then just document how to do "custom component config" without any templating changes needed.

[varshaprasad96]

Since there is majority on Option (2), @kkkkun would it be possible for you to deprecate the component config related code and tests in this PR for moving forward with this. If needed, I can help in creating a PR against your branch, to move things faster. (Having a separate PR would be a duplicate since we need c-r 0.15.0 for deprecation).

Feel free to let us know. Thanks for working on this!

cc: @camilamacedo86 @Kavinjsir

[kkkkun]

Since there is majority on Option (2), @kkkkun would it be possible for you to deprecate the component config related code and tests in this PR for moving forward with this. If needed, I can help in creating a PR against your branch, to move things faster. (Having a separate PR would be a duplicate since we need c-r 0.15.0 for deprecation).

Feel free to let us know. Thanks for working on this!

cc: @camilamacedo86 @Kavinjsir

That's OK. Thanks for your help.

[camilamacedo86]

HI @kkkkun,

(Having a separate PR would be a duplicate since we need c-r 0.15.0 for deprecation).
Yes, if you want to push a PR for the deprecation for we get it merged prior this one would be the best approach

Can you do that?
Note that we need to:

• Remove the e2e tests for component config
• Deprecate the flags and make clear that it was deprecated in controller-runtime
• Deprecate it in the docs. We need to

a) Create an issue in controller-runtime with:

This means when ctrl.File is called in cmd/main.go it calls in here which registers the ComponentConfig with the scheme. Though the field has been deprecated, the ComponentConfig type should still be registered with controller-runtime.sigs.k8s.io/v1alpha1 scheme which seems to be missing. This seems to be a bug somewhere in c-r if I'm understanding right (please correct me if I'm wrong and am missing something).

b) Create a WARNING note and link this issue to explain for our users that from the next release 3.11.0 we can no longer ensure that it is working due this issue + it is deprecated due CONTROLLER RUNTIME decide to deprecate it and we should link either: kubernetes-sigs/controller-runtime#895. Therefore, due controller runtime be unable to ensure the backwords compatibilities when it was deprecate and its release 0.15.0 we are no longer able to ensure this feature and it should be removed in the future.

c/c @varshaprasad96 @Kavinjsir @everettraven ^ ⚠️ IMPORTANT we make very clear that we have nothing to do about it and that is a controller-runtime decision and issue.

[camilamacedo86]

I could make the bump work in : #3394
But we must need to move forward with: #3442

@kkkkun @Kavinjsir @varshaprasad96 ˆ

[k8s-ci-robot]

@kkkkun: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubebuilder-test	52bac91	link	true	/test pull-kubebuilder-test
pull-kubebuilder-e2e-k8s-1-25-3	52bac91	link	true	/test pull-kubebuilder-e2e-k8s-1-25-3
pull-kubebuilder-e2e-k8s-1-26-0	52bac91	link	true	/test pull-kubebuilder-e2e-k8s-1-26-0
pull-kubebuilder-e2e-k8s-1-27-1	52bac91	link	true	/test pull-kubebuilder-e2e-k8s-1-27-1

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

[kkkkun]

@Kavinjsir @varshaprasad96 ˆ

Thanks you. Continue to bump in #3394. I would close this.