[JUJU-2955] Sidecar teardown logic refinement to respect charm hooks. #15313

tlm · 2023-03-20T02:58:34Z

With the introduction of sidecar charms and Pebble Juju had no logic to tear down charms. Specifically when a pod/unit inside of a Statefulset for Kubernetes was going away through the receiving of (SIGTERM) it would be given at most 5 seconds to run the charms tear down hooks. This also applied to cases of using juju scale-application as this subsequently just trigger a scale within Kubernetes.

Obviously in a lot of situations this isn't enough time for a charm to clean up after itself and push the state of the application into one that can be safely shut down. As part of this PR we are introducing the ability to use juju scale-application to now take individual units of an application into a "zombie" state by where tear down hooks are run and given an infinite amount of time to execute.

Once the desired number of units have entered a zombie/dead state then Juju will command the Kubernetes deployment resource to scale down.

To make sure that the zombie pod starts to be de-considered for traffic after tear down the charms readiness probe is set to failing.

This PR also fixes storage reattaching for sidecar applications.

Depends on juju/description#128

Checklist

Code style: imports ordered, good names, simple structure, etc
Comments saying why design decisions were made
Go unit tests, with comments saying what you're testing
Integration tests, with comments saying what you're testing
doc.go added or updated in changed packages

QA steps

create a sidecar charm that sleeps on remove hook
bootstrap k8s
juju deploy mycharm
juju scale-application 3
wait for units to settle
juju scale-application 0
wait for units to start running the remove hook
juju scale-application 3
observe that the units get removed/pods get removed, then the pods come back and new units come back.
verify storage is reattached to the same unit
verify all hooks have run before the unit and pod are removed

Things to also try:

during scale down, delete a pod from the application, it should come back and continue the hooks.
add a sleep in storage-attached or storage-detached hook and ensure the pod can be deleted and comes back during that hook and it reruns successfully.
manually scale the stateful set in kubernetes, notice that the unwanted pods block in the init container.

Documentation changes

N/A

Bug reference

https://bugs.launchpad.net/juju/+bug/1951415
https://bugs.launchpad.net/juju/+bug/1995466
https://bugs.launchpad.net/juju/+bug/2018147

wallyworld

A few comments to get started....

api/agent/lifeflag/client.go

api/controller/caasapplicationprovisioner/client.go

apiserver/facades/agent/lifeflag/register.go

apiserver/facades/controller/caasapplicationprovisioner/provisioner.go

worker/caasapplicationprovisioner/application.go

worker/uniter/uniter.go

worker/uniter/operation/errors.go

worker/simplesignalhandler/manifold.go

With Juju moving to sidecar charms running under Pebble it is necessary to make sure that when units for an application are being added or removed that they have their teardown hooks run to completion. Once all units needing to be removed have become dead Juju will issue the scale down command to Kubernetes. This commit does not deal with the units that can become dead through direct control of Kubernetes scale bypassing Juju. It won't stop new units from coming up before the desired scale has been reached. This will be rectified in a separate commit. Bug: https://bugs.launchpad.net/juju/+bug/1951415

api/common/lifeflag/facade.go

api/controller/lifeflag/client.go

apiserver/facades/controller/caasapplicationprovisioner/state.go

cmd/containeragent/unit/manifolds.go

cmd/jujud/agent/checkconnection.go

worker/simplesignalhandler/manifold_test.go

worker/simplesignalhandler/package.go

worker/simplesignalhandler/signalwatcher.go

worker/stateconverter/converter.go

worker/uniter/operation/errors.go

…worker.

hpidcock · 2023-04-18T08:42:59Z

/build

wallyworld

Some more comments so far....

api/common/lifeflag/facade_test.go

api/controller/caasapplicationprovisioner/client.go

api/agent/uniter/uniter.go

state/errors/application.go

state/storage.go

worker/caasapplicationprovisioner/ops.go

worker/simplesignalhandler/manifold_test.go

worker/uniter/operation/errors.go

worker/uniter/runner/context/context.go

docker/registry/internal/ecr.go

apiserver/facades/controller/caasapplicationprovisioner/provisioner.go

api/controller/caasapplicationprovisioner/client.go

PR Feedback for side tear down work. Have renamed the life flag ErrNotFound to ErrEntityNotFound to better reflect the error and it's purpose.

CAASApplicationProvisioner now has a facade method for destroying units. This commit adds missing unit tests to the client side of the Facade to assert behaviour.

Update the error name used for life flag worker and client to be more specific as to it's purpose.

worker/uniter/runner/context/context.go

worker/uniter/storage/attachments.go

worker/uniter/runner/context/contextfactory.go

hpidcock · 2023-05-29T05:15:17Z

/merge

#15693 Forward ports: - #15636 - #15650 - #15652 - #15653 - #15633 - #15657 - #15660 - #15661 - #15662 - #15663 - #15313 - #15542 - #15674 Conflicts: - cmd/containeragent/unit/manifolds.go - cmd/containeragent/unit/manifolds_test.go - tests/suites/machine/machine.sh - tests/suites/refresh/refresh.sh

#15719 Merge 3.2 No conflicts. Upgrade steps were deleted. #15636 [from jack-w-shaw/JUJU-3822_replace_ubuntu_w…](1a2759d) #15650 [from SimonRichardson/fix-machine-test-logs-aws](c316413) #15652 [from barrettj12/ojson](4436840) #15653 [from barrettj12/teardown-logs](ee886dd) #15633 [from anvial/JUJU-3821-fix-test-deploy-aks-c…](0bb6df2) #15657 [from anvial/JUJU-3842-fix-deploy-aks-clean-…](f16dd80) #15660 [from hpidcock/ignore-delete-network-interface](686b6f2) #15661 [from hpidcock/fix-data-race-getosfromseries](f639f09) #15662 [from hpidcock/fix-relation-departing-test](68ef945) #15663 [from hpidcock/fix-test-branch](c14bcb3) #15313 [from tlm/teardown-2.0](243fd66) #15542 [from masnax/project-fix](8ba03ec) #15676 [from wallyworld/newer-clients-migrate](7c1f884) #15672 [from hpidcock/bump-juju-description-v3.0.15](acec126) #15683 [from hpidcock/fix-persistent-storage-test](460dd21358378e7d1204348c37a9ba26e49b5871)https://github.com/juju/juju/pull/15673 [from barrettj12/check-merge](5c253c3) #15677 [from barrettj12/invalid-offer](3cb3f8b) #15654 [from ycliuhw/fix/backendRefCount](4e5ae3c) #15692 [from wallyworld/fix-secrets-cmr](a1fb0c4) #15709 [from wallyworld/hook-secret-revison](840bc09) #15701 [from hpidcock/fix-upgrade-podspec-sidecar](d465c93) #15681 [from anvial/JUJU-3882-fix-test-deploy-test-…](f54c1ad) #15573 [from jack-w-shaw/JUJU-3447_implement_ModelF…](f7082ca) #15714 [from wallyworld/offer-consume=sameapp](6390036) ## QA steps See PRs

#15721 Merge 3.3 #15636 [from jack-w-shaw/JUJU-3822_replace_ubuntu_w…](1a2759d) #15650 [from SimonRichardson/fix-machine-test-logs-aws](c316413) #15652 [from barrettj12/ojson](4436840) #15653 [from barrettj12/teardown-logs](ee886dd) #15633 [from anvial/JUJU-3821-fix-test-deploy-aks-c…](0bb6df2) #15657 [from anvial/JUJU-3842-fix-deploy-aks-clean-…](f16dd80) #15660 [from hpidcock/ignore-delete-network-interface](686b6f2) #15661 [from hpidcock/fix-data-race-getosfromseries](f639f09) #15662 [from hpidcock/fix-relation-departing-test](68ef945) #15663 [from hpidcock/fix-test-branch](c14bcb3) #15313 [from tlm/teardown-2.0](243fd66) #15542 [from masnax/project-fix](8ba03ec) #15676 [from wallyworld/newer-clients-migrate](7c1f884) #15672 [from hpidcock/bump-juju-description-v3.0.15](acec126) #15683 [from hpidcock/fix-persistent-storage-test](460dd21358378e7d1204348c37a9ba26e49b5871)https://github.com/juju/juju/pull/15673 [from barrettj12/check-merge](5c253c3) #15677 [from barrettj12/invalid-offer](3cb3f8b) #15654 [from ycliuhw/fix/backendRefCount](4e5ae3c) #15692 [from wallyworld/fix-secrets-cmr](a1fb0c4) #15709 [from wallyworld/hook-secret-revison](840bc09) #15701 [from hpidcock/fix-upgrade-podspec-sidecar](d465c93) #15681 [from anvial/JUJU-3882-fix-test-deploy-test-…](f54c1ad) #15573 [from jack-w-shaw/JUJU-3447_implement_ModelF…](f7082ca) #15714 [from wallyworld/offer-consume=sameapp](6390036) Conflicts ``` # Conflicts: # apiserver/facades/client/client/client_test.go # featuretests/package_test.go # ``` ## QA steps See PRs

#16231 Destroying a k8s controller or model would immediately move unit storage attachments from alive to dying to dead/removed instead of just marking them as dying and letting the uniter remove the attachments gracefully. This change makes model destruction to be more graceful on k8s units with storage attached, allowing them to correctly run their storage-detaching hooks. The reason this is happening now is due to [this](#15313) fix for [this storage bug](https://bugs.launchpad.net/juju/+bug/2018147). ## QA steps This should not work before this change and should cleanup correctly with this change. ``` $ juju bootstrap k8s $ juju add-model a $ juju deploy prometheus-k8s --trust # wait for it to settle $ juju destroy-controller k8s --destroy-all-models --destroy-storage # or release-storage ``` ## Documentation changes N/A ## Bug reference https://bugs.launchpad.net/juju/+bug/2035058

wallyworld reviewed Mar 20, 2023

View reviewed changes

tlm changed the title ~~Early draft for tear down work.~~ [JUJU-2955] Early draft for tear down work. Mar 21, 2023

tlm force-pushed the teardown-2.0 branch from 547e41e to 1f1f978 Compare March 21, 2023 04:24

taurus-forever mentioned this pull request Mar 21, 2023

DO NOT MERGE. TESTING JUJU 3.1 canonical/mongodb-k8s-operator#124

Closed

hpidcock added the 2.9 label Mar 23, 2023

tlm force-pushed the teardown-2.0 branch 2 times, most recently from fdfedf4 to 226d1ff Compare March 29, 2023 02:23

tlm marked this pull request as ready for review March 30, 2023 07:49

tlm force-pushed the teardown-2.0 branch from 226d1ff to 436228d Compare March 30, 2023 07:50

tlm force-pushed the teardown-2.0 branch from 436228d to d94e2e8 Compare March 31, 2023 00:20

barrettj12 reviewed Apr 3, 2023

View reviewed changes

tlm changed the title ~~[JUJU-2955] Early draft for tear down work.~~ [JUJU-2955] Start of teardown work. Apr 3, 2023

hpidcock added 5 commits April 13, 2023 20:53

Prevent introduction of dead units or unwanted units (by scale).

3a87c38

Handle unit not found as dead life for removed unit.

2791f38

Safeguard against unit introduction introducing new unit in scale down.

359b142

Fix lifeflag worker to check for ErrNotFound not errors.NotFound.

3e60fee

Improve caasapplicationprovisioner during startup and app removal.

4b9f022

hpidcock changed the title ~~[JUJU-2955] Start of teardown work.~~ [JUJU-2955] Sidecar teardown logic refinement to respect charm hooks. Apr 13, 2023

hpidcock added 9 commits April 14, 2023 00:04

Fix handling of errors for UnitIntroduction.

e0e3c96

Fix missing copyright statement

a8f5462

Add provisioning state to application for caasapplicationprovisioner …

9c47857

…worker.

Improve unit introduction within state.

8a52ce4

Remove GarbageCollect used by caasapplicationprovisioner + extra logs.

32459be

Enable public.ecr.aws

8ae2c6e

Cleanup + fix unit tests for sidecar teardown.

851208c

Fix linter issues.

cb35703

Merge branch '2.9' into teardown-2.0

e75fac5

hpidcock force-pushed the teardown-2.0 branch from 4610c97 to e75fac5 Compare April 18, 2023 08:03

hpidcock requested a review from barrettj12 April 28, 2023 07:43

hpidcock added the bug The PR addresses a bug label May 1, 2023

wallyworld reviewed May 4, 2023

View reviewed changes

wallyworld reviewed May 5, 2023

View reviewed changes

worker/simplesignalhandler/manifold_test.go Outdated Show resolved Hide resolved

worker/uniter/operation/errors.go Show resolved Hide resolved

worker/uniter/runner/context/context.go Show resolved Hide resolved

ycliuhw reviewed May 5, 2023

View reviewed changes

hpidcock and others added 9 commits May 29, 2023 09:55

Fix storage-attached hook that was requeued.

619ae83

Fix ApplicationWorkerSuite.TestWorker flapping.

4f58067

Fix LifeFlag watcher client test.

0d62596

Updates life flag not found error.

f747552

PR Feedback for side tear down work. Have renamed the life flag ErrNotFound to ErrEntityNotFound to better reflect the error and it's purpose.

Adds missing tests for DestroyUnits.

5d23dcc

CAASApplicationProvisioner now has a facade method for destroying units. This commit adds missing unit tests to the client side of the Facade to assert behaviour.

Update error name for life flag.

6e14eda

Update the error name used for life flag worker and client to be more specific as to it's purpose.

Apply feedback to caas sidecar teardown.

cb6e938

Fix storage not provisioned in TestNewHookRunnerWithStorage

52f4bee

Update juju/descriptions/v3

25ea0ca

hpidcock force-pushed the teardown-2.0 branch from 4c05bd7 to 25ea0ca Compare May 28, 2023 23:57

wallyworld approved these changes May 29, 2023

View reviewed changes

worker/uniter/runner/context/context.go Outdated Show resolved Hide resolved

worker/uniter/storage/attachments.go Show resolved Hide resolved

worker/uniter/runner/context/contextfactory.go Outdated Show resolved Hide resolved

hpidcock added 2 commits May 29, 2023 13:37

Fix error handling for storage + PR feedback.

1be46d0

Fix uniter remotestate storage tests.

0ab55fd

jujubot merged commit 243fd66 into juju:2.9 May 29, 2023
18 of 19 checks passed

taurus-forever mentioned this pull request May 30, 2023

avoid flaky planned units canonical/mysql-k8s-operator#234

Merged

hpidcock mentioned this pull request Jun 2, 2023

Merge 3.1 to 3.2 #15693

Merged

wallyworld mentioned this pull request Jun 9, 2023

Merge 3.2 #15719

Merged

wallyworld mentioned this pull request Jun 9, 2023

Merge 3.3 #15721

Merged

hpidcock mentioned this pull request Jul 6, 2023

Fix cleaner not running when the model is dying. #15864

Merged

hpidcock mentioned this pull request Sep 11, 2023

Fix k8s storage detaching hook during model/controller destruction. #16231

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JUJU-2955] Sidecar teardown logic refinement to respect charm hooks. #15313

[JUJU-2955] Sidecar teardown logic refinement to respect charm hooks. #15313

tlm commented Mar 20, 2023 •

edited by hpidcock

wallyworld left a comment

hpidcock commented Apr 18, 2023

wallyworld left a comment

hpidcock commented May 29, 2023

[JUJU-2955] Sidecar teardown logic refinement to respect charm hooks. #15313

[JUJU-2955] Sidecar teardown logic refinement to respect charm hooks. #15313

Conversation

tlm commented Mar 20, 2023 • edited by hpidcock

Checklist

QA steps

Documentation changes

Bug reference

wallyworld left a comment

Choose a reason for hiding this comment

hpidcock commented Apr 18, 2023

wallyworld left a comment

Choose a reason for hiding this comment

hpidcock commented May 29, 2023

tlm commented Mar 20, 2023 •

edited by hpidcock