feat: trim work object status if the object exceeds size limit #357
Conversation
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
|
This PR is added to address concerns in the status back-reporting PR. (See also #327) |
| } | ||
| } | ||
|
|
||
| manifestCond.BackReportedStatus = nil |
There was a problem hiding this comment.
How would the user know that back reporting is not working because of oversized work object status?
I am thinking if it is better to put something in the back reported status to let them know back reported status is nil because of oversized object not because of some misconfiguration on their side or a bug on our side.
There was a problem hiding this comment.
Hi Wei! This PR introduces a new work object condition: StatusTrimmed, which notifies users that back-reported status (in addition to drift/diff details) are not be relayed back. A concern we are facing right now is that the way some of the controllers handle conditions (expecting a specific sequence of conditions) makes it a bit difficult to expose this condition on the CRP\RP API side... But at least we can show it on the work object for now.
There was a problem hiding this comment.
TBH my first instinct was to add a message in the back-reported status data as well 😂 The setback I encountered was that the field is designed to be a runtime.RawExtension, which has to wrap a valid K8s object (otherwise serialization might fail). It is a bit complicated to inject an error message this way -> we can definitely do that, but it either requires us to invent a field (which must be dropped when doing the deserialization, i.e., an implict coupling), or re-use the metadata fields (labels/annotations, they are currently always left empty and added only for structual completeness reasons), which feels just a bit weird.
There was a problem hiding this comment.
It will look something like this:
backReportedStatus:
observedStatus:
apiVersion: apps/v1
kind: Deployment
metadata:
annotations:
- kubernetes-fleet.io/trimmed: true
status:or this:
backReportedStatus:
observedStatus:
apiVersion: apps/v1
kind: Deployment
metadata:
status:
# This must be dropped when doing the de-serialization, as this field is invented and is not
# part of the scheme for Deployments.
trimmed: trueThere was a problem hiding this comment.
I don't really have a preference on this though; if you feel that this looks better (or we should use both the Trimmed condition and this), please let me know.
There was a problem hiding this comment.
Understand it is difficult to do this. And the annotation might be an overkill for now. Also it is not much better than calling out in the status message
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
| WorkConditionTypeDiffReported = "DiffReported" | ||
|
|
||
| // WorkConditionTypeStatusTrimmed reports whether the member agent has to trim | ||
| // the status data in the Work object due to size constraints. |
There was a problem hiding this comment.
maybe we can call out in the doc that back reported status is getting trimmed as well given that there is no "(omitted)" hint on the backReportedStatus field?
There was a problem hiding this comment.
Yeah, I will refresh the docs. (Actually we haven't mentioned anything about back-reported status in the docs yet 😂 -> I'll remember this)
Signed-off-by: michaelawyu <chenyu1@microsoft.com>
2 participants
Description of your changes
This PR sets the work applier to trim the work object status if the object to update has exceeded the size limit.
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer
To control the PR size (and to validate the approach first), integration tests and E2E tests will be submitted in separate PRs.