fix: fixed logic to run certain operations even when the spec is unchanged #56

afugazzotto · 2024-06-12T22:35:17Z

Modifications

Operations such as pipeline pausing and status update based on pipeline and isbservice status need to be performed even if the specs are unchanged based on hash comparison.

Verification

Unit tests and manually checking via ArgoCD and simulating a pipeline pause.

NOTE: we cannot test by checking that the ChildResourcesHealthy condition is True because it is based on the pipeline status which will never be healthy in the unit tests since there is no numaflow-controller running. This could be checked in e2e tests.

…atus updates even when the spec is unchanged. Signed-off-by: Antonino Fugazzotto <[email protected]>

Signed-off-by: Antonino Fugazzotto <[email protected]>

internal/controller/shared.go

internal/controller/pipelinerollout_controller.go

Signed-off-by: Antonino Fugazzotto <[email protected]>

xdevxy · 2024-06-13T22:00:45Z

internal/controller/pipelinerollout_controller.go

@@ -168,7 +168,7 @@ func (r *PipelineRolloutReconciler) reconcile(
 		}

 		kubernetes.SetAnnotation(pipelineRollout, apiv1.KeyHash, childResouceSpecHash)
-	} else {
+	} else if rolloutChildOp != RolloutChildNoop {


nit: I'd prefer this to be if rolloutChildOp == RolloutChildUpdate to be more clear.

yeah, I was thinking the same. The logic below applies to when we're trying to update.

I actually just realized that we need to remove this. I had it in place in the previous PR but it needs to be removed because part of the logic inside the functions called in this if-statement branch need to execute in case the pipeline needs to be updated or not. The code in this branch needs to be refactored, but I wanted to do that in a separate PR as discussed with @xdevxy

I actually just realized that we need to remove this. I had it in place in the previous PR but it needs to be removed because part of the logic inside the functions called in this if-statement branch need to execute in case the pipeline needs to be updated or not.

You know, I made that comment back when you were updating the annotation immediately whenever there was a need to update. Now that you're not updating that annotation until the update has truly happened, I think it may be okay. Basically, we will revisit this "update" logic up until the update actually occurs.

The code in this branch needs to be refactored

are you referring partly to my other comment regarding the long function name here?

internal/controller/shared.go

juliev0 · 2024-06-13T22:15:49Z

internal/controller/shared.go

 // The operations are defined by the RolloutChildOperation constants.
-func makeChildResourceFromRolloutAndUpdateSpecHash(
+func makeChildResourceFromRolloutAndCalculateSpecHash(


I hate to be anal, but wondering if we can shorten this name? It's pretty long. I can see it's explicit about what it does, which can be nice. But if you have to name it that way, it sort of reveals a violation of the Single Responsibility Principle, that a function should do one thing. I'm not a total stickler for that rule. (I do think a function can do multiple things but still conceptually do one thing in general.)

In any case, can we either shorten the name or break it into multiple functions?

I initially envisioned a different usage for this function. But after realizing that certain logic needs to be executed independently of the fact of having a creation or update, lead me to streamline this function into a simpler function.

Signed-off-by: Antonino Fugazzotto <[email protected]>

juliev0 · 2024-06-13T22:52:55Z

internal/controller/shared.go

 		}
 	}

 	if rolloutChildOp == RolloutChildNoop {
 		annotations := rolloutObj.GetAnnotations()
-		if annotation, exists := annotations[apiv1.KeyHash]; exists && annotation != childResouceSpecHash {
+		if annotation := annotations[apiv1.KeyHash]; annotation != childResouceSpecHash {


ahh, that's a nice neat way to handle annotation not being present

juliev0 · 2024-06-13T23:06:34Z

internal/controller/pipelinerollout_controller.go

-	} else if rolloutChildOp == RolloutChildUpdate {
+
+		kubernetes.SetAnnotation(pipelineRollout, apiv1.KeyHash, childResouceSpecHash)
+	} else {


did you see my comment here? I actually think it does make sense for this logic to be when we're still in the process of trying to update...unless I'm missing something?

Signed-off-by: Antonino Fugazzotto <[email protected]>

afugazzotto · 2024-06-14T16:57:34Z

@juliev0 @xdevxy I made some changes to the spec hashing and annotation setting code based on your feedback to try and resolve all of the concerns. Please, take a look again and let me know what you think. Thanks

juliev0 · 2024-06-15T01:55:08Z

Thank you for the adjustments @afugazzotto!

Unfortunately, and I am so sorry for this - I think I didn’t think this whole annotation thing through before. I am realizing that given that we want to do auto-healing, we want to overwrite the pipeline/ISBservice if somebody hand modifies it (like through kubectl apply). This means that we ultimately want to compare a. The spec we intend to apply (including “pause” designation) to b. the spec currently on the cluster. (We should vet that.) Therefore, there may be no value to storing the hash after all. :(

cc @xdevxy @dpadhiar @a49ibrahim

xdevxy · 2024-06-17T20:29:29Z

Thank you for the adjustments @afugazzotto!

Unfortunately, and I am so sorry for this - I think I didn’t think this whole annotation thing through before. I am realizing that given that we want to do auto-healing, we want to overwrite the pipeline/ISBservice if somebody hand modifies it (like through kubectl apply). This means that we ultimately want to compare a. The spec we intend to apply (including “pause” designation) to b. the spec currently on the cluster. (We should vet that.) Therefore, there may be no value to storing the hash after all. :(

cc @xdevxy @dpadhiar @a49ibrahim

We can discuss this when we implement pipeline draining with concurrent concerns addressed. One way is to ignore the desired state in the spec when comparing hash and have dedicated logic to control the desired lifecycle of pipeline.

juliev0 · 2024-06-17T23:38:39Z

One way is to ignore the desired state in the spec when comparing hash and have dedicated logic to control the desired lifecycle of pipeline.

Can you clarify what you mean by "ignore the desired state in the spec"? You mean use the hash only for the purpose of auto-healing? (to compare the last applied state to what's running on the cluster?)

It seems to me that having the hash at all may be inefficient if we need to ultimately compare the desired state to what's running, but I could be wrong. Please do document the design you have in mind when you can.

xdevxy · 2024-06-18T18:42:35Z

One way is to ignore the desired state in the spec when comparing hash and have dedicated logic to control the desired lifecycle of pipeline.

Can you clarify what you mean by "ignore the desired state in the spec"? You mean use the hash only for the purpose of auto-healing? (to compare the last applied state to what's running on the cluster?)

It seems to me that having the hash at all may be inefficient if we need to ultimately compare the desired state to what's running, but I could be wrong. Please do document the design you have in mind when you can.

What I mean is we probably do no even want desired lifecycle to be included in the pipelinerollout spec but only pipeline spec. It's a good point we need to also consider auto healing logic, for it we can directly compare between Pipeline spec in live cluster vs the desired PipelineRollout spec after the first hash comparison. @afugazzotto is working on that for auto healing task.

afugazzotto · 2024-06-18T21:15:40Z

@juliev0 here is the issue I'm working on #64
It has more details on the idea to have both auto healing and hash of spec.

juliev0 · 2024-06-18T23:17:14Z

#64

thank you. I have responded over there.

…anged (#56) Signed-off-by: Antonino Fugazzotto <[email protected]> Signed-off-by: dsimha <[email protected]>

fix: fixed logic to run certain operations like pipeline pause and st…

792d42b

…atus updates even when the spec is unchanged. Signed-off-by: Antonino Fugazzotto <[email protected]>

afugazzotto changed the title ~~fix: fixed logic to run certain operations like pipeline pause and st…~~ fix: fixed logic to run certain operations even when the spec is unchanged Jun 12, 2024

afugazzotto added 2 commits June 12, 2024 16:19

moved spec hash annotation logic to be set after deployment.

4df8062

Signed-off-by: Antonino Fugazzotto <[email protected]>

moved SetAnnotation

d74c84b

Signed-off-by: Antonino Fugazzotto <[email protected]>

afugazzotto force-pushed the logicFix branch from 8d0b31d to d74c84b Compare June 12, 2024 23:41

afugazzotto marked this pull request as ready for review June 12, 2024 23:41

afugazzotto requested review from xdevxy and juliev0 as code owners June 12, 2024 23:41

afugazzotto mentioned this pull request Jun 12, 2024

Spec hash annotation #52

Merged

rename test

c6d0280

Signed-off-by: Antonino Fugazzotto <[email protected]>

xdevxy reviewed Jun 13, 2024

View reviewed changes

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

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

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

xdevxy mentioned this pull request Jun 13, 2024

feat: Add test for ISBServiceRollout #50

Merged

addressed PR comments

6dde73f

Signed-off-by: Antonino Fugazzotto <[email protected]>

xdevxy approved these changes Jun 13, 2024

View reviewed changes

juliev0 reviewed Jun 13, 2024

View reviewed changes

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

juliev0 reviewed Jun 13, 2024

View reviewed changes

addressing more PR comments

9b9cf75

Signed-off-by: Antonino Fugazzotto <[email protected]>

juliev0 reviewed Jun 13, 2024

View reviewed changes

restructured spec hashing and annotation logic.

cc8fc0d

Signed-off-by: Antonino Fugazzotto <[email protected]>

afugazzotto merged commit 154ac6f into main Jun 17, 2024
4 checks passed

afugazzotto deleted the logicFix branch June 17, 2024 19:30

darshansimha referenced this pull request in darshansimha/numaplane Jun 25, 2024

fix: fixed logic to run certain operations even when the spec is unch…

896cdaa

…anged (#56) Signed-off-by: Antonino Fugazzotto <[email protected]> Signed-off-by: dsimha <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: fixed logic to run certain operations even when the spec is unchanged #56

fix: fixed logic to run certain operations even when the spec is unchanged #56

afugazzotto commented Jun 12, 2024 •

edited

Loading

xdevxy Jun 13, 2024

juliev0 Jun 13, 2024

afugazzotto Jun 13, 2024

juliev0 Jun 13, 2024

juliev0 Jun 13, 2024

juliev0 Jun 13, 2024

juliev0 Jun 13, 2024

afugazzotto Jun 14, 2024

juliev0 Jun 13, 2024

juliev0 Jun 13, 2024

afugazzotto commented Jun 14, 2024

juliev0 commented Jun 15, 2024 •

edited

Loading

xdevxy commented Jun 17, 2024

juliev0 commented Jun 17, 2024

xdevxy commented Jun 18, 2024 •

edited

Loading

afugazzotto commented Jun 18, 2024

juliev0 commented Jun 18, 2024

fix: fixed logic to run certain operations even when the spec is unchanged #56

fix: fixed logic to run certain operations even when the spec is unchanged #56

Conversation

afugazzotto commented Jun 12, 2024 • edited Loading

Modifications

Verification

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afugazzotto commented Jun 14, 2024

juliev0 commented Jun 15, 2024 • edited Loading

xdevxy commented Jun 17, 2024

juliev0 commented Jun 17, 2024

xdevxy commented Jun 18, 2024 • edited Loading

afugazzotto commented Jun 18, 2024

juliev0 commented Jun 18, 2024

afugazzotto commented Jun 12, 2024 •

edited

Loading

juliev0 commented Jun 15, 2024 •

edited

Loading

xdevxy commented Jun 18, 2024 •

edited

Loading