Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

diff in Status field causes conflicts during apply stage in newer version of kapp #746

Closed
praveenrewar opened this issue May 5, 2023 · 5 comments · Fixed by #760
Closed
Labels
bug This issue describes a defect or unexpected behavior carvel accepted This issue should be considered for future work and that the triage process has been completed carvel triage This issue has not yet been reviewed for validity priority/important-soon Must be staffed and worked on currently or soon.

Comments

@praveenrewar
Copy link
Member

What steps did you take:
I tried deploying a resource and I noticed that there was some diff present for the status field, when I said yes to the prompt and kapp started applying the changes, it threw a conflict error suggesting that the diff has changed.

What happened:
The status of the resource changed in the background while kapp was trying to apply the changes which caused an update in the diff.

What did you expect:
I expected kapp to ignore the status completely during diffs.

Anything else you would like to add:
In earlier versions of kapp (< v0.55.0), we used to copy the entire status field, but starting v0.55.0, we stopped doing that and instead used diffAgainstLastAppliedFieldExclusionRules for status field, which works fine during smart diff, but shows the diff when non-smart diff is being used.

Relavant slack thread: https://kubernetes.slack.com/archives/CH8KCCKA5/p1682582134618589

Environment:

  • kapp version (use kapp --version): v0.55.0
  • OS (e.g. from /etc/os-release):
  • Kubernetes version (use kubectl version)

Vote on this request

This is an invitation to the community to vote on issues, to help us prioritize our backlog. Use the "smiley face" up to the right of this comment to vote.

👍 "I would like to see this addressed as soon as possible"
👎 "There are other more important things to focus on right now"

We are also happy to receive and review Pull Requests if you want to help working on this issue.

@praveenrewar praveenrewar added bug This issue describes a defect or unexpected behavior carvel triage This issue has not yet been reviewed for validity labels May 5, 2023
@praveenrewar praveenrewar added carvel accepted This issue should be considered for future work and that the triage process has been completed priority/important-soon Must be staffed and worked on currently or soon. and removed carvel triage This issue has not yet been reviewed for validity labels May 17, 2023
@theurichde
Copy link
Contributor

theurichde commented May 25, 2023

Hey there @praveenrewar 👋🏼

We are heavily using kapp in our CI/CD pipelines and are now affected by this behavior. I would like to assist you here or take that over (or at least having a look); if you can point me in the direction where to start?

@praveenrewar
Copy link
Member Author

Hi @theurichde!

Thank you so much for lending a helping hand. Let me try to add a brief summary of the issue and what I have been planning to start with to resolve the issue:

  • kapp has 2 types of diffing, smart diff and non-smart diff. Smart diff basically means that the diffing happens against the last applied resource and the non-smart diff hapens against the resource that exists on the cluster and it is used when the last applied resource is invalid, i.e. something has changed on the resource.
    lastAppliedRes := f.NewResourceWithHistory(existingRes).LastAppliedResource()
    if lastAppliedRes != nil {
    rebasedLastAppliedRes, err := NewRebasedResource(existingResForRebasing, lastAppliedRes, f.rebaseMods).Resource()
    if err != nil {
    return nil, err
    }
    existingRes = rebasedLastAppliedRes
    }
  • To ignore the status field of a resource diffAgainstLastAppliedFieldExclusionRules is used to exclude the status from the last applied resource, but this only works when smart diff is being used, otherwise the diffing happens against the existing resource and not the last applied resource.
  • So now when non smart diff is getting used, the diff would show the complete status being removed because we usually don't include it in our manifests and if something changes in the status during diffing and before applying, then the diff would also change resulting in a conflict.

I have been thinking of resolving this by adding a new configuration similar to diffAgainstLastAppliedFieldExclusionRules, but it should always exclude the paths from existing resource and not just while diffing against the last applied resource.
I didn't get a chance to think about any potential side effects yet but if you are interested in looking into this, then I would love to collaborate and help in any way possible.

Until then, if possible, you can switch to kapp version 0.54.0 or any version before that in your CI/CD pipelines.

@theurichde
Copy link
Contributor

Thank you for your detailed explanation @praveenrewar!

I will try and look into it 👀

@wanisfahmyDE
Copy link

Hi @praveenrewar is there a plan to create a release soon?
we are eagerly waiting for this fix

@github-actions github-actions bot added the carvel triage This issue has not yet been reviewed for validity label Jul 24, 2023
@praveenrewar
Copy link
Member Author

Fix is available in kapp v0.58.0.
cc @wanisfahmyDE @universam1

@praveenrewar praveenrewar removed the carvel triage This issue has not yet been reviewed for validity label Jul 26, 2023
@github-actions github-actions bot added the carvel triage This issue has not yet been reviewed for validity label Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue describes a defect or unexpected behavior carvel accepted This issue should be considered for future work and that the triage process has been completed carvel triage This issue has not yet been reviewed for validity priority/important-soon Must be staffed and worked on currently or soon.
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants