Skip to content

Conversation

@lilyhe123
Copy link
Contributor

@lilyhe123 lilyhe123 commented Feb 19, 2019

Update setup.sh:

  1. Parameterize release name for Voyager and Traefik operator.
  2. Change the release name of Voyager to avoid redundant words in the generated k8s resources.

For more information see https://jira.****/jira/browse/OWLS-71184.

# This script is to create or delete Ingress controllers. We support two ingress controllers: traefik and voyager.

MYDIR="$(dirname "$(readlink -f "$0")")"
VNAME=operator-v # release name of Voyager
Copy link
Member

Choose a reason for hiding this comment

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

What is the name of the generated service for Voyager operator after this change? voyager-operator-v?
And is it still traefik-operator for Traefik?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this change, install voyager via setup.sh:

$ kubectl -n voyager get all
NAME                                      READY     STATUS    RESTARTS   AGE
pod/voyager-operator-v-6fbd684b97-q7sf5   1/1       Running   0          56s

NAME                         TYPE        CLUSTER-IP      EXTERNAL-IP   PORT(S)             AGE
service/voyager-operator-v   ClusterIP   10.103.72.200   <none>        443/TCP,56791/TCP   56s

NAME                                 DESIRED   CURRENT   UP-TO-DATE   AVAILABLE   AGE
deployment.apps/voyager-operator-v   1         1         1            1           56s

NAME                                            DESIRED   CURRENT   READY     AGE
replicaset.apps/voyager-operator-v-6fbd684b97   1         1         1         56s

With this change, install traefik via setup.sh:

$ kubectl -n traefik get all
NAME                                      READY     STATUS    RESTARTS   AGE
pod/traefik-controller-56d9554485-pdvm6   1/1       Running   0          7d

NAME                                   TYPE        CLUSTER-IP      EXTERNAL-IP   PORT(S)                      AGE
service/traefik-controller             NodePort    10.107.235.71   <none>        80:30305/TCP,443:30443/TCP   7d
service/traefik-controller-dashboard   ClusterIP   10.103.77.77    <none>        80/TCP                       7d

NAME                                 DESIRED   CURRENT   UP-TO-DATE   AVAILABLE   AGE
deployment.apps/traefik-controller   1         1         1            1           7d

NAME                                            DESIRED   CURRENT   READY     AGE
replicaset.apps/traefik-controller-56d9554485   1         1         1         7d

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we still wants the release name has sort of identifier

Copy link
Member

@doxiao doxiao Feb 20, 2019

Choose a reason for hiding this comment

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

What about the resources that contain the TNAME (traefik-operator)? I was trying to see if we could make the resource names for voyager and traefik consistent.

Copy link
Contributor Author

@lilyhe123 lilyhe123 Feb 20, 2019

Choose a reason for hiding this comment

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

The resource names are controlled by the specific helm chart and the two charts: Voyager and Traefik, handle this differently. For Voyager, the names have pattern like "voyager-[release-name]xxx" while for Traefik the names have pattern like 'traefik-controllerxxx'.

Copy link
Member

@doxiao doxiao Feb 20, 2019

Choose a reason for hiding this comment

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

Thanks for the explanation. I personally vote for keeping the names as they are. Firstly, in the existing version, at least, the release names (which are what the samples have control) are consistent. Secondly, the official example in the Voyager helm site doc uses the same release name ("voyager-operator") as what the samples currently have.

Copy link
Contributor Author

@lilyhe123 lilyhe123 Feb 21, 2019

Choose a reason for hiding this comment

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

Thanks for your input.
As to the release name itself I also prefer "voyager-operator". The downside is the generated resource names will have redundant words as QA indicated. But since we know how the resource name is generated this is the expected behavior. And since the release name is now parameterized, QA can change it if they want. I'll change back the Voyager release name.

Copy link
Member

@doxiao doxiao Feb 21, 2019

Choose a reason for hiding this comment

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

Can we add something in the sample doc to explain?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Will do.

Copy link
Member

@doxiao doxiao left a comment

Choose a reason for hiding this comment

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

LGTM.

@rjeberhard rjeberhard merged commit 4cd4e62 into develop Feb 28, 2019
@lilyhe123 lilyhe123 deleted the voyager-fix branch March 1, 2019 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants