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

Prometheus/promdash replication controller #8536

Merged
merged 2 commits into from
May 26, 2015

Conversation

jayunit100
Copy link
Member

Here is the replicatoinController implementation of the original pod for prometheus/promdash.

@tnguyen-rh
Copy link
Contributor

It would be nice to see the YAML retained, IMHO. Makes for a smaller diff, maybe.

@jayunit100
Copy link
Member Author

@timothysc, heres the HA example with the service running.

  • kubectl create -f prometheus-all.json and then
  • kubectl create -f prometheus-service.json and then you can access everything via the service endpoint .
[jay@rhbd prometheus]$ wget 10.0.0.92:9090
--2015-05-19 18:55:31--  http://10.0.0.92:9090/
Connecting to 10.0.0.92:9090... connected.
HTTP request sent, awaiting response... Read error (Connection reset by peer) in headers.
Retrying.

--2015-05-19 18:55:32--  (try: 2)  http://10.0.0.92:9090/
Connecting to 10.0.0.92:9090... connected.
HTTP request sent, awaiting response... Read error (Connection reset by peer) in headers.
Retrying.

--2015-05-19 18:55:34--  (try: 3)  http://10.0.0.92:9090/
Connecting to 10.0.0.92:9090... connected.
HTTP request sent, awaiting response... Read error (Connection reset by peer) in headers.
Retrying.

--2015-05-19 18:55:37--  (try: 4)  http://10.0.0.92:9090/
Connecting to 10.0.0.92:9090... connected.
HTTP request sent, awaiting response... No data received.
Retrying.

--2015-05-19 18:55:41--  (try: 5)  http://10.0.0.92:9090/
Connecting to 10.0.0.92:9090... connected.
HTTP request sent, awaiting response... 200 OK
Length: unspecified [text/html]
Saving to: ‘index.html.1’

index.html.1                    [ <=>                                       ]   8.86K  --.-KB/s   in 0.001s 

2015-05-19 18:55:41 (9.52 MB/s) - ‘index.html.1’ saved [9075]

@yujuhong
Copy link
Contributor

@jayunit100, could you suggest reviewers for this PR?

@jayunit100
Copy link
Member Author

How about my buddy @zmerlynn to review (he reviewed the original one)

Regarding the port to json: The reason i ported it is that, simply,

  • YAML is more error prone than json (for me).
  • And regarding the diff - its bound to be pretty nasty since we're cleaning up the extra fields while also switching from pod to RC.
  • @tnguyen-rh i have an idea for you : in the next iteration: lets hide yaml and json from users : and do something like k8petstore.sh which generates the files intermediately on the fly ? im happy to do that if you prefer it - im a big fan of hiding the static json / yaml files in kube applicatoins and replacing them w/ executable script

@tnguyen-rh
Copy link
Contributor

Were this a serious concern, i would agree to the generative approach, but this is an example project, so IMHO it should be transparent and approachable by as wide an audience as possible. Better to leave things out in the open, good, bad and/or ugly as they may be. Any right-minded reader will obviously think along the lines you propose, anyway. Let's just hope they do not fault us for thinking of the less-capable, as well. (To preclude such noise, you can add a "exercise for the reader" blurb somewhere, perhaps.)

Update to use a public IP so that services have a sane web-root
@jayunit100
Copy link
Member Author

Okay, updated to use publicIPs so that graphing works properly. Otherwise the web-root is all mucked up. Also there is a prometheus web-root option that might be useful but that seems a little hacky.

Working properly now w/ the exact public IP (previous PR only worked if you used the ip of the bound host)

screenshot from 2015-05-20 10 35 00

@zmerlynn
Copy link
Member

Agree with @tnguyen-rh. We've actually been preferring YAML - can we keep it here?

@jayunit100
Copy link
Member Author

i sincerely think allowing json is the right thing here, not just trying to save myself some time --- so i'll try to make a case for why i used (and switched to) json, for this specific case . Please read below, and just let me know the final decision. final call is yours :)

There are basically two reasons to use yaml over json in this case, lets go through them one by one:

  1. yaml is preferred. this is invalid, imo, here is why.
  • right now, kubernetes fully supports json and yaml, that is undisputed.
  • kubernetes shouldn't, then, ever review a PR based on wether it is yaml or json - they are both first class citizens.
  • more so - the best way to confirm json and yaml are at parity is by organically allowing developers to use them interchangeably (see 2 below) and submit patches in either format which use the api liberally. restricting this will inevitably lead to better support for one format than another... which will in turn lead to a biased user community... and maybe even a buggier product (because end user tests are always the most important and catch things that unit tests never think of).
  1. the original patch was yaml, so we should stay yaml
  • in this case - continuity is meaningless because there is no possible regression, because the feature has completely changed (unless you just want an example of how to convert a pod to an RC - in which case it might be useful). the original patch was not usable as a replication controller / introspective service and was just a placeholder to get the ball rolling. The reason that it was converted was largely because, now that it is much larger with more nested types, it had become very tricky to debug in yaml.

now, why did i pick json ?

  • not because i hate yaml , but just, because
  • i think for complex applications, dealing with the idiosyncracies of yaml can be difficult for some teams.
  • my team (currently the ones maintaining this code), was able to get this working in json , but
  • failed at getting it to work w/ yaml... and testing this is pretty nasty - as
    • there is no 1-1 conversion as of yet.
    • the YAML dashes w/ command options were confusing (and possible error sources) and so on.

the long term solution ?

im happy to standardize on Yaml, if and when either (1) kubernetes stops supporting json or (2) there is a conversion utility so that you dont have to rebuild a complex json kube application in yaml or vice versa for the sake of patch submissions.

*personally... *

For me, right now, i have to look in types.go often, and grep around in the various examples/ snippets to get the right yaml syntax (which is highly variable), and so its quite tricky to get it right and do a full conversion. So this is a time sink - but i don't want my personal ambitions to drive the outcome of your decision, so feel free to weight that as low as possible in the final decision here.

either way

if you guys insist im happy to do it in YAML, but it will take some cycles to redo it again. please do share your honest feedback on my argument ..... thanks for the feedback to @tnguyen-rh and @zmerlynn . ; and thanks for reading if you got this far :) :) now its your turn for the final decision :)

@tnguyen-rh
Copy link
Contributor

I replied off-github w/ a json2yaml link (RH internal).
Probably it's a SMOP to put a GPLv3 on it and publish it to the world.
Let's see what the future holds.

@derekwaynecarr
Copy link
Member

If you are comfortable with JSON, you can edit JSON and when you test it, just get the resource back in YAML from the server via kubectl get -o yaml > file.yaml

Like you I sometimes struggle with YAML, but I find the ability to work in JSON and let server convert for me works well enough.

Sent from my iPhone

On May 20, 2015, at 6:57 PM, Thien-Thi Nguyen notifications@github.com wrote:

I replied off-github w/ a json2yaml link (RH internal).
Probably it's a SMOP to put a GPLv3 on it and publish it to the world.
Let's see what the future holds.


Reply to this email directly or view it on GitHub.

@jayunit100
Copy link
Member Author

hi @zmerlynn . Any final verdict on this ? If YAML is required to merge I'm happy to do it, just let me know :) !

ps thanks for the idea @derekwaynecarr . based on zack's final feedback i may need to use that trick.

@zmerlynn
Copy link
Member

Sorry, been underwater. If it's easier for you to maintain in json right now, that's fine.

LGTM

@zmerlynn zmerlynn added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2015
saad-ali added a commit that referenced this pull request May 26, 2015
Prometheus/promdash replication controller
@saad-ali saad-ali merged commit e873387 into kubernetes:master May 26, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants