USHIFT-6798: C2CC: RemoteClusters.microshift.io/v1alpha1 Custom Resource#6727
USHIFT-6798: C2CC: RemoteClusters.microshift.io/v1alpha1 Custom Resource#6727pmtk wants to merge 3 commits into
Conversation
|
@pmtk: This pull request references USHIFT-6798 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. |
|
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 RemoteCluster CRD, generation tooling, C2CC ProbeInterval config with validation and resolved ProbeIP, a healthcheck manager that reconciles RemoteCluster CRs, asset helpers for CRD deletion, unit tests, and Robot e2e tests. ChangesC2CC RemoteCluster Healthcheck
Sequence DiagramsequenceDiagram
participant ConfigMgr as Config Manager
participant C2CCCtrl as C2CC Controller
participant KubeAPI as Kubernetes API
participant HealthMgr as Healthcheck Manager
ConfigMgr->>ConfigMgr: Validate ProbeInterval (1s–5m)
ConfigMgr->>ConfigMgr: Compute ProbeIP from service CIDR (11th host)
ConfigMgr->>C2CCCtrl: Provide resolved C2CC config
C2CCCtrl->>KubeAPI: Apply RemoteCluster CRD
C2CCCtrl->>KubeAPI: Wait for CRD establishment
C2CCCtrl->>HealthMgr: Create healthcheck manager with typed client
HealthMgr->>KubeAPI: List existing RemoteCluster CRs (label filter)
HealthMgr->>KubeAPI: Create missing RemoteCluster CRs
HealthMgr->>KubeAPI: Update CRs when ProbeTarget/ProbeInterval differ
HealthMgr->>KubeAPI: Delete stale managed CRs
Note over C2CCCtrl: On shutdown
C2CCCtrl->>KubeAPI: Delete RemoteCluster CRD
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@cmd/generate-config/config/config-openapi-spec.json`:
- Around line 206-209: Update the probeInterval property in
config-openapi-spec.json to include an OpenAPI-level default and pattern
validation: add "default": "10s" and add a "pattern" such as
"^[0-9]+(ns|us|µs|ms|s|m|h)$" so consumers validate Go-style duration tokens;
keep the existing description about 1s–5m bounds (and ensure runtime/server-side
enforcement still checks the numeric range if regex cannot express the min/max).
Ensure you modify the probeInterval object (the JSON key "probeInterval") to
include these new fields.
In `@pkg/controllers/c2cc/healthcheck.go`:
- Around line 37-54: Reconcile should not hard-fail on Create errors caused by
an existing same-name CR missing our managed-by label; in the Create error
branch (around h.client.RemoteClusters().Create called for entries in desired)
detect apierrors.IsAlreadyExists(err) and instead fetch the existing object
(h.client.RemoteClusters().Get(ctx, name, metav1.GetOptions{})), add or set the
managedByLabel to managerName on its ObjectMeta.Labels, then Update or Patch
that existing RemoteCluster (handling update errors) so the resource becomes
managed; for non-AlreadyExists errors keep returning the error. Ensure you
import and use k8s.io/apimachinery/pkg/api/errors (apierrors) and reference the
identifiers h.client.RemoteClusters().Create, h.client.RemoteClusters().Get,
h.client.RemoteClusters().Update, desired, managedByLabel, and managerName.
In `@test/suites/c2cc/07-healthcheck.robot`:
- Line 85: The assertion uses a literal "${expected_ip}:8080" which fails for
IPv6 — build the expected host:port using bracketed IPv6 syntax when the IP
contains a colon; e.g. compute an expected_hostport variable by checking
${expected_ip} for ":" and setting expected_hostport to "[${expected_ip}]:8080"
for IPv6 or "${expected_ip}:8080" for IPv4, then compare ${stdout} to that
expected_hostport with Should Be Equal As Strings.
🪄 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: 5bab752a-3c53-473e-928e-f6cd3a4a4b16
⛔ Files ignored due to path filters (22)
pkg/apis/microshift/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/generated/clientset/versioned/clientset.gois excluded by!**/generated/**pkg/generated/clientset/versioned/fake/clientset_generated.gois excluded by!**/generated/**pkg/generated/clientset/versioned/fake/doc.gois excluded by!**/generated/**pkg/generated/clientset/versioned/fake/register.gois excluded by!**/generated/**pkg/generated/clientset/versioned/scheme/doc.gois excluded by!**/generated/**pkg/generated/clientset/versioned/scheme/register.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/microshift/v1alpha1/doc.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/microshift/v1alpha1/fake/doc.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/microshift/v1alpha1/fake/fake_microshift_client.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/microshift/v1alpha1/fake/fake_remotecluster.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/microshift/v1alpha1/generated_expansion.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/microshift/v1alpha1/microshift_client.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/microshift/v1alpha1/remotecluster.gois excluded by!**/generated/**pkg/generated/informers/externalversions/factory.gois excluded by!**/generated/**pkg/generated/informers/externalversions/generic.gois excluded by!**/generated/**pkg/generated/informers/externalversions/internalinterfaces/factory_interfaces.gois excluded by!**/generated/**pkg/generated/informers/externalversions/microshift/interface.gois excluded by!**/generated/**pkg/generated/informers/externalversions/microshift/v1alpha1/interface.gois excluded by!**/generated/**pkg/generated/informers/externalversions/microshift/v1alpha1/remotecluster.gois excluded by!**/generated/**pkg/generated/listers/microshift/v1alpha1/expansion_generated.gois excluded by!**/generated/**pkg/generated/listers/microshift/v1alpha1/remotecluster.gois excluded by!**/generated/**
📒 Files selected for processing (19)
Makefileassets/crd/microshift.io_remoteclusters.yamlcmd/generate-config/config/config-openapi-spec.jsondocs/user/howto_config.mdpackaging/microshift/config.yamlpkg/apis/microshift/v1alpha1/doc.gopkg/apis/microshift/v1alpha1/groupversion_info.gopkg/apis/microshift/v1alpha1/types.gopkg/assets/crd.gopkg/config/c2cc.gopkg/config/c2cc_test.gopkg/config/config.gopkg/controllers/c2cc/controller.gopkg/controllers/c2cc/healthcheck.gopkg/controllers/c2cc/healthcheck_test.goscripts/auto-rebase/assets.yamlscripts/boilerplate.go.txtscripts/generate-crds.shtest/suites/c2cc/07-healthcheck.robot
0f20caf to
fe24b49
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/suites/c2cc/07-healthcheck.robot`:
- Line 95: Update the regexp used in the healthcheck test so it matches the
entire probe interval string; replace the pattern in the assertion that checks
${interval} (the line using Should Match Regexp) from ^[0-9]+(s|m|h) to an
anchored pattern ^[0-9]+(s|m|h)$ so values like "10sfoo" no longer pass.
🪄 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: fb156625-8c3b-42df-8034-069ed4c299f7
📒 Files selected for processing (6)
pkg/assets/crd.gopkg/config/c2cc_test.gopkg/controllers/c2cc/controller.gopkg/controllers/c2cc/healthcheck.gopkg/controllers/c2cc/healthcheck_test.gotest/suites/c2cc/07-healthcheck.robot
fe24b49 to
638d177
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@pkg/config/c2cc_test.go`:
- Around line 863-896: The tests call cfg.fillDefaults() but ignore its returned
error, which can mask setup failures; update each test setup (in TestC2CC_*
blocks) to capture the error from Config.fillDefaults() and assert it succeeded
before proceeding (e.g., call err := cfg.fillDefaults() and assert/require no
error), so subsequent calls to cfg.incorporateUserSettings and assertions on
cfg.C2CC.ProbeInterval run only if defaults initialization succeeded.
🪄 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: cec90440-4564-4dee-8100-9fd0b40cbb71
📒 Files selected for processing (10)
docs/user/howto_config.mdpackaging/microshift/config.yamlpkg/assets/crd.gopkg/config/c2cc.gopkg/config/c2cc_test.gopkg/config/config.gopkg/controllers/c2cc/controller.gopkg/controllers/c2cc/healthcheck.gopkg/controllers/c2cc/healthcheck_test.gotest/suites/c2cc/07-healthcheck.robot
✅ Files skipped from review due to trivial changes (1)
- docs/user/howto_config.md
| cfg := &Config{} | ||
| cfg.fillDefaults() | ||
| assert.Equal(t, "10s", cfg.C2CC.ProbeInterval) | ||
| } | ||
|
|
||
| func TestC2CC_IncorporateUserSettings(t *testing.T) { | ||
| t.Run("user overrides probe interval", func(t *testing.T) { | ||
| cfg := &Config{} | ||
| cfg.fillDefaults() | ||
|
|
||
| user := &Config{ | ||
| C2CC: C2CC{ | ||
| ProbeInterval: "30s", | ||
| }, | ||
| } | ||
| cfg.incorporateUserSettings(user) | ||
| assert.Equal(t, "30s", cfg.C2CC.ProbeInterval) | ||
| }) | ||
|
|
||
| t.Run("user sets remoteClusters without probeInterval preserves default", func(t *testing.T) { | ||
| cfg := &Config{} | ||
| cfg.fillDefaults() | ||
|
|
||
| user := &Config{ | ||
| C2CC: C2CC{ | ||
| RemoteClusters: []RemoteCluster{{ | ||
| NextHop: "10.100.0.2", | ||
| ClusterNetwork: []string{"10.45.0.0/16"}, | ||
| ServiceNetwork: []string{"10.46.0.0/16"}, | ||
| }}, | ||
| }, | ||
| } | ||
| cfg.incorporateUserSettings(user) | ||
| assert.Equal(t, "10s", cfg.C2CC.ProbeInterval) |
There was a problem hiding this comment.
Assert fillDefaults() success in test setup.
Line 864, Line 871, and Line 884 ignore the returned error. If default initialization fails, later assertions can fail with misleading signals.
Suggested fix
func TestC2CC_ProbeIntervalDefault(t *testing.T) {
cfg := &Config{}
- cfg.fillDefaults()
+ require.NoError(t, cfg.fillDefaults())
assert.Equal(t, "10s", cfg.C2CC.ProbeInterval)
}
func TestC2CC_IncorporateUserSettings(t *testing.T) {
t.Run("user overrides probe interval", func(t *testing.T) {
cfg := &Config{}
- cfg.fillDefaults()
+ require.NoError(t, cfg.fillDefaults())
user := &Config{
C2CC: C2CC{
ProbeInterval: "30s",
@@
t.Run("user sets remoteClusters without probeInterval preserves default", func(t *testing.T) {
cfg := &Config{}
- cfg.fillDefaults()
+ require.NoError(t, cfg.fillDefaults())
user := &Config{
C2CC: C2CC{🤖 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 `@pkg/config/c2cc_test.go` around lines 863 - 896, The tests call
cfg.fillDefaults() but ignore its returned error, which can mask setup failures;
update each test setup (in TestC2CC_* blocks) to capture the error from
Config.fillDefaults() and assert it succeeded before proceeding (e.g., call err
:= cfg.fillDefaults() and assert/require no error), so subsequent calls to
cfg.incorporateUserSettings and assertions on cfg.C2CC.ProbeInterval run only if
defaults initialization succeeded.
638d177 to
9da5a21
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/suites/c2cc/healthcheck.robot (1)
91-95: 💤 Low valueDuration regex may be too narrow.
The pattern
^[0-9]+(s|m|h)$matches simple durations like "30s" but won't match compound Go durations like "1m30s" or millisecond units like "500ms". If the controller can produce such formats, this test could spuriously fail.Consider
^[0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h)+$or simply assert non-empty if strict format isn't required.🤖 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 `@test/suites/c2cc/healthcheck.robot` around lines 91 - 95, The duration regex in the healthcheck test around the ${interval} value is too narrow and rejects compound or millisecond Go durations; update the assertion in the test that follows the Oc On Cluster call (the variable ${interval} populated by the oc get remoteclusters... jsonpath) to use a more permissive regex such as one that accepts optional decimals and units (e.g. allow ns|us|µs|ms|s|m|h and compound forms) or simply assert non-empty; replace the current Should Match Regexp line with the broader pattern (or change to only Should Not Be Empty) to avoid spurious failures.
🤖 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 `@test/suites/c2cc/healthcheck.robot`:
- Around line 91-95: The duration regex in the healthcheck test around the
${interval} value is too narrow and rejects compound or millisecond Go
durations; update the assertion in the test that follows the Oc On Cluster call
(the variable ${interval} populated by the oc get remoteclusters... jsonpath) to
use a more permissive regex such as one that accepts optional decimals and units
(e.g. allow ns|us|µs|ms|s|m|h and compound forms) or simply assert non-empty;
replace the current Should Match Regexp line with the broader pattern (or change
to only Should Not Be Empty) to avoid spurious failures.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 0646dc20-4db3-4836-b3e6-582726ddbdc2
📒 Files selected for processing (6)
pkg/assets/crd.gopkg/config/c2cc_test.gopkg/controllers/c2cc/controller.gopkg/controllers/c2cc/healthcheck.gopkg/controllers/c2cc/healthcheck_test.gotest/suites/c2cc/healthcheck.robot
9da5a21 to
32ebff5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
pkg/config/c2cc_test.go (1)
863-864:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert
fillDefaults()success in test setup.Line 864, Line 871, and Line 884 ignore
fillDefaults()errors; this can hide setup failures and mislead assertions.Suggested fix
func TestC2CC_ProbeIntervalDefault(t *testing.T) { cfg := &Config{} - cfg.fillDefaults() + require.NoError(t, cfg.fillDefaults()) assert.Equal(t, "10s", cfg.C2CC.ProbeInterval) } func TestC2CC_IncorporateUserSettings(t *testing.T) { t.Run("user overrides probe interval", func(t *testing.T) { cfg := &Config{} - cfg.fillDefaults() + require.NoError(t, cfg.fillDefaults()) user := &Config{ C2CC: C2CC{ ProbeInterval: "30s", @@ t.Run("user sets remoteClusters without probeInterval preserves default", func(t *testing.T) { cfg := &Config{} - cfg.fillDefaults() + require.NoError(t, cfg.fillDefaults()) user := &Config{ C2CC: C2CC{Also applies to: 870-871, 883-884
🤖 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 `@pkg/config/c2cc_test.go` around lines 863 - 864, The test setup calls cfg.fillDefaults() but ignores its returned error, which can hide failures; update the test in pkg/config/c2cc_test.go to assert that fillDefaults() returns nil (e.g., fail the test or use t.Fatal/t.Fatalf) after calling cfg.fillDefaults() for the occurrences around the Config variable (refs: Config and fillDefaults()) so any setup error is surfaced immediately.
🤖 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.
Inline comments:
In `@test/suites/c2cc/healthcheck.robot`:
- Line 95: The current regex for ${interval} only accepts single integer units
(e.g., 10s) but probeInterval can be any Go duration (fractional and compound
like 1.5s or 1m30s); update the Robot assertion replacing ^[0-9]+(s|m|h)$ with a
pattern that accepts repeated numeric (with optional fractional) + Go unit
groups, e.g. ^([0-9]+(\.[0-9]+)?(ns|us|µs|ms|s|m|h))+$ so Should Match Regexp
${interval} validates durations parsed by time.ParseDuration.
---
Duplicate comments:
In `@pkg/config/c2cc_test.go`:
- Around line 863-864: The test setup calls cfg.fillDefaults() but ignores its
returned error, which can hide failures; update the test in
pkg/config/c2cc_test.go to assert that fillDefaults() returns nil (e.g., fail
the test or use t.Fatal/t.Fatalf) after calling cfg.fillDefaults() for the
occurrences around the Config variable (refs: Config and fillDefaults()) so any
setup error is surfaced immediately.
🪄 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: b7960763-96b2-4834-a9de-9133cf3b3d07
⛔ Files ignored due to path filters (22)
pkg/apis/microshift/v1alpha1/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/generated/clientset/versioned/clientset.gois excluded by!**/generated/**pkg/generated/clientset/versioned/fake/clientset_generated.gois excluded by!**/generated/**pkg/generated/clientset/versioned/fake/doc.gois excluded by!**/generated/**pkg/generated/clientset/versioned/fake/register.gois excluded by!**/generated/**pkg/generated/clientset/versioned/scheme/doc.gois excluded by!**/generated/**pkg/generated/clientset/versioned/scheme/register.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/microshift/v1alpha1/doc.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/microshift/v1alpha1/fake/doc.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/microshift/v1alpha1/fake/fake_microshift_client.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/microshift/v1alpha1/fake/fake_remotecluster.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/microshift/v1alpha1/generated_expansion.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/microshift/v1alpha1/microshift_client.gois excluded by!**/generated/**pkg/generated/clientset/versioned/typed/microshift/v1alpha1/remotecluster.gois excluded by!**/generated/**pkg/generated/informers/externalversions/factory.gois excluded by!**/generated/**pkg/generated/informers/externalversions/generic.gois excluded by!**/generated/**pkg/generated/informers/externalversions/internalinterfaces/factory_interfaces.gois excluded by!**/generated/**pkg/generated/informers/externalversions/microshift/interface.gois excluded by!**/generated/**pkg/generated/informers/externalversions/microshift/v1alpha1/interface.gois excluded by!**/generated/**pkg/generated/informers/externalversions/microshift/v1alpha1/remotecluster.gois excluded by!**/generated/**pkg/generated/listers/microshift/v1alpha1/expansion_generated.gois excluded by!**/generated/**pkg/generated/listers/microshift/v1alpha1/remotecluster.gois excluded by!**/generated/**
📒 Files selected for processing (19)
Makefileassets/crd/microshift.io_remoteclusters.yamlcmd/generate-config/config/config-openapi-spec.jsondocs/user/howto_config.mdpackaging/microshift/config.yamlpkg/apis/microshift/v1alpha1/doc.gopkg/apis/microshift/v1alpha1/groupversion_info.gopkg/apis/microshift/v1alpha1/types.gopkg/assets/crd.gopkg/config/c2cc.gopkg/config/c2cc_test.gopkg/config/config.gopkg/controllers/c2cc/controller.gopkg/controllers/c2cc/healthcheck.gopkg/controllers/c2cc/healthcheck_test.goscripts/auto-rebase/assets.yamlscripts/boilerplate.go.txtscripts/generate-crds.shtest/suites/c2cc/healthcheck.robot
✅ Files skipped from review due to trivial changes (2)
- pkg/apis/microshift/v1alpha1/doc.go
- docs/user/howto_config.md
32ebff5 to
467958f
Compare
467958f to
f6e1153
Compare
|
@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. |
Summary by CodeRabbit
New Features
Tests
Documentation
Chores