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

CNI charts are outdated #18576

Closed
jwendell opened this issue Nov 2, 2019 · 9 comments · Fixed by istio/installer#477
Closed

CNI charts are outdated #18576

jwendell opened this issue Nov 2, 2019 · 9 comments · Fixed by istio/installer#477
Milestone

Comments

@jwendell
Copy link
Member

jwendell commented Nov 2, 2019

Previously (or currently) CNI components can be - and are - encouraged to be installed on kube-system namespace:

$ helm template install/kubernetes/helm/istio-cni --name=istio-cni --namespace=kube-system | kubectl apply -f -

This way users can setup additional priorities for pod scheduling like system-node-critical.

With the istioctl manifest method, those components are installed in istio-system namespace.

Is there an way to install them in a different namespace like it's possible with helm?

@jwendell jwendell added this to the 1.4 milestone Nov 2, 2019
@jwendell
Copy link
Member Author

jwendell commented Nov 2, 2019

cc @sdake @ostromart @rlenglet

@rlenglet
Copy link
Contributor

rlenglet commented Nov 2, 2019

This is incorrect. It had to be created in kube-system due to using the system-node-critical priority class. This is no longer required in 1.4, where we use the system-cluster-critical priority class.

@rlenglet
Copy link
Contributor

rlenglet commented Nov 2, 2019

cc @ruigulala

@rlenglet
Copy link
Contributor

rlenglet commented Nov 2, 2019

That said, for some reason, istio/installer has forked the Helm chart seemingly months ago and never sinced again: https://github.com/istio/installer/blob/release-1.4/istio-cni/templates/daemonset.yaml It does even have the priorityClass. This is a recipe for disaster. @jwendell I'd recommend rewording the issue about istio/installer being way out of sync with upstream istio/cni.

@rlenglet
Copy link
Contributor

rlenglet commented Nov 2, 2019

istio/installer CNI charts being out of sync with istio/cni is a release blocker IMHO. @istio/release-managers-1-4

@jwendell
Copy link
Member Author

jwendell commented Nov 2, 2019

@rlenglet Do you know who is the source of truth when we talk about charts? It's confusing to me.

So, we have charts in istio/cni, then install/cni. We also have istio/operator. Where does istioctl manifest copy them from? Which one does it use?

@howardjohn
Copy link
Member

howardjohn commented Nov 2, 2019 via email

@jwendell
Copy link
Member Author

jwendell commented Nov 2, 2019

I've created istio/installer#477

Could someone from CNI team please verify if it's enough?

jwendell added a commit to jwendell/installer that referenced this issue Nov 2, 2019
jwendell added a commit to jwendell/installer that referenced this issue Nov 2, 2019
jwendell added a commit to jwendell/installer that referenced this issue Nov 2, 2019
@jwendell jwendell changed the title istioctl manifest: Install CNI daemonset/pods in another namespace CNI charts are outdated Nov 2, 2019
jwendell added a commit to jwendell/installer that referenced this issue Nov 2, 2019
jwendell added a commit to jwendell/installer that referenced this issue Nov 2, 2019
@sdake
Copy link
Member

sdake commented Nov 4, 2019

@rlenglet Do you know who is the source of truth when we talk about charts? It's confusing to me.

So, we have charts in istio/cni, then install/cni. We also have istio/operator. Where does istioctl manifest copy them from? Which one does it use?

The source of truth, for the time being, is istio/installer repo. We plan to delete charts from other repositories including this one to centralize the installation charts. In istio/istio, the install directory has taken a hard line on merging changes to installer first. Sadly that was not done with this repo.

Cheers
-steve

jwendell added a commit to jwendell/installer that referenced this issue Nov 4, 2019
And remove values_gke.yaml

Closes istio/istio#18576
istio-testing pushed a commit to istio/installer that referenced this issue Nov 4, 2019
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 a pull request may close this issue.

4 participants