[core-Feature] Set deadlines to zero #493

Ln11211 · 2022-10-11T06:25:35Z

TL;DR

Set the default values of node-execution-deadline, node-active-deadline, and workflow-active-deadline in flytepropeller to 0 as mentioned in issue #2950

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

Changed the previous values of 48h and 72h and made them 0h.

Tracking Issue

fixes flyteorg/flyte#2950

Follow-up issue

NA
OR
https://github.com/flyteorg/flyte/issues/

Ln11211 · 2022-10-11T13:48:50Z

Hello @hamersaw,
I have set the default time of the deadline nodes and workflows to 0 as requested in the issue. I couldn't test them though. Can you please review this PR? thank you :-)

codecov · 2022-10-12T13:17:27Z

Codecov Report

Merging #493 (3cb46f3) into master (7ffb502) will not change coverage.
The diff coverage is n/a.

❗ Current head 3cb46f3 differs from pull request most recent head d8259ea. Consider uploading reports for the commit d8259ea to get more accurate results

hamersaw · 2022-10-13T15:32:11Z

Hello @hamersaw, I have set the default time of the deadline nodes and workflows to 0 as requested in the issue. I couldn't test them though. Can you please review this PR? thank you :-)

@Ln11211 Thanks so much for the PR! Is there anyway you can test this change? Can you elaborate on what is blocking?

Ln11211 · 2022-10-13T16:58:45Z

Hello @hamersaw, I have set the default time of the deadline nodes and workflows to 0 as requested in the issue. I couldn't test them though. Can you please review this PR? thank you :-)

@Ln11211 Thanks so much for the PR! Is there anyway you can test this change? Can you elaborate on what is blocking?

I noticed there is a test file config_flags_test that tests these deadlines, (check from L664 in config_flags_test.go). The problem is I never program in Go and I don't have a IDE that can run the tests so I am manually ascertaining the definition and tests right now :)

Ln11211 · 2022-10-14T10:00:56Z

Hey there,
I managed to get VScode but I don't know how to test the deadlines, any help please?

Ln11211 · 2022-10-16T12:16:31Z

hello @hamersaw , is there anything more that needs to be done? btw how do I begin testing the change?

hamersaw · 2022-10-17T23:40:27Z

Hey @Ln11211, thanks so much for working on this. To address a few of your questions.

I noticed there is a test file config_flags_test that tests these deadlines, (check from L664 in config_flags_test.go). The problem is I never program in Go and I don't have a IDE that can run the tests so I am manually ascertaining the definition and tests right now :)

The config_flags_test.go file is an auto-generated file based on the config_flags.go file. You should not need to do anything here for unit testing this change as it is just a default configuration update.

btw how do I begin testing the change?

So right now the best way to test backend changes locally is to:
(1) Use the setup_local_dev.sh script to setup a local development environment. This basically installs k3d, helm, and kubectl locally. Then uses this to start a k8s cluster locally and install the dependencies necessary for Flyte (ie. postgres and minio in the k8s cluster).
(2) The flyte repo can compile a single binary with all of the Flyte components. This is the easiest way to test changes. You can use go replace github.com/flyteorg/flytepropeller => <path to local flytepropeller> to compile a single binary with your local changes to the flytepropeller repository. This can then be run with something like: ./flyte start --config ./flyte_local.yamlThe Flyte UI will be available onlocalhost:30080/console` and can be used to execute workflows using the local k3d cluster.

If you run into issues we can take this offline on slack - would love to help you setup a local dev environment!

hamersaw · 2022-10-17T23:41:59Z

pkg/controller/config/config.go

 			DefaultDeadlines: DefaultDeadlines{
-				DefaultNodeExecutionDeadline:  config.Duration{Duration: time.Hour * 48},
-				DefaultNodeActiveDeadline:     config.Duration{Duration: time.Hour * 48},
-				DefaultWorkflowActiveDeadline: config.Duration{Duration: time.Hour * 72},
+				DefaultNodeExecutionDeadline:  config.Duration{Duration: time.Hour * 0},
+				DefaultNodeActiveDeadline:     config.Duration{Duration: time.Hour * 0},
+				DefaultWorkflowActiveDeadline: config.Duration{Duration: time.Hour * 0},
 			},


I just did some minor testing locally - if you remove the DefaultDeadlines default configuration entirely then these values will be automatically set to '0'. I think that makes more sense than explicitly setting these values.

(2) The flyte repo can compile a single binary with all of the Flyte components. This is the easiest way to test changes. You can use go replace github.com/flyteorg/flytepropeller => <path to local flytepropeller> to compile a single binary with your local changes to the flytepropeller repository. This can then be run with something like: ./flyte start --config ./flyte_local.yamlThe Flyte UI will be available onlocalhost:30080/console` and can be used to execute workflows using the local k3d cluster.

I'll try this today, this seems easier

Set the default values to 0 Signed-off-by: LN <[email protected]> Signed-off-by: Ln11211 <[email protected]>

* setting MetricsBindAddress to 0 to disable controller-runtime manager metrics server Signed-off-by: Daniel Rammer <[email protected]> * and now in the webhook Signed-off-by: Daniel Rammer <[email protected]> Signed-off-by: Daniel Rammer <[email protected]> Signed-off-by: Ln11211 <[email protected]>

Before: A hardcoded string was used for setting the secret namespace After: The value for the secret namespace for settings is grabbed dynamically. Signed-off-by: Francisco J. Solis <[email protected]> Signed-off-by: Francisco J. Solis <[email protected]> Co-authored-by: Dan Rammer <[email protected]> Signed-off-by: Ln11211 <[email protected]>

Removed DefaultDeadlines Signed-off-by: Ln11211 <[email protected]>

hamersaw · 2022-10-24T15:46:31Z

@Ln11211 thanks so much for your patience on this!

* Update config.go Set the default values to 0 Signed-off-by: LN <[email protected]> Signed-off-by: Ln11211 <[email protected]> * disable k8s controller-runtime manager metrics server (flyteorg#492) * setting MetricsBindAddress to 0 to disable controller-runtime manager metrics server Signed-off-by: Daniel Rammer <[email protected]> * and now in the webhook Signed-off-by: Daniel Rammer <[email protected]> Signed-off-by: Daniel Rammer <[email protected]> Signed-off-by: Ln11211 <[email protected]> * fix: Add servicename in certs (flyteorg#491) Before: A hardcoded string was used for setting the secret namespace After: The value for the secret namespace for settings is grabbed dynamically. Signed-off-by: Francisco J. Solis <[email protected]> Signed-off-by: Francisco J. Solis <[email protected]> Co-authored-by: Dan Rammer <[email protected]> Signed-off-by: Ln11211 <[email protected]> * Update config.go Removed DefaultDeadlines Signed-off-by: Ln11211 <[email protected]> Signed-off-by: LN <[email protected]> Signed-off-by: Ln11211 <[email protected]> Signed-off-by: Daniel Rammer <[email protected]> Signed-off-by: Francisco J. Solis <[email protected]> Co-authored-by: Dan Rammer <[email protected]> Co-authored-by: Francisco J. Solis <[email protected]>

Ln11211 requested review from kumare3, EngHabu and hamersaw as code owners October 11, 2022 06:25

Ln11211 mentioned this pull request Oct 11, 2022

[Core feature] Set FlytePropeller node and workflow deadlines to infinite flyteorg/flyte#2950

Closed

2 tasks

Ln11211 force-pushed the deadlines_enhancement branch from 6d78282 to d43df61 Compare October 11, 2022 13:43

hamersaw reviewed Oct 17, 2022

View reviewed changes

Ln11211 and others added 4 commits October 18, 2022 07:03

Update config.go

86fb125

Set the default values to 0 Signed-off-by: LN <[email protected]> Signed-off-by: Ln11211 <[email protected]>

Update config.go

d757cd7

Removed DefaultDeadlines Signed-off-by: Ln11211 <[email protected]>

Ln11211 force-pushed the deadlines_enhancement branch from f8a5df8 to d757cd7 Compare October 18, 2022 01:33

Merge branch 'flyteorg:master' into deadlines_enhancement

d8259ea

hamersaw approved these changes Oct 24, 2022

View reviewed changes

hamersaw merged commit b1b1ae7 into flyteorg:master Oct 24, 2022

samhita-alla added the hacktoberfest-accepted label Oct 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core-Feature] Set deadlines to zero #493

[core-Feature] Set deadlines to zero #493

Ln11211 commented Oct 11, 2022 •

edited

Loading

Ln11211 commented Oct 11, 2022

codecov bot commented Oct 12, 2022 •

edited

Loading

hamersaw commented Oct 13, 2022 •

edited

Loading

Ln11211 commented Oct 13, 2022

Ln11211 commented Oct 14, 2022

Ln11211 commented Oct 16, 2022

hamersaw commented Oct 17, 2022 •

edited

Loading

hamersaw Oct 17, 2022 •

edited

Loading

Ln11211 Oct 18, 2022

hamersaw commented Oct 24, 2022

[core-Feature] Set deadlines to zero #493

[core-Feature] Set deadlines to zero #493

Conversation

Ln11211 commented Oct 11, 2022 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

Ln11211 commented Oct 11, 2022

codecov bot commented Oct 12, 2022 • edited Loading

Codecov Report

hamersaw commented Oct 13, 2022 • edited Loading

Ln11211 commented Oct 13, 2022

Ln11211 commented Oct 14, 2022

Ln11211 commented Oct 16, 2022

hamersaw commented Oct 17, 2022 • edited Loading

hamersaw Oct 17, 2022 • edited Loading

Choose a reason for hiding this comment

Ln11211 Oct 18, 2022

Choose a reason for hiding this comment

hamersaw commented Oct 24, 2022

Ln11211 commented Oct 11, 2022 •

edited

Loading

codecov bot commented Oct 12, 2022 •

edited

Loading

hamersaw commented Oct 13, 2022 •

edited

Loading

hamersaw commented Oct 17, 2022 •

edited

Loading

hamersaw Oct 17, 2022 •

edited

Loading