fix(controller): Workflow stop and resume by node didn't properly support offloaded nodes. Fixes #2543 #2548

markterm · 2020-03-30T23:01:22Z

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 have written unit and/or e2e tests for my change. PRs without these are unlike to be merged.
Optional. I've added My organization is added to the USERS.md.
I've signed the CLA and required builds are green.

Fixes #2621

markterm · 2020-03-30T23:01:52Z

Am starting off by running some builds with the increased timeouts to look at the logs.

codecov · 2020-03-31T00:11:45Z

Codecov Report

Merging #2548 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2548      +/-   ##
==========================================
+ Coverage   11.16%   11.19%   +0.02%     
==========================================
  Files          83       83              
  Lines       32673    32645      -28     
==========================================
+ Hits         3649     3653       +4     
+ Misses      28525    28495      -30     
+ Partials      499      497       -2

Impacted Files	Coverage Δ
workflow/common/convert.go	`78.57% <0.00%> (-10.00%)`	⬇️
workflow/validate/validate.go	`74.13% <0.00%> (-0.28%)`	⬇️
persist/sqldb/migrate.go	`0.00% <0.00%> (ø)`
pkg/apis/workflow/v1alpha1/workflow_types.go	`10.93% <0.00%> (+0.02%)`	⬆️
workflow/executor/executor.go	`4.41% <0.00%> (+0.03%)`	⬆️
workflow/controller/steps.go	`65.35% <0.00%> (+0.46%)`	⬆️
workflow/controller/operator.go	`61.62% <0.00%> (+0.58%)`	⬆️
workflow/controller/dag.go	`51.25% <0.00%> (+0.63%)`	⬆️
workflow/templateresolution/context.go	`69.46% <0.00%> (+4.28%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ffc43ce...ca3c4d6. Read the comment docs.

markterm · 2020-04-01T09:42:07Z

I'm currently unable to progress with this as I can't reproduce locally and the CI build is not executing.

alexec · 2020-04-05T19:01:30Z

Try merging master in?

test/e2e/cli_test.go

markterm · 2020-04-11T11:39:48Z

@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 · 2020-04-11T16:06:04Z

@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 in CLIWithServerSuite the Argo Server is given access to the Offload Node Status Repo, while in the CLISuite it is 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.

markterm · 2020-04-12T09:42:46Z

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 ***@***.***> 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 <#2548 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABO7LWHDD3U62I4MWTM2VTRMCIPTANCNFSM4LXCH5OA> .

markterm · 2020-04-27T09:36:53Z

@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?)

alexec

Am I right in thinking there was a bug in stopping workflows? If so, can the title of the PR be updated to reflect this?

markterm · 2020-04-27T15:44:39Z

The issue was workflow stop and resume by node didn't properly support offloaded nodes, I'll change the name.

ok

markterm · 2020-04-29T11:21:08Z

@alexec FYI I have just rebased on master.

alexec · 2020-04-29T16:28:50Z

workflow/util/util.go

+				wf.Status.Nodes = nil
+			}
+
+			err = packer.CompressWorkflowIfNeeded(wf)


This ordering is incorrect, CompressWorkflowIfNeeded should always come before repo.Save.

This file misses unit tests. Would you please be able to add them? "No - it is too hard" would be a fine answer.

In terms of the ordering, wouldn't that result in it always compressing the nodes if they're too big, whereas currently if offload is enabled for the workflow then it won't try to compress the nodes.

By missing tests, are you talking about adding a test to util_test.go that covers the updateWorkflowNodeByKey function?

you should be able to see this pattern elsewhere - compression can fail - then we offload

markterm · 2020-04-30T15:15:02Z

@alexec please take another look now.

alexec · 2020-04-30T16:30:30Z

Please fix lint error:

workflow/util/util.go:486:27: Error return value of `compressAndOffloadNodes` is not checked (errcheck)
 
			compressAndOffloadNodes(wf, repo, nodes)

alexec · 2020-04-30T21:45:55Z

Can you login to https://app.circleci.com/? I think you might need to do that to prevent the unauthorized error.

markterm · 2020-04-30T22:27:52Z

@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 · 2020-04-30T23:02:37Z

OK. I've just merged a fix to master that I think will fix it. Can you sync with master please?

sonarcloud · 2020-04-30T23:17:48Z

Kudos, SonarCloud Quality Gate passed!

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

74.1% Coverage
0.0% Duplication

markterm · 2020-04-30T23:59:18Z

@alexec
Looks like that worked 👍

alexec · 2020-05-01T00:17:44Z

Boom! Merged! 🚀

…port offloaded nodes. Fixes #2543 (#2548)

markterm force-pushed the suspend-test-fix branch from bf09b61 to 88dd2d9 Compare April 6, 2020 10:09

golangcibot reviewed Apr 6, 2020

View reviewed changes

test/e2e/cli_test.go Outdated Show resolved Hide resolved

alexec changed the title ~~Suspend test fix, fixes #2543~~ fix: Suspend test fix. Fixes #2543 Apr 6, 2020

markterm force-pushed the suspend-test-fix branch 2 times, most recently from e7d340b to 961000b Compare April 11, 2020 10:17

markterm force-pushed the suspend-test-fix branch from 961000b to 6eeae7c Compare April 24, 2020 10:53

alexec previously requested changes Apr 27, 2020

View reviewed changes

markterm changed the title ~~fix: Suspend test fix. Fixes #2543~~ fix: Workflow stop and resume by node didn't properly support offloaded nodes. Fixes #2543 Apr 27, 2020

alexec approved these changes Apr 28, 2020

View reviewed changes

alexec self-assigned this Apr 28, 2020

simster7 mentioned this pull request Apr 28, 2020

Fix TestNodeSuspendResume #2621

Closed

markterm force-pushed the suspend-test-fix branch from 0df8aaa to e84e313 Compare April 29, 2020 10:00

alexec approved these changes Apr 29, 2020

View reviewed changes

alexec changed the title ~~fix: Workflow stop and resume by node didn't properly support offloaded nodes. Fixes #2543~~ fix(controller): Workflow stop and resume by node didn't properly support offloaded nodes. Fixes #2543 Apr 29, 2020

alexec reviewed Apr 29, 2020

View reviewed changes

alexec approved these changes Apr 30, 2020

View reviewed changes

markterm added 9 commits May 1, 2020 00:12

Suspend test fix

d3333f7

Test node suspend/resume with persistence

8aaf0fb

Bug Fix

92e4467

Follow compress/save pattern used for offloaded nodes elsewhere

853f8d5

Extra unit tests

ebca6a6

Remove code duplication

3d814fe

Remove code duplication

4d85b11

Added extra test

c604c28

Fixed golint error

aad287b

markterm force-pushed the suspend-test-fix branch from 9e3e284 to aad287b Compare April 30, 2020 23:12

alexec merged commit 7d8818c into argoproj:master May 1, 2020

alexec pushed a commit that referenced this pull request May 1, 2020

fix(controller): Workflow stop and resume by node didn't properly sup…

ad28a9c

…port offloaded nodes. Fixes #2543 (#2548)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(controller): Workflow stop and resume by node didn't properly support offloaded nodes. Fixes #2543 #2548

fix(controller): Workflow stop and resume by node didn't properly support offloaded nodes. Fixes #2543 #2548

markterm commented Mar 30, 2020 •

edited by simster7

markterm commented Mar 30, 2020

codecov bot commented Mar 31, 2020 •

edited

markterm commented Apr 1, 2020

alexec commented Apr 5, 2020

markterm commented Apr 11, 2020

simster7 commented Apr 11, 2020 •

edited

markterm commented Apr 12, 2020 via email

markterm commented Apr 27, 2020

alexec left a comment •

edited

markterm commented Apr 27, 2020

markterm commented Apr 29, 2020

alexec Apr 29, 2020

markterm Apr 29, 2020

alexec Apr 29, 2020

markterm commented Apr 30, 2020

alexec commented Apr 30, 2020

alexec commented Apr 30, 2020

markterm commented Apr 30, 2020

alexec commented Apr 30, 2020

sonarcloud bot commented Apr 30, 2020

markterm commented Apr 30, 2020

alexec commented May 1, 2020

fix(controller): Workflow stop and resume by node didn't properly support offloaded nodes. Fixes #2543 #2548

fix(controller): Workflow stop and resume by node didn't properly support offloaded nodes. Fixes #2543 #2548

Conversation

markterm commented Mar 30, 2020 • edited by simster7

markterm commented Mar 30, 2020

codecov bot commented Mar 31, 2020 • edited

Codecov Report

markterm commented Apr 1, 2020

alexec commented Apr 5, 2020

markterm commented Apr 11, 2020

simster7 commented Apr 11, 2020 • edited

markterm commented Apr 12, 2020 via email

markterm commented Apr 27, 2020

alexec left a comment • edited

Choose a reason for hiding this comment

markterm commented Apr 27, 2020

markterm commented Apr 29, 2020

alexec Apr 29, 2020

Choose a reason for hiding this comment

markterm Apr 29, 2020

Choose a reason for hiding this comment

alexec Apr 29, 2020

Choose a reason for hiding this comment

markterm commented Apr 30, 2020

alexec commented Apr 30, 2020

alexec commented Apr 30, 2020

markterm commented Apr 30, 2020

alexec commented Apr 30, 2020

sonarcloud bot commented Apr 30, 2020

markterm commented Apr 30, 2020

alexec commented May 1, 2020

markterm commented Mar 30, 2020 •

edited by simster7

codecov bot commented Mar 31, 2020 •

edited

simster7 commented Apr 11, 2020 •

edited

alexec left a comment •

edited