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

Istio integration #421

Merged
merged 30 commits into from Feb 27, 2018

Conversation

Projects
None yet
3 participants
@life1347
Member

life1347 commented Dec 3, 2017

This is the very first step for fission to integrate with Istio and there are still couple problems need to be solved. For those interested in trying integrate fission with istio, following is the set up tutorial (Tutorial also post on here). (#278, #344)

Test Environment

  • Google Kubernetes Engine: 1.8.4-gke.0

Set Up

Create Kubernetes v1.8+ alpha cluster

Enable both RBAC & initializer features on kubernetes cluster.

** NOTICE ** kubectl logs -f is not working correctly on GKE alpha cluster right now.

$ gcloud container clusters create istio-demo \
	--enable-kubernetes-alpha \
    --machine-type=n1-standard-2 \
    --num-nodes=1 \
    --no-enable-legacy-authorization \
    --zone=<ZONE> \
    --cluster-version=1.8.4-gke.0

Grant cluster admin permissions

Grant admin permission for system:serviceaccount:kube-system:default and current user.

# for system:serviceaccount:kube-system:default
$ kubectl create clusterrolebinding --user system:serviceaccount:kube-system:default kube-system-cluster-admin --clusterrole cluster-admin

# for current user
$ kubectl create clusterrolebinding cluster-admin-binding --clusterrole=cluster-admin --user=$(gcloud config get-value core/account)

Set up Istio environment

Download Istio 0.2.12

curl -L https://git.io/getLatestIstio | sh -

Apply istio related YAML files

$ kubectl apply -f istio-0.2.12/install/kubernetes/istio.yaml
$ kubectl apply -f istio-0.2.12/install/kubernetes/istio-initializer.yaml

Since helm uses jobs to install charts, we have to remove jobs from initializers.rules.resources due to a known bug in istio pilot as a workaround.

$ kubectl edit initializerconfigurations
apiVersion: admissionregistration.k8s.io/v1alpha1
initializers:
- name: sidecar.initializer.istio.io
  rules:
  - apiGroups:
    - '*'
    apiVersions:
    - '*'
    resources:
    - deployments
    - statefulsets
    # As a workaround right now, remove jobs here 
    # due to a known bug (https://github.com/istio/issues/issues/93) 
    - jobs 
    - daemonsets

Set default namespace to fission

$ kubectl config set-context $(kubectl config current-context) --namespace=fission

Install fission

$ git clone https://github.com/fission/fission.git
$ cd fission/charts && git checkout istio-integration

fission-bundle docker image for demo: life1347/fission-istio-integration:1.0.0

$ helm init
$ helm install --debug --namespace fission --set enableIstio=true,image=life1347/fission-istio-integration,imageTag=latest,pullPolicy=Always --name istio-demo fission-all

Create custom function

Set environment

$ export FISSION_URL=http://$(kubectl --namespace fission get svc controller -o=jsonpath='{..ip}')
$ export FISSION_ROUTER=$(kubectl --namespace fission get svc router -o=jsonpath='{..ip}')

demo function: hello.js

module.exports = async function(context) {
    console.log(context.request.headers);
    return {
        status: 200,
        body: "Hello, World!\n"
    };
}

Create environment

$ fission env create --name nodejs --image fission/node-env

Create function

$ fission fn create --name h1 --env nodejs --code hello.js --method GET

Create route

$ fission route create --method GET --url /h1 --function h1

Access function

$ curl http://$FISSION_ROUTER/h1
Hello, World!

Install Add-ons

  • Prometheus
$ kubectl apply -f istio-0.2.12/install/kubernetes/addons/prometheus.yaml
$ kubectl -n istio-system port-forward $(kubectl -n istio-system get pod -l app=prometheus -o jsonpath='{.items[0].metadata.name}') 9090:9090

Web Link: http://127.0.0.1:9090/graph

  • Grafana

Please install Prometheus first.

grafana min

$ kubectl apply -f istio-0.2.12/install/kubernetes/addons/grafana.yaml
$ kubectl -n istio-system port-forward $(kubectl -n istio-system get pod -l app=grafana -o jsonpath='{.items[0].metadata.name}') 3000:3000

Web Link: http://127.0.0.1:3000/dashboard/db/istio-dashboard

  • Jaegar

jaeger min

$ kubectl apply -n istio-system -f https://raw.githubusercontent.com/jaegertracing/jaeger-kubernetes/master/all-in-one/jaeger-all-in-one-template.yml
$ kubectl port-forward -n istio-system $(kubectl get pod -n istio-system -l app=jaeger -o jsonpath='{.items[0].metadata.name}') 16686:16686

Web Link: http://localhost:16686


This change is Reviewable

@soamvasani soamvasani added this to the 0.5.0 milestone Jan 23, 2018

@life1347 life1347 force-pushed the istio-integration branch from e2f11b3 to b022533 Jan 24, 2018

@soamvasani soamvasani self-requested a review Jan 25, 2018

@life1347 life1347 force-pushed the istio-integration branch from b022533 to d4ce8c8 Jan 29, 2018

@soamvasani

This comment has been minimized.

Member

soamvasani commented Jan 29, 2018

Thanks @life1347 , this is pretty good. We should turn your PR description into a HOWTO on the docs site once it's merged.


Reviewed 20 of 20 files at r1.
Review status: all files reviewed at latest revision, 18 unresolved discussions, some commit checks failed.


common.go, line 30 at r1 (raw file):

}

func IsNetworkError(err error) bool {

Please add a function comment?


common.go, line 32 at r1 (raw file):

func IsNetworkError(err error) bool {
	if err == io.EOF {
		return true

Hmm... io.EOF is not necessarily a network error?


buildermgr/buildermgr.go, line 31 at r1 (raw file):

	useIstio := false
	if len(os.Getenv("ENABLE_ISTIO")) > 0 {

minor nit, can you make this have just one call to Getenv?


buildermgr/envwatcher.go, line 134 at r1 (raw file):

		if err != nil {
			if fission.IsNetworkError(err) {
				log.Printf("Encounter network error, retry again later: %v", err)

another small nit: "retrying later" or "will retry later"


buildermgr/envwatcher.go, line 165 at r1 (raw file):

		if fission.IsNetworkError(err) {
			log.Printf("Error syncing environment CRD resources due to network error: %v", err)
			return

Can you explain this a bit? Why do we return here? Should we retry?


controller/functionApi.go, line 130 at r1 (raw file):

				},
				Selector: sel,
				//ClusterIP: apiv1.ClusterIPNone,

remove commented out line?


controller/functionApi.go, line 133 at r1 (raw file):

			},
		}
		_, err = a.kubernetesClient.CoreV1().Services(a.functionNamespace).Create(&svc)

So I think we should have a separate controller loop that watches functions and does stuff like creating a service, instead of doing it inline in API requests. The intention of using CRDs is that users can create/modify our resources directly with kubectl, helm and such tools.

It this is too big a change I'm ok with this modification being done in a follow up PR.


controller/functionApi.go, line 218 at r1 (raw file):

	if a.useIstio {
		// delete all istio services belong to the function

Same story as the above comment. This will break if user does kubectl delete function ...


executor/fscache/functionServiceCache.go, line 166 at r1 (raw file):

			}
		} else {
			log.Printf("error caching fsvc: %v", err)

Hmm, why did you make this change? The new code won't log errors in some cases, right?


executor/poolmgr/gp.go, line 296 at r1 (raw file):

		// aggregation and storage will help.)
		log.Printf("Error in pod '%v', scheduling cleanup", name)
		// Ignore sleep here if istio feature is enabled, function pod

Why is this istio-related?


executor/poolmgr/gp.go, line 361 at r1 (raw file):

	// specialize pod with service
	if gp.useIstio {
		podIP = fmt.Sprintf("istio-%v.%v", metadata.Name, gp.namespace)

This is not unique enough... you need metadata.Namespace in there somewhere, otherwise name "A" in namespace "N1" will conflict with "A" in namespace "N2".


executor/poolmgr/gp.go, line 473 at r1 (raw file):

	var deployment *v1beta1.Deployment

	if gp.useIstio {

Instead of creating the whole deployment spec in two cases, could we have this if statement just modify the parts of the DeploymentSpec that are different for istio?


executor/poolmgr/gp.go, line 474 at r1 (raw file):

	if gp.useIstio {
		// Use long terminationGracePeriodSeconds for connection draining in case that

Would this be useful in non-istio case too?

(BTW we should put that function duration in the environment and function specs in the future.)


executor/poolmgr/gp.go, line 524 at r1 (raw file):

											Command: []string{
												"sleep",
												"360",

How about fmt.Sprintf("%v", gracePeriodSeconds) instead of the hard-coded constant?


executor/poolmgr/gp.go, line 666 at r1 (raw file):

	newLabels := gp.labelsForFunction(m)

	if gp.useIstio {

Can you explain this a bit more? Is this intended to remove old versions of the same function? Or something else...?


executor/poolmgr/gp.go, line 722 at r1 (raw file):

		svcHost = fmt.Sprintf("%v.%v", svcName, gp.namespace)
	} else if gp.useIstio {
		svcHost = fmt.Sprintf("istio-%v.%v:8888", m.Name, gp.namespace)

Again, we'll need m.Namespace in this name somewhere, to make it unique.


executor/poolmgr/gpm.go, line 154 at r1 (raw file):

		if err != nil {
			if fission.IsNetworkError(err) {
				log.Printf("Encounter network error, retry again: %v", err)

nit: "Encountered network error, retrying: ..."


test/test_utils.sh, line 211 at r1 (raw file):

    wait_for_service $id router

    echo Waiting for services become routable...

Waiting for services to become routable...


Comments from Reviewable

@soamvasani soamvasani modified the milestones: 0.5.0, 0.6.0 Jan 31, 2018

@life1347 life1347 force-pushed the istio-integration branch from d4ce8c8 to 073e930 Feb 14, 2018

@life1347

This comment has been minimized.

Member

life1347 commented Feb 14, 2018

Review status: 6 of 19 files reviewed at latest revision, 18 unresolved discussions.


common.go, line 30 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Please add a function comment?

Done.


common.go, line 32 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Hmm... io.EOF is not necessarily a network error?

Done.


buildermgr/buildermgr.go, line 31 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

minor nit, can you make this have just one call to Getenv?

Done.


buildermgr/envwatcher.go, line 134 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

another small nit: "retrying later" or "will retry later"

Done.


controller/functionApi.go, line 130 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

remove commented out line?

Done.


executor/fscache/functionServiceCache.go, line 166 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Hmm, why did you make this change? The new code won't log errors in some cases, right?

Done.


executor/poolmgr/gp.go, line 524 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

How about fmt.Sprintf("%v", gracePeriodSeconds) instead of the hard-coded constant?

Done.


executor/poolmgr/gpm.go, line 154 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

nit: "Encountered network error, retrying: ..."

Done.


test/test_utils.sh, line 211 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Waiting for services to become routable...

Readiness probe was added to wait_for_service(), remove this line.


Comments from Reviewable

@life1347

This comment has been minimized.

Member

life1347 commented Feb 14, 2018

Review status: 6 of 19 files reviewed at latest revision, 19 unresolved discussions.


buildermgr/envwatcher.go, line 165 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Can you explain this a bit? Why do we return here? Should we retry?

Yes, we should retry here. Done.


executor/poolmgr/gp.go, line 296 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Why is this istio-related?

If the istio feature is enabled, pods will be removed after terminationGracePeriod (6mins for now). It means that pod will stay alive for up to 11 mins (5 + 6 mins).

Buf from the comment below, we can remove these lines and adopt long terminationGracePeriodSeconds for non-istio case.


executor/poolmgr/gp.go, line 473 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Instead of creating the whole deployment spec in two cases, could we have this if statement just modify the parts of the DeploymentSpec that are different for istio?

Done.


executor/poolmgr/gp.go, line 474 at r1 (raw file):

	if gp.useIstio {
		// Use long terminationGracePeriodSeconds for connection draining in case that

Done.
Make sense to put function duration in specs, will do it in follow-up commit.


executor/poolmgr/gp.go, line 666 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Can you explain this a bit more? Is this intended to remove old versions of the same function? Or something else...?

Yes, it is intended to delete old version function pod.

As we discussed, if there are more than one function pods behind the services, then specialize request and function access requests may not be routed to the same pod, and thus the function cannot be served properly.

I've added comment to explain this.


Comments from Reviewable

@life1347 life1347 force-pushed the istio-integration branch from 5cbd693 to 738df1a Feb 21, 2018

@life1347

This comment has been minimized.

Member

life1347 commented Feb 21, 2018

Review status: 4 of 24 files reviewed at latest revision, 19 unresolved discussions.


executor/poolmgr/gp.go, line 361 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

This is not unique enough... you need metadata.Namespace in there somewhere, otherwise name "A" in namespace "N1" will conflict with "A" in namespace "N2".

Done.


executor/poolmgr/gp.go, line 722 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Again, we'll need m.Namespace in this name somewhere, to make it unique.

Done.


Comments from Reviewable

@life1347

This comment has been minimized.

Member

life1347 commented Feb 21, 2018

Review status: 4 of 24 files reviewed at latest revision, 19 unresolved discussions.


executor/poolmgr/gp.go, line 474 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Would this be useful in non-istio case too?

(BTW we should put that function duration in the environment and function specs in the future.)

Done.


Comments from Reviewable

@life1347

This comment has been minimized.

Member

life1347 commented Feb 21, 2018

Review status: 4 of 24 files reviewed at latest revision, 19 unresolved discussions.


controller/functionApi.go, line 133 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

So I think we should have a separate controller loop that watches functions and does stuff like creating a service, instead of doing it inline in API requests. The intention of using CRDs is that users can create/modify our resources directly with kubectl, helm and such tools.

It this is too big a change I'm ok with this modification being done in a follow up PR.

This change will be done in follow up PR.


controller/functionApi.go, line 218 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Same story as the above comment. This will break if user does kubectl delete function ...

Same above


Comments from Reviewable

@life1347 life1347 force-pushed the istio-integration branch 3 times, most recently from 610664e to 1f955d7 Feb 26, 2018

@soamvasani

This comment has been minimized.

Member

soamvasani commented Feb 27, 2018

LGTM. Please merge after addressing a few minor comments.


Reviewed 23 of 23 files at r2, 3 of 4 files at r3.
Review status: 28 of 29 files reviewed at latest revision, 9 unresolved discussions.


types.go, line 237 at r2 (raw file):

		// Optional, defaults to 'false'
		AllowedAccessExternalNetwork bool `json:"allowedAccessExternalNetwork,omitempty"`

Suggest "AllowAccessToExternalNetwork" instead.

Also suggest default to true. Most users will want to download dependencies.


builder/client/client.go, line 41 at r2 (raw file):

)

func MakeClient(builderUrl string, useIstio bool) *Client {

Can you call this flag something like "retry"? Because it is not an istio-specific flag at all. In fact, what if you unconditionally retry, does that cause any problems?


buildermgr/envwatcher.go, line 440 at r2 (raw file):

func (envw *environmentWatcher) createBuilderDeployment(env *crd.Environment) (*v1beta1.Deployment, error) {
	sharedMountPath := "/packages"
	sharedCfgMapPath := "/configs"

Not sure if this is intentionally in this change, but if you want configmap and secret suport in builders you will also need to add volumes to the deployment spec.


controller/functionApi.go, line 133 at r1 (raw file):

Previously, life1347 (Ta-Ching Chen) wrote…

This change will be done in follow up PR.

Please file an issue so we don't forget.


Documentation/docs-site/content/tutorial/istio-integration.md, line 2 at r3 (raw file):

---
title: "Enable Istio feature"

Awesome doc! How about "Enabling Istio on Fission" as the title?


Documentation/docs-site/content/tutorial/istio-integration.md, line 119 at r3 (raw file):

``` bash
$ helm install --debug --namespace $FISSION_NAMESPACE --set enableIstio=true --name istio-demo <chart-fission-all-url>

Remove --debug?


Documentation/docs-site/content/tutorial/istio-integration.md, line 122 at r3 (raw file):

Create a custom function

Just "Create a function" would work too.


environments/fetcher/cmd/main.go, line 131 at r2 (raw file):

			// Success
			resp.Body.Close()
			//On Success creates a file which is used as a readiness probe by Kubernetes for this container/pod

Why is this removed?


Comments from Reviewable

@life1347

This comment has been minimized.

Member

life1347 commented Feb 27, 2018

Review status: 11 of 29 files reviewed at latest revision, 9 unresolved discussions.


types.go, line 237 at r2 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Suggest "AllowAccessToExternalNetwork" instead.

Also suggest default to true. Most users will want to download dependencies.

Done.


builder/client/client.go, line 41 at r2 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Can you call this flag something like "retry"? Because it is not an istio-specific flag at all. In fact, what if you unconditionally retry, does that cause any problems?

Remove this flag, and do unconditionally retry.


buildermgr/envwatcher.go, line 440 at r2 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Not sure if this is intentionally in this change, but if you want configmap and secret suport in builders you will also need to add volumes to the deployment spec.

Let me fix this in PR512


controller/functionApi.go, line 133 at r1 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Please file an issue so we don't forget.

#515


environments/fetcher/cmd/main.go, line 131 at r2 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Why is this removed?

Discussed with @vishal, we decided to switch to httpget for overall consistency.
0f30727


Documentation/docs-site/content/tutorial/istio-integration.md, line 2 at r3 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Awesome doc! How about "Enabling Istio on Fission" as the title?

Done.


Documentation/docs-site/content/tutorial/istio-integration.md, line 119 at r3 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Remove --debug?

Done.


Documentation/docs-site/content/tutorial/istio-integration.md, line 122 at r3 (raw file):

Previously, soamvasani (Soam Vasani) wrote…

Just "Create a function" would work too.

Done.


Comments from Reviewable

@soamvasani

This comment has been minimized.

Member

soamvasani commented Feb 27, 2018

LGTM


Reviewed 1 of 5 files at r3, 18 of 18 files at r4.
Review status: all files reviewed at latest revision, 1 unresolved discussion.


Comments from Reviewable

@life1347 life1347 merged commit 23942fd into master Feb 27, 2018

1 of 2 checks passed

code-review/reviewable 1 discussion left
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@life1347

This comment has been minimized.

Member

life1347 commented Feb 27, 2018

Waiting for this moment for a long time. 🎉

@vishal-biyani

This comment has been minimized.

Collaborator

vishal-biyani commented Feb 28, 2018

Congrats on getting this done, I am looking forward to playing with it soon! 🎊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment