-
Notifications
You must be signed in to change notification settings - Fork 582
DevPreview: NetworkConnect feature gate
#2629
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.
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
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Surya Seetharaman <suryaseetharaman.9@gmail.com>
Signed-off-by: Surya Seetharaman <suryaseetharaman.9@gmail.com>
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @tssurya! Some important instructions when contributing to openshift/api: |
NetworkConnect feature gate
WalkthroughA new NetworkConnect feature gate is introduced across the codebase. The feature is defined in Go source code with configuration metadata, documented in features.md, and registered in deployment configuration manifests for both Hypershift and SelfManagedHA profiles across four different stages (Default, DevPreviewNoUpgrade, OKD, and TechPreviewNoUpgrade). Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (11)
🧰 Additional context used📓 Path-based instructions (1)**⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (1)features/features.go (1)
🔇 Additional comments (11)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: build linters: unable to load custom analyzer "kubeapilinter": tools/_output/bin/kube-api-linter.so, plugin: not implemented Comment |
|
@tssurya This is a new one on me (OKEP), the gate name you've chosen here, it's not going to be used at all by the OVN or upstream K8s systems right? If there is any overlap, we've had some issues in the past and I suggest we choose a downstream specific name for the downstream specific parts. Speaking of, is there anything downstream specific? Do we need an OCP EP to show how this upstream feature will be integrated into OCP or is it really a completely upstream feature? |
Hi Joel! good question, no its not the same name - in ovnk we still don't have fancy feature CRDs :D we are old school gating using bool flags (me cringes) so its called --network-connect-enable (nce) flag via CLI passed when deploying the networking binary. So this feature gate I add is specific to only OCP and in CNO we will translate this logic to in turn - go and pass that CLI flag when it kicks in the binary to start running ovnkube https://ovn-kubernetes.io/okeps/okep-5085-localnet-api/ is where we have our OKEPs published. This feature is: https://ovn-kubernetes.io/okeps/okep-5224-connecting-udns/okep-5224-connecting-udns/
Also a good question! There is nothing specific to OCP that warrants an OCP EP except for the feature gate we add and a small PR to enable the feature via CNO. The feature is completely an upstream feature E2E and easily pluggable to the point where if this gate is off, then the controller is just totally OFF in ovnkube and feature code does nothing. Upgrades in OCP will also be seamless and no extra thing to consider. I had thought about this if we needed a downstream specific enhancement and given nothing needed to be specially d/s didn't do one. |
Makes sense to me, thanks for the explanation
Also makes sense, thanks for taking the time to explain /lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@tssurya: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
This PR adds NetworkConnect DevPreview feature gate