Skip to content

rename in-memory channel#700

Closed
sbezverk wants to merge 3 commits into
knative:masterfrom
sbezverk:in_memory
Closed

rename in-memory channel#700
sbezverk wants to merge 3 commits into
knative:masterfrom
sbezverk:in_memory

Conversation

@sbezverk
Copy link
Copy Markdown
Contributor

@sbezverk sbezverk commented Jan 4, 2019

Signed-off-by: Serguei Bezverkhi sbezverk@cisco.com

Fixes #676

Rename in-memory-channel into in-memory

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Jan 4, 2019
@knative-prow-robot knative-prow-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 4, 2019
@knative-prow-robot
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sbezverk
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: grantr

If they are not already assigned, you can assign the PR to them by writing /assign @grantr in a comment when ready.

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@sbezverk
Copy link
Copy Markdown
Contributor Author

sbezverk commented Jan 4, 2019

/assign @Harwayne

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>
Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>
@sbezverk
Copy link
Copy Markdown
Contributor Author

sbezverk commented Jan 5, 2019

/assign @grantr for approval

@knative-prow-robot
Copy link
Copy Markdown
Contributor

@sbezverk: GitHub didn't allow me to assign the following users: for, approval.

Note that only knative members and repo collaborators can be assigned.
For more information please see the contributor guide

Details

In response to this:

/assign @grantr for approval

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.

Signed-off-by: Serguei Bezverkhi <sbezverk@cisco.com>
@Harwayne
Copy link
Copy Markdown
Contributor

Harwayne commented Jan 7, 2019

What happens to existing channels using the in-memory-channel provisioner? Do they keep working because the old CCP is unaltered by the update process?

@sbezverk
Copy link
Copy Markdown
Contributor Author

sbezverk commented Jan 7, 2019

@Harwayne the idea is, as long as CCPs are using old config map name, it should be transparent. I did not find any other dependencies.

@grantr
Copy link
Copy Markdown
Contributor

grantr commented Jan 7, 2019

Thanks @sbezverk!

What happens to existing channels using the in-memory-channel provisioner?

IIUC, after the upgrade there will be two CCPs each with an identical set of in-memory channel infrastructure. They'll both work, but future upgrades will only affect the new one. In a later release we'd want to delete the old one somehow.

This PR renames a bunch of objects that the operator and developer rarely/never need to know about:
in-memory-channel-controller -> in-memory-controller
in-memory-channel-dispatcher -> in-memory-dispatcher
in-memory-channel-dispatcher -> in-memory-dispatcher
in-memory-channel-dispatcher-config-map -> in-memory-dispatcher-config-map

Would the upgrade be easier if only the CCP object was renamed? We could leave the service accounts, deployments, etc that already exist in place, create a new CCP with the new name, and update the controller/dispatcher to handle both CCP names for this release.

/hold

@Harwayne just mentioned that he has more thoughts on this PR, so I'm holding it to give him time to weigh in.

@knative-prow-robot knative-prow-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 7, 2019
@grantr
Copy link
Copy Markdown
Contributor

grantr commented Jan 8, 2019

Moving this out of 0.3 milestone. @Harwayne and @sbezverk have conferred and decided that the path forward is a new PR that allows one controller to handle both the old CCP and a new asynchronous in-memory CCP.

/milestone clear

@Harwayne
Copy link
Copy Markdown
Contributor

Harwayne commented Jan 8, 2019

Talked with @grantr and have been convinced that we should try to re-use the existing infrastructure (Deployments, ConfigMap, etc.). Which means that their names shouldn't change. Instead have the single controller control both ClusterChannelProvisioners for one release, then remove the old in-memory-channel in the following release.

@sbezverk
Copy link
Copy Markdown
Contributor Author

sbezverk commented Jan 8, 2019

@grantr small correction. I think we should go with a separate controllers, one in-memory-channel (old) and in-memory controller with async (new) running in parallel for backward compatibility.

@Harwayne
Copy link
Copy Markdown
Contributor

I think this was superseded by #708. If so, can this PR be closed?

@matzew
Copy link
Copy Markdown
Member

matzew commented May 10, 2019

@Harwayne @grantr it's now in_memory ... in here:

https://github.com/knative/eventing/tree/master/cmd/in_memory

I guess... we are good?

@n3wscott
Copy link
Copy Markdown
Contributor

We have moved to in-memory and now are moving to CRDs over provisioners.
/close

@knative-prow-robot
Copy link
Copy Markdown
Contributor

@n3wscott: Closed this PR.

Details

In response to this:

We have moved to in-memory and now are moving to CRDs over provisioners.
/close

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.

maschmid pushed a commit to maschmid/eventing that referenced this pull request Aug 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Indicates the PR's author has signed the CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rename in-memory-channel ClusterChannelProvisioner

7 participants