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

r/kubernetes_pod: add support for init containers #156

Merged
merged 1 commit into from
May 2, 2018
Merged

r/kubernetes_pod: add support for init containers #156

merged 1 commit into from
May 2, 2018

Conversation

appilon
Copy link
Contributor

@appilon appilon commented Apr 18, 2018

Fix #61

Implement init containers. Docs and Api for K8s list it follows the exact same schema as a normal container but there are some subtle behavioural differences

Init Containers support all the fields and features of app Containers, including resource limits, volumes, and security settings. However, the resource requests and limits for an Init Container are handled slightly differently, which are documented in Resources below. Also, Init Containers do not support readiness probes because they must run to completion before the Pod can be ready.

Please let me know if we want acceptance tests that try to validate those things or if we want special validations on the init_container schema. The path of least resistance was to just piggyback the container schema

make testacc TEST=./kubernetes TESTARGS='-run=TestAccKubernetesPod'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./kubernetes -v -run=TestAccKubernetesPod -timeout 120m
=== RUN   TestAccKubernetesPod_basic
--- PASS: TestAccKubernetesPod_basic (8.39s)
=== RUN   TestAccKubernetesPod_initContainer_updateForcesNew
--- PASS: TestAccKubernetesPod_initContainer_updateForcesNew (80.98s)
=== RUN   TestAccKubernetesPod_updateArgsForceNew
--- PASS: TestAccKubernetesPod_updateArgsForceNew (90.99s)
=== RUN   TestAccKubernetesPod_updateEnvForceNew
--- PASS: TestAccKubernetesPod_updateEnvForceNew (24.49s)
=== RUN   TestAccKubernetesPod_importBasic
--- PASS: TestAccKubernetesPod_importBasic (9.58s)
=== RUN   TestAccKubernetesPod_with_pod_security_context
--- PASS: TestAccKubernetesPod_with_pod_security_context (60.32s)
=== RUN   TestAccKubernetesPod_with_container_liveness_probe_using_exec
--- PASS: TestAccKubernetesPod_with_container_liveness_probe_using_exec (38.77s)
=== RUN   TestAccKubernetesPod_with_container_liveness_probe_using_http_get
--- PASS: TestAccKubernetesPod_with_container_liveness_probe_using_http_get (18.53s)
=== RUN   TestAccKubernetesPod_with_container_liveness_probe_using_tcp
--- PASS: TestAccKubernetesPod_with_container_liveness_probe_using_tcp (10.39s)
=== RUN   TestAccKubernetesPod_with_container_lifecycle
--- PASS: TestAccKubernetesPod_with_container_lifecycle (50.49s)
=== RUN   TestAccKubernetesPod_with_container_security_context
--- PASS: TestAccKubernetesPod_with_container_security_context (6.42s)
=== RUN   TestAccKubernetesPod_with_volume_mount
--- PASS: TestAccKubernetesPod_with_volume_mount (6.84s)
=== RUN   TestAccKubernetesPod_with_cfg_map_volume_mount
--- PASS: TestAccKubernetesPod_with_cfg_map_volume_mount (7.52s)
=== RUN   TestAccKubernetesPod_with_resource_requirements
--- PASS: TestAccKubernetesPod_with_resource_requirements (5.51s)
=== RUN   TestAccKubernetesPod_with_empty_dir_volume
--- PASS: TestAccKubernetesPod_with_empty_dir_volume (18.98s)
=== RUN   TestAccKubernetesPod_with_secret_vol_items
--- PASS: TestAccKubernetesPod_with_secret_vol_items (11.04s)
=== RUN   TestAccKubernetesPod_gke_with_nodeSelector
--- PASS: TestAccKubernetesPod_gke_with_nodeSelector (18.63s)
PASS
ok  	github.com/terraform-providers/terraform-provider-kubernetes/kubernetes	467.969s

UPDATE: I might need to test the updating behaviour

A Pod can restart, causing re-execution of Init Containers, for the following reasons:
A user updates the PodSpec causing the Init Container image to change. App Container image changes only restart the app Container.

FINAL NOTES:
As far as I can tell updates are not permitted to an init container's spec. I needed to ForceNew on the Pod if that is changed. Unfortunately just adding that attribute on the list init_container was not enough, it appears the container spec I was piggy backing allows changes to the image always, which does not hold true for InitContainers, so I introduced a new boolean in the spec functions to forceNew for init containers and added some acceptance checks that actually check the UID of the pod (as a method of proving if it remained the same or a new one was made).

@paddycarver if you could do a quick second check since I changed the code a decent amount since your last review that would be aweseome!

@appilon appilon changed the title [WIP] add support for init containers r/kubernetes_pod: add support for init containers Apr 19, 2018
@appilon appilon requested a review from a team April 19, 2018 21:39
Copy link

@paddycarver paddycarver left a comment

Choose a reason for hiding this comment

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

The code on this seems right to me, though I agree having tests for update would be ideal.

@appilon
Copy link
Contributor Author

appilon commented Apr 26, 2018

@paddycarver Interestingly enough the api reference provided an important tidbit:

Init containers cannot currently be added or removed. Cannot be updated

Which is why when I added a test check to update the image json-patch was complaining

* kubernetes_pod.test: jsonpatch replace operation does not apply: doc is missing path: /spec/init-containers/0/image

(which I found out is written by a hashicorp employee Evan Phoenix, I'm super star-struck now!) So I will remove the ability to update that attribute. Would I place a ForceNew on this so the pod is destroyed and a new one is created?

UPDATE: investigating further the notion of updating an init-container does seem to exist in the wild... but it's unclear to me, the user guides and official api reference seems to contradict itself, but to me the init containers are basically like a startup script that persists things to the pod, it wouldn't make sense to me that it could be updated... I'm going to remove the ability to update for now

@appilon appilon changed the title r/kubernetes_pod: add support for init containers [WIP] r/kubernetes_pod: add support for init containers Apr 27, 2018
@sl1pm4t
Copy link
Contributor

sl1pm4t commented Apr 30, 2018

Hi @appilon - I wonder if the discrepancy you're seeing with updates to InitContainers relates to whether they are defined within a standalone Pod, or whether they are under a higher level resource like a Deployment.
Updates in the latter case would be allowed, but would trigger a rolling update of all pods controlled by the Deployment, so it's effectively a ForceNew at the pod level but managed by the Kubernetes backend instead of Terraform (not that Deployment resources are in this official provider yet).

@appilon appilon changed the title [WIP] r/kubernetes_pod: add support for init containers r/kubernetes_pod: add support for init containers Apr 30, 2018
@appilon
Copy link
Contributor Author

appilon commented May 1, 2018

@sl1pm4t Thanks for the heads up I am still new to Kubernetes so its good to get insight on its quirks and behaviours. At least for my PR I figured out a way to not allow updates for init containers (but still allow them for non init containers).

@appilon appilon requested a review from radeksimko May 1, 2018 13:39
only allow container spec image to be updated by non-init containers
add acceptance checks for forced new
add acceptance test to Replication Controller
@appilon appilon merged commit 0c0e4cf into hashicorp:master May 2, 2018
@andre-brongniart
Copy link

+1
Is this implemented yet or when will it be?

@appilon
Copy link
Contributor Author

appilon commented Aug 15, 2018

This has been released with version 1.2.0

@ghost ghost locked and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Kubernetes Pod Init Containers (1.6+ feature)
5 participants