feat: Enhanced Different TTLSecondsAfterFinished depending on if job is in Succeeded, Failed or Error, Fixes #1883

NikeNano · 2019-12-20T10:46:42Z

Checklist:

I've created an enhancement proposal and discussed it with the community.

Comment: This PR is note done but I have added it in order to get some more specific feedback and discuss the implementation.

claassistantio · 2019-12-20T10:46:50Z

All committers have signed the CLA.

NikeNano · 2019-12-20T14:40:04Z

Hi @sarabala1979 I have now made an initial PR with the workflow spec setup. I would be great if you could give some initial feedback.

Thoughts:

I decided to leave TTLSecondsAfterFinished and added TTLStragety as :

type TTLStrategy struct {
	SecondsAfterSuccess *int32 `json:"SecondsAfterSuccess:,omitempty" protobuf:"bytes,18,opt,name=SecondsAfterSuccess:"`
	SecondsAfterFailed  *int32 `json:"SecondsAfterFailed,omitempty" protobuf:"bytes,18,opt,name=SecondsAfterFailed"`
}

My thought was that now there is now no change to the current design of TTLSecondsAfterFinished which should improve backward compatibility.

I also added so that priority is given for SecondsAfterSuccess and SecondsAfterFailed. I updated with tests as well.

Concerning the configmap for the controller. I have started to look for it, is it correct that what you mean by configmap for the controller is (this)[https://github.com/argoproj/argo/blob/ce78227abe5a3c901e5b7a7dd823fb2551dff584/workflow/config/config.go#L10].

If so how do you think is the base way to pass the information from the configmap object to the ttlcontroller? Atm as I understand the configmap is not passed to the ttlcontroller.

Thanks for the feedback!

simster7

Thanks for your PR @NikeNano! A couple of changes needed.

Also, I would suggest including TTLStrategy.SecondsAfterCompleted to replace TTLSecondsAfterFinished. This way, we can mark TTLSecondsAfterFinished as deprecated. To allow for backwards compatibility, we can automatically convert TTLSecondsAfterFinished to TTLStrategy.SecondsAfterCompleted during Workflow creation and provide a warning that it is being deprecated.

Thoughts on this? @sarabala1979 @alexec

pkg/apis/workflow/v1alpha1/workflow_types.go

workflow/ttlcontroller/ttlcontroller.go

pkg/apis/workflow/v1alpha1/workflow_types.go

workflow/ttlcontroller/ttlcontroller.go

NikeNano · 2019-12-21T14:50:11Z

Thanks for the feedback @simster7 I Will update based upon your suggestions :)

Applied the suggestions from review. Co-Authored-By: Simon Behar <[email protected]>

NikeNano · 2019-12-23T08:02:28Z

FYI closed this by mistake, thus reopened it!

simster7

Hi @NikeNano, thanks for the chagnes. There are two comments from the previous review that are still unresolved

pkg/apis/workflow/v1alpha1/workflow_types.go

NikeNano · 2019-12-27T07:47:01Z

@simster7 I will fix it, havn't had time to look at it during Christmas:) Where in the code base is the workflow creation? I have looked around in the operation file but not really sure where the changes are needed in order to automatically convert TTLSecondsAfterFinished to TTLStrategy.SecondsAfterCompleted during Workflow creation?

Hope you had a great Christamas!

…y is added

…ttlSecondsFinsihed are set

NikeNano · 2020-01-02T16:11:00Z

@simster7 I have now updated and the tests pass locally. However I have some issues understanding how to read the tests from the github action. The "default" gets me to a argo UI where I cant load the results. The Build error gives "No configuration was found in your project. "

alexec · 2020-01-02T16:35:53Z

You should be able to fix build we’re by syncing with master.

alexec · 2020-01-02T16:36:44Z

The other error maybe a test failure. Run “make test” to check.

NikeNano · 2020-01-02T16:49:20Z

The other error maybe a test failure. Run “make test” to check.

Thanks! I assume you mean locally :) Running it and checking

simster7

Last change, otherwise LGTM!

pkg/apis/workflow/v1alpha1/workflow_types.go

NikeNano · 2020-01-04T20:17:06Z

Last change, otherwise LGTM!

I updated with the tag

simster7

Almost there! Some new changes are needed based on your last few commits.

Also you will need to update examples/gc-ttl.yaml to reflect these changes. Please make a note there of all the possible options in TTLStrategy

simster7 · 2020-01-06T18:41:29Z

pkg/apis/workflow/v1alpha1/workflow_types.go

 // Optional duration in seconds relative to the workflow start time which the workflow is
 // allowed to run before the controller terminates the workflow. A value of zero is used to
 // terminate a Running workflow
- ActiveDeadlineSeconds *int64 `json:"activeDeadlineSeconds,omitempty" protobuf:"bytes,19,opt,name=activeDeadlineSeconds"`
+ ActiveDeadlineSeconds *int64 `json:"activeDeadlineSeconds,omitempty" protobuf:"bytes,20,opt,name=activeDeadlineSeconds"`


The protobuf index numbers are not supposed to change when being updated.

Updating A Message Type
If an existing message type no longer meets all your needs – for example, you'd like the message format to have an extra field – but you'd still like to use code created with the old format, don't worry! It's very simple to update message types without breaking any of your existing code. Just remember the following rules:

Don't change the field numbers for any existing fields.

Source

Please revert all of the field number changes to what they were and just change the one for ttlStrategy to the current max + 1 (I think it should be 30). It's okay if the definition is out of order.

Ahh ok, will fix that!

I updated now, I might be wrong but I think is should be 28, based on that this is that last entry in

type WorkflowSpec struct {

https://github.com/argoproj/argo/blob/94449876f4a571ab279802d5ca4d5e938ca3d44d/pkg/apis/workflow/v1alpha1/workflow_types.go#L237 I am new to go and protobuff so not sure though :) Also thanks for all the feedback!

pkg/apis/workflow/v1alpha1/workflow_types.go

NikeNano

Removed trailing ":" which will break the unmarshalling, updated after feeback.

NikeNano

Updated to remove trailing ":" after feedback. Will break the unmarshalling

Co-Authored-By: Simon Behar <[email protected]>

codecov · 2020-01-07T15:06:45Z

Codecov Report

❗ No coverage uploaded for pull request base (master@2b9f50e). Click here to learn what that means.
The diff coverage is 13.57%.

@@            Coverage Diff            @@
##             master    #1883   +/-   ##
=========================================
  Coverage          ?   11.12%           
=========================================
  Files             ?       35           
  Lines             ?    23793           
  Branches          ?        0           
=========================================
  Hits              ?     2647           
  Misses            ?    20809           
  Partials          ?      337

Impacted Files	Coverage Δ
pkg/apis/workflow/v1alpha1/openapi_generated.go	`0% <0%> (ø)`
workflow/controller/config_controller.go	`12.26% <0%> (ø)`
...kg/apis/workflow/v1alpha1/zz_generated.deepcopy.go	`0% <0%> (ø)`
pkg/apis/workflow/v1alpha1/generated.pb.go	`0.45% <0.91%> (ø)`
workflow/config/config.go	`100% <100%> (ø)`
workflow/controller/dag.go	`50.8% <100%> (ø)`
workflow/common/placeholder.go	`100% <100%> (ø)`
pkg/apis/workflow/v1alpha1/workflow_types.go	`2.42% <100%> (ø)`
workflow/controller/workflowpod.go	`72.67% <100%> (ø)`
workflow/controller/operator.go	`56.4% <51.61%> (ø)`
... and 1 more

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 2b9f50e...e78dc26. Read the comment docs.

simster7

Thanks for all your work @NikeNano!

NikeNano · 2020-01-08T08:07:14Z

Thanks for all your work @NikeNano!

Awesome! Thanks for all the help as well @simster7!

NikeNano and others added 3 commits December 16, 2019 15:00

Set different ttl depending on failed or succeded

8494479

initial mash of work

4f35c92

First mash of work on ttlstrategy

79bacf0

NikeNano changed the title ~~Different TTLSecondsAfterFinished depending on if job is in Succeeded, Failed or Error, Fixes #1827~~ feat: Enhanced Different TTLSecondsAfterFinished depending on if job is in Succeeded, Failed or Error, Fixes Dec 20, 2019

NikeNano changed the title ~~[feat: Enhanced Different TTLSecondsAfterFinished depending on if job is in Succeeded, Failed or Error, Fixes ](https://github.com/argoproj/argo/issues/1827)~~ [conventional](https://github.com/argoproj/argo/issues/1827) Dec 20, 2019

NikeNano changed the title ~~[conventional](https://github.com/argoproj/argo/issues/1827)~~ "fix(controller): Updates such and such. Fixes #1827" Dec 20, 2019

NikeNano changed the title ~~"fix(controller): Updates such and such. Fixes #1827"~~ fix(controller): Updates such and such. Fixes #1827 Dec 20, 2019

NikeNano changed the title ~~fix(controller): Updates such and such. Fixes #1827~~ feat: Enhanced Different TTLSecondsAfterFinished depending on if job is in Succeeded, Failed or Error, Fixes Dec 20, 2019

NikeNano added 2 commits December 20, 2019 14:59

Updated the workflow

2f9f616

clean up

264c4bc

simster7 suggested changes Dec 20, 2019

View reviewed changes

alexec assigned simster7 Dec 20, 2019

Apply suggestions from code review

7be2d79

Applied the suggestions from review. Co-Authored-By: Simon Behar <[email protected]>

NikeNano closed this Dec 23, 2019

NikeNano added 2 commits December 23, 2019 08:25

comments that should have been deleted

e7b58cf

Merge branch 'master' of github.com:NikeNano/argo

2acfcb4

NikeNano reopened this Dec 23, 2019

simster7 suggested changes Dec 26, 2019

View reviewed changes

pkg/apis/workflow/v1alpha1/workflow_types.go Outdated Show resolved Hide resolved

pkg/apis/workflow/v1alpha1/workflow_types.go Show resolved Hide resolved

NikeNano added 3 commits December 28, 2019 17:06

updated to use pointers instead for ttlstrategy

1819279

removed things that was added by misstake and handled when ttlstrateg…

33120ff

…y is added

Updated test and now handle cases where both ttlstratetyComplete and …

2b9f50e

…ttlSecondsFinsihed are set

Merge branch 'master' of https://github.com/argoproj/argo

df60bde

simster7 suggested changes Jan 2, 2020

View reviewed changes

pkg/apis/workflow/v1alpha1/workflow_types.go Show resolved Hide resolved

NikeNano added 4 commits January 2, 2020 21:21

updated codegen

00af5a4

changed uppercase to lowercase on string

fc92e65

updated to lower case, to follow standard

ca55ede

added the Deprecated tag

fb76e4e

simster7 suggested changes Jan 6, 2020

View reviewed changes

NikeNano commented Jan 7, 2020

View reviewed changes

reverted the protobuff nbr after feedback

50df4fd

NikeNano commented Jan 7, 2020

View reviewed changes

NikeNano and others added 5 commits January 7, 2020 09:01

Update pkg/apis/workflow/v1alpha1/workflow_types.go

7b21974

Co-Authored-By: Simon Behar <[email protected]>

Update pkg/apis/workflow/v1alpha1/workflow_types.go

de463d9

Co-Authored-By: Simon Behar <[email protected]>

Update pkg/apis/workflow/v1alpha1/workflow_types.go

9f8821e

Co-Authored-By: Simon Behar <[email protected]>

updated in accordance with the code updates

e98aa2d

Codegen and minor edit

e78dc26

simster7 approved these changes Jan 7, 2020

View reviewed changes

simster7 merged commit de3ffd7 into argoproj:master Jan 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Enhanced Different TTLSecondsAfterFinished depending on if job is in Succeeded, Failed or Error, Fixes #1883

feat: Enhanced Different TTLSecondsAfterFinished depending on if job is in Succeeded, Failed or Error, Fixes #1883

NikeNano commented Dec 20, 2019

claassistantio commented Dec 20, 2019 •

edited

Loading

NikeNano commented Dec 20, 2019

simster7 left a comment

NikeNano commented Dec 21, 2019

NikeNano commented Dec 23, 2019 •

edited

Loading

simster7 left a comment

NikeNano commented Dec 27, 2019

NikeNano commented Jan 2, 2020

alexec commented Jan 2, 2020

alexec commented Jan 2, 2020

NikeNano commented Jan 2, 2020

simster7 left a comment

NikeNano commented Jan 4, 2020

simster7 left a comment •

edited

Loading

simster7 Jan 6, 2020

NikeNano Jan 7, 2020

NikeNano Jan 7, 2020

NikeNano left a comment

NikeNano left a comment

codecov bot commented Jan 7, 2020 •

edited

Loading

simster7 left a comment

NikeNano commented Jan 8, 2020

feat: Enhanced Different TTLSecondsAfterFinished depending on if job is in Succeeded, Failed or Error, Fixes #1883

feat: Enhanced Different TTLSecondsAfterFinished depending on if job is in Succeeded, Failed or Error, Fixes #1883

Conversation

NikeNano commented Dec 20, 2019

claassistantio commented Dec 20, 2019 • edited Loading

NikeNano commented Dec 20, 2019

simster7 left a comment

Choose a reason for hiding this comment

NikeNano commented Dec 21, 2019

NikeNano commented Dec 23, 2019 • edited Loading

simster7 left a comment

Choose a reason for hiding this comment

NikeNano commented Dec 27, 2019

NikeNano commented Jan 2, 2020

alexec commented Jan 2, 2020

alexec commented Jan 2, 2020

NikeNano commented Jan 2, 2020

simster7 left a comment

Choose a reason for hiding this comment

NikeNano commented Jan 4, 2020

simster7 left a comment • edited Loading

Choose a reason for hiding this comment

simster7 Jan 6, 2020

Choose a reason for hiding this comment

NikeNano Jan 7, 2020

Choose a reason for hiding this comment

NikeNano Jan 7, 2020

Choose a reason for hiding this comment

NikeNano left a comment

Choose a reason for hiding this comment

NikeNano left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 7, 2020 • edited Loading

Codecov Report

simster7 left a comment

Choose a reason for hiding this comment

NikeNano commented Jan 8, 2020

claassistantio commented Dec 20, 2019 •

edited

Loading

NikeNano commented Dec 23, 2019 •

edited

Loading

simster7 left a comment •

edited

Loading

codecov bot commented Jan 7, 2020 •

edited

Loading