- Notifications
You must be signed in to change notification settings - Fork640
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
fix: only wait for changeset if the result is not empty#4296
Conversation
8bbadf1 to3ecf95bCompare| @@ -76,8 +76,10 @@ func Apply(ctx context.Context, rcg genericclioptions.RESTClientGetter, opts *ru | |||
| changeSet.Append(cs.Entries) | |||
| } | |||
| if err := waitForSet(rcg, opts, changeSet); err != nil { | |||
| return "", err | |||
| if len(changeSet.Entries) > 0 { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure if this shouldn't be rather be made part ofResourceManager#WaitForSet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Hmmm I think if you want to wait you would want to wait forsomething and not wait for whatever even if there is nothing there. This makes more sense here readability wise.
Ultimately I don't care where this check lives. 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If I callResourceManager#WaitForSet with an empty set and it doesn't immediately return (because there's nothing to wait for) this is a bug in that method IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why would you call Wait if there is nothing to wait for? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That's like, I don't expect any visitors, but I open the door just in case. :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
A function that behaves indeterministically if called with certain input is a recipe for disaster. Be liberal in what you accept. But I really don't mind in this specific case so just leaving this here as a general remark.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I agree that we should look into the logic here and see where it hangs:https://github.com/fluxcd/pkg/blob/main/ssa/manager_wait.go But also the check in this PR is good, no need to instantiate a whole resource manager if there is nothing to wait for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I can do both if that's okay with you two?!
This looks Ok to me, but where did you encounter an empty changeset using the CLI? |
@stefanprodan I was monkeying around. :D Also, we re-used a tiny part of the flux code in OCM, like this Apply for a bootstrap command for OCM. ( We gave credit. :). And then I applied some manifests which didn't have type 1 objects in it and it hang. For flux, it would it mean during install or bootstrapping there is a corrupted manifest in Github I guess. |
A corrupted manifest will not pass validation which happens way before you get a changeset. |
Alright, so outside of me monkeying, if you think it's unlikely to occur, I'm fine with ignoring it. :) |
Your PR is Ok, thank you! I was really intrigued if you actually hit this issue with bootstrap, when I wrote that code I was convinced there is no possible way to get an empty changeset as the bootstrap logic would fail way before. |
@stefanprodan Hah, neat. :) WDYT about what Max is saying? I'm okay to move it into the wait instead if you also think it's better suited there. :) |
When the changeset could be empty, then we have this check in place, here is kustomize-controller: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
Thanks@Skarlso
3ecf95b to2b62106Compare@Skarlso any chance you can rebase this quickly to maintain your signature? 😄 |
Signed-off-by: Gergely Brautigam <182850+Skarlso@users.noreply.github.com>
2b62106 toa51ede6Comparedone |
Successfully created backport PR for |
This PR contains the following updates:| Package | Type | Update | Change ||---|---|---|---|| [fluxcd/flux2](https://github.com/fluxcd/flux2) | Kustomization | patch | `v2.1.1` -> `v2.1.2` |---> ⚠ **Warning**>> Some dependencies could not be looked up. Check the warning logs for more information.---### Release Notes<details><summary>fluxcd/flux2 (fluxcd/flux2)</summary>### [`v2.1.2`](https://github.com/fluxcd/flux2/releases/tag/v2.1.2)[Compare Source](fluxcd/flux2@v2.1.1...v2.1.2)#### HighlightsFlux `v2.1.2` is a patch release which comes with various fixes. Users are encouraged to upgrade for the best experience.##### Fixes- Ensures faster recovery of `Kustomization` and `HelmRelease` resources when the source-controller has restarted and is working on restoring the storage.- Prevent source-controller from failing to reconcile `OCIRepositories` when artifacts contain symlinks.- Addresses issue with helm-controller miss-labeling Custom Resource Definitions.- Detect immutable field errors in Google Cloud resources managed by Flux `Kustomizations`.- Better error reporting for `flux bootstrap` when the owner doesn't match the identity associated with the given token.- Allow `flux pull artifact` to fetch OCI artifacts produced by other tools.#### Components changelog- source-controller [v1.1.2](https://github.com/fluxcd/source-controller/blob/v1.1.2/CHANGELOG.md)- kustomize-controller [v1.1.1](https://github.com/fluxcd/kustomize-controller/blob/v1.1.1/CHANGELOG.md)- helm-controller [v0.36.2](https://github.com/fluxcd/helm-controller/blob/v0.36.2/CHANGELOG.md)#### CLI Changelog- PR [#​4324](fluxcd/flux2#4324) - [@​somtochiama](https://github.com/somtochiama) - bootstrap: Fix error msg when the Git token doesn't match the repo owner- PR [#​4323](fluxcd/flux2#4323) - [@​stefanprodan](https://github.com/stefanprodan) - e2e: Update Go dependencies- PR [#​4313](fluxcd/flux2#4313) - [@​fluxcdbot](https://github.com/fluxcdbot) - Update toolkit components- PR [#​4296](fluxcd/flux2#4296) - [@​Skarlso](https://github.com/Skarlso) - fix: only wait for changeset if the result is not empty- PR [#​4285](fluxcd/flux2#4285) - [@​matheuscscp](https://github.com/matheuscscp) - Add badge for SLSA Level 3- PR [#​4284](fluxcd/flux2#4284) - [@​errordeveloper](https://github.com/errordeveloper) - Make `flux pull` work for OCI artifacts produced by other tools</details>---### Configuration📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.🔕 **Ignore**: Close this PR and you won't be reminded about this update again.--- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box---This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4xMy4wIiwidXBkYXRlZEluVmVyIjoiMzcuMTMuMCIsInRhcmdldEJyYW5jaCI6Im1haW4ifQ==-->Reviewed-on:https://git.home/nrdufour/home-ops/pulls/143Co-authored-by: Renovate <renovate@ptinem.io>Co-committed-by: Renovate <renovate@ptinem.io>
When using Apply with a set of files that end up with an empty first change set, wait will timeout waiting for nothing.
Only wait if the changeSet isn't empty.