feat: make terminationGracePeriodSeconds configurable via field of CollectorSpec

[vparfonov]

Description

Problem

The collector pods have terminationGracePeriodSeconds hardcoded to 10 seconds. When deliveryMode: AtLeastOnce is configured, Vector uses disk buffers to persist log data. During shutdown, Vector needs time to flush in-memory events to disk and close connections to remote sinks. If the grace period is too short, Kubernetes sends SIGKILL while a write is in progress, corrupting the disk buffer and causing CrashLoopBackOff with InvalidProtobufPayload errors on restart.

Solution

Expose terminationGracePeriodSeconds as a configurable field in spec.collector, allowing users to set the appropriate grace period for their environment.

• New optional field: spec.collector.terminationGracePeriodSeconds
• Default remains 10 seconds (backward compatible — no change for existing users)

Example

apiVersion: observability.openshift.io/v1
 kind: ClusterLogForwarder
 metadata:
   name: my-forwarder
   namespace: openshift-logging
 spec:
   collector:
     terminationGracePeriodSeconds: 60
   ...

/cc @Clee2691
/assign @jcantrill

Links

• Depending on PR(s):
• GitHub issue:
• JIRA: https://redhat.atlassian.net/browse/LOG-9636
• Enhancement proposal:

Summary by CodeRabbit

• New Features

  • Added a configurable termination grace period for collector pods, with a default of 10 seconds when not set.
  • Updated product documentation in the UI/manifest to describe the new collector setting and clarify several logging/template configuration examples.

• Bug Fixes

  • Collector pod settings now respect user-provided termination grace period values instead of always using the default.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 92eeb37f-1b30-4cd1-8e1a-46cd8b6362c2

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

Walkthrough

This PR adds an optional TerminationGracePeriodSeconds field to CollectorSpec, propagates it into pod spec generation (defaulting to 10 seconds when unset), updates generated CRD/bundle/OLM manifests to expose the new field, and includes accompanying test coverage. Additional unrelated documentation reflow updates were made across CSV manifest descriptor text.

Changes

Termination grace period feature

Layer / File(s)	Summary
API type and CRD schema api/observability/v1/clusterlogforwarder_types.go, config/crd/bases/observability.openshift.io_clusterlogforwarders.yaml, bundle/manifests/observability.openshift.io_clusterlogforwarders.yaml, bundle/manifests/cluster-logging.clusterserviceversion.yaml, config/manifests/bases/cluster-logging.clusterserviceversion.yaml	Adds TerminationGracePeriodSeconds *int64 to CollectorSpec with validation (minimum 1, nullable) and exposes it in CRD schemas and CSV spec descriptors.
PodSpec wiring and tests internal/collector/collector.go, internal/collector/collector_test.go	NewPodSpec sets TerminationGracePeriodSeconds from CollectorSpec or defaults to 10; new Daemonset and Deployment tests verify default and override behavior.
CSV documentation reflow bundle/manifests/cluster-logging.clusterserviceversion.yaml, config/manifests/bases/cluster-logging.clusterserviceversion.yaml	Reformats and expands descriptor text for prune filters, inputs, TLS options, and multiple output types (CloudWatch, Elasticsearch, GoogleCloudLogging, Kafka, Loki, LokiStack, OTLP, Splunk, Syslog) plus pipeline documentation, and bumps the CSV createdAt timestamp.

Estimated code review effort: 2 (Simple) | ~15 minutes

Sequence Diagram(s)

Not applicable — the changes are a field addition with default-value logic and documentation updates, lacking a multi-component interaction flow warranting a sequence diagram.

🚥 Pre-merge checks | ✅ 15

✅ Passed checks (15 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically summarizes the main change: making CollectorSpec terminationGracePeriodSeconds configurable.
Description check	✅ Passed	The description includes the required sections, explains the problem and solution, and provides the mandatory /cc and /assign commands.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names	✅ Passed	New Ginkgo titles are static and deterministic; they contain no generated IDs, timestamps, namespaces, or other runtime values.
Test Structure And Quality	✅ Passed	New collector podSpec tests are unit-level, isolated with BeforeEach, no cluster ops/Eventually, and follow existing bare-Expect style; no cleanup needed.
Microshift Test Compatibility	✅ Passed	PASS: The only added Ginkgo tests are unit tests in internal/collector/collector_test.go; they only verify podSpec terminationGracePeriodSeconds and use no MicroShift-unsupported APIs.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	Added Ginkgo unit tests only; they verify terminationGracePeriodSeconds default/override and contain no multi-node or SNO assumptions.
Topology-Aware Scheduling Compatibility	✅ Passed	The PR only adds configurable terminationGracePeriodSeconds plus docs/tests; it introduces no anti-affinity, topology spread, nodeSelector, replica, or topology-aware scheduling changes.
Ote Binary Stdout Contract	✅ Passed	Changed files only add a configurable grace period and tests; no main/init/TestMain/RunSpecs setup or stdout-printing calls were introduced.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	No new e2e tests were added; the only new Ginkgo assertions are unit tests for pod spec defaults and they don't use IPv4 or external connectivity.
No-Weak-Crypto	✅ Passed	PR diff adds only grace-period plumbing; exact searches found no added MD5/SHA1/DES/RC4/3DES/Blowfish/ECB or constant-time comparison code.
Container-Privileges	✅ Passed	The commit only adds terminationGracePeriodSeconds/docs; no added lines introduce privileged:true, hostPID/Network/IPC, SYS_ADMIN, root, or allowPrivilegeEscalation:true.
No-Sensitive-Data-In-Logs	✅ Passed	No new log statements or sensitive-data-bearing log interpolation were added in the changed hunks; the PR only adds a configurable grace period field and tests.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[vparfonov]

/hold

[vparfonov]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

🧹 Nitpick comments (2)

internal/collector/collector.go (1)

131-142: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Correct wiring; consider a named constant for the default value.

The fallback logic correctly mirrors the documented default and matches the added test coverage in collector_test.go. The only nit: the default value 10 is now duplicated as a magic number here and in the API doc comment/CRD description (clusterlogforwarder_types.go). Extracting it into a shared constant (e.g., defaultTerminationGracePeriodSeconds) would reduce the risk of the two drifting apart if the default ever changes.

♻️ Optional refactor

+const defaultTerminationGracePeriodSeconds int64 = 10
+
 func (f *Factory) NewPodSpec(trustedCABundle *v1.ConfigMap, spec obs.ClusterLogForwarderSpec, clusterID string, tlsProfileSpec configv1.TLSProfileSpec, namespace string) *v1.PodSpec {

 	var gracePeriod *int64
 	if f.CollectorSpec.TerminationGracePeriodSeconds != nil {
 		gracePeriod = f.CollectorSpec.TerminationGracePeriodSeconds
 	} else {
-		gracePeriod = utils.GetPtr[int64](10)
+		gracePeriod = utils.GetPtr(defaultTerminationGracePeriodSeconds)
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/collector/collector.go` around lines 131 - 142, The fallback wiring
in collector pod spec is correct, but the default termination grace period is
duplicated as a magic number. Extract the shared default into a named constant
(for example, defaultTerminationGracePeriodSeconds) and use it in the collector
logic so it stays aligned with the API doc comment and CRD description in
clusterlogforwarder_types.go.

api/observability/v1/clusterlogforwarder_types.go (1)

125-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

New field looks correct; minor consistency nit on the marker syntax.

The new TerminationGracePeriodSeconds field is well-documented, uses a pointer type for optionality, and the generated CRD schema (in config/crd/bases/...yaml and bundle/manifests/...yaml) correctly reflects minimum: 1, nullable: true, confirming the marker syntax works as intended.

One small nit: +kubebuilder:validation:Minimum:=1 uses the := assignment form, while every other numeric marker in this file (e.g. exclusiveMinimum/minimum on other fields generated elsewhere) uses the plain = form. Since both produce the same output here, this is purely a style inconsistency, not a functional issue.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/observability/v1/clusterlogforwarder_types.go` around lines 125 - 133,
The new TerminationGracePeriodSeconds field is fine, but the kubebuilder
validation marker uses an inconsistent assignment style. Update the marker on
TerminationGracePeriodSeconds in clusterlogforwarder_types.go from the
colon-assignment form to the standard plain equals form used by other markers in
this API file, keeping the same minimum value and generated CRD behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@api/observability/v1/clusterlogforwarder_types.go`:
- Around line 125-133: The new TerminationGracePeriodSeconds field is fine, but
the kubebuilder validation marker uses an inconsistent assignment style. Update
the marker on TerminationGracePeriodSeconds in clusterlogforwarder_types.go from
the colon-assignment form to the standard plain equals form used by other
markers in this API file, keeping the same minimum value and generated CRD
behavior.

In `@internal/collector/collector.go`:
- Around line 131-142: The fallback wiring in collector pod spec is correct, but
the default termination grace period is duplicated as a magic number. Extract
the shared default into a named constant (for example,
defaultTerminationGracePeriodSeconds) and use it in the collector logic so it
stays aligned with the API doc comment and CRD description in
clusterlogforwarder_types.go.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ae918120-e55e-41ee-b11c-60939d4e5777

📥 Commits

Reviewing files that changed from the base of the PR and between ea096d4 and 9e45b34.

⛔ Files ignored due to path filters (1)

• api/observability/v1/zz_generated.deepcopy.go is excluded by !**/zz_generated*

📒 Files selected for processing (7)

• api/observability/v1/clusterlogforwarder_types.go
• bundle/manifests/cluster-logging.clusterserviceversion.yaml
• bundle/manifests/observability.openshift.io_clusterlogforwarders.yaml
• config/crd/bases/observability.openshift.io_clusterlogforwarders.yaml
• config/manifests/bases/cluster-logging.clusterserviceversion.yaml
• internal/collector/collector.go
• internal/collector/collector_test.go

[vparfonov]

/hold cancel

[openshift-ci]

@vparfonov: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

[jcantrill]

/approve
/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcantrill, vparfonov

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [jcantrill]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment