Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consistent and unique container names #2810

Closed
wants to merge 1 commit into from

Conversation

aliok
Copy link
Member

@aliok aliok commented Mar 23, 2020

Fixes #2794

Proposed Changes

  • Change container names for consistency and uniqueness

@knative-prow-robot knative-prow-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 23, 2020
@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Mar 23, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aliok
To complete the pull request process, please assign grantr
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.

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

Copy link
Member

@matzew matzew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

/cc @vaikas

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 23, 2020
@@ -33,7 +33,7 @@ spec:
serviceAccountName: eventing-controller

containers:
- name: eventing-controller
- name: channel-broker-controller
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should just be controller

@@ -33,7 +33,7 @@ spec:
serviceAccountName: eventing-controller

containers:
- name: eventing-controller
- name: mt-broker-controller
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

controller

@n3wscott
Copy link
Contributor

I don't like it.

I also do not like eventing's usage of non-standard container names but I want them all to be the same for the same purpose, not all unique.

/hold

@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 Mar 23, 2020
@aliok
Copy link
Member Author

aliok commented Mar 23, 2020

@n3wscott

Eventing operator (and also serving operator) has a feature to override the images for containers that are created by the operator. It works like this:

apiVersion: operator.knative.dev/v1alpha1
kind: KnativeEventing
metadata:
...
spec:
  registry:
    override:
      eventing-controller: docker.io/my-eventing-controller

In this example, image for the container created with name eventing-controller is changed with docker.io/my-eventing-controller.

If all container names are not unique, this feature is not usable.

Otherwise, I don't care about the container names :D

but I want them all to be the same for the same purpose, not all unique.

Maybe we can come up with a proper naming strategy so that they're unique but still very similar as they have the same purpose?

@n3wscott
Copy link
Contributor

I think the correct way to implement this in the operator is to use something like an obj ref with a path to know which controller this image override will be targeting. The current plan seems very fragile.

It is not the container name you are trying to find, it should be the deployment name + the container name + a namespace.

@aliok
Copy link
Member Author

aliok commented Mar 24, 2020

Fair enough. I think we found another way on our side to override images and I won't pursue this.

/close

@knative-prow-robot
Copy link
Contributor

@aliok: Closed this PR.

In response to this:

Fair enough. I think we found another way on our side to override images and I won't pursue this.

/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.

@aliok aliok deleted the unique-container-names branch April 28, 2021 06:56
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. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Eventing container names are not unique
5 participants