KEP-27: Detailed pod restart control by dependencies hash #1483

ANeumann82 · 2020-04-24T12:09:34Z

What this PR does / why we need it:

Restart pods from a deployment or stateful set only if the pod template spec or any of the used config maps or secrets are changed.
Removes the last-plan-execution-uid from the pod template spec
Calculate a hash from all used resources in a pod template spec and adds that to trigger pod restarts

Fixes #1424 #1036

Documention: kudobuilder/kudo.dev#238

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Reorganize code Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

gerred · 2020-04-24T15:12:16Z

@ANeumann82 I'll get this reviewed, sorry for the delay!

Added unit test Moved kuttl test to e2e, deployments won't work with integration Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

zen-dog

Nice work! 👏
I left a few comments, most of them are nits. I'd like to question the use of metav1.Object and the resulting reflection in the form of reflect.TypeOf. Aren't we better using the Unstructured and the GetObjectKind() method to build keys/cache/sort especially because the enhancer already converts all passed objects to unstructered?

pkg/controller/instance/instance_controller.go

pkg/engine/renderer/dependencies.go

nfnt

The hash dependency handling looks fine, I'm mostly worried about compatibility with older and newer API versions of Kubernetes objects. This is handled with Group/Version/Kind of runtime objects and we should do the same instead of using Go reflection. This will complicate functions like setTemplateHash but will also ensure that operators don't break when using different API versions of resources.

pkg/engine/renderer/dependencies_test.go

pkg/engine/renderer/dependencies.go

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Adjusted e2e test Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

zen-dog

Aside from a few nits, my only wish would be to have more unit test coverage. Lower test levels - higher development speed 😉

pkg/engine/renderer/dependencies.go

pkg/engine/renderer/enhancer.go

pkg/engine/renderer/enhancer_test.go

zen-dog · 2020-04-30T10:43:34Z

pkg/engine/renderer/enhancer_test.go

+		assert.Equal(t, "929a2dffa86ad2460fdcf72977998bd0", hash, "Hashes are not the same")
+		assert.True(t, ok, "Statefulset contains no dependency hash field")
+	}
+}


I'd like to see more test coverage on the unit test level. For example, could you add a test, where a parameter change leads to a hash change (where a hash is already set)? And could you also through in an independent object (e.g. a Deployment) and check that the hash is not set on it?

Added unit tests, let me know if you have another idea for one.

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

zen-dog

A few testing nits but this is great work! Definitely a release highlight (and needs some better PR description showcasing the greatness)! 👏

pkg/engine/renderer/dependencies_test.go

zen-dog · 2020-05-04T12:16:50Z

pkg/engine/renderer/enhancer_test.go

+	assert.Nil(t, hash, "Pod template spec annotations contains a dependency hash but no dependencies")
+}
+
+func TestEnhancerApply_dependencyHash_changes(t *testing.T) {


suggestion: for long tests like this, which go through multiple states ginkgo might offer a better structure (and closer to scalatest). Take a look at the instance_admission_integration_test.go

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

nfnt

LGTM!
Using Kubernetes's own typing mechanism instead of reflection makes this more abstract and, IMO, less readable but keeps us compatible with possible API changes.

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 added 3 commits April 23, 2020 18:13

Initial commit on dependencies hash

8570afe

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Skip ownerreference and namespace from hash calculation

631d63b

Reorganize code Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

More code cleanup and documentation

8110a1e

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 requested review from alenkacz, gerred, jbarrick-mesosphere, kensipe, nfnt and zen-dog as code owners April 24, 2020 12:09

ANeumann82 added 6 commits April 24, 2020 14:40

Removed logs

584bb0b

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Fixed kudo-test that i changed for testing

64ace23

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Updated hashes in integration tests

e00428f

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

-.-

14a9952

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Revert all changes to kudo-test.yaml

16dd36f

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Merge branch 'master' into an/kep-27-dependencies-hash

a5cb868

gerred self-assigned this Apr 24, 2020

ANeumann82 added 2 commits April 24, 2020 19:31

Fixed reflection bug

a4ff844

Added unit test Moved kuttl test to e2e, deployments won't work with integration Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

e2e tests need to wait for the controller to start up...

ebf71ac

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

zen-dog reviewed Apr 28, 2020

View reviewed changes

nfnt requested changes Apr 29, 2020

View reviewed changes

pkg/engine/renderer/dependencies_test.go Outdated Show resolved Hide resolved

pkg/engine/renderer/dependencies.go Outdated Show resolved Hide resolved

ANeumann82 added 3 commits April 29, 2020 12:57

First round of review requests

4d31ada

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Reworked dependency handling

03cdb61

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Converted dependency calculation handling to unstructured

f5f0377

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 requested a review from nfnt April 29, 2020 15:50

ANeumann82 added 2 commits April 29, 2020 19:44

Fixed bug in dependency hash calculation

aee59bc

Adjusted e2e test Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Fixed integration test

6341cb2

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

zen-dog reviewed Apr 30, 2020

View reviewed changes

ANeumann82 added 2 commits April 30, 2020 13:38

Review changes

56c90af

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

Added unit tests

07248d1

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

zen-dog approved these changes May 4, 2020

View reviewed changes

zen-dog added the release/highlight This PR is a highlight for the next release label May 4, 2020

Cleaned up unit tests and use t.Run

48488a3

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

nfnt approved these changes May 4, 2020

View reviewed changes

ANeumann82 added 2 commits May 5, 2020 09:50

Merge branch 'master' into an/kep-27-dependencies-hash

96d866f

Fixed e2e test assertations

daa7bdd

Signed-off-by: Andreas Neumann <aneumann@mesosphere.com>

ANeumann82 merged commit 1d1a9c1 into master May 5, 2020

ANeumann82 deleted the an/kep-27-dependencies-hash branch May 5, 2020 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEP-27: Detailed pod restart control by dependencies hash #1483

KEP-27: Detailed pod restart control by dependencies hash #1483

ANeumann82 commented Apr 24, 2020 •

edited

Loading

gerred commented Apr 24, 2020

zen-dog left a comment

nfnt left a comment

zen-dog left a comment

zen-dog Apr 30, 2020

ANeumann82 Apr 30, 2020

zen-dog left a comment •

edited

Loading

zen-dog May 4, 2020

nfnt left a comment

KEP-27: Detailed pod restart control by dependencies hash #1483

KEP-27: Detailed pod restart control by dependencies hash #1483

Conversation

ANeumann82 commented Apr 24, 2020 • edited Loading

gerred commented Apr 24, 2020

zen-dog left a comment

Choose a reason for hiding this comment

nfnt left a comment

Choose a reason for hiding this comment

zen-dog left a comment

Choose a reason for hiding this comment

zen-dog Apr 30, 2020

Choose a reason for hiding this comment

ANeumann82 Apr 30, 2020

Choose a reason for hiding this comment

zen-dog left a comment • edited Loading

Choose a reason for hiding this comment

zen-dog May 4, 2020

Choose a reason for hiding this comment

nfnt left a comment

Choose a reason for hiding this comment

ANeumann82 commented Apr 24, 2020 •

edited

Loading

zen-dog left a comment •

edited

Loading