Closed gabegorelick closed 2 years ago
@sethiyash , Please take a look at this.
Sure @renuy.
Possible Approach: We can stop adding kapp.k14s.io/original
annotation and its value in case when resource bytes exceed max length of 262144 and hence it will not record the applied resource changes and we will be unable to do smart diffing of the change made.
@cppforlife, @rohitagg2020 , @sethiyash ,
Would we need to add one more annotation (say, kapp.k14s.io/original-remove-on-error
) so that users can opt-in for the required behaviour? If they don't opt-in we continue to fail on error.
My two cents:
As of today, this is the user flow when he has a resource > 256 KB.
kapp deploy
, kapp deploy
fails. Thus user becomes aware that resource size is > 256 KB. e.g.
kapp: Error: Applying update configmap/config-1 (v1) namespace: default:
Recording last applied resource:
Expected annotation 'kapp.k14s.io/original' value length 300709 to be <= max length 262144 (hint: consider using annotation 'kapp.k14s.io/disable-original')
kapp.k14s.io/disable-original
and now kapp deploy runs successfully.As we already have the kapp.k14s.io/disable-original
annotation, I personally feel we don't need another annotation that will do the same work that kapp.k14s.io/disable-original
is already doing.
Since the only way kapp deploy
finish successfully is if we don't store the original resource as annotation, the question is if kapp
should handle this scenario automatically or user has to opt-in specifically(which means, user knows the consequences)? If we handle it inside kapp, then how does diff
gets impacted for different scenarios. For this @sethiyash is already looking and will share his findings. I think once we have that answer, we can decide further.
Findings on how diffing will be impacted if we don't record applied resource changes in kapp.k14s.io/original
-
Would we need to add one more annotation (say, kapp.k14s.io/original-remove-on-error) so that users can opt-in for the required behaviour? If they don't opt-in we continue to fail on error.
I think it would be much better to add a possible value for the disable-original
annotation (something like onMaxLength), which will enable the annotation only when the size of the ConfigMap goes beyond max length.
@gabegorelick Do you think that would solve the problem that you are facing?
@gabegorelick Firstly, I'm curious to know about the scenario when you face this applied resource limit exceed and how this can benefit you? To summarize we can think of two approaches I want to know your preferred way out of these two -
Approach1: As mentioned by @praveenrewar to add a possible value for the disable-original annotation.
Approach2: Automatically add the disable annotation when the resource limit exceeds.
original
annotation while in Approach1 user will be aware. Please do share your thoughts.
Firstly, I'm curious to know about the scenario when you face this applied resource limit exceed and how this can benefit you?
I had a big CRD. I don't remember the exact flow, but it was something like kapp deploy
failed -> we realized we had to add the annotation -> confirmed with the docs that when deploying big CRD you will have to add said annotation -> annotation added -> subsequent deploy succeeded.
Do you think that would solve the problem that you are facing?
My immediate problem (kapp deploy
not working) is already solved by adding the disable-original
annotation. I filed this issue because IMHO, kapp deploy
should strive to just work :tm: without needing to add extra annotations. Clearly the user wants to deploy this resource, that's why they ran kapp deploy
. Forcing the user to add an extra annotation to inform kapp "seriously, deploy this resource even though it's big" adds friction to the kapp deploy
experience.
Approach1: As mentioned by @praveenrewar to add a possible value for the disable-original annotation
If it defaults to 262144 then I think that makes sense. If it has no default then it doesn't seem like it would solve my issue (I want kapp deploy
to work by default even if I have large resources).
Automatically add the disable annotation when the resource limit exceeds
This is more in line with how I expected kapp to work.
while making it automatic user won't be aware that we will stop recording resource in our original annotation while in Approach1 user will be aware.
It seems like kapp could log a warning if you think this is something that a user needs to be informed about. And then that warning can be silenced with an annotation, perhaps by the user explicitly adding the disable-original
annotation themselves.
@gabegorelick, Thanks for sharing. Your inputs make lots of sense in making kapp
a smooth experience. We will just discuss this internally and keep you posted.
Adding this to the Unprioritized Backlog. @gabegorelick , We will try to get to this soon. However, if you want to submit a pul request for this, we could prioritize the review for this.
Release v0.46.0
Describe the problem/challenge you have
When a resource is exceeds Kubernetes' max annotation length, you need to set
kapp.k14s.io/disable-original
.From https://carvel.dev/kapp/docs/latest/diff/#kappk14siodisable-original:
Describe the solution you'd like
kapp should automatically disable original when saving it would exceed Kubernetes' limit.
Anything else you would like to add:
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.