Documentation: grpc example #2307

pcj · 2018-04-08T03:32:30Z

k8s-ci-robot · 2018-04-08T03:32:33Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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. I understand the commands that are listed here.

pcj · 2018-04-08T03:36:55Z

/assign @aledbf

aledbf · 2018-04-08T15:09:16Z

@pcj thank you for this example. Please check the headers of the .go files to pass the validations.

About the PR:

Please remove the installation steps (and k8s manifest). Just assume the cluster is up and running with the ingress controller installed. We already tried this approach before and is really hard to maintain
You are adding too many requirements for the example. For instance I would prefer to build and host the docker image in quay.io with the ingress-nginx image and not require the use of gcloud registry
Same comment for the dns requirement. You are adding too many references to gcloud and some user can think this only works in that cloud provider.
I like the use of bazel but again most of the user just need grpcurl to test this. Can you just use that?

pcj · 2018-04-08T15:47:49Z

thanks @aledbf I'll fix up the go headers. So that I understand you correctly:

Remove the nginx-ingress/* directory completely, replacing it with a paragraph in the README.md to "install nginx-ingress in standard fashion";
Remove references to gcloud, replacing them with statements like "have persistent disk allocated", or something like that.
Remove dns instructions, replacing it with again a paragraph to the effect "have a DNS pointing to your ingress controller".
Remove all the bazel stuff, including the build steps for all the images, and all the clients. Also remove the fortune-teller application (?), replacing with an paragraph like "get your gRPC application up and running..".

What about the kube-cert-manager?

Anyway, I guess I should have asked this already, but what is your vision of the files necessary to have a functioning gRPC example? The one I have provided is pretty complete. So, what is the best combination of maintainability but still approachable? An example with multiple magical leaps within it will be pretty frustrating for people wanting to try it out.

Thanks for the prompt review! I'll try to keep to the same standard but I have to steal time away from other commitments, but I'll do my best.

aledbf · 2018-04-08T16:01:20Z

Remove the nginx-ingress/* directory completely, replacing it with a paragraph in the README.md to "install nginx-ingress in standard fashion";

Yes

Remove references to gcloud, replacing them with statements like "have persistent disk allocated", or something like that.

Remove dns instructions, replacing it with again a paragraph to the effect "have a DNS pointing to your ingress controller".

Yes

Remove all the bazel stuff, including the build steps for all the images, and all the clients. Also remove the fortune-teller application (?), replacing with an paragraph like "get your gRPC application up and running..".

No. Just use grpcurl to show the example working. We can publish your image in quay.io so the user doesn't need to build a custom one and have a reference to your code to see how to build a gRPC app

What about the kube-cert-manager?
Anyway, I guess I should have asked this already, but what is your vision of the files necessary to have a functioning gRPC example? The one I have provided is pretty complete.

Agree. Your example is self-contained which I like but you assume too much about the environment where the cluster is running.

So, what is the best combination of maintainability but still approachable? An example with multiple magical leaps within it will be pretty frustrating for people wanting to try it out.

Agree. That is why I just said you should use the default user guide and avoid using too many references to a particular cloud provider. Keep in mind this must work OOTB with minikube too.

aledbf · 2018-04-25T11:57:34Z

@pcj ping

pcj · 2018-04-25T12:58:58Z

Hi @aledbf sorry for delay, have not lost interest here... traveling now but I think I can get this taken care of by Friday or Saturday.

pcj · 2018-05-01T03:01:06Z

@aledbf Apologies for delay. Example is now just a README.md and a few yaml files. I moved implementation of the grpc service to a separate repo and pushed that to a container registry.

codecov-io · 2018-05-01T03:21:46Z

Codecov Report

Merging #2307 into master will decrease coverage by 0.13%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2307      +/-   ##
==========================================
- Coverage   41.75%   41.61%   -0.14%     
==========================================
  Files          74       74              
  Lines        5291     5291              
==========================================
- Hits         2209     2202       -7     
- Misses       2786     2792       +6     
- Partials      296      297       +1

Impacted Files	Coverage Δ
cmd/nginx/main.go	`21.37% <0%> (-4.83%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 87d1b8b...326a274. Read the comment docs.

aledbf · 2018-05-01T17:50:40Z

docs/examples/grpc/ingress.yaml

+metadata:
+  annotations:
+    kubernetes.io/ingress.class: "nginx"
+    ingress.kubernetes.io/ssl-redirect: "true"


nginx prefix missing

aledbf · 2018-05-01T17:51:08Z

docs/examples/grpc/cert.yaml

+  name: fortune-teller.stack.build
+  namespace: default
+  labels:
+    stable.k8s.psg.io/kcm.class: "kube-cert-manager"


is this label required?

aledbf · 2018-05-01T17:53:31Z

@pcj just two comments. Also please squash the commits and we are ready to merge

aledbf · 2018-05-01T18:47:49Z

/approve

pcj · 2018-05-02T12:55:22Z

OK, comments addressed & fixed, squashed into single commit and force pushed. Should be ready for merge. Thx.

antoineco · 2018-05-02T13:01:32Z

docs/examples/grpc/README.md

+
+### Step 2: the kubernetes `Service`
+
+```yaml


Maybe a link to the corresponding YAML files would be better than copying their content directly here?
Or the other way around, keep the content of the manifests in the markdown but delete the files.

Sure I can delete the redundant content. I prefer the readability of the yaml in the markdown but could be convinced otherwise. Unless another feels strongly, I'll just delete the yaml files.

I like the style in other docs where we provide complete kubectl command:

cat docs/examples/grpc/svc.yaml | kubectl apply --namespace=fortune-teller-app -f -

Instead of relative path docs/examples/grpc/svc.yaml we could also use remote path - I don't have any preference there.

@ElvinEfendi done.

antoineco · 2018-05-02T13:37:58Z

docs/examples/grpc/app.yaml

+    spec:
+      containers:
+      - name: fortune-teller-app
+        image: container.stack.build/pcj/fortune-teller:latest


Should we host this in the repo together with echoserver and other tools @aledbf?

That makes sense

I was planning on putting this in quay.io, but it seems they still don't support docker registry v2.2 so I decided against it (ref bazelbuild/rules_docker#102 gentle ping @philips).

E0502 07:46:27.210516 20444 docker_session_.py:335] Error during upload of: quay.io/pcj/fortune-teller:latest F0502 07:46:27.210813 20444 fast_pusher_.py:162] Error publishing quay.io/pcj/fortune-teller:latest: response: {'status': '415', 'content-length': '131', 'server': 'nginx/1.13.6', 'connection': 'keep-alive', 'date': 'Wed, 02 May 2018 13:46:27 GMT', 'content-type': 'application/json'} manifest invalid: {u'message': u'manifest schema version not supported'}

@aledbf Is hosting this image as @antoineco suggests part of this PR?

Sorry for the chatter here but I'm unclear on the requirements. Can I just mkdir images/grpc-fortune-teller, copy the existing code from https://github.com/stackb/grpc-fortune-teller and call it good, or do the images there need to work within some larger build workflow?

Can I just mkdir images/grpc-fortune-teller

Yes. After merging the PR I will publish the image in the same place than the ingress image

What will the anticipated image name be? (for the app.yaml image name). Something under quay.io, gcr.io/google_containers, or elsewhere?

quay.io/kubernetes-ingress-controller/grpc-fortune-teller:0.1

pcj · 2018-05-03T14:56:03Z

OK, I've added the grpc-fortune-teller under images/, updated image references to quay.io/kubernetes-ingress-controller/grpc-fortune-teller:0.1.

@aledbf, assuming you have bazel installed, you should be able to just type $ bazel run //app:push to push the image if your quay.io credentials are stored in ~/.docker/config.json (docker login). However, since quay.io does not support v2.2, it will likely fail with 'manifest version not supported'. In this case you'll just have to bazel run //app:image, re-tag the image and push it manually via docker push.

pcj · 2018-05-03T14:59:12Z

Unclear on why travis failed.

antoineco · 2018-05-03T15:52:26Z

My general feeling is that we could definitely use a simpler example for demonstrating the use of gRPC. Fileutil.go and Bazel especially make me feel like we're showcasing a full-fledged Go project.

Regarding the content of the PR itself:
/lgtm

aledbf · 2018-05-03T21:28:44Z

@pcj you need to add boilerplate code (license) to one or more files. Please execute ./hack/verify-boilerplate.sh to see where's the issue

pcj · 2018-05-03T22:53:02Z

Done. Boilerplate added to *.go files and refactored fileutil away.

Deps are managed by bazel so these will fail to show up in the vendor tree, triggering false positive build fail.

aledbf · 2018-05-10T16:25:57Z

/lgtm

k8s-ci-robot · 2018-05-10T16:26:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aledbf, antoineco, pcj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [aledbf]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 8, 2018

k8s-ci-robot assigned aledbf Apr 8, 2018

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 8, 2018

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 1, 2018

aledbf reviewed May 1, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 1, 2018

aledbf assigned ElvinEfendi and antoineco May 1, 2018

pcj force-pushed the grpc-example branch from 3f5002f to 308a8bb Compare May 2, 2018 12:53

antoineco suggested changes May 2, 2018

View reviewed changes

antoineco reviewed May 2, 2018

View reviewed changes

pcj force-pushed the grpc-example branch 2 times, most recently from 075aaa5 to 710127c Compare May 3, 2018 03:05

k8s-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 3, 2018

k8s-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label May 3, 2018

pcj force-pushed the grpc-example branch from 710127c to 3649fce Compare May 3, 2018 14:48

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 3, 2018

pcj force-pushed the grpc-example branch from 3649fce to 9a91a32 Compare May 3, 2018 22:51

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 3, 2018

pcj added 2 commits May 3, 2018 20:39

Example of using nginx-ingress with gRPC

e98e3f3

Exclude grpc-fortune-teller from go list

326a274

Deps are managed by bazel so these will fail to show up in the vendor tree, triggering false positive build fail.

pcj force-pushed the grpc-example branch from 4a2eef4 to 326a274 Compare May 4, 2018 02:40

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 10, 2018

k8s-ci-robot merged commit c80365d into kubernetes:master May 10, 2018

Documentation: grpc example #2307

Documentation: grpc example #2307

Conversation

pcj commented Apr 8, 2018

k8s-ci-robot commented Apr 8, 2018

pcj commented Apr 8, 2018

aledbf commented Apr 8, 2018

pcj commented Apr 8, 2018 • edited Loading

aledbf commented Apr 8, 2018

aledbf commented Apr 25, 2018

pcj commented Apr 25, 2018

pcj commented May 1, 2018

codecov-io commented May 1, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aledbf commented May 1, 2018

aledbf commented May 1, 2018

pcj commented May 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElvinEfendi May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcj May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcj May 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcj commented May 3, 2018

pcj commented May 3, 2018

antoineco commented May 3, 2018

aledbf commented May 3, 2018

pcj commented May 3, 2018

aledbf commented May 10, 2018

k8s-ci-robot commented May 10, 2018

pcj commented Apr 8, 2018 •

edited

Loading

codecov-io commented May 1, 2018 •

edited

Loading

ElvinEfendi May 2, 2018 •

edited

Loading

pcj May 2, 2018 •

edited

Loading

pcj May 2, 2018 •

edited

Loading