search

minishift / minishift-addons

A repository for the community to exchange Minishift add-ons
Apache License 2.0
71 stars 86 forks source link

Issue #77 Istio 1.0.1 #158

Closed literalice closed 6 years ago

literalice commented 6 years ago

I've update the istio addon to 1.0.1.

I want to use knative according to this document. https://github.com/knative/docs/blob/master/install/Knative-with-OpenShift.md

If I want to setup knative correctly, this addon should have been applied.

centos-ci commented 6 years ago

Can one of the admins verify this patch?

literalice commented 6 years ago

The addon would be for the issue. https://github.com/minishift/minishift/issues/2677

praveenkumar commented 6 years ago

Add to whitelist.

praveenkumar commented 6 years ago

@literalice Check the CI failure also once.

literalice commented 6 years ago

@praveenkumar Thank you for your confirmation. I'm not sure why this istio-addon integration test is not stable. For now, please let me turn off the tests.

LalatenduMohanty commented 6 years ago

@kameshsampath can you please review this?

kameshsampath commented 6 years ago

@literalice - why is Istio 1.0.1 needed for knative here ? Knative uses a custom Istio and am not sure if that will work with knative.

@LalatenduMohanty i will start looking it into this w.r.t Istio upgrade

literalice commented 6 years ago

@kameshsampath Thank you for reviewing my code.

Knative uses a custom Istio and am not sure if that will work with knative.

I've confirm that this knative serving works on the istio addon at least. https://github.com/knative/docs/blob/master/install/getting-started-knative-app.md

Now official istio looks like to be used from 1.0.1 for knative. https://github.com/knative/serving/commit/7c20145326a35aae167ffcb23827f66f80d20543

kameshsampath commented 6 years ago

@literalice - i just now successfully deployed knative (0.11.1) on minishift. I see there is a discrepancy with the latest docs, as knative latest still uses 0.8.0 isito bits wheres the istio that is getting installed was v1.0.1 which was causing unsuccessful deployments, hence I decided to use the LTS version (0.8.0) of Istio to deploy on , i just replied on the severless ML on this.

Having said that I review this w.r.t Istio v1.0.1 to make sure its upgraded. Thanks for the PR :)

kameshsampath commented 6 years ago

comments on slack from @literalice

"In my PR, unfortunately I have to turn off SELinux on minishift vm… otherwise I ran into CrashLoop istio-init container. Is it related to your CrashLoopBackoff? https://github.com/minishift/minishift-addons/blob/4b5fa4e2052ab4358f7d5fbb94cbd5dcb6bef00b/add-ons/istio/istio.addon#L10"

praveenkumar commented 6 years ago

@literalice Thanks can you also please squash all the commits into a single commit because as of now I can see 6 different commits and it will not look good in the commit history or master once we merge it.

praveenkumar commented 6 years ago

@kameshsampath can you try out this PR and if it works as expected then please put a comment, we will merge it soon.

kameshsampath commented 6 years ago

@praveenkumar sorry for late reply, will do it tomorrow first job and confirm

kameshsampath commented 6 years ago

@literalice - why was the templates folder created. I feel that we should follow the https://github.com/Maistra/openshift-ansible/blob/maistra-0.1.0-ocp-3.1.0-istio-1.0.0/istio/Installation.md approach to install as that will make things easier for us and align with our Istio upstream efforts. Can you please update the add-on for it ?

literalice commented 6 years ago

@kameshsampath I've fixed it to use the istio-operator of Maistra. Could you confirm the fix? thanks.

kameshsampath commented 6 years ago

Thanks @literalice will check it tomorrow and confirm

matzew commented 6 years ago

@kameshsampath

What about the change in Istio v1.0.1 release, where automatic sidecar injection has removed privileged:true from init contianers...

should this PR do something like:


oc get cm istio-sidecar-injector -n istio-system -oyaml  \
| sed -e 's/securityContext:/securityContext:\\n      privileged: true/' \
| oc replace -f -
kameshsampath commented 6 years ago

@matzew as you have seen in the comments, we will be using Maistra (Origin Istio bits) which already takes care of this,

kameshsampath commented 6 years ago

@literalice can you please merge https://github.com/literalice/minishift-addons/pull/1 to your branch ?

@praveenkumar @LalatenduMohanty - I have reviewed the PR and tested locally on my minishift. It looks good to me. We can merge once @literalice can merge the changes to his branch

kameshsampath commented 6 years ago

@praveenkumar @LalatenduMohanty can we merge this ?

praveenkumar commented 6 years ago

@kameshsampath I will squash the commits and merge it.

praveenkumar commented 6 years ago

Merged to master.

matzew commented 6 years ago

great!

will this be auto installed ?

On Thu 4. Oct 2018 at 06:04, Praveen Kumar notifications@github.com wrote:

Merged to master.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/minishift/minishift-addons/pull/158#issuecomment-426874470, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJnzssPz0kavbsZXcTMO3putWuim2rpks5uhYH8gaJpZM4WWjvu .

-- Sent from Gmail Mobile

kameshsampath commented 6 years ago

@matzew no, need to install, enable and apply manually. check the readme of the addon for more instructions