USHIFT-6795: C2CC: Routes controller#6599
Open
pmtk wants to merge 43 commits intoopenshift:mainfrom
Open
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pmtk: This pull request references USHIFT-6795 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this: 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new Cluster-to-Cluster (C2CC) route manager to MicroShift that implements OVN NB route management, Linux policy/service routing, nftables bypass rules, node SNAT annotation management, a C2CC NetworkPolicy, service registration, unit tests, and Robot Framework end-to-end scenarios. ChangesC2CC Route Manager (single cohesive change DAG)
Sequence Diagram(s)sequenceDiagram
participant MS as MicroShift
participant CM as C2CCRouteManager
participant OVN as OVN_NB
participant K8s as Kubernetes_API
participant NL as Linux_Netlink
participant NFT as nftables
MS->>CM: Start manager (NewC2CCRouteManager / Run)
CM->>OVN: Connect & Monitor NB (unix socket, backoff)
CM->>K8s: Init kube client
CM->>NL: Subscribe routes/rules events
CM->>NFT: Subscribe nft events
CM->>CM: Start reconcile loop (periodic + event-driven)
alt On reconcile (tick or event)
CM->>OVN: List/Create/Delete Logical_Router_Static_Route
CM->>K8s: Get/Patch Node annotations, Reconcile NetworkPolicy
CM->>NL: Reconcile kernel policy routes and ip rules
CM->>NFT: Reconcile nftables bypass rules (add/remove)
OVN-->>CM: state / events
K8s-->>CM: state / events
NL-->>CM: state / events
NFT-->>CM: state / events
end
MS->>CM: Shutdown
CM->>OVN: Cleanup routes
CM->>NL: Cleanup routes/rules
CM->>NFT: Cleanup nft rules
CM->>K8s: Delete NetworkPolicy / clear annotations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes 🚥 Pre-merge checks | ✅ 10 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pmtk 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 |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (4)
pkg/controllers/c2cc/nbdb_test.go (1)
16-41:FieldTagstests are currently tautological.These assertions only re-check values set in the same struct literal, so they won’t catch
ovsdbtag/schema regressions. Please validate tag mappings directly (e.g., reflection on struct tags or model metadata checks).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/c2cc/nbdb_test.go` around lines 16 - 41, The tests TestLogicalRouterStaticRoute_FieldTags and TestLogicalRouter_FieldTags are tautological because they only re-assert values set in struct literals; replace these with reflection-based assertions that verify the actual struct field tags (e.g., the ovsdb/model tags) on the types instead of field values. Use reflect.TypeOf(LogicalRouterStaticRoute{}) and reflect.TypeOf(LogicalRouter{}) to inspect fields UUID, IPPrefix, Nexthop, ExternalIDs, Policy and UUID, Name, StaticRoutes respectively, and assert that Tag.Get("ovsdb") (or the correct tag key used by your OVSDB mapping) equals the expected tag/schema name for each field; update the test expectations to the correct tag strings so the tests fail on tag/schema regressions. Ensure you keep the original test names and replace value assertions with these tag checks.test/suites/c2cc/reconciliation.robot (1)
19-21: 30s timeout may be tight for some reconciliation scenarios.The controller's periodic reconcile is 10s (
reconcileInterval). With 5s retry, you get ~6 attempts. Should be sufficient, but event-driven reconciliation should trigger faster.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/suites/c2cc/reconciliation.robot` around lines 19 - 21, The test's reconciliation timeout (${RECONCILE_TIMEOUT}) is likely too short for slower environments; increase ${RECONCILE_TIMEOUT} (or adjust ${RECONCILE_RETRY}) so the suite allows more attempts given the controller's reconcileInterval (~10s). Update the Variables block to set a larger ${RECONCILE_TIMEOUT} (for example to 60s or more) or reduce ${RECONCILE_RETRY} gap to ensure enough reconciliation attempts relative to reconcileInterval and event-driven triggers.pkg/controllers/c2cc/ovn.go (1)
25-28: Consider URL-safe encoding for named UUIDs.The replacer handles common characters but
.,/,:,-may not be exhaustive for all CIDR formats. IPv6 CIDRs could introduce other characters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/c2cc/ovn.go` around lines 25 - 28, The current buildNamedUUID uses strings.NewReplacer to sanitize a few characters but misses other characters (e.g., from IPv6 CIDRs); change buildNamedUUID to produce a URL-safe deterministic token by encoding the combined prefix+suffix with a safe encoder: e.g., compute a SHA-256 (or other stable) hash of prefix+suffix and return its URL-safe base64 (base64.RawURLEncoding.EncodeToString(hash[:])) or, if readability is desired, return url.PathEscape(prefix+suffix) to percent-encode all unsafe characters; update the buildNamedUUID function to use one of these approaches so all possible characters are handled safely.test/resources/c2cc.resource (1)
174-180: Python inline script for JSON parsing works but is fragile.Consider using
oc get node -o jsonpathwith the annotation key directly, similar to line 186. The current approach handles edge cases but adds complexity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/resources/c2cc.resource` around lines 174 - 180, Replace the fragile Python inline in the "Verify Node SNAT Annotation" check: instead of piping oc JSON to python3 -c, call Oc On Cluster to run oc get node with a jsonpath that directly extracts the annotation key 'k8s.ovn.org/node-ingress-snat-exclude-subnets' (for example using oc get node -o jsonpath="{.items[0].metadata.annotations['k8s.ovn.org/node-ingress-snat-exclude-subnets']}") so ${stdout} contains the annotation value reliably; update the command that currently uses python3 -c in the test to use this jsonpath form and keep the subsequent Should Contain assertions unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controllers/c2cc/annotation.go`:
- Around line 74-97: The node watch loop currently can leak watches and spin on
reconnects; ensure each watcher is explicitly stopped and that unexpected
channel closure backs off: after successfully creating watcher (from
a.kubeClient.CoreV1().Nodes().Watch with FieldSelector
"metadata.name="+a.nodeName) defer or call watcher.Stop() when you exit that
watch scope, iterate using: ch, ok := <-watcher.ResultChan() and if !ok break
the inner loop and sleep/backoff (e.g., time.Sleep(10*time.Second)) before
continuing, and keep the existing ctx.Done() checks and reconcileCh send
(reconcileCh <- "node-annotation-changed") behavior unchanged so the loop won't
rapidly spin or leak goroutines.
In `@pkg/controllers/c2cc/nbdb.go`:
- Around line 56-58: The current loop around os.Stat(ovnNBSocketPath) treats
every error as "not found" and keeps retrying; update the check so that if
os.Stat returns an error that is not os.IsNotExist(err) you immediately return
that error (or wrap/propagate it) instead of retrying, and only continue retry
logic when os.IsNotExist(err) is true; locate the os.Stat(ovnNBSocketPath) call
in nbdb.go and change the conditional to explicitly handle non-`not exists`
errors (using os.IsNotExist) and return them promptly.
In `@pkg/controllers/c2cc/networkpolicy.go`:
- Around line 27-53: newNetworkPolicyManager currently builds an ingress rule
from ingressPeers which, if remotePodCIDRs is empty, results in an ingress rule
with an empty From (matching all sources); change the logic so you only add the
ingress rule and set networkingv1.PolicyTypeIngress when ingressPeers is
non-empty: compute ingressPeers as before, then conditionally populate
policy.Spec.Ingress and policy.Spec.PolicyTypes only when len(ingressPeers) > 0
(otherwise leave Spec.Ingress nil/empty and omit PolicyTypeIngress) so the
NetworkPolicy does not accidentally allow all ingress; update references to
ingressPeers, newNetworkPolicyManager, policy.Spec.Ingress, and PolicyTypes
accordingly.
- Around line 65-96: The reconcile/update and cleanup paths are mutating a
fixed-named NetworkPolicy without verifying ownership; before
Create/Update/Delete, fetch the existing object (existing from client.Get in the
reconcile flow and the object returned by Delete error handling in cleanup) and
check its labels (specifically "app.kubernetes.io/managed-by") against the
controller's expected manager value (compare
existing.Labels["app.kubernetes.io/managed-by"] to the value used for your
managed resources—e.g., the label on m.desired); if the label exists and does
not match, do not Update or Delete (instead log and return nil or a
non-destructive result), and when creating ensure you set the managed-by label
on m.desired so ownership is explicit; apply this logic inside the
reconcile/update code paths where client.Get, client.Create, client.Update are
used and inside networkPolicyManager.cleanup before deleting the named policy.
In `@pkg/controllers/c2cc/nftables.go`:
- Around line 161-206: The debounce goroutine is leaked because rawCh is never
closed when unsubscribing; change the returned unsubscribe to a closure that
closes rawCh and then calls sock.Close (use a sync.Once to ensure idempotence)
so the receive goroutine unblocks and the debounce goroutine can exit (refer to
rawCh, reconcileCh, the anonymous debounce goroutine, and sock.Close in your
change).
In `@pkg/controllers/c2cc/policy_routes.go`:
- Around line 41-45: The route reconcile code currently only logs
netlink.RouteReplace/RouteDel failures and always returns nil; update
reconcileRoutes to collect and propagate errors so callers
(linuxRouteManager.reconcile and serviceRouteManager.reconcile) see failures:
inside reconcileRoutes (and the block handling netlink.RouteReplace and
netlink.RouteDel) accumulate any failures (e.g., append to a slice or use
multierror) and return a non-nil error if any netlink operation fails, and
adjust callers that expect reconcileRoutes to return error so they propagate
that error instead of treating reconciliation as success.
- Around line 38-45: The equality check that skips RouteReplace only compares
gateways (actual.Gw == route.Gw) but must also compare the interface index;
update the condition that uses actualByDst/actual to require both
actual.Gw.Equal(route.Gw) AND actual.LinkIndex == route.LinkIndex (or
equivalent) before skipping; this ensures stale routes with a changed ifindex
(e.g., ovn-k8s-mp0 recreated) will be replaced by netlink.RouteReplace and
matches how routes are programmed in service_routes.go.
In `@pkg/controllers/c2cc/service_routes.go`:
- Around line 57-72: The loop is using the single IPv4 gateway returned by
getMgmtPortGateway() for every svc CIDR in m.localSvcCIDRs, which breaks
IPv6/dual‑stack; update the logic to pick a gateway that matches the address
family of each svc CIDR (or have getMgmtPortGateway return gateways per family)
before creating netlink.Route entries: for each svcCIDR in m.localSvcCIDRs,
determine whether dst is IPv4 or IPv6 and use the matching gateway IP (or leave
Gw nil if no same‑family gateway exists) when constructing the netlink.Route
(fields Dst, Gw, Table, Protocol, LinkIndex), so only same‑family next hops are
applied.
In `@test/bin/scenario.sh`:
- Line 1242: The local declaration combines with command substitution (local
var_name="${var_prefix}_$(echo "${prop}" | tr '[:lower:]' '[:upper:]')") which
can hide failures under set -e; fix by splitting into two statements: first
declare the variable (local var_name) and then perform the assignment to
var_name using the command substitution (var_name="${var_prefix}_$(echo
"${prop}" | tr '[:lower:]' '[:upper:]')"), referencing the same var_prefix and
prop variables and the var_name identifier so command-substitution failures
correctly propagate.
In `@test/scenarios-bootc/el9/presubmits/el98-src`@c2cc.sh:
- Around line 132-133: The script copies the cluster-admin kubeadmin kubeconfig
to /tmp and makes it world-readable (chmod 644), which exposes credentials;
change the flow so the kubeconfig remains owned by redhat and only readable by
its owner (mode 0600) or avoid the shared temp file entirely. Update the
run_command_on_vm call that creates /tmp/kubeconfig-b to either set ownership
and permissions (chown redhat && chmod 600) instead of chmod 644, or skip
writing to /tmp and have the scp call pull directly from the source path
(/var/lib/microshift/resources/kubeadmin/${host2_ip}/kubeconfig) so you remove
the temporary shared file; reference the run_command_on_vm invocation and the
scp line that uses "${kubeconfig_b}" when making the change.
In `@test/suites/c2cc/reconciliation.robot`:
- Around line 117-123: Replace the non-disruptive lookup with a disruptive
command to avoid a race between discovery and deletion: change the step that
sets ${handle} which currently uses "Command On Cluster" (running `nft list
chain inet ovn-kubernetes ovn-kube-pod-subnet-masq -a | grep
'c2cc-no-masq:${cidr}' | awk '/# handle/{print $NF}'`) to use "Disruptive
Command On Cluster" instead, so the handle discovery and the subsequent
"Disruptive Command On Cluster ... nft delete rule ... handle ${handle}"
both use the disruptive execution path (and consider adding a check for an empty
${handle} before attempting deletion).
---
Nitpick comments:
In `@pkg/controllers/c2cc/nbdb_test.go`:
- Around line 16-41: The tests TestLogicalRouterStaticRoute_FieldTags and
TestLogicalRouter_FieldTags are tautological because they only re-assert values
set in struct literals; replace these with reflection-based assertions that
verify the actual struct field tags (e.g., the ovsdb/model tags) on the types
instead of field values. Use reflect.TypeOf(LogicalRouterStaticRoute{}) and
reflect.TypeOf(LogicalRouter{}) to inspect fields UUID, IPPrefix, Nexthop,
ExternalIDs, Policy and UUID, Name, StaticRoutes respectively, and assert that
Tag.Get("ovsdb") (or the correct tag key used by your OVSDB mapping) equals the
expected tag/schema name for each field; update the test expectations to the
correct tag strings so the tests fail on tag/schema regressions. Ensure you keep
the original test names and replace value assertions with these tag checks.
In `@pkg/controllers/c2cc/ovn.go`:
- Around line 25-28: The current buildNamedUUID uses strings.NewReplacer to
sanitize a few characters but misses other characters (e.g., from IPv6 CIDRs);
change buildNamedUUID to produce a URL-safe deterministic token by encoding the
combined prefix+suffix with a safe encoder: e.g., compute a SHA-256 (or other
stable) hash of prefix+suffix and return its URL-safe base64
(base64.RawURLEncoding.EncodeToString(hash[:])) or, if readability is desired,
return url.PathEscape(prefix+suffix) to percent-encode all unsafe characters;
update the buildNamedUUID function to use one of these approaches so all
possible characters are handled safely.
In `@test/resources/c2cc.resource`:
- Around line 174-180: Replace the fragile Python inline in the "Verify Node
SNAT Annotation" check: instead of piping oc JSON to python3 -c, call Oc On
Cluster to run oc get node with a jsonpath that directly extracts the annotation
key 'k8s.ovn.org/node-ingress-snat-exclude-subnets' (for example using oc get
node -o
jsonpath="{.items[0].metadata.annotations['k8s.ovn.org/node-ingress-snat-exclude-subnets']}")
so ${stdout} contains the annotation value reliably; update the command that
currently uses python3 -c in the test to use this jsonpath form and keep the
subsequent Should Contain assertions unchanged.
In `@test/suites/c2cc/reconciliation.robot`:
- Around line 19-21: The test's reconciliation timeout (${RECONCILE_TIMEOUT}) is
likely too short for slower environments; increase ${RECONCILE_TIMEOUT} (or
adjust ${RECONCILE_RETRY}) so the suite allows more attempts given the
controller's reconcileInterval (~10s). Update the Variables block to set a
larger ${RECONCILE_TIMEOUT} (for example to 60s or more) or reduce
${RECONCILE_RETRY} gap to ensure enough reconciliation attempts relative to
reconcileInterval and event-driven triggers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 66502359-d58b-4535-b110-837f5171a8be
⛔ Files ignored due to path filters (179)
go.sumis excluded by!**/*.sumvendor/github.com/cenkalti/backoff/v4/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/backoff.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/context.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/exponential.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/retry.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/ticker.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/timer.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/backoff/v4/tries.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/hub/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/hub/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/hub/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/hub/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/hub/hub.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/codec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/debug.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/jsonrpc/jsonrpc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/server.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/cenkalti/rpc2/state.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/.gitattributesis excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/.golangci.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/charset/charset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/csv/parser.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/json/parser.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/archive.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/audio.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/binary.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/database.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/document.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/font.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/ftyp.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/geo.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/image.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/magic.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/ms_office.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/netpbm.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/ogg.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/text.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/text_csv.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/video.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/magic/zip.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/markup/markup.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/internal/scan/bytes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/mime.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/mimetype.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/supported_mimes.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/gabriel-vasile/mimetype/tree.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/currency/currency.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/logo.pngis excluded by!**/*.png,!**/vendor/**,!vendor/**vendor/github.com/go-playground/locales/rules.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/.travis.ymlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/import_export.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/logo.pngis excluded by!**/*.png,!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/translator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/universal-translator/universal_translator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/MAINTAINERS.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/Makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/baked_in.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/cache.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/country_codes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/currency_codes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/errors.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/field_level.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/logo.pngis excluded by!**/*.png,!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/postcode_regexes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/regexes.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/struct_level.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/translations.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/validator.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/go-playground/validator/v10/validator_instance.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/README.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/kind.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/machine.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/machine.go.rlis excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/makefileis excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/parsing_mode.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/scim.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/scim/schema/type.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/urn.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/leodido/go-urn/urn8141.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/NOTICEis excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/cache/cache.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/cache/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/cache/uuidset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/api.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/api_test_model.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/condition.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/config.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/metrics.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/monitor.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/options.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/client/validation.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/database/database.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/database/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/database/references.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/mapper/info.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/mapper/mapper.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/model/client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/model/database.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/model/model.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/bindings.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/condition.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/error.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/map.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/monitor_select.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/mutation.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/named_uuid.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/notation.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/row.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/rpc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/schema.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/serverdb/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/serverdb/database.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/serverdb/gen.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/serverdb/model.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/set.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/update3.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/updates.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/updates2.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/ovsdb/uuid.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/updates/difference.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/updates/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/updates/merge.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/updates/mutate.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/updates/references.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/ovn-kubernetes/libovsdb/updates/updates.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/sha3/hashes.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/sha3/legacy_hash.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/sha3/legacy_keccakf.gois excluded by!**/vendor/**,!vendor/**vendor/golang.org/x/crypto/sha3/shake.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/.gitignoreis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/CHANGELOG.mdis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/CONTRIBUTING.mdis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/LICENSEis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/Makefileis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/OWNERSis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/README.mdis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/SECURITY_CONTACTSis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/code-of-conduct.mdis excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/error.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/exec.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/fake.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/nftables.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/objects.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/transaction.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/types.gois excluded by!**/vendor/**,!vendor/**vendor/sigs.k8s.io/knftables/util.gois excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (29)
go.modpkg/cmd/run.gopkg/config/c2cc.gopkg/controllers/c2cc/annotation.gopkg/controllers/c2cc/annotation_test.gopkg/controllers/c2cc/controller.gopkg/controllers/c2cc/helpers_test.gopkg/controllers/c2cc/nbdb.gopkg/controllers/c2cc/nbdb_test.gopkg/controllers/c2cc/networkpolicy.gopkg/controllers/c2cc/nftables.gopkg/controllers/c2cc/nftables_test.gopkg/controllers/c2cc/ovn.gopkg/controllers/c2cc/ovn_test.gopkg/controllers/c2cc/policy_routes.gopkg/controllers/c2cc/policy_routes_test.gopkg/controllers/c2cc/routes.gopkg/controllers/c2cc/routes_test.gopkg/controllers/c2cc/service_routes.gopkg/controllers/c2cc/service_routes_test.gotest/assets/c2cc/curl-pod.yamltest/assets/c2cc/hello-microshift.yamltest/bin/scenario.shtest/resources/c2cc.resourcetest/scenarios-bootc/el9/presubmits/el98-src@c2cc.shtest/suites/c2cc/connectivity.robottest/suites/c2cc/infrastructure.robottest/suites/c2cc/reconciliation.robottest/suites/c2cc/sanity.robot
Member
Author
|
/test ? |
Member
Author
|
/test pull-ci-openshift-microshift-main-e2e-aws-tests-bootc-el9 |
Member
Author
|
/test e2e-aws-tests-bootc-arm-el9 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/controllers/c2cc/nftables.go (1)
187-201: Use a resettable timer for the debounce loop.
time.Aftercreates a new timer for every event and can't be canceled when a newer event arrives. A reusabletime.Timerwould cut timer churn and make shutdown cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/c2cc/nftables.go` around lines 187 - 201, Replace the time.After-based debounce in the goroutine that watches rawCh/reconcileCh with a reusable time.Timer: create a timer variable (e.g., debounceTimer *time.Timer) initially nil, on receiving a rawCh event start the timer if nil (time.NewTimer(2*time.Second)) or call debounceTimer.Reset(2*time.Second) if already exists, on the timer channel fire send the "nftables-change" to reconcileCh, and on goroutine shutdown stop the timer and drain its channel if needed (use debounceTimer.Stop() and drain to avoid races). Ensure you reference the existing rawCh and reconcileCh identifiers and replace uses of the local debounce <-chan time.Time with this resettable timer logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/controllers/c2cc/nftables.go`:
- Around line 164-167: The current handler tears down the nftables watcher by
returning on a single sock.Receive() error; change it to keep the watcher alive
by replacing the immediate return with a retry/backoff loop that either
continues on transient errors or attempts to recreate the netlink socket (the
sock used for sock.Receive()) and resume receiving; update the logic around
msgs, _, err := sock.Receive() so errors are logged, waited-on with exponential
backoff, and the socket is reinitialized when necessary instead of returning,
ensuring the watcher goroutine does not exit on the first read error.
In `@pkg/controllers/c2cc/policy_routes.go`:
- Around line 67-98: cleanupRoutes and cleanupRules currently swallow deletion
errors (they log but always return nil); change both functions to collect errors
from netlink.RouteDel and netlink.RuleDel (e.g., store the first error or
aggregate multiple errors) and return a non-nil error when any deletion fails.
Specifically, in cleanupRoutes iterate over routes and on netlink.RouteDel
append or wrap the error (include context like route.Dst, t.table) instead of
only klog.Errorf, and similarly in cleanupRules capture errors from
netlink.RuleDel for rules matching t.priority and t.table and return an
aggregated/wrapped error at the end so callers see cleanup failures.
---
Nitpick comments:
In `@pkg/controllers/c2cc/nftables.go`:
- Around line 187-201: Replace the time.After-based debounce in the goroutine
that watches rawCh/reconcileCh with a reusable time.Timer: create a timer
variable (e.g., debounceTimer *time.Timer) initially nil, on receiving a rawCh
event start the timer if nil (time.NewTimer(2*time.Second)) or call
debounceTimer.Reset(2*time.Second) if already exists, on the timer channel fire
send the "nftables-change" to reconcileCh, and on goroutine shutdown stop the
timer and drain its channel if needed (use debounceTimer.Stop() and drain to
avoid races). Ensure you reference the existing rawCh and reconcileCh
identifiers and replace uses of the local debounce <-chan time.Time with this
resettable timer logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: ca3c2996-c214-452b-92cd-c6c0af3d6b18
📒 Files selected for processing (5)
pkg/controllers/c2cc/annotation.gopkg/controllers/c2cc/nbdb.gopkg/controllers/c2cc/nftables.gopkg/controllers/c2cc/policy_routes.gopkg/controllers/c2cc/service_routes.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controllers/c2cc/annotation.go
- pkg/controllers/c2cc/service_routes.go
Member
Author
|
/test e2e-aws-tests-bootc-arm-el9 |
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/scenarios-bootc/el9/presubmits/el98-src`@c2cc.sh:
- Around line 58-63: The calls to wait_for_greenboot_on_hosts and
wait_for_microshift_to_be_ready currently rely on global errexit; change each
invocation (e.g. wait_for_greenboot_on_hosts "c2cc_pre_greenboot",
wait_for_greenboot_on_hosts "c2cc_greenboot", and
wait_for_microshift_to_be_ready) to perform an explicit return-code check: run
the function, test its exit status with if ! <call>; then emit a clear error
message (e.g. via echo or logger) and exit or return a non-zero status (exit 1)
to fail fast; keep configure_c2cc_host calls unchanged but ensure subsequent
waits follow this explicit-check pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: c46ac14a-0f05-4c99-b70d-121c97d50990
📒 Files selected for processing (2)
pkg/controllers/c2cc/ovn.gotest/scenarios-bootc/el9/presubmits/el98-src@c2cc.sh
✅ Files skipped from review due to trivial changes (1)
- pkg/controllers/c2cc/ovn.go
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/controllers/c2cc/ovn.go (1)
230-252: 💤 Low valueConsider using a backoff for reconnect polling.
The 1-second polling loop at lines 238-244 is functional but will generate constant CPU wake-ups during prolonged outages. An exponential backoff would be gentler, though not critical for MicroShift's typical deployment.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controllers/c2cc/ovn.go` around lines 230 - 252, The reconnect loop spawned in the goroutine using m.nbClient.DisconnectNotify() currently polls m.nbClient.Connected() every 1s; replace that tight fixed-interval loop with an exponential backoff retry (e.g., start at ~1s, double on each failed try up to a max like 30s) and reset the backoff when Connected() returns true. Update the inner select that checks ctx.Done() and the time.After to use the backoff duration, and keep sending "ovn-reconnected" on reconcileCh as before; reference the goroutine, m.nbClient.DisconnectNotify(), m.nbClient.Connected(), ctx.Done(), and reconcileCh when making the change.test/resources/c2cc.resource (1)
205-217: 💤 Low valueInline Python JSON manipulation could fail silently on malformed input.
If
${current}is empty or malformed JSON, the Python one-liner will raise an exception that may produce unclear test failures. Consider adding a sanity check or fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/resources/c2cc.resource` around lines 205 - 217, The inline Python one-liner that builds ${new_value} from ${current} can fail if ${current} is empty or malformed; update the Command On Cluster invocation that generates ${new_value} so the Python snippet defensively handles bad input (check if stdin is empty or not valid JSON and default to an empty list), then append ${foreign_cidr} and print the JSON-sorted result; reference the variables and keywords: Get Node SNAT Annotation (to obtain ${current}), ${current}, ${new_value}, and the python3 one-liner used in Command On Cluster, and ensure the patched oc annotate step still receives the JSON string output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/controllers/c2cc/ovn.go`:
- Around line 230-252: The reconnect loop spawned in the goroutine using
m.nbClient.DisconnectNotify() currently polls m.nbClient.Connected() every 1s;
replace that tight fixed-interval loop with an exponential backoff retry (e.g.,
start at ~1s, double on each failed try up to a max like 30s) and reset the
backoff when Connected() returns true. Update the inner select that checks
ctx.Done() and the time.After to use the backoff duration, and keep sending
"ovn-reconnected" on reconcileCh as before; reference the goroutine,
m.nbClient.DisconnectNotify(), m.nbClient.Connected(), ctx.Done(), and
reconcileCh when making the change.
In `@test/resources/c2cc.resource`:
- Around line 205-217: The inline Python one-liner that builds ${new_value} from
${current} can fail if ${current} is empty or malformed; update the Command On
Cluster invocation that generates ${new_value} so the Python snippet defensively
handles bad input (check if stdin is empty or not valid JSON and default to an
empty list), then append ${foreign_cidr} and print the JSON-sorted result;
reference the variables and keywords: Get Node SNAT Annotation (to obtain
${current}), ${current}, ${new_value}, and the python3 one-liner used in Command
On Cluster, and ensure the patched oc annotate step still receives the JSON
string output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: e8939fdf-a12c-4c50-8eba-742f89423f79
📒 Files selected for processing (4)
pkg/controllers/c2cc/ovn.gotest/resources/c2cc.resourcetest/scenarios-bootc/el9/presubmits/el98-src@c2cc.shtest/suites/c2cc/cleanup.robot
🚧 Files skipped from review as they are similar to previous changes (2)
- test/suites/c2cc/cleanup.robot
- test/scenarios-bootc/el9/presubmits/el98-src@c2cc.sh
c2cc blocks readiness waiting for OVN socket, which might result in MicroShift restart: - MicroShift starts, wants to create kubepods.slice - Systemd queue waits in sequence: first MicroShift, then kubepods.slice - MicroShift does not become ready, no kubepods.slice, no Node, no Pods - No Pods means no OVN socket
Contributor
Code Review: Critical Issues1. BUG: Double
|
Contributor
Code Review: Medium Issues4.
|
Member
Author
|
/retest |
Member
Author
|
/retest |
Member
Author
|
/verified by @pmtk |
|
@pmtk: This PR has been marked as verified by DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
verified
Contributor
|
/assign |
pacevedom
reviewed
May 6, 2026
|
|
||
| // Declaring ready even before init because many of the components it tries to communicate with are not up yet | ||
| // and excessive waiting before readiness can cause them to never become ready resulting in MicroShift restart. | ||
| close(ready) |
Contributor
There was a problem hiding this comment.
Is there any other way to signal readiness? I imagine a pod coming up requiring this before it becomes ready. Is that a possibility?
Member
Author
There was a problem hiding this comment.
There will be readiness custom resource which will report true status of C2CC reconciliation and connectivity
|
|
||
| run_command_on_vm "${host}" "sudo mkdir -p /etc/microshift/config.d" | ||
| run_command_on_vm "${host}" "sudo tee /etc/microshift/config.d/50-c2cc.yaml > /dev/null << EOF | ||
| clusterToCluster: |
Contributor
There was a problem hiding this comment.
I think we need to update the enhancement to match this name instead of c2cc
| ) | ||
|
|
||
| const ( | ||
| c2ccSvcRouteTable = 201 |
Contributor
There was a problem hiding this comment.
According to the enhancement this is configurable, right?
Member
Author
There was a problem hiding this comment.
Not yet. Enhancement is about feature as whole, configuring this will come later.
| ) | ||
|
|
||
| const ( | ||
| c2ccRouteTable = 200 |
Contributor
There was a problem hiding this comment.
According to the enhancement this is configurable, right?
Member
Author
There was a problem hiding this comment.
Not yet. Enhancement is about feature as whole, configuring this will come later.
Contributor
There was a problem hiding this comment.
Shouldnt we have a rhel10 test too?
| ${NAMESPACE} c2cc-test | ||
|
|
||
|
|
||
| *** Test Cases *** |
Contributor
There was a problem hiding this comment.
Are we adding the DNS tests later on, or should they be in this PR?
Member
Author
There was a problem hiding this comment.
Later, together with the implementation
Comment on lines
+181
to
+193
| if addr.IP.To4() != nil { | ||
| ip4 := addr.IP.To4() | ||
| gwIP := make(net.IP, len(ip4)) | ||
| copy(gwIP, ip4) | ||
| gwIP = gwIP.Mask(addr.Mask) | ||
| gwIP[len(gwIP)-1] = 1 | ||
| gateways[netlink.FAMILY_V4] = mgmtPortGateway{ip: gwIP, linkIdx: linkIdx} | ||
| } else if addr.IP.To16() != nil { | ||
| ip6 := make(net.IP, len(addr.IP.To16())) | ||
| copy(ip6, addr.IP.To16()) | ||
| ip6 = ip6.Mask(addr.Mask) | ||
| ip6[len(ip6)-1] = 1 | ||
| gateways[netlink.FAMILY_V6] = mgmtPortGateway{ip: ip6, linkIdx: linkIdx} |
Contributor
There was a problem hiding this comment.
Should we grab the gateway address from the routing table instead? That would be automatic and not subject to changes in ovnk.
Comment on lines
+47
to
+48
| allCIDRs := append([]*net.IPNet{}, rc.ClusterNetwork...) | ||
| allCIDRs = append(allCIDRs, rc.ServiceNetwork...) |
Contributor
There was a problem hiding this comment.
There are four places where we append cluster network and service network cidrs, is it possible to have a function?
openshift-ci-robot
removed
the
verified
pmtk
changed the title
Member
Author
|
/retest |
Contributor
|
@pmtk: 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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When reviewing, skip
Vendor libovsdb & knftablescommitSummary by CodeRabbit
New Features
Tests