feat: Upgrade Decision engine incorporated into reconciliation logic #270

juliev0 · 2024-09-16T20:34:58Z

Modifications

From PipelineRollout Reconciler, if user has set a preference for PPND, we determine which strategy we need to do based on 1. whether/which fields changed, 2. was there an external request to pause from NumaflowController or ISBService? If so, we put ourselves in the PPND InProgressStrategy and set that value in memory and in the Rollout Status.

From NumaflowControllerRO Reconciler and ISBServiceRO reconciler, if user has set a preference for PPND, we perform the PPND strategy always (later we will make it dependent on which isbsvc fields changed).

Verification

Added unit test for PPND to PipelineRollout, testing many different cases, including whether the InProgressStrategy is set correctly.

Enhanced e2e test to include verification of InProgressStrategy, and also now includes verification of the cases of:

user sets desiredPhase=Paused
user sets desiredPhase=Running when it was previously paused

To Do

perhaps e2e test could be updated to include an actual user config
address any TODOs in the code
Would like to move PPND specific functions into a separate file

Signed-off-by: Julie Vogelman <[email protected]>

…n PipelineRollout Signed-off-by: Julie Vogelman <[email protected]>

Signed-off-by: Julie Vogelman <[email protected]>

juliev0 · 2024-09-19T22:23:02Z

internal/controller/pause.go

+	if err != nil {
+		return err
+	}
+	return nil


by calling UpdateCR() here, we get the latest pipeline with the revised ResourceVersion from K8S, so any subsequent call to do an update within this reconcile call will use the latest ResourceVersion and won't result in a conflict error.

Signed-off-by: Julie Vogelman <[email protected]>

xdevxy · 2024-09-20T20:33:35Z

internal/controller/config/config.go

-	DataLossPrevention bool `json:"dataLossPrevention" mapstructure:"dataLossPrevention"`
+	// If user's config doesn't exist or doesn't specify strategy, this is the default
+	// TODO: should we put this here or in usde config?
+	DefaultUpgradeStrategy USDEUserStrategy `json:"defaultUpgradeStrategy" mapstructure:"defaultUpgradeStrategy"`


yeah, it is better to move to USDE config so that we have all upgrade related config in one place

xdevxy · 2024-09-20T20:56:37Z

tests/e2e/functional_test.go

+		verifyPipelineRolloutDeployed(pipelineRolloutName)
+
+		// Give it a little while to get to Paused and then verify that it stays that way
+		// TODO: add back after Numaflow fixes this to not go from Paused to Pausing


which numaflow issue is tracking the bug?

Sidhant actually fixed it here - I just need for us to incorporate it, which I will do shortly so as to get a few fixes at once

internal/controller/pipelinerollout_controller.go

internal/controller/in_progress_strategy.go

internal/controller/isbservicerollout_controller.go

Signed-off-by: Julie Vogelman <[email protected]>

…decision-engine

Signed-off-by: Julie Vogelman <[email protected]>

xdevxy · 2024-09-23T21:25:21Z

internal/usde/usde.go

+		case config.NoStrategyID:
+			return true, apiv1.UpgradeStrategyApply, nil


This block of code according to the comment at L59 should only return PPND or Progressive?

Good catch that that's a mismatch. I am going to change the comment. Perhaps in the future, those would be the only strategies allowed, but for now "no strategy" is also allowed.

xdevxy · 2024-09-23T21:33:26Z

internal/controller/pipelinerollout_controller.go

-	if err := json.Unmarshal(existingPipelineDef.Spec.Raw, &existingPipelineSpec); err != nil {
-		return fmt.Errorf("failed to convert existing Pipeline spec %q into PipelineSpec type, err=%v", string(existingPipelineDef.Spec.Raw), err)
+	// what is the preferred strategy for this namespace?
+	userPreferredStrategy, err := usde.GetUserStrategy(ctx, newPipelineDef.Namespace)


I feel the logic from L443 to L528 to determine which strategy to be used is too interleaved between code blocks and be further condensed. But I'm good to leave it to a follow up.

can you clarify what you mean by "interleaved between code blocks"?

I specifically wanted to design the overall flow as:

figure out what "In progress strategy" should be and set it if it's not set

Do whatever the "In progress strategy" is

Do you disagree with that approach?

Feel free to refactor if you see a potential improvement

Okay, I thought about this a little more, and I can kind of see what you're saying. I think I can see keep the PPND logic collocated, and keep the Progressive logic collocated.

On second thought, I started to do this and it didn't seem to be better with what I was doing so I decided not to spend time on it. But do feel free to refactor it if you want to. :)

Signed-off-by: Julie Vogelman <[email protected]>

juliev0 added 20 commits September 11, 2024 16:24

remove needsPausing() function

a5e8c04

Signed-off-by: Julie Vogelman <[email protected]>

Merge remote-tracking branch 'origin/main' into decision-engine

fc9bd6f

usde

fae0aed

Signed-off-by: Julie Vogelman <[email protected]>

save state

3f27424

Signed-off-by: Julie Vogelman <[email protected]>

decision engine

49f148d

Signed-off-by: Julie Vogelman <[email protected]>

decision engine

78635dd

Signed-off-by: Julie Vogelman <[email protected]>

decision engine

5c60a19

Signed-off-by: Julie Vogelman <[email protected]>

decision engine

9ee9a02

Signed-off-by: Julie Vogelman <[email protected]>

decision engine

8ba1f03

Signed-off-by: Julie Vogelman <[email protected]>

renaming

529a687

Signed-off-by: Julie Vogelman <[email protected]>

fix inProgressStrategy unsetting to be idempotent

8bb849a

Signed-off-by: Julie Vogelman <[email protected]>

decision engine

d236aef

Signed-off-by: Julie Vogelman <[email protected]>

Merge remote-tracking branch 'origin/main' into decision-engine

7c64207

fix: empty commit

2f245b3

Signed-off-by: Julie Vogelman <[email protected]>

fix: empty commit

f130c1d

Signed-off-by: Julie Vogelman <[email protected]>

refactoring to enable this to work if somebody changes desiredPhase i…

81f1702

…n PipelineRollout Signed-off-by: Julie Vogelman <[email protected]>

decision engine

7aca76d

Signed-off-by: Julie Vogelman <[email protected]>

bump timeout for isbservice

f83fdc5

Signed-off-by: Julie Vogelman <[email protected]>

Merge remote-tracking branch 'origin/main' into decision-engine

d816e37

fix: empty commit

6a2c3d8

Signed-off-by: Julie Vogelman <[email protected]>

juliev0 changed the title ~~Decision engine~~ feat: Upgrade Decision engine incorporated into reconciliation logic Sep 17, 2024

juliev0 added 9 commits September 17, 2024 12:39

decision engine

4f0cff7

Signed-off-by: Julie Vogelman <[email protected]>

decision engine

4fdad36

Signed-off-by: Julie Vogelman <[email protected]>

fix spelling

758611d

Signed-off-by: Julie Vogelman <[email protected]>

usde unit test

753c2cf

Signed-off-by: Julie Vogelman <[email protected]>

fix compilation error

9352548

Signed-off-by: Julie Vogelman <[email protected]>

decision engine

c7a1747

Signed-off-by: Julie Vogelman <[email protected]>

merge main

a417b21

Signed-off-by: Julie Vogelman <[email protected]>

fix unit test

53c1d8b

Signed-off-by: Julie Vogelman <[email protected]>

adding e2e test for manual pausing and unpausing

d14b92b

Signed-off-by: Julie Vogelman <[email protected]>

consolidate desiredphase functionality

06b94ed

Signed-off-by: Julie Vogelman <[email protected]>

juliev0 commented Sep 19, 2024

View reviewed changes

decision engine

4483d05

Signed-off-by: Julie Vogelman <[email protected]>

juliev0 marked this pull request as ready for review September 19, 2024 22:40

juliev0 requested review from xdevxy and afugazzotto as code owners September 19, 2024 22:40

xdevxy reviewed Sep 20, 2024

View reviewed changes

internal/controller/pipelinerollout_controller.go Outdated Show resolved Hide resolved

xdevxy reviewed Sep 20, 2024

View reviewed changes

internal/controller/in_progress_strategy.go Show resolved Hide resolved

xdevxy reviewed Sep 20, 2024

View reviewed changes

internal/controller/isbservicerollout_controller.go Show resolved Hide resolved

juliev0 added 14 commits September 20, 2024 15:44

decision engine - resolve some comments

a458c68

Signed-off-by: Julie Vogelman <[email protected]>

move defaultUpgradeStrategy field from global config to USDE config

fc1866d

Signed-off-by: Julie Vogelman <[email protected]>

explictly set user strategy for unit test

2cb63ae

Signed-off-by: Julie Vogelman <[email protected]>

fix unit test

946964e

Signed-off-by: Julie Vogelman <[email protected]>

Merge branch 'main' into decision-engine

dcf37e5

remove feature flag from numaplane main config

e58d343

Signed-off-by: Julie Vogelman <[email protected]>

Merge branch 'decision-engine' of github.com:numaproj/numaplane into …

46929f2

…decision-engine

remove unneeded file

3cdd599

Signed-off-by: Julie Vogelman <[email protected]>

typo

c01d119

Signed-off-by: Julie Vogelman <[email protected]>

remove defaultUpgradeStrategy from numaplane config

a9eed6a

Signed-off-by: Julie Vogelman <[email protected]>

check inProgressStrategy

1483dd7

Signed-off-by: Julie Vogelman <[email protected]>

fix: empty commit

2dde94a

Signed-off-by: Julie Vogelman <[email protected]>

renaming

fb97193

Signed-off-by: Julie Vogelman <[email protected]>

merge main

eea2ad0

Signed-off-by: Julie Vogelman <[email protected]>

xdevxy reviewed Sep 23, 2024

View reviewed changes

xdevxy approved these changes Sep 23, 2024

View reviewed changes

xdevxy reviewed Sep 23, 2024

View reviewed changes

comment

fe52380

Signed-off-by: Julie Vogelman <[email protected]>

juliev0 merged commit 84b8c74 into main Sep 23, 2024
7 checks passed

juliev0 deleted the decision-engine branch September 23, 2024 22:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Upgrade Decision engine incorporated into reconciliation logic #270

feat: Upgrade Decision engine incorporated into reconciliation logic #270

juliev0 commented Sep 16, 2024 •

edited by xdevxy

Loading

juliev0 Sep 19, 2024

xdevxy Sep 20, 2024

xdevxy Sep 20, 2024

juliev0 Sep 20, 2024 •

edited

Loading

xdevxy Sep 23, 2024

juliev0 Sep 23, 2024

xdevxy Sep 23, 2024

juliev0 Sep 23, 2024

juliev0 Sep 23, 2024

juliev0 Sep 24, 2024

juliev0 Sep 24, 2024

		case config.NoStrategyID:
		return true, apiv1.UpgradeStrategyApply, nil

feat: Upgrade Decision engine incorporated into reconciliation logic #270

feat: Upgrade Decision engine incorporated into reconciliation logic #270

Conversation

juliev0 commented Sep 16, 2024 • edited by xdevxy Loading

Modifications

Verification

To Do

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juliev0 Sep 20, 2024 • edited Loading

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

juliev0 commented Sep 16, 2024 •

edited by xdevxy

Loading

juliev0 Sep 20, 2024 •

edited

Loading