Update status controller to use a subfield and structured status messages #17564
Conversation
googlebot
added
the
cla: yes
istio-testing
added
the
size/L
| // Get the map of status objects. If it doesn't already exist, create it. | ||
| statusObj, ok := u.Object["status"] | ||
| if !ok { | ||
| statusObj = make(map[string]interface{}) |
There was a problem hiding this comment.
It looks like you can avoid this allocation in certain cases (e.g. desiredStatus == nil). Which may save from allocating a large number of small objects.
There was a problem hiding this comment.
We'd only allocate if the status field was entirely blank already, which should normally only trigger when this runs for the first time for a resource. So I don't think it will be expensive.
There was a problem hiding this comment.
It means you'd be allocating a map for each resource that has a new message on it. It can add up in a large cluster. It looks like it would be cheap to avoid the allocation by slightly restructuring the code here.
| lines.WriteString("\n") | ||
| // For the purposes of status update, the origin field is redundant | ||
| // since we're attaching the message to the origin resource. | ||
| result = append(result, m.Unstructured(false)) |
There was a problem hiding this comment.
Do you ever need the origin field in Unstructured? Is it for the istioctl JSON output case?
There was a problem hiding this comment.
That's what I had in mind, yes.
I also considered something like m.Unstructured().WithoutOrigin() to avoid the awkward boolean flag, but thought it probably wasn't worth it.
|
/test gencheck_istio |
|
/test racetest_istio |
1 similar comment
|
/test racetest_istio |
4 participants
Before:
After:
Solves #17454
[X] Configuration Infrastructure
[ ] Docs
[ ] Installation
[ ] Networking
[ ] Performance and Scalability
[ ] Policies and Telemetry
[ ] Security
[ ] Test and Release
[ ] User Experience
[ ] Developer Infrastructure