Generate names with kmeta.ChildName. Delete deprecated named resources

[odacremolbap]

Fixes #2842

Proposed Changes

Description

• There are 2 functions used to generate names for child objects. The one we should stick to is kmeta.ChildName while the one we should no longer use is util.GenerateFixedName
• For short generated names we can make both functions return the same name, but for long ones they do not.
• We can't be backwards compatible if we want to use only one function. This PR implements this candidate solution that will:
  • If the resource with the new name is found, behave as usual
  • If the resource with the new name is not found, look for the resource with the old name
    • If the resource with the old name is found, delete it
    • Create the resource with the new name

Notice

Some of the code added here, mainly tests and the bridging logic that looks for previously named resources, will need to be deleted after the deprecation period. Tests haven't been refactored to address the needs for long named resources, since all that code will be gone.

There are usages of util.GenerateFixedName at other sources at eventing-contrib and at the sample-source. I have opened an issue to track the later

Deprecation

Ideally util.GenerateFixedName would be removed to avoid further usage in 2(?) releases. A comment has been added to the function as a warning. Probably some further notice is needed.

Action Required

Subscriptions, API Server source adapter and Ping source adapters with long names will be re-created when their controllers included in this release run. That will trigger an unsubscribe action at the channel and a new subscribe, losing any state information like offsets along the way.

Release Note

- 🧽 Use a common generated names pattern for all objects

In order to homogenize all generated names API Server source, Ping source and Subscriptions might need to be regenerated with the new names. This release will delete any of those resources named with a non compatible pattern and create new ones. This will cause a temporary downtime.

[knative-prow-robot]

Hi @odacremolbap. Thanks for your PR.

I'm waiting for a knative 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.

[odacremolbap]

/assign @matzew

[tzununbekov]

/ok-to-test

[odacremolbap]

/hold

[knative-metrics-robot]

The following is the coverage report on the affected files.
Say /test pull-knative-eventing-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/apiserversource/apiserversource.go	80.2%	84.2%	4.0
pkg/reconciler/broker/trigger.go	87.5%	87.0%	-0.5
pkg/reconciler/mtbroker/trigger.go	88.2%	87.7%	-0.5
pkg/reconciler/pingsource/controller/pingsource.go	64.0%	68.6%	4.6

[odacremolbap]

/unhold

[n3wscott]

/lgtm
/approve

[knative-prow-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: n3wscott, odacremolbap

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 [n3wscott]

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

[grantr]

Subscriptions might need to be regenerated with the new names. This release will delete any of those resources named with a non compatible pattern and create new ones. This may cause a temporary downtime.

@odacremolbap Deleting and recreating a Subscription causes data loss in addition to downtime. The subscription's delivery state is also deleted (e.g. for a KafkaChannel, the offsets into the topic). From the user's perspective, any queued messages will be lost when the Subscription is deleted and recreated.

We need to be very clear to the user that their existing Broker state may be lost on upgrade if they have Subscriptions with names that would change. If we can't tell users exactly what their impact will be BEFORE upgrading, then I'd prefer to revert this until we can.

[odacremolbap]

yes, that was mentioned here #2800 (comment) , I though we all were aware of that and that we were ok with that change sooner than later.

I should've highlighted that at the release note, in any case the place to warn for this is the Action Required section at the release notes when cutting it.

Let's double check if everyone is ok with the change, if not, let's remove an plan for something different

@grantr @n3wscott

[grantr]

Sure, it may be that all we need is a clearer release note that tells users before they upgrade if they'll be affected or not and what the effect is. You can update the release note in your initial comment.

/cc @lionelvillard @vaikas for alternate perspectives.

[grantr]

Discussed this in the meeting and decided to write some guidance or a tool for users to help determine which of their brokers will be affected before upgrading. @odacremolbap graciously volunteered to help with this.

[odacremolbap]

@grantr @vaikas @lionelvillard @n3wscott

I'm working on the mini-reporting here: https://github.com/triggermesh/eventing-upgrade-check
Will have provide a container image, binaries and add usage to the Readme tomorrow.

How should we proceed?

• There is a cmd/upgrade at eventing repo I could try to integrate with. The reason I discarded it was that the tool I built returns a report and might require human action. Didn't sound natural to mix with an upgrade script

• The source and the image are hosted at Triggermesh repo/registry. If you want to host it at the project's repo/registry I can add the headers to all files and leave them ready.

• When both questions above are clear I will update the Actions required above.

[grantr]

Thanks, that repo looks great! Docs 😍

Sorry for the delay in answering:

• I agree, no need to integrate with the upgrade script
• Seems fine to me to host in triggermesh org and link to the repo there if you're ok with that

[grantr]

I added a pointer to the tool to the release note above:

Subscriptions (including those used internally by Brokers) will be re-created on upgrade from a previous release. That will trigger an unsubscribe action at the channel and a new subscribe, losing any state information like offsets along the way. To check if your cluster has any Subscription that would be affected, use the upgrade check tool at https://github.com/triggermesh/eventing-upgrade-check.