Skip to content

Conversation

@palonsoro
Copy link

So far, one can override log levels in specific nodes via env-overrides configmap. However, only specific nodes could be overridden, there was not a global override. This modifies ovnkube-lib.sh to allow specifying a _global node whose overrides are sourced in all nodes when present. If _global and node-specific keys are present in the configmap, values on the node-specific one should take precedence.

This would be useful for scenarios where there is machine autoscaling, because you cannot know the node names in advance and manually maintain the env-overrides configmap if the node machines change automatically.

So far, one can override log levels in specific nodes via
`env-overrides` configmap. However, only specific nodes could be
overridden, there was not a global override. This modifies
`ovnkube-lib.sh` to allow specifying a `_global` node whose overrides
are sourced in all nodes when present. If `_global` and node-specific
keys are present in the configmap, values on the node-specific one
should take precedence.

This would be useful for scenarios where there is machine autoscaling,
because you cannot know the node names in advance and manually maintain
the `env-overrides` configmap if the node machines change automatically.
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 4, 2025
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 4, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@coderabbitai
Copy link

coderabbitai bot commented Dec 4, 2025

Walkthrough

A global overrides sourcing block is added to the script library before node-specific overrides. The block conditionally sources /env/_global using allexport to ensure variable exports, with no modifications to existing error handling or control flow.

Changes

Cohort / File(s) Summary
Shell script global overrides initialization
bindata/network/ovn-kubernetes/common/008-script-lib.yaml
Adds conditional sourcing of global environment overrides from /env/_global using allexport for automatic variable export. Executes prior to existing node-specific overrides.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Single file with a straightforward shell sourcing block addition
  • Follows standard shell scripting patterns (allexport/set +a usage)
  • No logic complexity, error handling changes, or control flow modifications
  • Verify the file path /env/_global is correct and the ordering relative to node-specific overrides is intentional
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Cache: Disabled due to data retention organization setting

Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting

📥 Commits

Reviewing files that changed from the base of the PR and between 8bd3f1c and 0816180.

📒 Files selected for processing (1)
  • bindata/network/ovn-kubernetes/common/008-script-lib.yaml (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**

⚙️ CodeRabbit configuration file

-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.

Files:

  • bindata/network/ovn-kubernetes/common/008-script-lib.yaml
🔇 Additional comments (1)
bindata/network/ovn-kubernetes/common/008-script-lib.yaml (1)

23-28: Well‑structured global override sourcing with correct precedence.

The implementation correctly introduces global overrides while maintaining the precedence model: the global block sources first (lines 23–28), then node-specific overrides source after (lines 30–36), ensuring node-specific values override global ones. The pattern mirrors the existing node-specific block exactly, promoting consistency and maintainability. The file-existence guard and allexport approach are both appropriate.


Comment @coderabbitai help to get the list of available commands and usage tips.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 4, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: palonsoro
Once this PR has been reviewed and has the lgtm label, please assign pliurh for approval. For more information see the Code Review Process.

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

Details Needs approval from an approver in each of these files:

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

@palonsoro
Copy link
Author

Tested on a 4.21 nightly. It works as expected:

  • If a _global key is added to env-overrides configmap, it is applied on all nodes.
  • If there is a _global key and a node name key in env-overrides configmap, the node name one takes precedence.

So the PR should be ready for review.

@palonsoro palonsoro marked this pull request as ready for review December 5, 2025 10:35
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 5, 2025
@palonsoro
Copy link
Author

This PR would also address what is requested in RFE-8572

@palonsoro
Copy link
Author

/retest-required

@palonsoro
Copy link
Author

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Dec 6, 2025

@palonsoro: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/security 0816180 link false /test security

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.

@palonsoro
Copy link
Author

The failed test complains about stuff not related to the PR.

So @bpickard22 @pliurh not sure if you could please help me reviewing and getting this PR. Not in a rush, but it would help us troubleshooting if we could set this globally, specially in autoscaling scenarios (as exemplified in the RFE).

Thanks in advance.

@pliurh
Copy link
Contributor

pliurh commented Dec 11, 2025

The script ovnkube-lib.sh is only used by ovnkube-node pods. Shall we also set this for the ovnkube-control-plane pods?
You shall also update the README.md to include an example of the _global key.

@palonsoro
Copy link
Author

Hi

The script ovnkube-lib.sh is only used by ovnkube-node pods. Shall we also set this for the ovnkube-control-plane pods?

I originally thought of nodes only, but it makes sense to include the control plane too. Let me update the control plane too.

You shall also update the README.md to include an example of the _global key.

Got it. I'll be doing it shortly.

@palonsoro
Copy link
Author

By the way, while reviewing this, I find it confusing that we still use _master for nbdb, sbdb, northd... This comes from the pre-IC era. However, it is really confusing that we set configurations in the nodes with the _master key.

So I see 2 alternatives:

  • Add _global also on the containers of ovnkube-node namespace that use _master for the log levels.
  • Make the containers that use _master on the ovnkube-node use the node-specific configmap key and _global. That way, all the containers in ovnkube-node daemonset would use the node-specific setting or global and only the ovnkube-control-plane deployment would use _master and _global.

Which one would you prefer? I'd definitely advocate for the latter. It is a bit of a wider change (maybe I should re-title the PR, then), but IMHO it makes much more sense to have the node-specific setting impact everything in the node, _master to impact only what now is the real control plane (ovnkube-control-plane deployment ) and _global impact everything.

What do you think?

@palonsoro
Copy link
Author

@pliurh not sure if you could please comment on the above. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants