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

[WIP] Initial deployment lifecycle hooks proposal #33545

Closed
wants to merge 1 commit into from

Conversation

mfojtik
Copy link
Contributor

@mfojtik mfojtik commented Sep 27, 2016

@smarterclayton @Kargakis

This proposal is a summary collected from various sources.

A PoC branch (that is heavily an WIP): https://github.com/mfojtik/kubernetes/tree/deployment-hooks


This change is Reviewable

@k8s-github-robot k8s-github-robot added kind/design Categorizes issue or PR as related to design. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Sep 27, 2016
@0xmichalis
Copy link
Contributor

@kubernetes/deployment ptal


* **Retry** - using this policy will cause that in case of a deployment lifecycle
hook failure, the lifecycle Pod will be restarted until the deployment reached the
`TimeoutSeconds` or the hook succeeds.
Copy link
Contributor

Choose a reason for hiding this comment

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

TimeoutSeconds is ProgressDeadlineSeconds added in the perma-failed PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, I will remove it and use progressdeadline

### Default values

Initially the default `FailurePolicy` should be set to `Retry` until the deployments can
safely be rolled back automatically.
Copy link
Contributor

Choose a reason for hiding this comment

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

Link to the issue regarding automatic rollback.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

type RollingUpdateDeployment struct {
// TimeoutSeconds is the time to wait for updates before giving up. If the
// value is nil, a default will be used.
TimeoutSeconds *int64
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for this, ProgressDeadlineSeconds will be in the spec. You should mention that this proposal is depending on the perma-failed PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit 7d193a9. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 6cf98c0. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

Because deployment processes typically involve coupling between state (assumed elsewhere)
and code (frequently the target of the deployment), it should be easy for a hook to be
coupled to a particular version of code, and easy for a deployer to use the code under
deployment from the hook.
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a background section here including:

  1. Why we did this on OpenShift and lessons learned
  2. How this might be applicable to pet sets
  3. Why this may or may not be applicable to daemon sets.

Kubernetes provides container lifecycle hooks for containers within a Pod. Currently,
post-start and pre-stop hooks are supported. For deployments, post-start is the most
relevant. Because these hooks are implemented by the Kubelet, the post-start hook provides
some unique guarantees:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't I used an init container?

Copy link
Member

Choose a reason for hiding this comment

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

+1 to this question

@smarterclayton
Copy link
Contributor

smarterclayton commented Sep 27, 2016 via email

@bgrant0607
Copy link
Member

Ref #31571 re. custom controller. I like that approach, esp. in the short term (1.5/Q4). Review bandwidth is extremely limited right now.

Also, I think hooks are more likely to be relevant to PetSet than to Deployment. Deployment is really aimed at stateless applications with lots of rollout flexibility.

relevant. Because these hooks are implemented by the Kubelet, the post-start hook provides
some unique guarantees:

* The hook is executed synchronously during the pod lifecycle.
Copy link
Member

Choose a reason for hiding this comment

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

PostStart is async, unless I'm missing something.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bgrant0607 PostStart is definitely synchronous. According to the docs and my testing

Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify, it can start the entry point and the hook at the same time, but the hook has to finish running before a container is considered to be started

* The status of the pod is linked to the status and outcome of the hook execution.
* The pod will not enter a ready status until the hook has completed successfully.
* Service endpoints will not be created for the pod until the pod has a ready status.
* If the hook fails, the pod's creation is considered a failure, and the retry behavior is
Copy link
Member

Choose a reason for hiding this comment

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

I think pod and container are confused here. Post-start is at the container level.

```

```go
type ExecNewPodHook struct {
Copy link
Member

Choose a reason for hiding this comment

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

Why not a distinct pod or job spec?

@bgrant0607
Copy link
Member

cc @erictune @pwittrock

@smarterclayton
Copy link
Contributor

smarterclayton commented Oct 7, 2016 via email

@bgrant0607
Copy link
Member

@smarterclayton Thanks much for the detailed background.

Do the web-db cases put the DB in the same container/pod as the web app?

@smarterclayton
Copy link
Contributor

They typically do not - schema and "execution" logic come from web app, db
is usually assumed to be a completely separate service.

On Oct 6, 2016, at 10:44 PM, Brian Grant notifications@github.com wrote:

@smarterclayton https://github.com/smarterclayton Thanks much for the
detailed background.

Do the web-db cases put the DB in the same container/pod as the web app?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#33545 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_p0MgMVAaMUhoBxY83XyrTZY6K-T3ks5qxc49gaJpZM4KHckS
.

@bgrant0607 bgrant0607 added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Oct 24, 2016
@0xmichalis
Copy link
Contributor

ref #14512

@k8s-github-robot
Copy link

Adding label:do-not-merge because PR changes docs prohibited to auto merge
See http://kubernetes.io/editdocs/ for information about editing docs

@k8s-github-robot k8s-github-robot added kind/design Categorizes issue or PR as related to design. kind/old-docs do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Dec 1, 2016
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] Needs approval from an approver in each of these OWNERS Files:

We suggest the following people:
cc @brendandburns
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

This PR hasn't been active in 90 days. Closing this PR. Please reopen if you would like to work towards merging this change, if/when the PR is ready for the next round of review.

cc @brendandburns @erictune @mfojtik @thockin

You can add 'keep-open' label to prevent this from happening again, or add a comment to keep it open another 90 days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. kind/design Categorizes issue or PR as related to design. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.