⚠️ Migration to the new events API

[clebs]

This Pull Request migrates from the old events API (k8s.io/client-go/tools/record) to the new API (k8s.io/client-go/tools/events).

Closes #2141

[k8s-ci-robot]

Hi @clebs. 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-sigs/prow repository.

[clebs]

Open Items:

• StartRecordingToSink: manage the context/channel in the provider on ensureRecording and Stop.
• StartWatcher: see if this is still needed and how to adapt it if it is.
• Leader election: resourcelock.ResourceLockConfig is still using the old API.
  This has now been solved by adding an adapter that turns old API calls into new API calls (albeit pretty simplistic). So that way on the backend we have only the new API and places that require the old can use the adapter.
• Update unit tests.

[alvaroaleman]

/ok-to-test

[alvaroaleman]

two questions

[sbueringer]

/assign

[sbueringer]

@clebs Just a heads up. If we want to get this into the next CR minor release the deadline is in 2-3 weeks. We want to release the next CR minor shortly after the Kubernetes minor which is scheduled for 27th August

[clebs]

Damn :/

I'm starting to lean towards intentionally not merging this into the current minor release, even if the tests are fine a few times in a row eventually, because I would prefer data from testgrid over a longer period to ensure we're not introducing an issue

Agreed, this needs more effort.
The goroutine from the watcher was definitely leaking since we were never stopping it but there seems to be more to it.

I am not able to properly work on this with my current laptop. I will resume testing when I have access to modern hardware in a few days.

[clebs]

@sbueringer I have revisited this and tipped by this comment I suspected that the event broadcaster goroutine leaks may happen due to a race condition where the broadcaster is started and shutdown in quick succession.

To avoid the race on the test, a short sleep has been added between start and shutdown. I went for this solution based on the comment linked above deliberately not attempting to fix this weakness in the current implementation.
It might be worth reconsidering once the old API is removed.

I have extensively tested this last commit and there were no leaks.

Realistically this is not an issue to usage since the broadcaster is usually long lived.

[sbueringer]

Thx!

Last two nits from my side

[sbueringer]

/assign @alvaroaleman

@clebs Feel free to squash if you want (merge bot can also take care of it)

[alvaroaleman]

looks good to me, agree regarding the comment about explaining the sleep. Could we link in our code to an upstream issue wrt using the new api in the leader elector?

Thanks a lot for your work here.

[clebs]

looks good to me, agree regarding the comment about explaining the sleep. Could we link in our code to an upstream issue wrt using the new api in the leader elector?

Thanks a lot for your work here.

@alvaroaleman added comment explaining the sleep and a comment on leader election referencing kubernetes/kubernetes#82846.

I would assume that the fact that apidiff is failing is expected and not required to merge?

[clebs]

/retest

[alvaroaleman]

I would assume that the fact that apidiff is failing is expected and not required to merge?

Yes, its an optional job and its purpose only is to tell us if there are breaking changes

[k8s-ci-robot]

@clebs: The following test 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-controller-runtime-apidiff	572fad4	link	false	/test pull-controller-runtime-apidiff

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-sigs/prow repository. I understand the commands that are listed here.

[clebs]

/test pull-controller-runtime-test

[alvaroaleman]

/hold

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: b9886ef51a4349573c33c1654905f0856b3b4486

[sbueringer]

Thank you very much!!

Really appreciate the persistence. As a side note, this is not the typical experience when contributing to CR :)

/lgtm
/approve
/hold cancel

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, clebs, sbueringer

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 [alvaroaleman,sbueringer]

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