fix(controller): Implement offloading for workflow updates that are re-applied. Fixes #2856 #2941

alexec · 2020-05-04T17:06:12Z

Checklist:

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
I've signed the CLA.
I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
My builds are green. Try syncing with master if they are not.
My organization is added to USERS.md.

alexec · 2020-05-06T17:14:36Z

workflow/controller/operator_persist_test.go

@@ -103,7 +100,7 @@ func TestPersistWithLargeWfSupport(t *testing.T) {
 	assert.Empty(t, wf.Status.Nodes)
 	assert.Empty(t, wf.Status.CompressedNodes)
 	// check the updated in-memory version is pre-offloaded state
-	assert.True(t, woc.wf.Status.IsOffloadNodeStatus())
+	assert.False(t, woc.wf.Status.IsOffloadNodeStatus())


behaviour change

alexec · 2020-05-06T17:14:49Z

workflow/controller/operator_persist_test.go

-	assert.Equal(t, wfv1.NodeRunning, woc.wf.Status.Phase)
-	assert.NotEmpty(t, woc.wf.Status.Nodes)
-	assert.Empty(t, woc.wf.Status.CompressedNodes)
+	assert.Equal(t, wfv1.NodeError, wf.Status.Phase)


corrected behaviour

alexec · 2020-05-06T17:15:18Z

workflow/packer/packer.go

@@ -83,7 +84,12 @@ func compressWorkflow(wf *wfv1.Workflow) error {
 		return err
 	}
 	if large {
-		compressedSize, _ := getSize(wf)
+		compressedSize, err := getSize(wf)
+		wf.Status.CompressedNodes = ""


this was a bug

alexec · 2020-05-06T17:16:22Z

workflow/util/util_test.go

-}
-
-//workflow is too big but offload is disabled
-func TestResumeWorkflowOffloadDisabled(t *testing.T) {


these tests are all irrelevant now - they were testing the hydration code by proxy - we don't need that - or want that

alexec · 2020-05-06T21:54:06Z

TestWorkflowRetryNoPersistence

alexec · 2020-05-07T02:36:52Z

Makefile

@@ -311,8 +311,7 @@ dist/postgres.yaml: $(MANIFESTS) $(E2E_MANIFESTS) $(VERSION_FILE)
 	kustomize build --load_restrictor=none test/e2e/manifests/postgres | sed 's/:$(MANIFESTS_VERSION)/:$(VERSION)/' | sed 's/pns/$(E2E_EXECUTOR)/' > dist/postgres.yaml

 dist/no-db.yaml: $(MANIFESTS) $(E2E_MANIFESTS) $(VERSION_FILE)
-	# We additionally disable ALWAYS_OFFLOAD_NODE_STATUS
-	kustomize build --load_restrictor=none test/e2e/manifests/no-db | sed 's/:$(MANIFESTS_VERSION)/:$(VERSION)/' | sed 's/pns/$(E2E_EXECUTOR)/' | sed 's/"true"/"false"/' > dist/no-db.yaml
+	kustomize build --load_restrictor=none test/e2e/manifests/no-db | sed 's/:$(MANIFESTS_VERSION)/:$(VERSION)/' | sed 's/pns/$(E2E_EXECUTOR)/' > dist/no-db.yaml


alexec · 2020-05-07T02:37:47Z

workflow/controller/operator.go

+
+	if !woc.controller.hydrator.IsHydrated(woc.wf) {
+		woc.log.Error("workflow should be hydrated")
+		return


we should fail here

alexec · 2020-05-07T02:38:24Z

workflow/hydrator/hydrator.go

@@ -0,0 +1,79 @@
+package hydrator


any thoughts on another name for hydration?

workflow/util/util.go

markterm · 2020-05-12T15:53:27Z

workflow/controller/controller.go

 	if err != nil {
-		woc.log.Errorf("workflow decompression failed: %v", err)
+		woc.log.Errorf("hydration failed: %v", err)
 		woc.markWorkflowError(err, true)


markWorkflowError tries to access the workflow nodes, which may not exist if the hydration has failed.

catch-22: if we cannot hydrate the workflow, then we cannot access the nodes

markterm · 2020-05-12T15:53:43Z

workflow/controller/operator.go

-		woc.log.Warnf("Error compressing workflow: %v", err)
+	err := woc.controller.hydrator.Dehydrate(woc.wf)
+	if err != nil {
+		woc.log.Warnf("Failed to dehydrate: %v", err)
 		woc.markWorkflowError(err, true)


Same problem here

I don't think we need to fix this - I've made changes to Dehydrate to ensure that error branches do not result in a dehydrated workflow - could still fail to persist mind you

markterm · 2020-05-12T15:57:39Z

workflow/controller/operator.go

@@ -554,43 +541,56 @@ func (woc *wfOperationCtx) persistWorkflowSizeLimitErr(wfClient v1alpha1.Workflo
 // reapplyUpdate GETs the latest version of the workflow, re-applies the updates and
 // retries the UPDATE multiple times. For reasoning behind this technique, see:
 // https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#concurrency-control-and-consistency
-func (woc *wfOperationCtx) reapplyUpdate(wfClient v1alpha1.WorkflowInterface) (*wfv1.Workflow, error) {
+func (woc *wfOperationCtx) reapplyUpdate(wfClient v1alpha1.WorkflowInterface, nodes wfv1.Nodes) (*wfv1.Workflow, error) {


Could this do with extra unit tests, eg around the multiple attempt handling and merge scenarios?

I think you're right about this - toughie to test, but all mockable now.

alexec · 2020-05-12T18:54:14Z

workflow/packer/packer.go

@@ -80,10 +81,17 @@ func compressWorkflow(wf *wfv1.Workflow) error {
 	// still too large?
 	large, err := IsLargeWorkflow(wf)
 	if err != nil {
+		wf.Status.CompressedNodes = ""


an existing bug

alexec · 2020-05-13T22:14:09Z

@mark9white if you're happy that you ran this and it worked, I'll look to get a core team review and appoval.

markterm · 2020-05-14T00:55:12Z

@mark9white if you're happy that you ran this and it worked, I'll look to get a core team review and appoval.

That would be great :)

alexec · 2020-05-15T21:35:43Z

@sarabala1979 could I please ask you to review this PR for me?

sarabala1979

LGTM

sonarcloud · 2020-05-22T23:38:49Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
4 Code Smells

63.9% Coverage
0.0% Duplication

chore(controller): Refactor hydration code

dd2341d

alexec closed this May 4, 2020

Merge branch 'master' into chore-hydrate

715a0ba

alexec reopened this May 6, 2020

alexec added 3 commits May 6, 2020 08:53

chore-hydrate

e9e0a27

chore-hydrate

be91d47

chore-hydrate

bdaacf8

alexec commented May 6, 2020

View reviewed changes

alexec added 7 commits May 6, 2020 10:24

has noop methods

1349d40

chore-hydrate

9371463

chore-hydrate

c436a9b

chore-hydrate

6ae55b3

chore-hydrate

4851421

chore-hydrate

f5402da

lint

6bbc743

chore-hydrate

3870f26

alexec linked an issue May 7, 2020 that may be closed by this pull request

Persistence race condition with merging offloaded nodes #2856

Closed

alexec changed the title ~~chore(controller): Refactor hydration code~~ fix(controller): Implement offloading for workflow updates that are re-applied. Fixes #2856 May 7, 2020

alexec commented May 7, 2020

View reviewed changes

workflow/hydrator/hydrator.go

@@ -0,0 +1,79 @@

package hydrator

Copy link

Contributor Author

alexec May 7, 2020

Choose a reason for hiding this comment

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

any thoughts on another name for hydration?

alexec commented May 7, 2020

View reviewed changes

workflow/util/util.go Outdated Show resolved Hide resolved

alexec and others added 3 commits May 6, 2020 19:57

chore-hydrate

08d9e48

chore-hydrate

8791ca4

Merge branch 'master' into chore-hydrate

250efc6

alexec added the needs-discussion label May 7, 2020

alexec added 10 commits May 11, 2020 11:08

Merge branch 'master' into chore-hydrate

ac2b455

chore-hydrate

17eb397

chore-hydrate

1e2f397

chore-hydrate

64f8bb0

Merge branch 'master' into chore-hydrate

1577b07

chore-hydrate

4cc9dc2

Merge branch 'master' into chore-hydrate

d0e92ea

chore-hydrate

b47739a

chore-hydrate

97fc1ac

chore-hydrate

84d1e08

alexec removed the needs-discussion label May 12, 2020

markterm reviewed May 12, 2020

View reviewed changes

alexec added 2 commits May 12, 2020 10:33

Merge branch 'master' into chore-hydrate

cb07707

chore-hydrate

3fe1bb7

alexec commented May 12, 2020

View reviewed changes

alexec marked this pull request as ready for review May 12, 2020 20:48

alexec added 2 commits May 13, 2020 15:16

chore-hydrate

e5aa52c

chore-hydrate

ad6bb44

alexec assigned sarabala1979 May 18, 2020

sarabala1979 approved these changes May 22, 2020

View reviewed changes

alexec added 2 commits May 22, 2020 16:25

Merge branch 'master' into chore-hydrate

043e058

chore-hydrate

ac9ae1e

alexec merged commit 6464bd1 into argoproj:master May 23, 2020

alexec deleted the chore-hydrate branch May 23, 2020 00:10

alexec added this to the v2.9 milestone May 23, 2020

markterm mentioned this pull request Jun 8, 2020

fix(cli): Test workflow retry with and without persistence. Fixes #2850 #2880

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(controller): Implement offloading for workflow updates that are re-applied. Fixes #2856 #2941

fix(controller): Implement offloading for workflow updates that are re-applied. Fixes #2856 #2941

alexec commented May 4, 2020 •

edited

alexec May 6, 2020

alexec May 6, 2020

alexec May 6, 2020

alexec May 6, 2020

alexec commented May 6, 2020

alexec May 7, 2020

alexec May 7, 2020

alexec May 7, 2020

markterm May 12, 2020

alexec May 12, 2020

markterm May 12, 2020

alexec May 12, 2020

markterm May 12, 2020

alexec May 12, 2020

alexec May 12, 2020

alexec commented May 13, 2020

markterm commented May 14, 2020

alexec commented May 15, 2020

sarabala1979 left a comment

sonarcloud bot commented May 22, 2020

fix(controller): Implement offloading for workflow updates that are re-applied. Fixes #2856 #2941

fix(controller): Implement offloading for workflow updates that are re-applied. Fixes #2856 #2941

Conversation

alexec commented May 4, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexec commented May 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexec commented May 13, 2020

markterm commented May 14, 2020

alexec commented May 15, 2020

sarabala1979 left a comment

Choose a reason for hiding this comment

sonarcloud bot commented May 22, 2020

alexec commented May 4, 2020 •

edited