Skip to content

Introduce --force-device-detection option #32730

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 5, 2024
Merged

Introduce --force-device-detection option #32730

merged 1 commit into from
Jun 5, 2024

Conversation

kvaps
Copy link
Member

@kvaps kvaps commented May 27, 2024 •
edited by joamaki
Loading

  • Introduce --enforce-device-detection option
  • Helm chart: enforceDeviceDetection option
  • Add tests for EnforceDeviceDetection option

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • If your commit description contains a Fixes: <commit-id> tag, then
    please add the commit author[s] as reviewer[s] to this issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Are you a user of Cilium? Added Add Ænix to the cilium users #32738
  • Thanks for contributing!

Fixes: #32721

Introduce --force-device-detection option to apply the auto-detection criteria also when devices are explicitly listed with --devices.

@kvaps kvaps requested review from a team as code owners May 27, 2024 11:18
@kvaps kvaps requested review from ldelossa, asauber and nebril May 27, 2024 11:18
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 27, 2024
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label May 27, 2024
@kvaps kvaps changed the title enforce device detection v1.16 Introduce --enforce-device-detection option (v1.16) May 27, 2024
@kvaps kvaps force-pushed the enforce-device-detection-v1.16 branch from d313db1 to 16f00d0 Compare May 27, 2024 11:23
@borkmann borkmann requested a review from joamaki May 27, 2024 11:29
@kvaps kvaps force-pushed the enforce-device-detection-v1.16 branch 4 times, most recently from 03d7554 to e151d49 Compare May 27, 2024 14:54
@maintainer-s-little-helper
Copy link

Commit e08b21e does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 27, 2024
@kvaps kvaps changed the title Introduce --enforce-device-detection option (v1.16) Introduce --enforce-device-detection option May 27, 2024
@kvaps kvaps force-pushed the enforce-device-detection-v1.16 branch from e08b21e to 7470f86 Compare May 27, 2024 16:43
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 27, 2024
@kvaps kvaps force-pushed the enforce-device-detection-v1.16 branch 3 times, most recently from e8c3fca to 5378e49 Compare May 27, 2024 16:55
@kvaps kvaps mentioned this pull request May 28, 2024
Closed
8 tasks
ldelossa
ldelossa approved these changes May 28, 2024
View reviewed changes
Copy link
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall these changes look OK to me.

The same set of checks over devices is performed, eliminating the possibility of 'over-selecting'.

@joestringer
Copy link
Member

Could you speak a bit about why we need three options for configuring devices? I would have expected one of two:

(a) Auto-detection. Cilium tries its best to select devices, and Cilium is responsible for picking the devices.
(b) Case (a) didn't work for some reason (bug?), so the user wants to override the set of devices. In this case Cilium is clearly not responsible for picking the devices.

Case (c) Both Cilium and the user specify devices seems like it confuses who is responsible for this logic, and it's yet another complicated flag in an already complicated set of flags for the agent. Can't we just fix (a) or (b) to better cover your use case?

@kvaps
Copy link
Member Author

kvaps commented May 29, 2024 •
edited
Loading

@joestringer sure, I can explain. The reason is that we're not end user, we're adopting Cilium together with kube-ovn in Cozystack a free platform for running managed services. We need kube-ovn as it provides extensive network fabric for running VirtualMachines using KubeVirt.

Our users might have various hardware configurations with various interfaces, eg. some of them using bonding and vlan interfaces, some of them are not, so devices must be auto-detected.

But in order to make this configuration working, the ovn0 device should also be added to the list of auto-detected devices.

The reason I'm insist for adding an extra option and not extend autodetection rules, is that I don't want to break exiting installations. Eg. OpenShift project also uses OVN but different implementation ovn-kubernetes. Some other users might want to run Kubernetes with Cilium on hypervisors with OVN which is unrelated to the Kubernetes networking. Eg, Mirantis uses Kubernetes to provision OpenStack platform, OVN is a default networking for OpenStack for now.

Another user-story that I see is that there might be other users who might want to do the same, eg. to create an extra VPN device on every node, that must be always specified in a list in addition to auto-detection rules.

@kvaps kvaps mentioned this pull request May 29, 2024
Merged
@kvaps
Copy link
Member Author

kvaps commented Jun 5, 2024

/test

@kvaps
Copy link
Member Author

kvaps commented Jun 5, 2024

/ci-e2e

@kvaps
Copy link
Member Author

kvaps commented Jun 5, 2024

/ci-ginkgo

@kvaps
Copy link
Member Author

kvaps commented Jun 5, 2024

/ci-ingress

@kvaps
Copy link
Member Author

kvaps commented Jun 5, 2024

/ci-ginkgo

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 5, 2024
@kvaps
Copy link
Member Author

kvaps commented Jun 5, 2024

@aanm @joestringer all tests have passed 🎉

@aanm aanm added this pull request to the merge queue Jun 5, 2024
Merged via the queue into cilium:main with commit ce59386 Jun 5, 2024
@brb
Copy link
Member

brb commented Jun 7, 2024

@kvaps Thanks for the PR. Do you mind to update the documentation too ( Documentation/network/kubernetes/kubeproxy-free.rst)?

@kvaps kvaps mentioned this pull request Aug 12, 2024
Merged
kvaps added a commit to cozystack/cozystack that referenced this pull request Aug 12, 2024
@kvaps
Update Cilium v1.16 (#277)
bbdec9b 
The new Cilium already enables our patch
cilium/cilium#32730. It should be better to
update instead of keeping it in-tree

Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
@kvaps kvaps mentioned this pull request Aug 30, 2024
Closed
@dswaffordcw dswaffordcw mentioned this pull request Feb 11, 2025
Closed
3 tasks
dswaffordcw added a commit to dswaffordcw/cilium that referenced this pull request Mar 8, 2025
@dswaffordcw
docs: improve clarity for cilium-agent --force-device-detection option
0083a15 
Under cilium#32730, the --force-device-detection option was introduced to cilium-agent.  The current documentation is ambiguous:
*  It is unclear that this option can only be used when the --devices option is set
*  It is unclear that this option is additive, and is not a post-matching filter for those devices matched by --devices.

One can only understand the true meaning of this flag by reading through the comments of the original PR.

This commit improves the help shown for cilium-agent, the comments on the Helm chart, and the comments in the source where the feature is implemented.

Signed-off-by: David Swafford <dswafford@coreweave.com>
@dswaffordcw dswaffordcw mentioned this pull request Mar 8, 2025
Closed
8 tasks
dswaffordcw added a commit to dswaffordcw/cilium that referenced this pull request Mar 8, 2025
@dswaffordcw
docs: improve clarity for cilium-agent --force-device-detection option
6979936 
Under cilium#32730, the --force-device-detection option was introduced to cilium-agent.  The current documentation is ambiguous:
*  It is unclear that this option can only be used when the --devices option is set
*  It is unclear that this option is additive, and is not a post-matching filter for those devices matched by --devices.

One can only understand the true meaning of this flag by reading through the comments of the original PR.

This commit improves the help shown for cilium-agent, the comments on the Helm chart, and the comments in the source where the feature is implemented.

Signed-off-by: David Swafford <dswafford@coreweave.com>
dswaffordcw added a commit to dswaffordcw/cilium that referenced this pull request Mar 8, 2025
@dswaffordcw
docs: improve clarity for cilium-agent --force-device-detection option
2c43d6a 
Under cilium#32730, the --force-device-detection option was introduced to cilium-agent.  The current documentation is ambiguous:
*  It is unclear that this option can only be used when the --devices option is set
*  It is unclear that this option is additive, and is not a post-matching filter for those devices matched by --devices.

One can only understand the true meaning of this flag by reading through the comments of the original PR.

This commit improves the help shown for cilium-agent, the comments on the Helm chart, and the comments in the source where the feature is implemented.

Signed-off-by: David Swafford <dswafford@coreweave.com>
dswaffordcw added a commit to dswaffordcw/cilium that referenced this pull request Mar 8, 2025
@dswaffordcw
docs: improve clarity for cilium-agent --force-device-detection option
4e21783 
Under cilium#32730, the --force-device-detection option was introduced to cilium-agent.  The current documentation is ambiguous:
*  It is unclear that this option can only be used when the --devices option is set
*  It is unclear that this option is additive, and is not a post-matching filter for those devices matched by --devices.

One can only understand the true meaning of this flag by reading through the comments of the original PR.

This commit improves the help shown for cilium-agent, the comments on the Helm chart, and the comments in the source where the feature is implemented.

Signed-off-by: David Swafford <dswafford@coreweave.com>
dswaffordcw added a commit to dswaffordcw/cilium that referenced this pull request Mar 8, 2025
@dswaffordcw
docs: improve clarity for cilium-agent --force-device-detection option
b3916a5 
Under cilium#32730, the --force-device-detection option was introduced to cilium-agent.  The current documentation is ambiguous.

This commit aims to reduce confusion by making the following changes:
1) Added the --devices flag to the Helm command reference guide.
2) Revised the description shown for forceDeviceDetection in Helm and --force-device-detection in the Agent's command reference guide.
3) Revised the comments across related source files.
4) Renamed the variable enforceDeviceDetection to forceDeviceDetection on the related datapath source code to be consistent with the user-facing name. In the original PR, the option was named enforce and reviewers asked that it be renamed to force. One variable with two references was left using the original name.

Signed-off-by: David Swafford <dswafford@coreweave.com>
dswaffordcw added a commit to dswaffordcw/cilium that referenced this pull request Mar 8, 2025
@dswaffordcw
docs: improve clarity for cilium-agent --force-device-detection option
9f41c64 
Under cilium#32730, the --force-device-detection option was introduced to cilium-agent. The current documentation is ambiguous.

This commit aims to reduce confusion by making the following changes:
1) Added the --devices flag to the Helm command reference guide.
2) Revised the description shown for forceDeviceDetection in Helm and --force-device-detection in the Agent's command reference guide.
3) Revised the comments across related source files.
4) Renamed the variable enforceDeviceDetection to forceDeviceDetection on the related datapath source code to be consistent with the user-facing name. In the original PR, the option was named enforce and reviewers asked that it be renamed to force. One variable with two references was left using the original name.

Signed-off-by: David Swafford <dswafford@coreweave.com>
dswaffordcw added a commit to dswaffordcw/cilium that referenced this pull request Mar 15, 2025
@dswaffordcw
docs: improve clarity for cilium-agent --force-device-detection option
0634eca 
Under cilium#32730, the --force-device-detection option was introduced to cilium-agent. The current documentation is ambiguous.

This commit aims to reduce confusion by making the following changes:
1) Added the --devices flag to the Helm command reference guide.
2) Revised the description shown for forceDeviceDetection in Helm and --force-device-detection in the Agent's command reference guide.
3) Revised the comments across related source files.
4) Renamed the variable enforceDeviceDetection to forceDeviceDetection on the related datapath source code to be consistent with the user-facing name. In the original PR, the option was named enforce and reviewers asked that it be renamed to force. One variable with two references was left using the original name.

Signed-off-by: David Swafford <dswafford@coreweave.com>
dswaffordcw added a commit to dswaffordcw/cilium that referenced this pull request Mar 15, 2025
@dswaffordcw
docs: improve clarity for cilium-agent --force-device-detection option
c74ee51 
Under cilium#32730, the --force-device-detection option was introduced to cilium-agent. The current documentation is ambiguous.

This commit aims to reduce confusion by making the following changes:
1) Added the --devices flag to the Helm command reference guide.
2) Revised the description shown for forceDeviceDetection in Helm and --force-device-detection in the Agent's command reference guide.
3) Revised the comments across related source files.
4) Renamed the variable enforceDeviceDetection to forceDeviceDetection on the related datapath source code to be consistent with the user-facing name. In the original PR, the option was named enforce and reviewers asked that it be renamed to force. One variable with two references was left using the original name.

Fixes: cilium#37425

Signed-off-by: David Swafford <dswafford@coreweave.com>
dswaffordcw added a commit to dswaffordcw/cilium that referenced this pull request Mar 15, 2025
@dswaffordcw
docs: improve clarity for cilium-agent --force-device-detection option
8f7339f 
Under cilium#32730, the --force-device-detection option was introduced to cilium-agent. The current documentation is ambiguous.

This commit aims to reduce confusion by making the following changes:
1) Added the --devices flag to the Helm command reference guide.
2) Revised the description shown for forceDeviceDetection in Helm and --force-device-detection in the Agent's command reference guide.
3) Revised the comments across related source files.
4) Renamed the variable enforceDeviceDetection to forceDeviceDetection on the related datapath source code to be consistent with the user-facing name. In the original PR, the option was named enforce and reviewers asked that it be renamed to force. One variable with two references was left using the original name.

Fixes: cilium#37425

Signed-off-by: David Swafford <dswafford@coreweave.com>
dswaffordcw added a commit to dswaffordcw/cilium that referenced this pull request Mar 15, 2025
@dswaffordcw
docs: improve clarity for cilium-agent --force-device-detection option
3526e37 
Under cilium#32730, the --force-device-detection option was introduced to cilium-agent. The current documentation is ambiguous.

This commit aims to reduce confusion by making the following changes:
1) Added the --devices flag to the Helm command reference guide.
2) Revised the description shown for forceDeviceDetection in Helm and --force-device-detection in the Agent's command reference guide.
3) Revised the comments across related source files.
4) Renamed the variable enforceDeviceDetection to forceDeviceDetection on the related datapath source code to be consistent with the user-facing name. In the original PR, the option was named enforce and reviewers asked that it be renamed to force. One variable with two references was left using the original name.

Fixes: cilium#37425

Signed-off-by: David Swafford <dswafford@coreweave.com>
dswaffordcw added a commit to dswaffordcw/cilium that referenced this pull request May 14, 2025
@dswaffordcw
docs: improve clarity for cilium-agent --force-device-detection option
10f3e5f 
Under cilium#32730, the --force-device-detection option was introduced to cilium-agent. The current documentation is ambiguous.

This commit aims to reduce confusion by making the following changes:
1) Added the --devices flag to the Helm command reference guide.
2) Revised the description shown for forceDeviceDetection in Helm and --force-device-detection in the Agent's command reference guide.
3) Revised the comments across related source files.
4) Renamed the variable enforceDeviceDetection to forceDeviceDetection on the related datapath source code to be consistent with the user-facing name. In the original PR, the option was named enforce and reviewers asked that it be renamed to force. One variable with two references was left using the original name.

Fixes: cilium#37425

Signed-off-by: David Swafford <dswafford@coreweave.com>
dswaffordcw added a commit to dswaffordcw/cilium that referenced this pull request May 14, 2025
@dswaffordcw
docs: improve clarity for cilium-agent --force-device-detection option
8b5e69d 
Under cilium#32730, the --force-device-detection option was introduced to cilium-agent. The current documentation is ambiguous.

This commit aims to reduce confusion by making the following changes:
1) Added the --devices flag to the Helm command reference guide.
2) Revised the description shown for forceDeviceDetection in Helm and --force-device-detection in the Agent's command reference guide.
3) Revised the comments across related source files.
4) Renamed the variable enforceDeviceDetection to forceDeviceDetection on the related datapath source code to be consistent with the user-facing name. In the original PR, the option was named enforce and reviewers asked that it be renamed to force. One variable with two references was left using the original name.

Fixes: cilium#37425

Signed-off-by: David Swafford <dswafford@coreweave.com>
dswaffordcw added a commit to dswaffordcw/cilium that referenced this pull request May 14, 2025
@dswaffordcw
docs: improve clarity for cilium-agent --force-device-detection option
f30944a 
Under cilium#32730, the --force-device-detection option was introduced to cilium-agent. The current documentation is ambiguous.

This commit aims to reduce confusion by making the following changes:
1) Added the --devices flag to the Helm command reference guide.
2) Revised the description shown for forceDeviceDetection in Helm and --force-device-detection in the Agent's command reference guide.
3) Revised the comments across related source files.
4) Renamed the variable enforceDeviceDetection to forceDeviceDetection on the related datapath source code to be consistent with the user-facing name. In the original PR, the option was named enforce and reviewers asked that it be renamed to force. One variable with two references was left using the original name.

Fixes: cilium#37425

Signed-off-by: David Swafford <dswafford@coreweave.com>
dswaffordcw added a commit to dswaffordcw/cilium that referenced this pull request May 14, 2025
@dswaffordcw
docs: improve clarity for cilium-agent --force-device-detection option
d49ac7a 
Under cilium#32730, the --force-device-detection option was introduced to cilium-agent. The current documentation is ambiguous.

This commit aims to reduce confusion by making the following changes:
1) Added the --devices flag to the Helm command reference guide.
2) Revised the description shown for forceDeviceDetection in Helm and --force-device-detection in the Agent's command reference guide.
3) Revised the comments across related source files.
4) Renamed the variable enforceDeviceDetection to forceDeviceDetection on the related datapath source code to be consistent with the user-facing name. In the original PR, the option was named enforce and reviewers asked that it be renamed to force. One variable with two references was left using the original name.

Fixes: cilium#37425

Signed-off-by: David Swafford <dswafford@coreweave.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joamaki joamaki joamaki approved these changes

@nebril nebril nebril approved these changes

@asauber asauber asauber approved these changes

@ldelossa ldelossa ldelossa approved these changes

@aanm aanm aanm approved these changes

Assignees
No one assigned
Labels
kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CFP: Option to append device to autodetected devices list
8 participants
@kvaps @joestringer @joamaki @asauber @aanm @brb @nebril @ldelossa