Repository: argoproj/argo
Pull request #2548 (2020-03-30 23:01:22)

fix(controller): Workflow stop and resume by node didn't properly support offloaded nodes. Fixes #2543
Checklist:

* [X] Either (a) I've created an [enhancement proposal](https://github.com/argoproj/argo/issues/new/choose) 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](https://www.conventionalcommits.org/en/v1.0.0/), (b) states what changed, and (c) suffixes the related issues number. E.g. `"fix(controller): Updates such and such. Fixes #1234"`.  
* [X] I have written unit and/or e2e tests for my change. PRs without these are unlike to be merged.
* [X] Optional. I've added My organization is added to the USERS.md.
* [X] I've signed the CLA and required builds are green. 

Fixes https://github.com/argoproj/argo/issues/2621

mark9white: Am starting off by running some builds with the increased timeouts to look at the logs.
codecov[bot]: # [Codecov](https://codecov.io/gh/argoproj/argo/pull/2548?src=pr&el=h1) Report
> :exclamation: No coverage uploaded for pull request base (`master@79dc969`). [Click here to learn what that means](https://docs.codecov.io/docs/error-reference#section-missing-base-commit).
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/argoproj/argo/pull/2548/graphs/tree.svg?width=650&token=4c3DPSTq8o&height=150&src=pr)](https://codecov.io/gh/argoproj/argo/pull/2548?src=pr&el=tree)

```diff
@@            Coverage Diff            @@
##             master    #2548   +/-   ##
=========================================
  Coverage          ?   11.16%           
=========================================
  Files             ?       75           
  Lines             ?    31818           
  Branches          ?        0           
=========================================
  Hits              ?     3553           
  Misses            ?    27781           
  Partials          ?      484
```



------

[Continue to review full report at Codecov](https://codecov.io/gh/argoproj/argo/pull/2548?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/argoproj/argo/pull/2548?src=pr&el=footer). Last update [79dc969...fc3833e](https://codecov.io/gh/argoproj/argo/pull/2548?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

mark9white: I'm currently unable to progress with this as I can't reproduce locally and the CI build is not executing.
alexec: Try merging master in?
sonarcloud[bot]: Kudos, SonarCloud Quality Gate passed!

[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=BUG)  
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=SECURITY_HOTSPOT) to review)  
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=CODE_SMELL)

[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/NoCoverageInfo.png' alt='No Coverage information' width='16' height='16' />](https://sonarcloud.io/component_measures?id=argoproj_argo&pullRequest=2548) No Coverage information  
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/5.png' alt='4.3%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=argoproj_argo&pullRequest=2548&metric=new_duplicated_lines_density&view=list) [4.3% Duplication](https://sonarcloud.io/component_measures?id=argoproj_argo&pullRequest=2548&metric=new_duplicated_lines_density&view=list)


mark9white: @alexec it looks like the issue here was related to node offload. Why is there a different set of tests in CLIWithServerSuite as compared to CLISuite? It seems we would want to run this test in both.
simster7: > @alexec it looks like the issue here was related to node offload. Why is there a different set of tests in CLIWithServerSuite as compared to CLISuite? It seems we would want to run this test in both.

The difference is that `CLIWithServerSuite` the Argo Server used has access to the Offload Node Status Repo, while in the `CLISuite` it does not. If your test needs access to the repo, add it to `CLIWithServerSuite`, if it doesn't add it to `CLISuite`. You can also add a:

```go
if !s.Persistence.Enabled() {
    s.T().SkipNow()
}
```

check to protect tests that you only want to run in an environment where the Argo Server has (or doesn't have) persistence enabled.

I just did something similar, take a look at https://github.com/argoproj/argo/pull/2645/files#diff-4f326ca1865929af6dabf0119581b09cR473 for an example.
mark9white: Thanks. I came across that, but in this case I would think the test should
run in both cases but I don’t want to duplicate the code. I’ll have a think
how to do this.

On Sat, 11 Apr 2020 at 17:06, Simon Behar <notifications@github.com> wrote:

> @alexec <https://github.com/alexec> it looks like the issue here was
> related to node offload. Why is there a different set of tests in
> CLIWithServerSuite as compared to CLISuite? It seems we would want to run
> this test in both.
>
> The difference is that CLIWithServerSuite the Argo Server used has access
> to the Offload Node Status Repo, while in the CLISuite it does not. If
> your test needs access to the repo, add it to CLIWithServerSuite, if it
> doesn't add it to CLISuite. You can also add a:
>
> if !s.Persistence.Enabled() {
>     s.T().SkipNow()
> }
>
> check to protect tests that you only want to run in an environment where
> the Argo Server has (or doesn't have) persistence enabled.
>
> I just did something similar, take a look at
> https://github.com/argoproj/argo/pull/2645/files#diff-4f326ca1865929af6dabf0119581b09cR473
> for an example.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/argoproj/argo/pull/2548#issuecomment-612453881>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABO7LWHDD3U62I4MWTM2VTRMCIPTANCNFSM4LXCH5OA>
> .
>

mark9white: @alexec Can you please tell me if this approach is ok before I move onto #2850

(and do you know why ci/circleci: test didn't run?)
mark9white: The issue was workflow stop and resume by node didn't properly support offloaded nodes, I'll change the name.
mark9white: @alexec FYI I have just rebased on master.
sonarcloud[bot]: SonarCloud Quality Gate failed.

[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=BUG)  
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=SECURITY_HOTSPOT) to review)  
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/B.png' alt='B' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=CODE_SMELL) [4 Code Smells](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=CODE_SMELL)

[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/0.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=argoproj_argo&pullRequest=2548&metric=new_coverage&view=list) [0.0% Coverage](https://sonarcloud.io/component_measures?id=argoproj_argo&pullRequest=2548&metric=new_coverage&view=list)  
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=argoproj_argo&pullRequest=2548&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=argoproj_argo&pullRequest=2548&metric=new_duplicated_lines_density&view=list)


mark9white: @alexec please take another look now.
sonarcloud[bot]: Kudos, SonarCloud Quality Gate passed!

[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=BUG)  
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=SECURITY_HOTSPOT) to review)  
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=CODE_SMELL)

[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60.png' alt='74.1%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=argoproj_argo&pullRequest=2548&metric=new_coverage&view=list) [74.1% Coverage](https://sonarcloud.io/component_measures?id=argoproj_argo&pullRequest=2548&metric=new_coverage&view=list)  
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=argoproj_argo&pullRequest=2548&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=argoproj_argo&pullRequest=2548&metric=new_duplicated_lines_density&view=list)


alexec: Please fix lint error:
```
workflow/util/util.go:486:27: Error return value of `compressAndOffloadNodes` is not checked (errcheck)
 
			compressAndOffloadNodes(wf, repo, nodes)
 
```
alexec: Can you login to https://app.circleci.com/? I think you might need to do that to prevent the unauthorized error.
mark9white: @alexec I get a 404 at https://app.circleci.com but I have logged into https://circleci.com/dashboard - unfortunately it didn't help.
alexec: OK. I've just merged a fix to master that I think will fix it. Can you sync with master please?

sonarcloud[bot]: Kudos, SonarCloud Quality Gate passed!

[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=BUG)  
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=VULNERABILITY) (and [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=SECURITY_HOTSPOT) to review)  
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=argoproj_argo&pullRequest=2548&resolved=false&types=CODE_SMELL)

[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/60.png' alt='74.1%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=argoproj_argo&pullRequest=2548&metric=new_coverage&view=list) [74.1% Coverage](https://sonarcloud.io/component_measures?id=argoproj_argo&pullRequest=2548&metric=new_coverage&view=list)  
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=argoproj_argo&pullRequest=2548&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=argoproj_argo&pullRequest=2548&metric=new_duplicated_lines_density&view=list)


mark9white: @alexec 
Looks like that worked 👍

Patch:
diff --git a/server/workflow/workflow_server.go b/server/workflow/workflow_server.go
index ad85c69cf..beea6c103 100644
--- a/server/workflow/workflow_server.go
+++ b/server/workflow/workflow_server.go
@@ -323,7 +323,7 @@ func (s *workflowServer) TerminateWorkflow(ctx context.Context, req *workflowpkg
 
 func (s *workflowServer) StopWorkflow(ctx context.Context, req *workflowpkg.WorkflowStopRequest) (*v1alpha1.Workflow, error) {
 	wfClient := auth.GetWfClient(ctx)
-	err := util.StopWorkflow(wfClient.ArgoprojV1alpha1().Workflows(req.Namespace), req.Name, req.NodeFieldSelector, req.Message)
+	err := util.StopWorkflow(wfClient.ArgoprojV1alpha1().Workflows(req.Namespace), s.offloadNodeStatusRepo, req.Name, req.NodeFieldSelector, req.Message)
 	if err != nil {
 		return nil, err
 	}
diff --git a/workflow/util/util.go b/workflow/util/util.go
index f1e41b097..3a5cea0ed 100644
--- a/workflow/util/util.go
+++ b/workflow/util/util.go
@@ -329,7 +329,7 @@ func SuspendWorkflow(wfIf v1alpha1.WorkflowInterface, workflowName string) error
 // Retries conflict errors
 func ResumeWorkflow(wfIf v1alpha1.WorkflowInterface, repo sqldb.OffloadNodeStatusRepo, workflowName string, nodeFieldSelector string) error {
 	if len(nodeFieldSelector) > 0 {
-		return updateWorkflowNodeByKey(wfIf, workflowName, nodeFieldSelector, wfv1.NodeSucceeded, "")
+		return updateWorkflowNodeByKey(wfIf, repo, workflowName, nodeFieldSelector, wfv1.NodeSucceeded, "")
 	} else {
 		err := wait.ExponentialBackoff(retry.DefaultRetry, func() (bool, error) {
 			wf, err := wfIf.Get(workflowName, metav1.GetOptions{})
@@ -337,9 +337,9 @@ func ResumeWorkflow(wfIf v1alpha1.WorkflowInterface, repo sqldb.OffloadNodeStatu
 				return false, err
 			}
 
-			err = packer.DecompressWorkflow(wf)
+			err = decompressAndFetchOffloadedNodes(wf, repo)
 			if err != nil {
-				return false, fmt.Errorf("unable to decompress workflow: %s", err)
+				return false, err
 			}
 
 			workflowUpdated := false
@@ -348,21 +348,10 @@ func ResumeWorkflow(wfIf v1alpha1.WorkflowInterface, repo sqldb.OffloadNodeStatu
 				workflowUpdated = true
 			}
 
-			nodes := wf.Status.Nodes
-			if wf.Status.IsOffloadNodeStatus() {
-				if !repo.IsEnabled() {
-					return false, fmt.Errorf(sqldb.OffloadNodeStatusDisabled)
-				}
-				var err error
-				nodes, err = repo.Get(string(wf.UID), wf.GetOffloadNodeStatusVersion())
-				if err != nil {
-					return false, fmt.Errorf("unable to retrieve offloaded nodes: %s", err)
-				}
-			}
-			newNodes := nodes.DeepCopy()
+			newNodes := wf.Status.Nodes.DeepCopy()
 
 			// To resume a workflow with a suspended node we simply mark the node as Successful
-			for nodeID, node := range nodes {
+			for nodeID, node := range wf.Status.Nodes {
 				if node.IsActiveSuspendNode() {
 					node.Phase = wfv1.NodeSucceeded
 					node.FinishedAt = metav1.Time{Time: time.Now().UTC()}
@@ -372,24 +361,9 @@ func ResumeWorkflow(wfIf v1alpha1.WorkflowInterface, repo sqldb.OffloadNodeStatu
 			}
 
 			if workflowUpdated {
-				if wf.Status.IsOffloadNodeStatus() {
-					if !repo.IsEnabled() {
-						return false, fmt.Errorf(sqldb.OffloadNodeStatusDisabled)
-					}
-					offloadVersion, err := repo.Save(string(wf.UID), wf.Namespace, newNodes)
-					if err != nil {
-						return false, fmt.Errorf("unable to save offloaded nodes: %s", err)
-					}
-					wf.Status.OffloadNodeStatusVersion = offloadVersion
-					wf.Status.CompressedNodes = ""
-					wf.Status.Nodes = nil
-				} else {
-					wf.Status.Nodes = newNodes
-				}
-
-				err = packer.CompressWorkflowIfNeeded(wf)
+				err = compressAndOffloadNodes(wf, repo, newNodes)
 				if err != nil {
-					return false, fmt.Errorf("unable to compress workflow: %s", err)
+					return false, fmt.Errorf("unable to compress or offload workflow nodes: %s", err)
 				}
 
 				_, err = wfIf.Update(wf)
@@ -406,6 +380,57 @@ func ResumeWorkflow(wfIf v1alpha1.WorkflowInterface, repo sqldb.OffloadNodeStatu
 	}
 }
 
+func decompressAndFetchOffloadedNodes(wf *wfv1.Workflow, repo sqldb.OffloadNodeStatusRepo) error {
+	err := packer.DecompressWorkflow(wf)
+	if err != nil {
+		return err
+	}
+
+	if wf.Status.IsOffloadNodeStatus() {
+		if !repo.IsEnabled() {
+			return fmt.Errorf(sqldb.OffloadNodeStatusDisabled)
+		}
+		nodes, err := repo.Get(string(wf.UID), wf.GetOffloadNodeStatusVersion())
+		if err != nil {
+			return fmt.Errorf("unable to retrieve offloaded nodes: %s", err)
+		} else {
+			wf.Status.Nodes = nodes
+		}
+	}
+	return nil
+}
+
+func compressAndOffloadNodes(wf *wfv1.Workflow, repo sqldb.OffloadNodeStatusRepo, newNodes wfv1.Nodes) error {
+	wf.Status.Nodes = newNodes
+	doOffload := wf.Status.IsOffloadNodeStatus() || os.Getenv("ALWAYS_OFFLOAD_NODE_STATUS") == "true"
+
+	if !doOffload {
+		err := packer.CompressWorkflowIfNeeded(wf)
+		if packer.IsTooLargeError(err) && repo.IsEnabled() {
+			doOffload = true
+			err = nil
+		}
+		if err != nil {
+			return err
+		}
+	}
+
+	if doOffload {
+		if !repo.IsEnabled() {
+			return fmt.Errorf(sqldb.OffloadNodeStatusDisabled)
+		}
+		offloadVersion, err := repo.Save(string(wf.UID), wf.Namespace, wf.Status.Nodes)
+		if err == nil {
+			wf.Status.Nodes = nil
+			wf.Status.CompressedNodes = ""
+			wf.Status.OffloadNodeStatusVersion = offloadVersion
+		} else {
+			return err
+		}
+	}
+	return nil
+}
+
 func selectorMatchesNode(selector fields.Selector, node wfv1.NodeStatus) bool {
 	nodeFields := fields.Set{
 		"displayName":  node.DisplayName,
@@ -425,7 +450,7 @@ func selectorMatchesNode(selector fields.Selector, node wfv1.NodeStatus) bool {
 	return selector.Matches(nodeFields)
 }
 
-func updateWorkflowNodeByKey(wfIf v1alpha1.WorkflowInterface, workflowName string, nodeFieldSelector string, phase wfv1.NodePhase, message string) error {
+func updateWorkflowNodeByKey(wfIf v1alpha1.WorkflowInterface, repo sqldb.OffloadNodeStatusRepo, workflowName string, nodeFieldSelector string, phase wfv1.NodePhase, message string) error {
 	selector, err := fields.ParseSelector(nodeFieldSelector)
 
 	if err != nil {
@@ -437,13 +462,14 @@ func updateWorkflowNodeByKey(wfIf v1alpha1.WorkflowInterface, workflowName strin
 			return false, err
 		}
 
-		err = packer.DecompressWorkflow(wf)
+		err = decompressAndFetchOffloadedNodes(wf, repo)
 		if err != nil {
-			log.Fatal(err)
+			return false, err
 		}
 
 		nodeUpdated := false
-		for nodeID, node := range wf.Status.Nodes {
+		nodes := wf.Status.Nodes
+		for nodeID, node := range nodes {
 			if node.IsActiveSuspendNode() {
 				if selectorMatchesNode(selector, node) {
 					node.Phase = phase
@@ -451,12 +477,17 @@ func updateWorkflowNodeByKey(wfIf v1alpha1.WorkflowInterface, workflowName strin
 					if len(message) > 0 {
 						node.Message = message
 					}
-					wf.Status.Nodes[nodeID] = node
+					nodes[nodeID] = node
 					nodeUpdated = true
 				}
 			}
 		}
 		if nodeUpdated {
+			err = compressAndOffloadNodes(wf, repo, nodes)
+			if err != nil {
+				return false, fmt.Errorf("unable to compress or offload workflow nodes: %s", err)
+			}
+
 			_, err = wfIf.Update(wf)
 			if err != nil {
 				if apierr.IsConflict(err) {
@@ -614,9 +645,9 @@ func RetryWorkflow(kubeClient kubernetes.Interface, repo sqldb.OffloadNodeStatus
 		return nil, errors.Errorf(errors.CodeBadRequest, "workflow must be Failed/Error to retry")
 	}
 
-	err := packer.DecompressWorkflow(wf)
+	err := decompressAndFetchOffloadedNodes(wf, repo)
 	if err != nil {
-		return nil, fmt.Errorf("unable to decompress workflow: %s", err)
+		return nil, err
 	}
 
 	newWF := wf.DeepCopy()
@@ -638,16 +669,6 @@ func RetryWorkflow(kubeClient kubernetes.Interface, repo sqldb.OffloadNodeStatus
 	newNodes := make(map[string]wfv1.NodeStatus)
 	onExitNodeName := wf.ObjectMeta.Name + ".onExit"
 	nodes := wf.Status.Nodes
-	if wf.Status.IsOffloadNodeStatus() {
-		if !repo.IsEnabled() {
-			return nil, fmt.Errorf(sqldb.OffloadNodeStatusDisabled)
-		}
-		var err error
-		nodes, err = repo.Get(string(wf.UID), wf.GetOffloadNodeStatusVersion())
-		if err != nil {
-			return nil, fmt.Errorf("unable to retrieve offloaded nodes: %s", err)
-		}
-	}
 
 	// Get all children of nodes that match filter
 	nodeIDsToReset, err := getNodeIDsToReset(restartSuccessful, nodeFieldSelector, nodes)
@@ -698,19 +719,9 @@ func RetryWorkflow(kubeClient kubernetes.Interface, repo sqldb.OffloadNodeStatus
 		}
 	}
 
-	if wf.Status.IsOffloadNodeStatus() {
-		if !repo.IsEnabled() {
-			return nil, fmt.Errorf(sqldb.OffloadNodeStatusDisabled)
-		}
-		offloadVersion, err := repo.Save(string(newWF.UID), newWF.Namespace, newNodes)
-		if err != nil {
-			return nil, fmt.Errorf("unable to save offloaded nodes: %s", err)
-		}
-		newWF.Status.OffloadNodeStatusVersion = offloadVersion
-		newWF.Status.CompressedNodes = ""
-		newWF.Status.Nodes = nil
-	} else {
-		newWF.Status.Nodes = newNodes
+	err = compressAndOffloadNodes(newWF, repo, newNodes)
+	if err != nil {
+		return nil, fmt.Errorf("unable to compress or offload workflow nodes: %s", err)
 	}
 
 	newWF.Status.StoredTemplates = make(map[string]wfv1.Template)
@@ -718,11 +729,6 @@ func RetryWorkflow(kubeClient kubernetes.Interface, repo sqldb.OffloadNodeStatus
 		newWF.Status.StoredTemplates[id] = tmpl
 	}
 
-	err = packer.CompressWorkflowIfNeeded(newWF)
-	if err != nil {
-		return nil, fmt.Errorf("unable to compress workflow: %s", err)
-	}
-
 	return wfClient.Update(newWF)
 }
 
@@ -799,9 +805,9 @@ func TerminateWorkflow(wfClient v1alpha1.WorkflowInterface, name string) error {
 
 // StopWorkflow terminates a workflow by setting its spec.shutdown to ShutdownStrategyStop
 // Or terminates a single resume step referenced by nodeFieldSelector
-func StopWorkflow(wfClient v1alpha1.WorkflowInterface, name string, nodeFieldSelector string, message string) error {
+func StopWorkflow(wfClient v1alpha1.WorkflowInterface, repo sqldb.OffloadNodeStatusRepo, name string, nodeFieldSelector string, message string) error {
 	if len(nodeFieldSelector) > 0 {
-		return updateWorkflowNodeByKey(wfClient, name, nodeFieldSelector, wfv1.NodeFailed, message)
+		return updateWorkflowNodeByKey(wfClient, repo, name, nodeFieldSelector, wfv1.NodeFailed, message)
 	} else {
 		patchObj := map[string]interface{}{
 			"spec": map[string]interface{}{


Test patch:
diff --git a/test/e2e/cli_test.go b/test/e2e/cli_test.go
index 04b9d36b7..702d72d7a 100644
--- a/test/e2e/cli_test.go
+++ b/test/e2e/cli_test.go
@@ -307,9 +307,15 @@ func (s *CLISuite) TestWorkflowSuspendResume() {
 		})
 }
 
-func (s *CLISuite) TestNodeSuspendResume() {
-	// https://github.com/argoproj/argo/issues/2621
-	s.T().SkipNow()
+func (s *CLISuite) TestNodeSuspendResumeNoPersistence() {
+	if s.Persistence.IsEnabled() {
+		// Persistence is enabled for this test, but it is not enabled for the Argo Server in this test suite.
+		s.T().SkipNow()
+	}
+	NodeSuspendResumeCommon(s.E2ESuite)
+}
+
+func NodeSuspendResumeCommon(s fixtures.E2ESuite) {
 	s.Given().
 		Workflow("@testdata/node-suspend.yaml").
 		When().
diff --git a/test/e2e/cli_with_server_test.go b/test/e2e/cli_with_server_test.go
index 844b55109..817bb7d37 100644
--- a/test/e2e/cli_with_server_test.go
+++ b/test/e2e/cli_with_server_test.go
@@ -193,6 +193,15 @@ func (s *CLIWithServerSuite) TestWorkflowSuspendResumePersistence() {
 		})
 }
 
+func (s *CLIWithServerSuite) TestNodeSuspendResumePersistence() {
+	if !s.Persistence.IsEnabled() {
+		// Persistence is disabled for this test, but it is enabled for the Argo Server in this test suite.
+		// When this is the case, this behavior is tested in cli_test.go
+		s.T().SkipNow()
+	}
+	NodeSuspendResumeCommon(s.E2ESuite)
+}
+
 func TestCLIWithServerSuite(t *testing.T) {
 	suite.Run(t, new(CLIWithServerSuite))
 }
diff --git a/workflow/controller/operator_test.go b/workflow/controller/operator_test.go
index df536c315..f613c9bab 100644
--- a/workflow/controller/operator_test.go
+++ b/workflow/controller/operator_test.go
@@ -1228,7 +1228,7 @@ func TestSuspendTemplateWithFailedResume(t *testing.T) {
 	assert.Equal(t, 0, len(pods.Items))
 
 	// resume the workflow. verify resume workflow edits nodestatus correctly
-	err = util.StopWorkflow(wfcset, wf.ObjectMeta.Name, "inputs.parameters.param1.value=value1", "Step failed!")
+	err = util.StopWorkflow(wfcset, sqldb.ExplosiveOffloadNodeStatusRepo, wf.ObjectMeta.Name, "inputs.parameters.param1.value=value1", "Step failed!")
 	assert.NoError(t, err)
 	wf, err = wfcset.Get(wf.ObjectMeta.Name, metav1.GetOptions{})
 	assert.NoError(t, err)
diff --git a/workflow/util/util_test.go b/workflow/util/util_test.go
index 2ed2c85bf..d31462f83 100644
--- a/workflow/util/util_test.go
+++ b/workflow/util/util_test.go
@@ -303,6 +303,10 @@ func TestResumeWorkflowOffloaded(t *testing.T) {
 	wfIf := fakeClientset.NewSimpleClientset().ArgoprojV1alpha1().Workflows("")
 	origWf := unmarshalWF(suspendedWf)
 
+	//set threshold so this workflow is too big for compression to be valid
+	clearFunc := packer.SetMaxWorkflowSize(10)
+	defer clearFunc()
+
 	origNodes := origWf.Status.Nodes
 
 	origWf.Status.Nodes = nil
@@ -324,6 +328,78 @@ func TestResumeWorkflowOffloaded(t *testing.T) {
 	assert.Equal(t, "1234", wf.Status.OffloadNodeStatusVersion)
 }
 
+//workflow is too big but offload is disabled
+func TestResumeWorkflowOffloadDisabled(t *testing.T) {
+	wfIf := fakeClientset.NewSimpleClientset().ArgoprojV1alpha1().Workflows("")
+	origWf := unmarshalWF(suspendedWf)
+
+	//set threshold so this workflow is too big for compression to be valid
+	clearFunc := packer.SetMaxWorkflowSize(10)
+	defer clearFunc()
+
+
+	_, err := wfIf.Create(origWf)
+	assert.NoError(t, err)
+
+	offloadNodeStatusRepo := &mocks.OffloadNodeStatusRepo{}
+	offloadNodeStatusRepo.On("IsEnabled", mock.Anything).Return(false)
+
+	err = ResumeWorkflow(wfIf, offloadNodeStatusRepo, "suspend", "")
+	assert.Error(t, err)
+}
+
+
+func TestResumeWorkflowByNodeName(t *testing.T) {
+	wfIf := fakeClientset.NewSimpleClientset().ArgoprojV1alpha1().Workflows("")
+	origWf := unmarshalWF(suspendedWf)
+
+	_, err := wfIf.Create(origWf)
+	assert.NoError(t, err)
+
+	//will return error as displayName does not match any nodes
+	err = ResumeWorkflow(wfIf, nil, "suspend", "displayName=nonexistant")
+	assert.Error(t, err)
+
+	//displayName didn't match suspend node so should still be running
+	wf, err := wfIf.Get("suspend", metav1.GetOptions{})
+	assert.NoError(t, err)
+	assert.Equal(t, wfv1.NodeRunning, wf.Status.Nodes.FindByDisplayName("approve").Phase)
+
+	err = ResumeWorkflow(wfIf, nil, "suspend", "displayName=approve")
+	assert.NoError(t, err)
+
+	//displayName matched node so has succeeded
+	wf, err = wfIf.Get("suspend", metav1.GetOptions{})
+	assert.NoError(t, err)
+	assert.Equal(t, wfv1.NodeSucceeded, wf.Status.Nodes.FindByDisplayName("approve").Phase)
+}
+
+func TestStopWorkflowByNodeName(t *testing.T) {
+	wfIf := fakeClientset.NewSimpleClientset().ArgoprojV1alpha1().Workflows("")
+	origWf := unmarshalWF(suspendedWf)
+
+	_, err := wfIf.Create(origWf)
+	assert.NoError(t, err)
+
+	//will return error as displayName does not match any nodes
+	err = StopWorkflow(wfIf, nil, "suspend", "displayName=nonexistant", "error occurred")
+	assert.Error(t, err)
+
+	//displayName didn't match suspend node so should still be running
+	wf, err := wfIf.Get("suspend", metav1.GetOptions{})
+	assert.NoError(t, err)
+	assert.Equal(t, wfv1.NodeRunning, wf.Status.Nodes.FindByDisplayName("approve").Phase)
+
+	err = StopWorkflow(wfIf, nil, "suspend", "displayName=approve", "error occurred")
+	assert.NoError(t, err)
+
+	//displayName matched node so has succeeded
+	wf, err = wfIf.Get("suspend", metav1.GetOptions{})
+	assert.NoError(t, err)
+	assert.Equal(t, wfv1.NodeFailed, wf.Status.Nodes.FindByDisplayName("approve").Phase)
+}
+
+
 var failedWf = `
 apiVersion: argoproj.io/v1alpha1
 kind: Workflow
@@ -473,6 +549,10 @@ func TestRetryWorkflowOffloaded(t *testing.T) {
 	origWf.Status.Nodes = nil
 	origWf.Status.OffloadNodeStatusVersion = "123"
 
+	//set threshold so this workflow is too big for compression to be valid
+	clearFunc := packer.SetMaxWorkflowSize(10)
+	defer clearFunc()
+
 	_, err = wfIf.Create(origWf)
 	assert.NoError(t, err)