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 a watch function to wait for a deployment to be fully available #38

Merged
merged 1 commit into from
Apr 18, 2019

Conversation

Devatoria
Copy link
Contributor

@Devatoria Devatoria commented Apr 17, 2019

Here's a small PR to add the same feature as deployment_is_not_fully_available function but to wait for a deployment to be fully available instead.

It aims to be used as a probe in the steady state hypothesis to wait for the deployment to be healthy after some actions, like after killing a pod. It avoids to use the pauses field which can be really annoying when running multiple experiments since it takes a fixed time.

@Devatoria Devatoria force-pushed the joris/deployment_available branch 3 times, most recently from bb579bc to 309e13c Compare April 18, 2019 08:11
@codecov-io
Copy link

codecov-io commented Apr 18, 2019

Codecov Report

Merging #38 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #38   +/-   ##
=======================================
  Coverage   75.23%   75.23%           
=======================================
  Files           4        4           
  Lines         323      323           
=======================================
  Hits          243      243           
  Misses         80       80

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 02ea1bc...7311872. Read the comment docs.

@Lawouach
Copy link
Contributor

Thanks for the PR. I think it makes a lot of sense indeed.

I merely wonder if this could create surprises for people used to the previous behavior?

@Devatoria
Copy link
Contributor Author

Thanks for the PR. I think it makes a lot of sense indeed.

I merely wonder if this could create surprises for people used to the previous behavior?

You're welcome!

I didn't change the behavior of any functions but indeed it can be redundant with the microservice_available_and_healthy probe which is almost the same without the watch (link here).

Anyway the whole Kubernetes integration is now a bit messy because it has multiple words for the same thing (cf. microservice_ functions vs. deployment_ functions) or the same word for different things (cf. microservice_available_and_healthy function which looks for a deployment vs. microservice_is_not_available which looks for pods). And as a user, I can tell it's sometimes a bit hard to know which one I really want.

Maybe we could start some cleanup on that in another PR to put things at the right place to be more understandable. We could keep the backward compatibility by using the same trick as done here.

What do you think about that?

@Lawouach
Copy link
Contributor

I didn't change the behavior of any functions

Great!

Maybe we could start some cleanup on that in another PR to put things at the right place to be more understandable.

MUCH AGREEEDDD!

Sorry I had to yell that one because I really agree :D

The bunch of probes/actions at the top level (those microservices_ are our oldest actions/probes and they date from a time where we thought we needed these to be clever and hig-level. We've stopped that approach rapidly for things lower level that users can understand more easily and use in a greater variety of contexts. This is not an excuse I'm making for myself but sharing a bit of background and history ;)

In effect, I believe these actions should be marked deprecated altogether. I prefer the approach we took by having actions/probes per entities (pods, etc.).

Maybe worth an issue to keep the discussion going and gather community consensus?

Thanks again for the nice spirit :)

@Lawouach
Copy link
Contributor

Incidentally this branc hlikely need a rebase after the merge we did from a different PR. sorry :/

@Devatoria
Copy link
Contributor Author

Maybe worth an issue to keep the discussion going and gather community consensus?

Yes, definitely, I can open one if you want. And of course I can help to rework the repository too!

Incidentally this branc hlikely need a rebase after the merge we did from a different PR. sorry :/

No problem, I'll rebase in a few minutes.

@Devatoria
Copy link
Contributor Author

Rebased!

Copy link
Contributor

@Lawouach Lawouach left a comment

Choose a reason for hiding this comment

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

LGTM

Missing the changelog entry I believe? :)

@Devatoria Devatoria force-pushed the joris/deployment_available branch 2 times, most recently from 5da2e15 to 0f94d45 Compare April 18, 2019 12:54
Signed-off-by: Devatoria <joris.bonnefoy@datadoghq.com>
@Devatoria
Copy link
Contributor Author

Oops, good catch. Done.

@Lawouach Lawouach merged commit 9c7886d into chaostoolkit:master Apr 18, 2019
@Lawouach
Copy link
Contributor

Cheers

@Devatoria Devatoria deleted the joris/deployment_available branch April 18, 2019 13:40
@Devatoria Devatoria mentioned this pull request Apr 18, 2019
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.

None yet

3 participants