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 init containers to pods #23567

Merged
merged 6 commits into from May 18, 2016

Conversation

Projects
None yet
@smarterclayton
Contributor

smarterclayton commented Mar 29, 2016

This implements #1589 as per proposal #23666

Incorporates feedback on #1589, creates parallel structure for InitContainers and Containers, adds validation for InitContainers that requires name uniqueness, and comments on a number of implications of init containers.

This is a complete alpha implementation.


This change is Reviewable

@googlebot googlebot added the cla: yes label Mar 29, 2016

@smarterclayton smarterclayton referenced this pull request Mar 29, 2016

Closed

Init container #1589

@smarterclayton smarterclayton changed the title from Adding init containers to pods to WIP - Adding init containers to pods Mar 29, 2016

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 Mar 30, 2016

Member

I would have preferred #831 first.

Member

bgrant0607 commented Mar 30, 2016

I would have preferred #831 first.

Show outdated Hide outdated pkg/api/v1/types.go Outdated
// init container fails, the pod is considered to have failed and is handled according
// to its restartPolicy. The name for an init container or normal container must be
// unique among all containers.
// Init containers may not have Lifecycle actions, Readiness probes, or Liveness probes.

This comment has been minimized.

@bgrant0607

bgrant0607 Mar 30, 2016

Member

We've been bit several times by unsupported fields, so if we go with this approach, we should create a more specific container type that omits those fields.

@bgrant0607

bgrant0607 Mar 30, 2016

Member

We've been bit several times by unsupported fields, so if we go with this approach, we should create a more specific container type that omits those fields.

This comment has been minimized.

@smarterclayton

smarterclayton Mar 30, 2016

Contributor
@smarterclayton

smarterclayton via email Mar 30, 2016

Contributor
@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 Mar 30, 2016

Member

Looks ok. Not the approach I'd recommend for initializing volumes.

Member

bgrant0607 commented Mar 30, 2016

Looks ok. Not the approach I'd recommend for initializing volumes.

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Mar 30, 2016

Contributor

Specifically because? I'm envisioning the "filling volumes" usecase
much more than custom volume behaviors (fuse, etc). Filling volumes
supports:

  • run container from URL
  • inject binaries into arbitrary other images and then execute them
  • Git volumes and any other data driven volume
  • standard "wait for services container"
Contributor

smarterclayton commented Mar 30, 2016

Specifically because? I'm envisioning the "filling volumes" usecase
much more than custom volume behaviors (fuse, etc). Filling volumes
supports:

  • run container from URL
  • inject binaries into arbitrary other images and then execute them
  • Git volumes and any other data driven volume
  • standard "wait for services container"
@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Mar 30, 2016

Contributor
Contributor

smarterclayton commented Mar 30, 2016

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton Mar 31, 2016

Contributor

Opened a proposal under #23666, this is a prototype PR until that is approved.

Contributor

smarterclayton commented Mar 31, 2016

Opened a proposal under #23666, this is a prototype PR until that is approved.

@k8s-merge-robot k8s-merge-robot added size/L and removed size/M labels Mar 31, 2016

@smarterclayton smarterclayton changed the title from WIP - Adding init containers to pods to WIP - Prototype init containers in pods Mar 31, 2016

@bgrant0607

This comment has been minimized.

Show comment
Hide comment
@bgrant0607

bgrant0607 Apr 6, 2016

Member

Sadly, I probably won't be able to get to this until next week.
cc @bprashanth @thockin

Member

bgrant0607 commented Apr 6, 2016

Sadly, I probably won't be able to get to this until next week.
cc @bprashanth @thockin

@smarterclayton smarterclayton changed the title from WIP - Prototype init containers in pods to Add init containers to pods Apr 8, 2016

Show outdated Hide outdated test/e2e/pods.go Outdated
allErrs = append(allErrs, field.Invalid(idxPath.Child("livenessProbe"), ctr.LivenessProbe, "must not be set for init containers"))
}
if ctr.ReadinessProbe != nil {
allErrs = append(allErrs, field.Invalid(idxPath.Child("readinessProbe"), ctr.ReadinessProbe, "must not be set for init containers"))

This comment has been minimized.

@bprashanth

bprashanth Apr 8, 2016

Member

also stdin, tty, stdinonce? though I suppose it would be nice to have a -it that dropped you into an init container so you can eg: write out nginx.conf before nginx starts

@bprashanth

bprashanth Apr 8, 2016

Member

also stdin, tty, stdinonce? though I suppose it would be nice to have a -it that dropped you into an init container so you can eg: write out nginx.conf before nginx starts

This comment has been minimized.

@smarterclayton

smarterclayton Apr 13, 2016

Contributor

Originally was going to remove, decided that we could tolerate them being
attachable.

On Fri, Apr 8, 2016 at 4:41 PM, Prashanth B notifications@github.com
wrote:

In pkg/api/validation/validation.go
#23567 (comment)
:

  • for i, ctr := range containers {
  •   idxPath := fldPath.Index(i)
    
  •   if allNames.Has(ctr.Name) {
    
  •       allErrs = append(allErrs, field.Duplicate(idxPath.Child("name"), ctr.Name))
    
  •   }
    
  •   if len(ctr.Name) > 0 {
    
  •       allNames.Insert(ctr.Name)
    
  •   }
    
  •   if ctr.Lifecycle != nil {
    
  •       allErrs = append(allErrs, field.Invalid(idxPath.Child("lifecycle"), ctr.Lifecycle, "must not be set for init containers"))
    
  •   }
    
  •   if ctr.LivenessProbe != nil {
    
  •       allErrs = append(allErrs, field.Invalid(idxPath.Child("livenessProbe"), ctr.LivenessProbe, "must not be set for init containers"))
    
  •   }
    
  •   if ctr.ReadinessProbe != nil {
    
  •       allErrs = append(allErrs, field.Invalid(idxPath.Child("readinessProbe"), ctr.ReadinessProbe, "must not be set for init containers"))
    

also stdin, tty, stdinonce? though I suppose it would be nice to have a
-it that dropped you into an init container so you can eg: write out
nginx.conf before nginx starts


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/23567/files/53b5dd4d022c2b97f02e40d07c81600fc249b5ae..c75dc6a7e701f3e4484e6545658cf85711a2d6d7#r59086228

@smarterclayton

smarterclayton Apr 13, 2016

Contributor

Originally was going to remove, decided that we could tolerate them being
attachable.

On Fri, Apr 8, 2016 at 4:41 PM, Prashanth B notifications@github.com
wrote:

In pkg/api/validation/validation.go
#23567 (comment)
:

  • for i, ctr := range containers {
  •   idxPath := fldPath.Index(i)
    
  •   if allNames.Has(ctr.Name) {
    
  •       allErrs = append(allErrs, field.Duplicate(idxPath.Child("name"), ctr.Name))
    
  •   }
    
  •   if len(ctr.Name) > 0 {
    
  •       allNames.Insert(ctr.Name)
    
  •   }
    
  •   if ctr.Lifecycle != nil {
    
  •       allErrs = append(allErrs, field.Invalid(idxPath.Child("lifecycle"), ctr.Lifecycle, "must not be set for init containers"))
    
  •   }
    
  •   if ctr.LivenessProbe != nil {
    
  •       allErrs = append(allErrs, field.Invalid(idxPath.Child("livenessProbe"), ctr.LivenessProbe, "must not be set for init containers"))
    
  •   }
    
  •   if ctr.ReadinessProbe != nil {
    
  •       allErrs = append(allErrs, field.Invalid(idxPath.Child("readinessProbe"), ctr.ReadinessProbe, "must not be set for init containers"))
    

also stdin, tty, stdinonce? though I suppose it would be nice to have a
-it that dropped you into an init container so you can eg: write out
nginx.conf before nginx starts


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
https://github.com/kubernetes/kubernetes/pull/23567/files/53b5dd4d022c2b97f02e40d07c81600fc249b5ae..c75dc6a7e701f3e4484e6545658cf85711a2d6d7#r59086228

Show outdated Hide outdated pkg/api/v1/types.go Outdated
@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton May 15, 2016

Contributor

@k8s-bot test this issue #25375

Contributor

smarterclayton commented May 15, 2016

@k8s-bot test this issue #25375

@bprashanth

This comment has been minimized.

Show comment
Hide comment
@bprashanth

bprashanth May 15, 2016

Member

@k8s-bot test this github issue: #25629

Member

bprashanth commented May 15, 2016

@k8s-bot test this github issue: #25629

1 similar comment
@bprashanth

This comment has been minimized.

Show comment
Hide comment
@bprashanth

bprashanth May 15, 2016

Member

@k8s-bot test this github issue: #25629

Member

bprashanth commented May 15, 2016

@k8s-bot test this github issue: #25629

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton May 16, 2016

Contributor
... Starting cluster in us-central1-f using provider gce
... calling verify-prereqs


Your current Cloud SDK version is: 109.0.0
Installing components from version: 109.0.0

+----------------------------------------------+
|     These components will be installed.      |
+-----------------------+------------+---------+
|          Name         |  Version   |   Size  |
+-----------------------+------------+---------+
| gcloud Alpha Commands | 2016.01.12 | < 1 MiB |
+-----------------------+------------+---------+

For the latest full release notes, please visit:
  https://cloud.google.com/sdk/release_notes

Do you want to continue (Y/n)?  
#============================================================#
#= Creating update staging area                             =#
#============================================================#
ERROR: gcloud crashed (OSError): [Errno 39] Directory not empty: '/usr/local/share/google/google-cloud-sdk.staging'

If you would like to report this issue, please run the following command:
  gcloud feedback
ERROR: gcloud crashed (IOError): [Errno 2] No such file or directory: '/usr/local/share/google/google-cloud-sdk/.install/bq-nix.snapshot.json'

If you would like to report this issue, please run the following command:
  gcloud feedback
Contributor

smarterclayton commented May 16, 2016

... Starting cluster in us-central1-f using provider gce
... calling verify-prereqs


Your current Cloud SDK version is: 109.0.0
Installing components from version: 109.0.0

+----------------------------------------------+
|     These components will be installed.      |
+-----------------------+------------+---------+
|          Name         |  Version   |   Size  |
+-----------------------+------------+---------+
| gcloud Alpha Commands | 2016.01.12 | < 1 MiB |
+-----------------------+------------+---------+

For the latest full release notes, please visit:
  https://cloud.google.com/sdk/release_notes

Do you want to continue (Y/n)?  
#============================================================#
#= Creating update staging area                             =#
#============================================================#
ERROR: gcloud crashed (OSError): [Errno 39] Directory not empty: '/usr/local/share/google/google-cloud-sdk.staging'

If you would like to report this issue, please run the following command:
  gcloud feedback
ERROR: gcloud crashed (IOError): [Errno 2] No such file or directory: '/usr/local/share/google/google-cloud-sdk/.install/bq-nix.snapshot.json'

If you would like to report this issue, please run the following command:
  gcloud feedback
@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton May 16, 2016

Contributor

Also

docker: error while loading shared libraries: libltdl.so.7: cannot open shared object file: No such file or directory
!!! Error in ./hack/update-api-reference-docs.sh:71
  'docker run ${user_flags} --rm -v "${TMP_IN_HOST}":/output:z -v "${SWAGGER_PATH}":/swagger-source:z gcr.io/google_containers/gen-swagger-docs:v5 "${SWAGGER_JSON_NAME}" "${REGISTER_FILE_URL}"' exited with status 127
Call stack:
  1: ./hack/update-api-reference-docs.sh:71 main(...)
Exiting with status 1
!!! Error in ./hack/../hack/verify-api-reference-docs.sh:34
  '"./hack/update-api-reference-docs.sh" "${OUTPUT_DIR}"' exited with status 1
Call stack:
  1: ./hack/../hack/verify-api-reference-docs.sh:34 main(...)
Exiting with status 1
FAILED   ./hack/../hack/verify-api-reference-docs.sh    1s

All sorts of goodness this weekend.

Contributor

smarterclayton commented May 16, 2016

Also

docker: error while loading shared libraries: libltdl.so.7: cannot open shared object file: No such file or directory
!!! Error in ./hack/update-api-reference-docs.sh:71
  'docker run ${user_flags} --rm -v "${TMP_IN_HOST}":/output:z -v "${SWAGGER_PATH}":/swagger-source:z gcr.io/google_containers/gen-swagger-docs:v5 "${SWAGGER_JSON_NAME}" "${REGISTER_FILE_URL}"' exited with status 127
Call stack:
  1: ./hack/update-api-reference-docs.sh:71 main(...)
Exiting with status 1
!!! Error in ./hack/../hack/verify-api-reference-docs.sh:34
  '"./hack/update-api-reference-docs.sh" "${OUTPUT_DIR}"' exited with status 1
Call stack:
  1: ./hack/../hack/verify-api-reference-docs.sh:34 main(...)
Exiting with status 1
FAILED   ./hack/../hack/verify-api-reference-docs.sh    1s

All sorts of goodness this weekend.

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton May 16, 2016

Contributor

@k8s-bot test this issue #25635

Contributor

smarterclayton commented May 16, 2016

@k8s-bot test this issue #25635

@smarterclayton smarterclayton removed the lgtm label May 16, 2016

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton May 16, 2016

Contributor

Found a failure mode (caught by the e2e test checking the message) - init containers are only ready when they have a recorded container that is terminated with exit code 0. Was introduced in the status rebase (the new code was incorrect, the old code was correct). PTAL.

Contributor

smarterclayton commented May 16, 2016

Found a failure mode (caught by the e2e test checking the message) - init containers are only ready when they have a recorded container that is terminated with exit code 0. Was introduced in the status rebase (the new code was incorrect, the old code was correct). PTAL.

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton May 16, 2016

Contributor

@k8s-bot test this github issue: #IGNORE (bot got stuck last night)

Contributor

smarterclayton commented May 16, 2016

@k8s-bot test this github issue: #IGNORE (bot got stuck last night)

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton May 16, 2016

Contributor

@k8s-bot test this issue #IGNORE (node e2e never started, looks like job was terminated early).

Contributor

smarterclayton commented May 16, 2016

@k8s-bot test this issue #IGNORE (node e2e never started, looks like job was terminated early).

@bprashanth

This comment has been minimized.

Show comment
Hide comment
@bprashanth

bprashanth May 17, 2016

Member

I guess kubelet though it was ready because of the absence of a probe in the previous version. LGTM, feel free to re-apply label when you've squashed the last fix commit.

Member

bprashanth commented May 17, 2016

I guess kubelet though it was ready because of the absence of a probe in the previous version. LGTM, feel free to re-apply label when you've squashed the last fix commit.

smarterclayton added some commits Mar 29, 2016

@smarterclayton smarterclayton added the lgtm label May 17, 2016

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot commented May 18, 2016

GCE e2e build/test passed for commit 2a53330.

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot May 18, 2016

Contributor

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

Contributor

k8s-merge-robot commented May 18, 2016

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot

This comment has been minimized.

Show comment
Hide comment
@k8s-bot

k8s-bot commented May 18, 2016

GCE e2e build/test passed for commit 2a53330.

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot May 18, 2016

Contributor

Automatic merge from submit-queue

Contributor

k8s-merge-robot commented May 18, 2016

Automatic merge from submit-queue

@k8s-merge-robot k8s-merge-robot merged commit bf4f841 into kubernetes:master May 18, 2016

4 of 5 checks passed

Submit Queue Github CI tests are not green.
Details
Jenkins GCE Node e2e Build finished.
Details
Jenkins GCE e2e 295 tests run, 120 skipped, 0 failed.
Details
Jenkins unit/integration 6270 tests run, 24 skipped, 0 failed.
Details
cla/google All necessary CLAs are signed
@bprashanth

This comment has been minimized.

Show comment
Hide comment
@bprashanth

bprashanth May 18, 2016

Member

Congratulations to all!

Member

bprashanth commented May 18, 2016

Congratulations to all!

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton May 18, 2016

Contributor

I need to update the proposal and merge it.

On Wed, May 18, 2016 at 9:02 AM, Prashanth B notifications@github.com
wrote:

Congratulations to all!


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#23567 (comment)

Contributor

smarterclayton commented May 18, 2016

I need to update the proposal and merge it.

On Wed, May 18, 2016 at 9:02 AM, Prashanth B notifications@github.com
wrote:

Congratulations to all!


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#23567 (comment)

@bprashanth

This comment has been minimized.

Show comment
Hide comment
@bprashanth

bprashanth May 18, 2016

Member

Best way to get agreement on any proposal

Member

bprashanth commented May 18, 2016

Best way to get agreement on any proposal

@smarterclayton

This comment has been minimized.

Show comment
Hide comment
@smarterclayton

smarterclayton May 18, 2016

Contributor

Shipping code wins.

On Wed, May 18, 2016 at 12:55 PM, Prashanth B notifications@github.com
wrote:

Best way to get agreement on any proposal


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#23567 (comment)

Contributor

smarterclayton commented May 18, 2016

Shipping code wins.

On Wed, May 18, 2016 at 12:55 PM, Prashanth B notifications@github.com
wrote:

Best way to get agreement on any proposal


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#23567 (comment)

k8s-merge-robot added a commit that referenced this pull request Jun 18, 2016

Merge pull request #23666 from smarterclayton/initctrproposal
Automatic merge from submit-queue

Proposal for implementing init containers

Addresses #1589. Implemented in #23567. Docs in kubernetes/website#679

```release-note
Init containers enable pod authors to perform tasks before their normal containers start. Each init container is started in order, and failing containers will prevent the application from starting.
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment