[FlowAggregator] Fix live config updates for IPFIXExporter #6385
Conversation
In particular, updates to recordContents.podLabels and recordFormat were ignored, without any kind of warning. Signed-off-by: Antonin Bas <antonin.bas@broadcom.com>
| if address == e.externalFlowCollectorAddr && protocol == e.externalFlowCollectorProto { | ||
| func (e *IPFIXExporter) UpdateOptions(opt *options.Options) { | ||
| config := opt.Config.FlowCollector | ||
| if reflect.DeepEqual(config, e.config) && opt.Config.RecordContents.PodLabels == e.includePodLabels { |
There was a problem hiding this comment.
It seems unsupported changes could return false here and leads to "New IPFIXExporter configuration" log even nothing changes, for example, by updating enable. Could it be confusing?
According to the description "updates to recordContents.podLabels and recordFormat were ignored, without any kind of warning.", should it warn users that it's unsupported?
There was a problem hiding this comment.
If there is an update to enable, this code will never be executed:
antrea/pkg/flowaggregator/flowaggregator.go
Lines 618 to 634 in 264372d
All other fields in
opt.Config.FlowCollector are now handled correctly with this PR, which is why I used reflect.DeepEqual on the struct. I took a similar approach for the "LogExporter": antrea/pkg/flowaggregator/exporter/log.go
Lines 150 to 161 in 264372d
One could argue that this code is a bit clumsy. It may not have been a great idea to support live config updates for the Flow Aggregator in the first place...
According to the description "updates to recordContents.podLabels and recordFormat were ignored, without any kind of warning.", should it warn users that it's unsupported?
With this PR, these updates should now be supported.
There was a problem hiding this comment.
Got it, thanks for the explanation.
|
/test-all |
In particular, updates to recordContents.podLabels and recordFormat were ignored, without any kind of warning.