feat(fluentd): add pluginSortOrder field to control plugin @id sort behavior#1927
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes Fluentd plugin rendering order in generated configs when a single CR defines 10+ filter/output plugins by sorting plugin @id suffix indexes numerically (avoiding lexicographic mis-order like -10 before -2). It also adds regression fixtures and unit tests to lock in correct ordering for filters and outputs, plus a guard test around input ordering.
Changes:
- Update
PluginStoreByNameById.Lessto compare@idtrailing numeric segments numerically. - Add new Fluentd ClusterFilter/ClusterInput/ClusterOutput test fixtures that include 11 plugins (0..10).
- Add expected rendered config fixtures + unit tests covering the 10+ plugin ordering regression.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| apis/fluentd/v1alpha1/plugins/params/model.go | Adds numeric-aware @id comparison for plugin child sorting. |
| apis/fluentd/v1alpha1/tests/tools.go | Introduces CR fixtures with 11 filters/inputs/outputs to reproduce the ordering issue. |
| apis/fluentd/v1alpha1/tests/helper_test.go | Adds regression tests validating numeric ordering for filters/outputs and a guard for input order. |
| apis/fluentd/v1alpha1/tests/expected/fluentd-cluster-cfg-output-order-by-index.cfg | New expected config validating output ordering ...-0 through ...-10. |
| apis/fluentd/v1alpha1/tests/expected/fluentd-cluster-cfg-input-order-by-index.cfg | New expected config validating input rendering order in a single CR. |
| apis/fluentd/v1alpha1/tests/expected/fluentd-cluster-cfg-filter-order-by-index.cfg | New expected config validating filter ordering ...-0 through ...-10. |
| if p1 != p2 { | ||
| return p1 < p2 | ||
| } | ||
| return n1 < n2 |
There was a problem hiding this comment.
When ok1 && ok2 and p1 == p2, lessId returns n1 < n2 without a tie-breaker for n1 == n2. If two ids differ only in formatting (e.g., leading zeros like "...-2" vs "...-02"), Less will return false in both directions, letting sort.Sort reorder them non-deterministically. Add a deterministic tie-break (for example, fall back to full id1 < id2 when n1 == n2).
| return n1 < n2 | |
| if n1 != n2 { | |
| return n1 < n2 | |
| } | |
| return id1 < id2 |
There was a problem hiding this comment.
There shouldn't be any issues, as the two IDs have different formats and there should be no case where the numerical values are the same.
458e9af to
f548ad3
Compare
3dae5ee to
416a9bf
Compare
|
Hi @sugaf1204 , thanks for the contributions!. This looks good but it is a breaking change. To make sure smoothly upgrading experience, can you add a gate for this feature? |
416a9bf to
0fdf2f3
Compare
| // WithCfgResources will collect all plugins to generate main config | ||
| cfgResouces.NumericIdSort = cfg.Spec.PluginSortOrder == "index" | ||
| var msg string | ||
| err = gpr.WithCfgResources(cfgRouterLabel, cfgResouces) |
There was a problem hiding this comment.
The new numeric-aware ordering is only enabled when spec.pluginSortOrder is explicitly set to "index". That keeps the current lexicographic behavior (and the 10+ plugin mis-ordering) for existing CRs and for CRs that omit the field. If the intent/PR messaging is that ordering is fixed by default, consider either defaulting pluginSortOrder to "index" (API defaulting) or updating the user-facing notes to make it clear this is an opt-in behavior change.
| pluginSortOrder: | ||
| description: |- | ||
| PluginSortOrder controls how child plugins within a label section are | ||
| ordered by their @id. "lexicographic" (default) preserves the original | ||
| string-comparison behaviour. "index" switches to numeric-aware ordering | ||
| so that a CR with more than nine plugins renders in definition order | ||
| (e.g. plugin-2 before plugin-10). | ||
| enum: | ||
| - lexicographic | ||
| - index | ||
| type: string |
There was a problem hiding this comment.
The CRD schema for pluginSortOrder documents a default of "lexicographic", but there is no default: set in the OpenAPI schema. If the intent is to have a CRD default (as implied by the Go type marker), regenerate manifests so the schema includes default: lexicographic; otherwise adjust the description to avoid claiming a CRD default.
| // string-comparison behaviour. "index" switches to numeric-aware ordering | ||
| // so that a CR with more than nine plugins renders in definition order | ||
| // (e.g. plugin-2 before plugin-10). | ||
| // +kubebuilder:validation:Enum:=lexicographic;index |
There was a problem hiding this comment.
FluentdConfigSpec.PluginSortOrder is documented as having a default of "lexicographic", but unlike ClusterFluentdConfigSpec it has no +kubebuilder:default marker, so the default isn't visible/expressed in the CRD schema. Consider adding +kubebuilder:default:=lexicographic here as well (and regenerate manifests/docs) to keep the API consistent.
| // +kubebuilder:validation:Enum:=lexicographic;index | |
| // +kubebuilder:validation:Enum:=lexicographic;index | |
| // +kubebuilder:default:=lexicographic |
| // PluginSortOrder controls how child plugins within a label section are | ||
| // ordered by their @id. "lexicographic" (default) preserves the original | ||
| // string-comparison behaviour. "index" switches to numeric-aware ordering | ||
| // so that a CR with more than nine plugins renders in definition order | ||
| // (e.g. plugin-2 before plugin-10). | ||
| // +kubebuilder:validation:Enum:=lexicographic;index | ||
| PluginSortOrder string `json:"pluginSortOrder,omitempty"` |
There was a problem hiding this comment.
The PR description/release note states plugin ordering is fixed for CRs with 10+ plugins, but the implementation keeps lexicographic ordering as the default and only enables numeric ordering when pluginSortOrder: index is set. Either update the PR description/release note to reflect the opt-in behavior, or consider making numeric ordering the default if backward compatibility allows.
There was a problem hiding this comment.
updated PR description/release note
77b8c02 to
cf6307b
Compare
cf6307b to
f938113
Compare
|
|
||
| However, for filter plugins, if you want a filter chain, the order of filters matters. You need to organize multiple filters into an array as the demo [logging stack](https://github.com/fluent/fluent-operator/blob/master/manifests/logging-stack/filter-kubernetes.yaml) suggests.For more info on various use cases of Fluent Operator Fluentd CRDs, you can refer to [Fluent-Operator-Walkthrough](https://github.com/kubesphere-sigs/fluent-operator-walkthrough#fluent-bit--fluentd-mode). | ||
|
|
||
| For Fluentd configs that select more than nine child plugins in one label section, set `pluginSortOrder: index` on `FluentdConfig` or `ClusterFluentdConfig` to use numeric-aware plugin ordering. The default `lexicographic` order is kept for backward compatibility. |
There was a problem hiding this comment.
The doc says to set pluginSortOrder: index “on FluentdConfig / ClusterFluentdConfig”, but this field lives under .spec. As written, it’s ambiguous and could be read as a top-level field. Please clarify the path (e.g., .spec.pluginSortOrder: index) so users apply it correctly.
| expectedCfg := string(getExpectedCfg("./expected/fluentd-cluster-cfg-output-order-by-index.cfg")) | ||
| g.Expect(strings.TrimRight(expectedCfg, "\n")).To(Equal(config)) | ||
| } |
There was a problem hiding this comment.
These new tests special-case the expected fixtures by trimming trailing newlines, while the rest of this test file compares fixtures verbatim. To keep expectations consistent (and avoid hiding newline mismatches), consider normalizing this in one place (e.g., update getExpectedCfg to trim the trailing newline, or ensure these new expected/*.cfg fixtures match the existing convention so no per-test trimming is needed).
f938113 to
1baf828
Compare
…ehavior Adds pluginSortOrder field to FluentdConfig and ClusterFluentdConfig. Default is "lexicographic" to preserve existing behavior. Set pluginSortOrder: index to enable numeric-aware @id comparison, which maintains definition order when a CR contains many plugins. Adds regression tests for inputs, filters, and outputs. CRD schema default is set to "lexicographic" in generated manifests. Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: sugaf1204 <sugaf1204@icloud.com>
1baf828 to
b4d7a3f
Compare
|
Hi @cw-Guo , thank you for the feedback. I have updated the implementation to maintain the original behavior by default and added a flag to control the sorting behavior. |
|
Thanks @sugaf1204 for your contributions! |
What this PR does / why we need it:
Adds a
pluginSortOrderfield toFluentdConfigandClusterFluentdConfig.When a CR contains many plugins, lexicographic
@idsorting causes the renderedconfig order to diverge from the CR definition order. Setting
pluginSortOrder: indexenables numeric-aware comparison to maintain definition order.
Default remains
lexicographicfor backward compatibility.Which issue(s) this PR fixes:
Fixes #
Does this PR introduced a user-facing change?
Additional documentation, usage docs, etc.: