fix: gitops-engine fixes, plus enable Numaflow Controller deleted and then recreated #471
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… to Unhealthy when the NumaflowController is not healthy (phase failed) and the ChildResourceHealthy is not present in the NumaflowController Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: Antonino Fugazzotto <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
…wner reference Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
…tedPipelineDefinition function Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
juliev0
commented
Dec 29, 2024
| err := r.deleteChildren(ctx, controller.Namespace, controller.Name) | ||
| if err != nil { | ||
| return ctrl.Result{}, err | ||
| } |
There was a problem hiding this comment.
Because the manifests don't have an OwnerReference anymore, the deletion of the children is no longer automatic when NumaflowController is deleted, so we explicitly do it.
juliev0
commented
Dec 29, 2024
| // No need to explicitly requeue since a new reconciliation will occur due to changes in watched child resources | ||
| return ctrl.Result{}, nil | ||
| } | ||
| return ctrl.Result{}, err |
There was a problem hiding this comment.
previous code looked specifically for "version change" to determine if we need to delete/recreate - now do it any time the manifest has a difference, regardless of whether it's due to "version change"
Signed-off-by: Julie Vogelman <[email protected]>
juliev0
commented
Dec 29, 2024
| manifestsWithOwnership, err := applyOwnershipToManifests(manifests, controller) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to apply ownership reference, %w", err) | ||
| } |
There was a problem hiding this comment.
we no longer apply OwnerReference since gitops-engine assumes there's no OwnerReferences
juliev0
commented
Dec 29, 2024
| if err != nil { | ||
| return nil, fmt.Errorf("failed to parse the manifest, %w", err) | ||
| } | ||
| for _, obj := range targetObjs { | ||
| obj.SetNamespace(namespace) | ||
| } |
There was a problem hiding this comment.
In order to compare the live vs target manifests effectively, the target manifests need to have "namespace" applied
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
…o replace with force should be sufficient Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Signed-off-by: Julie Vogelman <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #455
Modifications
This is a follow-up to this previous PR. That PR made it so that if the version number changes, then the previous Numaflow Controller manifests are deleted and then recreated. However, it doesn't take care of the cases in which the version number doesn't change but there's some other change to the manifest of a given version. (In fact, we saw this issue when deploying.)
When the NumaflowController reconciler is invoked either through version change or something else, this PR makes it so that we follow this process:
NumaflowControllerreconciler sees the change and finds the manifests associated with the versionScenario 1: It finds the version:
NumaflowControllerreconciler deletes the previous manifests and requeuesScenario 2: it doesn't find the version:
NumaflowControlleris marked "Failed"gitops-engine code wasn't really working
While working on this PR, I realized that the code we had around the gitops-engine (for applying Numaflow Controller manifests) wasn't truly working the way it was supposed to. The way it's supposed to work is that we first determine if we need to do an "apply" based on whether there's a difference between what we want to apply and what is live, and then only do an "apply" if there's a difference. However, the code was never able to detect what was live, so it was always just doing an "apply".
I realized that the reason it wasn't able to detect what was live is because gitops-engine assumes that manifests don't have "Owner references". I am guessing since it needs to manage the lifecycle of the resources, it doesn't want something else to have applied an OwnerRef to it? In any case, if I remove the owner references that we were applying previously, it is able to detect the live resources and perform the comparison.
Because I removed the Owner reference from the manifests, I had to do the following:
NumaflowControlleris deleted (using Finalizer)Verification
Performed the following:
NumaflowControllerCR was created since that's newNumaflowControllermarked "Failed" andNumaflowControllerRolloutHealthyChildCondition marked False ("degraded"). Numaflow Controller pod still running 1.4.0 and pipeline unpaused.NumaflowControllerwent back to "Deployed" andNumaflowControllerRolloutHealthyChildCondition back to True.Backward incompatibilities