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

Add are you alive example #5646

Merged
merged 8 commits into from
Mar 2, 2020
Merged

Conversation

J0
Copy link
Contributor

@J0 J0 commented Jan 2, 2020

This Pull Request aims to add an example of how to set up an are-you-alive deployment for Vitess. This is open sourced from PlanetScale's internal version of Are-You-Alive.

Signed-off-by: Lee Yi Jie Joel lee.yi.jie.joel@gmail.com

Signed-off-by: Lee Yi Jie Joel <lee.yi.jie.joel@gmail.com>
@J0 J0 marked this pull request as ready for review January 2, 2020 22:57
@J0 J0 requested a review from sougou as a code owner January 2, 2020 22:57
@deepthi
Copy link
Member

deepthi commented Jan 21, 2020

@sverch @aquarapid can you review this?

@aquarapid
Copy link
Contributor

LGTM

Copy link
Contributor

@sverch sverch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote most of this originally, so I put more weight on @aquarapid's review.

Although I do think we should change that one line and start hosting this from our public registry. Waiting for @dctrwatson to see what he thinks.

@@ -0,0 +1,15 @@
# We are using the "dev" project in our registry for this
NAME := "us.gcr.io/planetscale-dev/are-you-alive"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be us.gcr.io/planetscale-vitess/are-you-alive. Does that sound good to you @dctrwatson?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this is meant for public consumption, yes :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, updated, but now I need to fix the build.

Signed-off-by: Shaun Verch <shaun@planetscale.com>
This wasn't added because it was gitignored.

Signed-off-by: Shaun Verch <shaun@planetscale.com>
Now everything described in the README works with the new paths.

Signed-off-by: Shaun Verch <shaun@planetscale.com>
Signed-off-by: Shaun Verch <shaun@planetscale.com>
Signed-off-by: Shaun Verch <shaun@planetscale.com>
@sverch
Copy link
Contributor

sverch commented Feb 26, 2020

@deepthi Is this good to merge? One test timed out but it looks unrelated to this change and isn't listed as required.

Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions and suggestions inline.

Comment on lines 3 to 7
What does it mean to be alive?

Well we don't know what it means for you, but we know what it means for our
Cloud Database!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe delete these lines?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing from "Cloud Database" to "Vitess Cluster".

examples/are-you-alive/README.md Outdated Show resolved Hide resolved
Comment on lines 77 to 82
## Push to Registry

```
make build
make push
```
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to include this? most people won't have the necessary access.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll add a note about access here, but I think documenting the commands makes sense.


Configuration for deploying `are-you-alive` on Kubernetes.

Also deploys Prometheus and Alertmanager.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are we really deploying AlertManager? I don't see it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks for calling this one out. I'll just delete this directory.

@sverch
Copy link
Contributor

sverch commented Mar 2, 2020

Thanks @deepthi ! I think I addressed your comments. Let me know if there's anything else I should do before this is good to merge.

sverch and others added 2 commits March 2, 2020 14:53
Co-Authored-By: Deepthi Sigireddi <deepthi.sigireddi@gmail.com>
Signed-off-by: Shaun Verch <shaun@planetscale.com>
Signed-off-by: Shaun Verch <shaun@planetscale.com>
Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@deepthi deepthi merged commit b61bb5b into vitessio:master Mar 2, 2020
@deepthi deepthi deleted the j0_add_are_you_alive branch March 2, 2020 22:05
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 this pull request may close these issues.

5 participants