OCPEDGE-2084: Add PacemakerStatus CRD for two-node fencing #2544
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jaypoulz: This pull request references OCPEDGE-2084 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 "4.21.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. |
|
Hello @jaypoulz! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XL
openshift-ci
bot
added
the
do-not-merge/invalid-owners-file
|
@jaypoulz: This pull request references OCPEDGE-2084 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 "4.21.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. |
jaypoulz
force-pushed
the
OCPEDGE-2084
branch
from
58218ce to
96e327f
Compare
openshift-ci
bot
removed
the
do-not-merge/invalid-owners-file
jaypoulz
force-pushed
the
OCPEDGE-2084
branch
4 times, most recently
from
2ba442d to
29b9fec
Compare
|
@jaypoulz thank you for the PR, do you mind making the CI happy? |
jaypoulz
force-pushed
the
OCPEDGE-2084
branch
from
29b9fec to
26f7821
Compare
|
Hi @saschagrunert :) Working on it! :D |
|
A few open questions I have:
That said, it doesn't work like a normal config - there's no spec and it shouldn't be created during bootstrap. The CRD just needs to be present when the CEO runs an cronjob to post an update to it.
|
jaypoulz
force-pushed
the
OCPEDGE-2084
branch
2 times, most recently
from
b0ff230 to
1b57b09
Compare
openshift-ci
bot
added
size/XXL
jaypoulz
force-pushed
the
OCPEDGE-2084
branch
4 times, most recently
from
b9b727f to
fdd53e9
Compare
|
Yeah, I'll ignore the CI failures for now, running
I'm new to API review, but my gut feeling tells me that a dedicated
You can also try to run it in a container by
Do you mind elaborating on that? Do you mean generating the code for the unions? API docs ref: https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#writing-a-union-in-go @jaypoulz is there an OpenShift enhancement available for this change? |
etcd/tnf/v1alpha1/tests/pacemakerstatuses.tnf.etcd.openshift.io/DualReplica.yaml
Outdated
Show resolved
Hide resolved
jaypoulz
force-pushed
the
OCPEDGE-2084
branch
from
e7c826c to
39086fa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
openapi/openapi.json (3)
10586-10586: Fix typo in profileCustomizations deprecation note.The text has a typo: "by'" should be "by". The sentence should read "and is enabled by default in the cluster" instead of "and is enabled by' default in the cluster".
Apply this diff:
- "description": "profileCustomizations contains configuration for modifying the default behavior of existing scheduler profiles. Deprecated: no longer needed, since DRA is GA starting with 4.21, and is enabled by' default in the cluster, this field will be removed in 4.24.", + "description": "profileCustomizations contains configuration for modifying the default behavior of existing scheduler profiles. Deprecated: no longer needed, since DRA is GA starting with 4.21, and is enabled by default in the cluster, this field will be removed in 4.24.",Note: This file is auto-generated. The fix must be applied to the source Go code comment in the corresponding types file, then regenerated.
14014-14014: Remove duplicate text in PacemakerCluster Compatibility level description.The description contains duplicated text: "These capabilities should not be used by applications needing long term support. These capabilities should not be used by applications needing long term support." The phrase should appear only once.
Apply this diff:
- "description": "PacemakerCluster represents the current state of the pacemaker cluster as reported by the pcs status command. PacemakerCluster is a cluster-scoped singleton resource. The name of this instance is \"cluster\". This resource provides a view into the health and status of a pacemaker-managed cluster in TNF deployments.\n\nCompatibility level 4: No compatibility is provided, the API can change at any point for any reason. These capabilities should not be used by applications needing long term support. These capabilities should not be used by applications needing long term support.", + "description": "PacemakerCluster represents the current state of the pacemaker cluster as reported by the pcs status command. PacemakerCluster is a cluster-scoped singleton resource. The name of this instance is \"cluster\". This resource provides a view into the health and status of a pacemaker-managed cluster in TNF deployments.\n\nCompatibility level 4: No compatibility is provided, the API can change at any point for any reason. These capabilities should not be used by applications needing long term support.",Note: This file is auto-generated. Fix the source Go code comment in
etcd/v1alpha1/types_pacemakercluster.go, then regenerate.
14040-14040: Remove duplicate text in PacemakerClusterList Compatibility level description.The description contains the same duplicated phrase as PacemakerCluster (line 14014). Apply the same fix here.
Apply this diff:
- "description": "PacemakerClusterList contains a list of PacemakerCluster objects. PacemakerCluster is a cluster-scoped singleton resource; only one instance named \"cluster\" may exist. This list type exists only to satisfy Kubernetes API conventions.\n\nCompatibility level 4: No compatibility is provided, the API can change at any point for any reason. These capabilities should not be used by applications needing long term support. These capabilities should not be used by applications needing long term support.", + "description": "PacemakerClusterList contains a list of PacemakerCluster objects. PacemakerCluster is a cluster-scoped singleton resource; only one instance named \"cluster\" may exist. This list type exists only to satisfy Kubernetes API conventions.\n\nCompatibility level 4: No compatibility is provided, the API can change at any point for any reason. These capabilities should not be used by applications needing long term support.",Note: This file is auto-generated. Fix the source Go code comment and regenerate.
etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml (1)
255-258: Critical: Fix ipAddress CEL validation syntax.The CEL rule
ip.isCanonical(self)is invalid. Per Kubernetes CEL IP library,isCanonical()must be called as a member function on the IP object.Apply this fix to the source type annotation (likely in
types_pacemakercluster.go) and regenerate:- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
255-258: Critical: Fix ipAddress CEL validation syntax.The CEL rule
ip.isCanonical(self)is invalid and will fail at runtime. This must be corrected in the source type annotation and regenerated across all CRD variants.- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (1)
255-258: Critical: Fix ipAddress CEL validation syntax.The CEL rule
ip.isCanonical(self)must be corrected toip(self).isCanonical()in the source type definition and all generated CRD manifests regenerated.- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (1)
23-29: Deduplicate the compatibility sentence in the description.Lines [28]–[29] repeat the “These capabilities should not be used by applications needing long term support.” sentence; keep a single instance to avoid redundancy and keep the CRD docs tight.
🧹 Nitpick comments (1)
etcd/v1alpha1/Makefile (1)
1-3: Consider adding standard Makefile targets for robustness.The Makefile currently defines only a
testtarget. Standard Makefile conventions expectall(default target) andcleantargets. While the current minimal setup works for focused testing, adding these targets would align with Makefile best practices and prevent issues if CI or build scripts expect them.If you'd like, consider adding:
.PHONY: all clean test all: test clean: @echo "Nothing to clean" test: make -C ../../tests test GINKGO_EXTRA_ARGS=--focus="etcd.openshift.io/v1alpha1"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (23)
api(1 hunks)etcd/README.md(1 hunks)etcd/install.go(1 hunks)etcd/v1alpha1/Makefile(1 hunks)etcd/v1alpha1/doc.go(1 hunks)etcd/v1alpha1/register.go(1 hunks)etcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml(1 hunks)etcd/v1alpha1/types_pacemakercluster.go(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/doc.go(1 hunks)etcd/v1alpha1/zz_generated.deepcopy.go(1 hunks)etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yaml(1 hunks)etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml(1 hunks)etcd/v1alpha1/zz_generated.swagger_doc_generated.go(1 hunks)hack/update-payload-crds.sh(1 hunks)install.go(2 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(6 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- etcd/README.md
🚧 Files skipped from review as they are similar to previous changes (8)
- api
- openapi/generated_openapi/zz_generated.openapi.go
- hack/update-payload-crds.sh
- etcd/v1alpha1/doc.go
- install.go
- payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml
- etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yaml
- etcd/v1alpha1/zz_generated.deepcopy.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
etcd/v1alpha1/register.goetcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yamletcd/v1alpha1/zz_generated.swagger_doc_generated.goetcd/v1alpha1/zz_generated.crd-manifests/doc.goetcd/install.goetcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yamletcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yamletcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yamlpayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yamletcd/v1alpha1/types_pacemakercluster.goopenapi/openapi.jsonetcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yamletcd/v1alpha1/Makefile
🧬 Code graph analysis (3)
etcd/v1alpha1/zz_generated.swagger_doc_generated.go (1)
etcd/v1alpha1/types_pacemakercluster.go (4)
PacemakerCluster(93-106)PacemakerClusterList(189-198)PacemakerClusterNodeStatus(143-180)PacemakerClusterStatus(112-139)
etcd/install.go (3)
etcd/v1alpha1/register.go (4)
Install(14-14)AddToScheme(21-21)Resource(26-28)GroupName(10-10)install.go (1)
Install(119-119)authorization/v1/types.go (1)
GroupKind(21-21)
etcd/v1alpha1/types_pacemakercluster.go (1)
etcd/v1alpha1/zz_generated.swagger_doc_generated.go (3)
PacemakerClusterStatus(54-56)PacemakerClusterNodeStatus(43-45)PacemakerCluster(20-22)
🪛 checkmake (0.2.2)
etcd/v1alpha1/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
🔇 Additional comments (9)
openapi/openapi.json (2)
14013-14131: PacemakerCluster OpenAPI schema definitions look sound.The new OpenAPI definitions for PacemakerCluster (lines 14013–14131) properly capture the status-only resource structure, including per-node health conditions for kubelet, etcd, and fencing resources. Field descriptions are clear and validation constraints are appropriate (e.g., required fields, IP address format, node name validation, timestamp ordering). The singleton pattern is correctly documented.
Once you fix the duplicate compatibility text (in lines 14014 and 14040 above), this section will be ready.
24874-25000: Verify scope of OSImageStream changes.Hunks 3–6 (lines 24874–25000) introduce or modify definitions for OSImageStream, OSImageStreamSet, and related status types. These changes appear orthogonal to the PR objective of adding the PacemakerStatus CRD. Clarify whether these changes are intentional as part of this PR or should be excluded.
etcd/v1alpha1/register.go (1)
1-39: LGTM!The API registration scaffolding is well-structured and follows standard Kubernetes conventions for scheme registration, with proper GroupVersion construction and type registration.
etcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml (1)
1-435: LGTM!Comprehensive test coverage validating both success scenarios (single/dual node, IPv6) and error cases (invalid IPs, loopback, node count constraints, empty names).
etcd/install.go (1)
1-26: LGTM!The etcd group installation correctly wires up v1alpha1.Install and provides standard helper functions following OpenShift API patterns.
etcd/v1alpha1/zz_generated.swagger_doc_generated.go (1)
1-58: LGTM!The generated Swagger documentation accurately reflects all PacemakerCluster type definitions with no phantom fields. Field descriptions are clear and comprehensive.
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (1)
30-351: Schema and CEL validations look consistent with the Go types.The
statusshape (includingexpectedNodeCount,lastUpdated,nodes[]), thenodeslist constraints (map keyed byname,minItems=1,maxItems=2), the IP address validation, and the singletonmetadata.name == 'cluster'rule all align withPacemakerClusterStatus/PacemakerClusterNodeStatusinetcd/v1alpha1/types_pacemakercluster.go. I don’t see further issues in this CRD.etcd/v1alpha1/types_pacemakercluster.go (1)
7-198: PacemakerCluster types and validations look well-shaped and aligned with the CRDs.The condition/reason constants, singleton
PacemakerClusterdefinition,lastUpdatedXValidation, node list constraints, and per-node health/IP validation are consistent with the generated CRDs and follow Kubernetes API conventions; no issues from my side.payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (1)
30-351: DevPreview CRD variant matches the Go types and TechPreview schema.The DevPreview manifest mirrors the TechPreview CRD: same status schema, node constraints, IP CEL validation,
lastUpdatedrule, and singletonmetadata.name == 'cluster'enforcement. Aside from the small description duplication noted separately, this looks good.
...1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml
Outdated
Show resolved
Hide resolved
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml
Outdated
Show resolved
Hide resolved
jaypoulz
force-pushed
the
OCPEDGE-2084
branch
from
39086fa to
64efd90
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
etcd/README.md (1)
1-115: Consolidate docs; avoid duplicating v1alpha1 README.This largely repeats the v1alpha1 README. Keep a brief overview here and point to the canonical v1alpha1 docs to prevent drift.
-# etcd.openshift.io API Group -... -### Usage -... - way to determine overall health. +# etcd.openshift.io API Group + +This directory contains the etcd API group. For detailed PacemakerCluster (v1alpha1) documentation, see: + +- etcd/v1alpha1/README.md (canonical spec and validation details) + +This file intentionally stays brief to avoid duplication; update the v1alpha1 README for changes to the API surface.etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
443-446: Fix CEL IP canonical check (wrong call form).The CEL validation rule uses
ip.isCanonical(self)which is invalid syntax. Theip()function returns an IP object; callisCanonical()on that object, not as a static method. Change toip(self).isCanonical()to match the correct API and be consistent withip(self).isGlobalUnicast()on the same line.- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()Update the kubebuilder marker in
etcd/v1alpha1/types_pacemakercluster.goand regenerate all CRD variants to maintain consistency.payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (1)
443-445: Fix IP validation CEL syntax bug.Same issue as in TechPreviewNoUpgrade variant:
ip.isCanonical(self)should beip(self).isCanonical()per JoelSpeed's clarification.Apply this fix:
- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (1)
443-445: Fix IP validation CEL syntax bug.Per reviewer JoelSpeed's clarification in past comments,
ip.isCanonical(self)is a bug in the IP library that was missed during review. The correct syntax isip(self).isCanonical()to match the pattern used for other IP methods likeisGlobalUnicast().Apply this fix:
- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()openapi/openapi.json (2)
14014-14014: Verify duplicate text resolution in Compatibility descriptions.Past review flagged duplicate text: "These capabilities should not be used by applications needing long term support." appearing twice in each description. The provided code shows each sentence appearing once. If this has been resolved in the source Go types (e.g., etcd/v1alpha1/types_pacemakercluster.go), no further action is needed. If duplication persists in the source, ensure the source comment is corrected so regeneration produces the correct schema.
#!/bin/bash # Verify that the source Go type comments are free of duplication rg -n "Compatibility level 4" etcd/v1alpha1/types_pacemakercluster.go -A 2 -B 2Also applies to: 14040-14040
10586-10586: Fix typo in profileCustomizations deprecation note.The text has a typo: "by'" should be "by". The sentence should read "and is enabled by default in the cluster".
- "description": "profileCustomizations contains configuration for modifying the default behavior of existing scheduler profiles. Deprecated: no longer needed, since DRA is GA starting with 4.21, and is enabled by' default in the cluster, this field will be removed in 4.24.", + "description": "profileCustomizations contains configuration for modifying the default behavior of existing scheduler profiles. Deprecated: no longer needed, since DRA is GA starting with 4.21, and is enabled by default in the cluster, this field will be removed in 4.24.",
🧹 Nitpick comments (2)
etcd/v1alpha1/Makefile (1)
1-3: Add standard Make targets (all, clean) for tooling and consistency.Helps local/CI tooling (e.g., checkmake) and mirrors other API group Makefiles.
-.PHONY: test +.PHONY: all clean test +all: test + test: make -C ../../tests test GINKGO_EXTRA_ARGS=--focus="etcd.openshift.io/v1alpha1" + +clean: + @trueetcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (1)
443-446: Use consistent CEL syntax for IP method calls.The current rule mixes library function form (
ip.isCanonical(self)) with method form (ip(self).isGlobalUnicast()). For consistency, use the method form throughout:- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()Both forms are valid in CEL, but the method form is preferred for uniformity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (23)
api(1 hunks)etcd/README.md(1 hunks)etcd/install.go(1 hunks)etcd/v1alpha1/Makefile(1 hunks)etcd/v1alpha1/doc.go(1 hunks)etcd/v1alpha1/register.go(1 hunks)etcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml(1 hunks)etcd/v1alpha1/types_pacemakercluster.go(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/doc.go(1 hunks)etcd/v1alpha1/zz_generated.deepcopy.go(1 hunks)etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yaml(1 hunks)etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml(1 hunks)etcd/v1alpha1/zz_generated.swagger_doc_generated.go(1 hunks)hack/update-payload-crds.sh(1 hunks)install.go(2 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(6 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- etcd/v1alpha1/zz_generated.crd-manifests/doc.go
🚧 Files skipped from review as they are similar to previous changes (8)
- api
- hack/update-payload-crds.sh
- install.go
- etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yaml
- openapi/generated_openapi/zz_generated.openapi.go
- etcd/install.go
- etcd/v1alpha1/register.go
- etcd/v1alpha1/doc.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
etcd/README.mdetcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yamletcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yamletcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yamletcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yamletcd/v1alpha1/zz_generated.deepcopy.gopayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yamletcd/v1alpha1/zz_generated.swagger_doc_generated.gopayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yamletcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yamlopenapi/openapi.jsonetcd/v1alpha1/types_pacemakercluster.goetcd/v1alpha1/Makefile
🧬 Code graph analysis (2)
etcd/v1alpha1/zz_generated.deepcopy.go (2)
etcd/v1alpha1/types_pacemakercluster.go (5)
PacemakerCluster(379-392)PacemakerClusterStatus(398-433)PacemakerClusterList(540-549)PacemakerClusterNodeStatus(437-495)PacemakerClusterResourceStatus(504-531)etcd/v1alpha1/zz_generated.swagger_doc_generated.go (5)
PacemakerCluster(20-22)PacemakerClusterStatus(64-66)PacemakerClusterList(30-32)PacemakerClusterNodeStatus(44-46)PacemakerClusterResourceStatus(53-55)
etcd/v1alpha1/zz_generated.swagger_doc_generated.go (1)
etcd/v1alpha1/types_pacemakercluster.go (5)
PacemakerCluster(379-392)PacemakerClusterList(540-549)PacemakerClusterNodeStatus(437-495)PacemakerClusterResourceStatus(504-531)PacemakerClusterStatus(398-433)
🪛 checkmake (0.2.2)
etcd/v1alpha1/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
🔇 Additional comments (7)
etcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml (1)
318-361: Re-run tests after CEL rule change; expected errors remain valid.The error messages don’t change, but the canonical check implementation does. After regenerating CRDs, re-run this suite to confirm IP pass/fail cases still match expectations.
etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (1)
443-446: Call isCanonical() on the IP object for consistency.Both
ip.isCanonical(self)andip(self).isCanonical()are valid CEL approaches, but using the object method ensures consistency with related CRD definitions. Apply the fix:- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()etcd/v1alpha1/zz_generated.deepcopy.go (1)
1-167: Generated deepcopy code is correct.The autogenerated deepcopy implementations properly handle all PacemakerCluster types with correct pointer allocation, slice copying, and nested struct handling. Past issues flagged in reviews have been resolved.
etcd/v1alpha1/zz_generated.swagger_doc_generated.go (1)
1-68: Generated Swagger documentation is correct.All swagger doc maps accurately reflect the current PacemakerCluster type definitions. Past issues with non-existent fields have been resolved.
openapi/openapi.json (2)
14013-14162: Schema definitions for PacemakerCluster v1alpha1 look well-structured.The new OpenAPI definitions for etcd/v1alpha1 PacemakerCluster and its related types (PacemakerClusterList, PacemakerClusterNodeStatus, PacemakerClusterResourceStatus, PacemakerClusterStatus) follow Kubernetes API conventions:
- Proper use of required fields, optional fields, and references.
- Conditions use correct list semantics (x-kubernetes-list-type: map, x-kubernetes-list-map-keys).
- Descriptions are detailed and include validation guidance (e.g., IPv4/IPv6 canonicalization, node count bounds, RFC 1123 subdomain constraints).
- Resource status hierarchy (cluster → nodes → resources) is clear and maintainable.
24968-25018: OSImageStreamSet and OSImageStreamStatus schema updates look correct.The new OSImageStreamSet definition and OSImageStreamStatus required field declarations are well-documented with appropriate validation guidance (RFC 1123 subdomain constraints, digest formats, item count bounds). No issues detected.
etcd/v1alpha1/types_pacemakercluster.go (1)
363-549: Overall PacemakerCluster type and condition modeling looks solidThe cluster-/node-/resource-level condition types, reason constants, singleton enforcement (
self.metadata.name == 'cluster'), and status subresource/validation wiring are idiomatic and align well with Kubernetes API conventions for status-only resources. I don’t see other major correctness or maintainability issues in this file.
...zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml
Outdated
Show resolved
Hide resolved
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml
Outdated
Show resolved
Hide resolved
saschagrunert
left a comment
saschagrunert
left a comment
There was a problem hiding this comment.
I just have 3 nits, otherwise LGTM 👍
| // +required | ||
| Name string `json:"name,omitempty"` | ||
|
|
||
| // ipAddress is the canonical IPv4 or IPv6 address of the node. It must be a valid canonical global unicast IPv4 |
There was a problem hiding this comment.
Be careful with the word canonical here. This could mean a "canonical IP" as in an IP that is in canonical form, or it could mean the IP for the default route on the node depending on who you ask.
Also, which IP address is this? Nodes may have more than one, and quite often do. Does pacemaker care? Does it require/bind a specific address as part of its operations?
Have you considered making this a list to allow for multiple IP addresses? We have a bunch of examples of IP address lists that allow two addresses, where if there is more than one, each must be from a different IP family (4 vs 6)
There was a problem hiding this comment.
This is the IP address that will be used for the etcd peer URLs. I don't think it's a valid state to have more than one, but I'll ask @clobrano
There was a problem hiding this comment.
This turned out to be a deeper rabbit hole than expected. I'll check in with the etcd guys, but it looks like we will want this to be list, but I want to understand how etcd endpoints are handled for dualstack clusters.
There was a problem hiding this comment.
After conversations with the SMEs, I made this a list with a minimum of 1. CoroSync supports having multiple IPv4 and IPv6 addresses, so I just gave an upper bound of 8.
Etcd only uses the first listed address for the node, even in dualstack clusters. Our current implementation follows that convention, so the expectation is always 1, but this alllows for future flexibility.
jaypoulz
force-pushed
the
OCPEDGE-2084
branch
from
64efd90 to
74edc40
Compare
openshift-merge-robot
added
the
needs-rebase
jaypoulz
force-pushed
the
OCPEDGE-2084
branch
from
74edc40 to
b9dbd77
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (9)
etcd/README.md (1)
1-140: Documentation is comprehensive and well-structured.The README effectively documents the PacemakerCluster CRD including conditions, validation rules, and status structure. Note that a past reviewer flagged potential duplication with content in the v1alpha1 directory—consider consolidating to avoid maintenance drift.
etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml (1)
255-258: CEL IP validation mixes function and member forms.The validation rule uses
ip.isCanonical(self)(function form) butip(self).isGlobalUnicast()(member form). Per Kubernetes CEL IP library documentation, use the member form consistently:- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
255-258: CEL IP validation uses incorrect function form.Same issue as the DualReplica manifest—
ip.isCanonical(self)should use the member formip(self).isCanonical()for consistency with the Kubernetes CEL IP library API:- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()This was flagged in a past review and appears to still need correction.
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (1)
255-258: Same CEL IP validation issue as zz_generated CRD.The rule at line 258 has the same
ip.isCanonical(self)syntax issue. This will be fixed when the source type is updated and CRDs are regenerated.payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
255-258: Same CEL IP validation issue.This CustomNoUpgrade variant has the same
ip.isCanonical(self)issue at line 258. Will be resolved with the source type fix.etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (1)
255-258: CEL IP validation syntax inconsistency persists.The rule mixes
ip.isCanonical(self)withip(self).isGlobalUnicast(). JoelSpeed confirmed in previous review thatip.isCanonical(self)is incorrect and should beip(self).isCanonical()per the Kubernetes CEL IP library pattern.This appears to be generated from source types. Please update the kubebuilder marker in
types_pacemakercluster.goand regenerate.x-kubernetes-validations: - message: ipAddress must be a valid canonical global unicast IPv4 or IPv6 address - rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (1)
246-258: Critical: Fix the CEL syntax for IP validation.The CEL rule on line 258 still uses the incorrect syntax
ip.isCanonical(self). Per the Kubernetes CEL IP library documentation,isCanonical()must be called as a member function on the IP object returned byip(), not as a library function.Apply this diff to fix the CEL rule:
- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()This aligns
isCanonical()with the correct member function form, consistent withisGlobalUnicast()on the same line.payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (1)
100-106: Fix inconsistent enum quoting for condition status.The
statusenum for condition objects still uses inconsistent quoting:"True"and"False"are quoted, butUnknownis unquoted. This inconsistency appears in all three condition types (cluster-level, node-level, and resource-level).Apply this pattern consistently across all three occurrences:
status: description: status of the condition, one of True, False, Unknown. enum: - "True" - "False" - - Unknown + - "Unknown" type: stringAlso applies to: 208-212, 346-350
etcd/v1alpha1/types_pacemakercluster.go (1)
505-505: Fix critical CEL syntax error in ipAddress validation.The CEL expression uses incorrect syntax:
ip.isCanonical(self)should beip(self).isCanonical(). According to Kubernetes CEL IP library documentation, the pattern isip(<string>).<method>(), notip.<method>(<string>). This will cause CRD validation to fail.Apply this fix:
-// +kubebuilder:validation:XValidation:rule="isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast()",message="ipAddress must be a valid canonical global unicast IPv4 or IPv6 address" +// +kubebuilder:validation:XValidation:rule="isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()",message="ipAddress must be a valid canonical global unicast IPv4 or IPv6 address"
🧹 Nitpick comments (1)
etcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml (1)
412-607: Consider adding IPv6 loopback test case.The IP validation tests cover invalid strings and IPv4 loopback (
127.0.0.1), but IPv6 loopback (::1) is not tested. Since the CRD supports both IPv4 and IPv6, adding an IPv6 loopback rejection test would strengthen coverage.Also applies to: 608-803
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (21)
etcd/README.md(1 hunks)etcd/install.go(1 hunks)etcd/v1alpha1/Makefile(1 hunks)etcd/v1alpha1/doc.go(1 hunks)etcd/v1alpha1/register.go(1 hunks)etcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml(1 hunks)etcd/v1alpha1/types_pacemakercluster.go(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/doc.go(1 hunks)etcd/v1alpha1/zz_generated.deepcopy.go(1 hunks)etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yaml(1 hunks)etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml(1 hunks)etcd/v1alpha1/zz_generated.swagger_doc_generated.go(1 hunks)hack/update-payload-crds.sh(1 hunks)install.go(2 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- etcd/v1alpha1/zz_generated.swagger_doc_generated.go
- etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yaml
- install.go
- hack/update-payload-crds.sh
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
etcd/v1alpha1/zz_generated.crd-manifests/doc.goetcd/README.mdetcd/install.goetcd/v1alpha1/zz_generated.deepcopy.gopayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yamletcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yamletcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yamlopenapi/generated_openapi/zz_generated.openapi.goetcd/v1alpha1/doc.goetcd/v1alpha1/register.goetcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yamlpayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yamletcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yamletcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yamletcd/v1alpha1/types_pacemakercluster.goetcd/v1alpha1/Makefile
🧬 Code graph analysis (4)
etcd/install.go (1)
etcd/v1alpha1/register.go (1)
AddToScheme(21-21)
etcd/v1alpha1/zz_generated.deepcopy.go (2)
etcd/v1alpha1/types_pacemakercluster.go (5)
PacemakerCluster(396-409)PacemakerClusterStatus(416-457)PacemakerClusterList(581-590)PacemakerClusterNodeStatus(461-523)PacemakerClusterResourceStatus(532-572)etcd/v1alpha1/zz_generated.swagger_doc_generated.go (5)
PacemakerCluster(20-22)PacemakerClusterStatus(63-65)PacemakerClusterList(30-32)PacemakerClusterNodeStatus(42-44)PacemakerClusterResourceStatus(52-54)
etcd/v1alpha1/register.go (2)
etcd/install.go (2)
GroupName(11-11)Resource(20-22)etcd/v1alpha1/types_pacemakercluster.go (2)
PacemakerCluster(396-409)PacemakerClusterList(581-590)
etcd/v1alpha1/types_pacemakercluster.go (1)
etcd/v1alpha1/zz_generated.swagger_doc_generated.go (4)
PacemakerClusterStatus(63-65)PacemakerClusterNodeStatus(42-44)PacemakerClusterResourceStatus(52-54)PacemakerCluster(20-22)
🪛 checkmake (0.2.2)
etcd/v1alpha1/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
🔇 Additional comments (23)
etcd/v1alpha1/zz_generated.crd-manifests/doc.go (1)
1-1: LGTM!Standard package declaration for generated CRD manifests.
etcd/v1alpha1/Makefile (1)
1-3: LGTM!The test target is appropriately configured. The static analysis warnings about missing
allandcleantargets can be safely ignored since this Makefile is intentionally minimal and only needs the test target.etcd/v1alpha1/doc.go (1)
1-6: LGTM!Standard code-generation annotations for the v1alpha1 package. The groupName and generation markers are correctly configured.
etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml (1)
1-449: CRD schema is well-designed with comprehensive validations.The schema correctly enforces:
- Singleton naming (
cluster)- Required conditions at all levels with CEL existence checks
- Monotonic
lastUpdatedtimestamps- RFC 1123 subdomain validation for node names
- Enum constraints for resource names
The status-only design with
minItemsvalidations for required conditions is appropriate for this observational resource.etcd/install.go (1)
1-26: LGTM!Standard OpenShift API group installation wiring. The
ResourceandKindhelpers follow established patterns, and the scheme builder correctly registers v1alpha1.etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
1-449: CRD manifest structure is sound.The CustomNoUpgrade variant correctly mirrors the schema structure with appropriate feature-set annotations. The validation rules for conditions, timestamps, and resource requirements are well-defined.
etcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml (2)
1-24: Well-structured test fixture with good validation coverage.The test setup and
onCreatecases properly validate the singleton naming constraint (cluster) and minimal creation path.
1001-1031: Good coverage for required condition validation.The test correctly validates that the
Healthycondition must be present in cluster-level conditions, exercising the CELself.exists(c, c.type == 'Healthy')rule.etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (2)
1-28: CRD structure and metadata are well-defined.The CRD properly configures cluster scope, status subresource, feature-set annotation, and API approval reference. The singleton enforcement via CEL validation (
self.metadata.name == 'cluster') at line 444-445 is appropriate.
434-439: LGTM:lastUpdatedtransition rules are well-designed.The CEL rules correctly prevent removal of
lastUpdatedonce set and prevent backdating timestamps. Using>=allows idempotent updates which is appropriate for status fields.payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (1)
47-131: Comprehensive status schema with appropriate validations.The conditions schema with
minItems: 3,maxItems: 8, list-map handling, and CEL existence checks for required condition types (Healthy, InService, NodeCountAsExpected) follows Kubernetes API conventions correctly.etcd/v1alpha1/register.go (1)
1-39: LGTM: Standard API registration scaffolding.The registration follows the established OpenShift API pattern:
GroupNameandGroupVersionare correctly definedInstallentry point is exposed for scheme registration- Deprecated aliases (
SchemeGroupVersion,AddToScheme,Resource) maintained for compatibilityaddKnownTypescorrectly registersPacemakerClusterandPacemakerClusterListThis aligns with the parent package's
etcd/install.gowiring.payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (2)
272-422: Well-structured resources validation schema.The nested resources schema properly enforces:
- Required resources (Kubelet, Etcd, FencingAgent) via CEL
existsrules- Enum constraint on resource names
- Per-resource condition requirements (8 condition types)
- List-map handling with
nameas the keyThis accurately models Pacemaker resource states.
443-445: Singleton naming constraint correctly enforced.The CEL rule
self.metadata.name == 'cluster'ensures only the expected singleton instance can be created, complementing the cluster-scoped design.etcd/v1alpha1/zz_generated.deepcopy.go (5)
13-42: LGTM!The deepcopy implementation for
PacemakerClustercorrectly handles all fields:TypeMetais copied directly (no pointers),ObjectMetais deep copied via its ownDeepCopyIntomethod, and theStatuspointer is properly allocated and deep copied when non-nil. The pattern follows Kubernetes conventions.
44-75: LGTM!The deepcopy implementation for
PacemakerClusterListcorrectly deep copies theItemsslice by allocating a new slice and invokingDeepCopyIntoon each element. TheListMetais also properly deep copied.
77-105: LGTM!The deepcopy implementation for
PacemakerClusterNodeStatuscorrectly handles both slice fields (ConditionsandResources) with per-element deep copy loops. The scalar string fields (NameandIPAddress) are copied by the initial shallow copy.
107-128: LGTM!The deepcopy implementation for
PacemakerClusterResourceStatuscorrectly deep copies theConditionsslice while theNamefield (a string-based type) is copied by the shallow copy.
130-159: LGTM!The deepcopy implementation for
PacemakerClusterStatuscorrectly handles all three fields:ConditionsandNodesslices are deep copied with per-element loops, andLastUpdatedis deep copied via its ownDeepCopyIntomethod. The past issues regarding missingConditionshandling and orphanedSummaryfield have been successfully addressed.etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (3)
1-46: LGTM!The CRD metadata and basic structure are correct. The
DevPreviewNoUpgradefeature set annotation is appropriate for an alpha API (Compatibility level 4), and the group/version/kind definitions follow Kubernetes conventions.
47-141: LGTM!The status schema and cluster-level conditions are well-defined. The CEL validations correctly enforce the presence of three required condition types (
Healthy,InService,NodeCountAsExpected), and thelastUpdatedfield follows Kubernetes conventions by being marked optional while having immutability and monotonic-increase validations enforced via CEL rules later in the manifest.
272-449: LGTM!The resource-level schema and top-level validations are comprehensive and correct. The CEL validations properly enforce the presence of all three required resources (
Kubelet,Etcd,FencingAgent) with their respective conditions. ThelastUpdatedimmutability and monotonic-increase validations are semantically correct, and the singleton name constraint (metadata.name == 'cluster') is properly enforced. The status subresource configuration is appropriate for a status-only CRD.openapi/generated_openapi/zz_generated.openapi.go (1)
523-527: LGTM! Generated OpenAPI schemas are correctly structured.The generated OpenAPI definitions for the five PacemakerCluster-related types are structurally sound:
- Registration in
GetOpenAPIDefinitionsfollows the standard pattern- All schema functions properly define properties, types, and dependencies
- Dependency arrays correctly match all
Ref()calls- Required fields follow Kubernetes conventions (minimal requirements, status fields optional)
- Comprehensive descriptions document constraints and validation requirements
Note: Validation constraints mentioned in descriptions (e.g., minItems for conditions, enum values for resource names, IP address format) are appropriately left for CEL validation rules in the CRD manifests rather than enforced in the OpenAPI schema—this is the standard pattern for generated OpenAPI.
Also applies to: 25298-25574
jaypoulz
force-pushed
the
OCPEDGE-2084
branch
from
b9dbd77 to
f202779
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
255-258: Incorrect CEL IP validation syntax.The CEL rule
ip.isCanonical(self)is incorrect. The Kubernetes CEL IP library requires callingip(self)to parse the string into an IP object before invokingisCanonical().This was already flagged in a previous review. The fix should be:
- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()Since this is a generated file, update the source Go type annotations and regenerate.
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
255-258: Same CEL syntax issue as the generated manifest.The
ip.isCanonical(self)call is incorrect; it should beip(self).isCanonical().This was flagged in past reviews. Fix the source types and regenerate both manifests together:
- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml (1)
255-258: Inconsistent CEL IP validation form (previously flagged).The validation mixes function form (
ip.isCanonical(self)) with member form (ip(self).isGlobalUnicast()). For consistency, use member form for both:- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()Since this is a generated file, apply the fix to the source types/templates that generate this manifest.
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (1)
100-106: Inconsistent enum value quoting for Unknown.The
statusenum values are inconsistently quoted across all condition types:"True"and"False"are quoted, butUnknownis unquoted. This should be normalized for API clarity and consistency.Apply this diff to quote all enum values consistently in all three locations (cluster conditions, node conditions, and resource conditions):
status: description: status of the condition, one of True, False, Unknown. enum: - "True" - "False" - - Unknown + - "Unknown" type: stringAlso applies to: 205-212, 343-350
etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (2)
246-258: Critical: Incorrect CEL syntax for ipAddress validation.The CEL rule on line 258 uses
ip.isCanonical(self), which is invalid syntax. Per the Kubernetes CEL IP library documentation,isCanonical()must be called as a member function on an IP object returned byip(), not as a library function.Apply this diff to fix the CEL rule:
- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()This aligns
isCanonical()with the correct member function calling convention, consistent with howisGlobalUnicast()is already called on the same line.
100-106: Inconsistent enum value quoting for Unknown.The
statusenum values are inconsistently quoted across all condition types:"True"and"False"are quoted, butUnknownis unquoted. This should be normalized for API clarity and consistency.Apply this diff to quote all enum values consistently in all three locations (cluster conditions, node conditions, and resource conditions):
status: description: status of the condition, one of True, False, Unknown. enum: - "True" - "False" - - Unknown + - "Unknown" type: stringAlso applies to: 205-212, 343-350
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (22)
etcd/README.md(1 hunks)etcd/install.go(1 hunks)etcd/v1alpha1/Makefile(1 hunks)etcd/v1alpha1/doc.go(1 hunks)etcd/v1alpha1/register.go(1 hunks)etcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml(1 hunks)etcd/v1alpha1/types_pacemakercluster.go(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/doc.go(1 hunks)etcd/v1alpha1/zz_generated.deepcopy.go(1 hunks)etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yaml(1 hunks)etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml(1 hunks)etcd/v1alpha1/zz_generated.swagger_doc_generated.go(1 hunks)hack/update-payload-crds.sh(1 hunks)install.go(2 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(2 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- install.go
- etcd/v1alpha1/zz_generated.crd-manifests/doc.go
- etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml
- hack/update-payload-crds.sh
- etcd/install.go
- etcd/v1alpha1/zz_generated.swagger_doc_generated.go
- payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml
- etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yaml
- etcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml
- etcd/v1alpha1/types_pacemakercluster.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yamletcd/v1alpha1/doc.goetcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yamletcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yamletcd/README.mdetcd/v1alpha1/zz_generated.deepcopy.goopenapi/generated_openapi/zz_generated.openapi.gopayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yamletcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yamletcd/v1alpha1/register.goopenapi/openapi.jsonetcd/v1alpha1/Makefile
🧬 Code graph analysis (1)
etcd/v1alpha1/register.go (2)
etcd/install.go (2)
GroupName(11-11)Resource(20-22)etcd/v1alpha1/types_pacemakercluster.go (2)
PacemakerCluster(396-409)PacemakerClusterList(581-590)
🪛 checkmake (0.2.2)
etcd/v1alpha1/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
🔇 Additional comments (17)
etcd/v1alpha1/doc.go (1)
1-6: LGTM!The code-generation annotations are correctly configured for the etcd.openshift.io API group. The package follows standard OpenShift API patterns.
etcd/v1alpha1/Makefile (1)
1-3: LGTM!The Makefile correctly provides a test target that focuses on the etcd.openshift.io/v1alpha1 API tests. The static analysis warnings about missing
allandcleantargets are not applicable here as this is a focused test helper Makefile, consistent with other API group Makefiles in the repository.etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
1-449: CRD schema is well-designed for the two-node fencing use case.The manifest includes comprehensive validations: singleton enforcement, required conditions at all levels, proper list-map semantics, and monotonic timestamp constraints. The status subresource is correctly enabled. Aside from the CEL syntax issue noted above, the schema is solid.
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
1-449: Payload manifest correctly mirrors the generated CRD.The structure, validations, and constraints properly match the zz_generated source. Once the CEL syntax is fixed in the source and regenerated, this file should be updated via
hack/update-payload-crds.sh.etcd/README.md (1)
1-140: Documentation is comprehensive and well-structured.The README provides clear API documentation for the etcd.openshift.io API group, including feature gates, conditions, validation rules, and usage guidance. The content is well-organized and appropriate for this scope.
etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml (2)
1-20: LGTM: CRD metadata and spec configuration.The feature-gated CRD is properly configured with:
- Correct API group (etcd.openshift.io) and version (v1alpha1)
- Cluster scope appropriate for singleton "cluster" resource
- Status subresource enabled as required for status-only CRD
- DualReplica feature gate annotation
125-131: Well-structured validation rules.The CEL validations effectively enforce:
- Required condition types at cluster, node, and resource levels
- Required resources (Kubelet, Etcd, FencingAgent) for two-node deployments
- Proper list constraints and immutability rules
These validations align well with the Pacemaker cluster health monitoring requirements.
Also applies to: 231-245, 370-394, 416-422
etcd/v1alpha1/register.go (1)
1-39: LGTM: Standard API registration implementation.The registration scaffolding correctly:
- Defines the etcd.openshift.io group with v1alpha1 version
- Exposes Install function for scheme integration
- Maintains deprecated aliases for backward compatibility
- Registers PacemakerCluster and PacemakerClusterList types
- Follows OpenShift API conventions
etcd/v1alpha1/zz_generated.deepcopy.go (1)
1-159: LGTM: Properly generated deepcopy implementations.All deepcopy methods correctly:
- Handle nil checks for pointer and slice fields
- Perform deep copies of nested structures (ObjectMeta, Conditions, Resources, Nodes)
- Implement standard DeepCopyInto, DeepCopy, and DeepCopyObject patterns
- Match the current type definitions (no orphaned references)
Past issues with Summary field and orphaned PacemakerSummary type have been properly resolved.
openapi/generated_openapi/zz_generated.openapi.go (3)
523-527: LGTM: Schema registration entries are correct.The new PacemakerCluster type registrations follow the existing pattern and are properly ordered alphabetically within the map.
25511-25574: Validation constraints are already enforced in the source code.The concerns raised in this review have already been addressed:
conditionshas+kubebuilder:validation:MinItems=3(line 425 of types_pacemakercluster.go)nodeshas+kubebuilder:validation:MaxItems=32(line 454 of types_pacemakercluster.go)lastUpdatedmonotonicity is enforced via CEL validation rules (lines 414-415 of types_pacemakercluster.go):
- Prevents removal:
rule="!has(oldSelf.lastUpdated) || has(self.lastUpdated)"- Prevents earlier timestamp:
rule="!has(oldSelf.lastUpdated) || !has(self.lastUpdated) || self.lastUpdated >= oldSelf.lastUpdated"These validations are correctly reflected in the CRD manifests. The generated OpenAPI file does not expose CEL rules because OpenAPI 3.0 has limited support for them, but they are enforced in the actual Kubernetes API.
Likely an incorrect or invalid review comment.
25392-25464: The validations are already present in the source types with kubebuilder markers and correctly generated in the CRD manifests. The OpenAPI schema is missing validation constraints that are properly defined at the source (e.g.,MinItems=7for conditions,MaxLength=253for name,MinItems=3for resources). This indicates a gap in OpenAPI schema generation rather than missing markers in the source types. Verify whether the OpenAPI generation tooling is properly translating kubebuilder validation markers to the OpenAPI schema output.Likely an incorrect or invalid review comment.
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (1)
443-445: Singleton name validation correctly enforced.The CEL validation rule properly enforces that
PacemakerClustermust be namedcluster, ensuring singleton semantics as documented.openapi/openapi.json (4)
9884-9887: LGTM!The policyType description update correctly documents the "PKI" option alongside existing policy types.
14013-14038: Well-designed status-only CRD structure.The PacemakerCluster definition correctly implements a status-only resource with no spec field, appropriate for a cluster-scoped singleton populated by a privileged controller. The compatibility level 4 warning is correctly documented for this alpha API.
14069-14111: Node and resource status structures are well-defined.The
PacemakerClusterNodeStatusschema properly usesx-kubernetes-list-type: mapwith appropriate keys for conditions and resources, enabling strategic merge patch. The comprehensive condition type documentation (Healthy, Online, InService, Active, Ready, Clean, Member) provides clear observability semantics.
14137-14170: Status field design follows conventions with appropriate immutability notes.The
lastUpdatedfield documentation correctly describes the monotonic timestamp constraint (cannot be removed or set earlier). The nodes list appropriately documents the 32-node Pacemaker limit while noting the expected 2-node configuration for Two Node OpenShift with Fencing.
jaypoulz
force-pushed
the
OCPEDGE-2084
branch
from
f202779 to
37e92e9
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (1)
257-260: CEL validation rule syntax error persists for ipAddresses.The rule at line 260 uses
ip.isCanonical(self)which is incorrect syntax. According to the Kubernetes CEL IP library,isCanonical()is a method on the IP object returned byip(), not a namespace-level function.This will cause validation failures at runtime.
Apply this diff to fix the syntax:
x-kubernetes-validations: - message: must be a valid global unicast IPv4 or IPv6 address in canonical form - rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
257-260: Same CEL validation syntax error as TechPreviewNoUpgrade variant.The ipAddresses validation rule has the same incorrect
ip.isCanonical(self)syntax.Apply this diff:
x-kubernetes-validations: - message: must be a valid global unicast IPv4 or IPv6 address in canonical form - rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
257-260: Same IP validation syntax issue as other CRD manifests.This file has the same CEL validation inconsistency as the DualReplica manifest. The fix in the source types will propagate to all generated CRD variants.
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (1)
257-260: Same IP validation syntax issue.This TechPreviewNoUpgrade variant has the same CEL validation inconsistency as the other manifests. The fix in the source types will apply to all variants.
etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml (1)
257-260: Fix IP validation CEL syntax inconsistency.Based on JoelSpeed's earlier comment, the CEL validation mixes syntax forms inconsistently. Line 260 uses
ip.isCanonical(self)(library function form) whileip(self).isGlobalUnicast()uses the member form. For consistency with theip(self).<method>pattern, update the isCanonical check:- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()Since this is a generated file, the fix should be applied to the kubebuilder marker in
etcd/v1alpha1/types_pacemakercluster.go(line ~508), then regenerate the CRDs.payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (1)
100-106: Inconsistent enum value quoting persists across condition status fields.The
statusenum for condition objects continues to use mixed quoting:"True"and"False"are quoted, butUnknownremains unquoted at three locations (cluster conditions at lines 100-106, node conditions at lines 208-212, and resource conditions at lines 352-356). While functionally equivalent, consistent quoting improves readability and aligns with common CRD style conventions.Apply this diff pattern to quote all enum values consistently at each location:
status: description: status of the condition, one of True, False, Unknown. enum: - "True" - "False" - - Unknown + - "Unknown" type: stringAlso applies to: 208-212, 352-356
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (22)
etcd/README.md(1 hunks)etcd/install.go(1 hunks)etcd/v1alpha1/Makefile(1 hunks)etcd/v1alpha1/doc.go(1 hunks)etcd/v1alpha1/register.go(1 hunks)etcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml(1 hunks)etcd/v1alpha1/types_pacemakercluster.go(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/doc.go(1 hunks)etcd/v1alpha1/zz_generated.deepcopy.go(1 hunks)etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yaml(1 hunks)etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml(1 hunks)etcd/v1alpha1/zz_generated.swagger_doc_generated.go(1 hunks)hack/update-payload-crds.sh(1 hunks)install.go(2 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(2 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- hack/update-payload-crds.sh
- etcd/README.md
- openapi/generated_openapi/zz_generated.openapi.go
- etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml
- etcd/v1alpha1/doc.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
install.goetcd/install.goetcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yamletcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yamletcd/v1alpha1/zz_generated.swagger_doc_generated.goetcd/v1alpha1/zz_generated.featuregated-crd-manifests.yamlpayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yamletcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yamletcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yamletcd/v1alpha1/register.goetcd/v1alpha1/zz_generated.crd-manifests/doc.goetcd/v1alpha1/zz_generated.deepcopy.gopayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yamlopenapi/openapi.jsonpayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yamletcd/v1alpha1/types_pacemakercluster.goetcd/v1alpha1/Makefile
🧬 Code graph analysis (4)
install.go (2)
etcd/install.go (1)
Install(17-17)etcd/v1alpha1/register.go (1)
Install(14-14)
etcd/install.go (2)
etcd/v1alpha1/register.go (4)
Install(14-14)AddToScheme(21-21)Resource(26-28)GroupName(10-10)install.go (1)
Install(119-119)
etcd/v1alpha1/register.go (3)
etcd/install.go (2)
GroupName(11-11)Resource(20-22)etcd/v1alpha1/types_pacemakercluster.go (2)
PacemakerCluster(396-409)PacemakerClusterList(585-594)etcd/v1alpha1/zz_generated.swagger_doc_generated.go (2)
PacemakerCluster(20-22)PacemakerClusterList(30-32)
etcd/v1alpha1/zz_generated.deepcopy.go (2)
etcd/v1alpha1/types_pacemakercluster.go (5)
PacemakerCluster(396-409)PacemakerClusterStatus(416-457)PacemakerClusterList(585-594)PacemakerClusterNodeStatus(461-527)PacemakerClusterResourceStatus(536-576)etcd/v1alpha1/zz_generated.swagger_doc_generated.go (5)
PacemakerCluster(20-22)PacemakerClusterStatus(63-65)PacemakerClusterList(30-32)PacemakerClusterNodeStatus(42-44)PacemakerClusterResourceStatus(52-54)
🪛 checkmake (0.2.2)
etcd/v1alpha1/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
🔇 Additional comments (11)
etcd/v1alpha1/zz_generated.crd-manifests/doc.go (1)
1-1: LGTM!Standard package declaration for CRD manifest scaffolding, follows existing conventions in the repository.
install.go (1)
57-57: LGTM!The etcd API group import and scheme registration follow the established patterns used by other API groups in this file.
Also applies to: 93-93
etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yaml (1)
1-22: LGTM!The feature-gated CRD manifest metadata is correctly configured with DualReplica feature gating, cluster scope, and status-only design as described in the PR objectives.
etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (1)
1-455: Well-structured CRD with comprehensive validations.The CRD schema is well-designed with:
- Proper singleton enforcement via name constraint
- Thorough condition validations at cluster, node, and resource levels
- Appropriate list type semantics and cardinality bounds
- Transition rules protecting lastUpdated integrity
The only issue is the CEL syntax error for IP validation noted above.
etcd/v1alpha1/Makefile (1)
1-3: LGTM!The Makefile provides the expected
testtarget for running v1alpha1-specific tests with appropriate Ginkgo focus filtering. The static analysis warnings about missingallandcleantargets are not applicable here—this is intentionally a minimal test-runner Makefile.etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
1-455: CRD structure is otherwise correct.The CustomNoUpgrade variant correctly mirrors the TechPreviewNoUpgrade CRD with only the feature-set annotation differing. Once the CEL syntax is fixed, this manifest is ready.
etcd/install.go (1)
1-26: LGTM! Clean API group integration.The etcd API group wiring follows standard Kubernetes patterns. The GroupName, schemeBuilder, Install function, and helper methods (Resource, Kind) are all correctly implemented.
etcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml (1)
1-1192: Comprehensive test coverage.This test fixture thoroughly exercises the PacemakerCluster CRD validation rules, including:
- Singleton name validation
- Full healthy cluster status with all required conditions
- IP address validation (format and unicast requirements)
- Node name validation (RFC 1123 subdomain rules)
- Required condition validation
- Required resource validation
The test suite covers both success and failure paths effectively.
etcd/v1alpha1/zz_generated.swagger_doc_generated.go (1)
1-67: Generated Swagger documentation is correct.The SwaggerDoc maps accurately reflect the current type definitions for all PacemakerCluster-related types. All documented fields (conditions, lastUpdated, nodes, ipAddresses, resources, etc.) match the actual struct fields.
etcd/v1alpha1/zz_generated.deepcopy.go (1)
1-164: Correctly generated deepcopy implementations.All DeepCopy and DeepCopyInto methods properly handle:
- Nested struct fields (ObjectMeta, TypeMeta, ListMeta)
- Pointer fields with nil checks
- Slice fields with proper allocation and per-element deep copying (Conditions, IPAddresses, Resources, Nodes)
- DeepCopyObject implementations for runtime.Object interface
The generated code aligns with the current type definitions.
etcd/v1alpha1/register.go (1)
1-39: Standard API registration implementation.The v1alpha1 registration follows Kubernetes conventions correctly:
- GroupName and GroupVersion properly defined
- SchemeBuilder configured with addKnownTypes
- Install function exported for use by parent etcd package
- Deprecated aliases (SchemeGroupVersion, AddToScheme, Resource) maintained for backward compatibility
- addKnownTypes registers PacemakerCluster and PacemakerClusterList types
jaypoulz
force-pushed
the
OCPEDGE-2084
branch
from
37e92e9 to
de2dc2c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml (1)
257-260: CEL IP validation mixes function and member forms.The rule uses
ip.isCanonical(self)(function form) alongsideip(self).isGlobalUnicast()(member form). While both are valid CEL syntax, using consistent member form improves readability.- - message: must be a valid global unicast IPv4 or IPv6 address - in canonical form - rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + - message: must be a valid global unicast IPv4 or IPv6 address + in canonical form + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()etcd/README.md (1)
1-142: Comprehensive API documentation.The README provides excellent coverage of the PacemakerCluster API including condition types, validation rules, and usage patterns. The status structure example and condition tables are particularly helpful.
Note: A previous reviewer mentioned this may duplicate content in the v1alpha1 directory. Consider consolidating to a single location to avoid maintenance drift.
etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
257-260: CEL IP validation uses inconsistent forms.Same issue as the DualReplica variant:
ip.isCanonical(self)should beip(self).isCanonical()for consistency with the adjacentip(self).isGlobalUnicast()call.- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()openapi/openapi.json (1)
14090-14098: Typo in ipAddresses description still present.The description reads "is a list IPv4 or IPv6 addresses" but should be "is a list of IPv4 or IPv6 addresses". This was flagged in a previous review and needs to be fixed in the source Go type comments (
etcd/v1alpha1/types_pacemakercluster.go), then regenerate this file.etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (1)
246-260: Fix CEL syntax error in IP validation rule.Line 260 has an incorrect CEL expression:
ip.isCanonical(self)calls isCanonical as ifipwere an object, butip()is a function that must be called first to convert the string to an IP object. The correct syntax isip(self).isCanonical().Note that the same line correctly uses
ip(self).isGlobalUnicast()— apply the same pattern to isCanonical.Apply this diff:
- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
246-260: Fix CEL syntax error in IP validation rule.Line 260 contains the same CEL syntax error as the other CRD variants:
ip.isCanonical(self)should beip(self).isCanonical(). Theip()function must be called to parse the string before calling member methods.Apply this diff:
- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (1)
246-260: Fix CEL syntax error in IP validation rule.Line 260 has the incorrect CEL expression
ip.isCanonical(self). Theip()function must be called to parse the string parameter before calling theisCanonical()member method.Apply this diff:
- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (1)
246-260: Fix CEL syntax error in IP validation rule.Line 260 contains the incorrect CEL call
ip.isCanonical(self). This should beip(self).isCanonical()to properly call the ip() function and then invoke the member method on the returned IP object.Apply this diff:
- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()
🧹 Nitpick comments (1)
etcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml (1)
1-1192: Good test coverage for CRD validations.The test fixtures comprehensively exercise:
- Singleton naming constraint
- IP address validation (invalid format, loopback rejection)
- Node name RFC1123 validation
- Required condition type presence
- Required resource presence
Consider adding tests for:
lastUpdatedmonotonic constraint (rejecting earlier timestamps on update)- Valid IPv6 address acceptance (e.g.,
2001:db8::1)- Non-canonical IPv4 rejection (e.g.,
192.168.001.001)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (22)
etcd/README.md(1 hunks)etcd/install.go(1 hunks)etcd/v1alpha1/Makefile(1 hunks)etcd/v1alpha1/doc.go(1 hunks)etcd/v1alpha1/register.go(1 hunks)etcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml(1 hunks)etcd/v1alpha1/types_pacemakercluster.go(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/doc.go(1 hunks)etcd/v1alpha1/zz_generated.deepcopy.go(1 hunks)etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yaml(1 hunks)etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml(1 hunks)etcd/v1alpha1/zz_generated.swagger_doc_generated.go(1 hunks)hack/update-payload-crds.sh(1 hunks)install.go(2 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(2 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- etcd/v1alpha1/zz_generated.crd-manifests/doc.go
🚧 Files skipped from review as they are similar to previous changes (8)
- hack/update-payload-crds.sh
- install.go
- etcd/install.go
- etcd/v1alpha1/doc.go
- etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yaml
- payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml
- openapi/generated_openapi/zz_generated.openapi.go
- etcd/v1alpha1/types_pacemakercluster.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yamletcd/README.mdetcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yamletcd/v1alpha1/zz_generated.deepcopy.gopayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yamletcd/v1alpha1/zz_generated.swagger_doc_generated.gopayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yamletcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yamletcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yamlopenapi/openapi.jsonetcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yamletcd/v1alpha1/register.goetcd/v1alpha1/Makefile
🧬 Code graph analysis (2)
etcd/v1alpha1/zz_generated.deepcopy.go (1)
etcd/v1alpha1/types_pacemakercluster.go (5)
PacemakerCluster(396-409)PacemakerClusterStatus(416-457)PacemakerClusterList(585-594)PacemakerClusterNodeStatus(461-527)PacemakerClusterResourceStatus(536-576)
etcd/v1alpha1/zz_generated.swagger_doc_generated.go (1)
etcd/v1alpha1/types_pacemakercluster.go (5)
PacemakerCluster(396-409)PacemakerClusterList(585-594)PacemakerClusterNodeStatus(461-527)PacemakerClusterResourceStatus(536-576)PacemakerClusterStatus(416-457)
🪛 checkmake (0.2.2)
etcd/v1alpha1/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
🔇 Additional comments (9)
etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml (1)
1-455: Well-structured CRD with comprehensive validations.The schema correctly implements:
- Singleton naming constraint via CEL rule
- Monotonic
lastUpdatedtimestamp enforcement- Required condition types at cluster, node, and resource levels
- Map-based list indexing for conditions and nodes
- Appropriate min/max bounds for arrays
etcd/v1alpha1/Makefile (1)
1-3: LGTM with minor note.The test target correctly focuses on the v1alpha1 API group. The static analysis warnings about missing
allandcleantargets are false positives—this is a subdirectory Makefile that only needs to provide the test target.Per past feedback: ensure there's a trailing newline at the end of the file.
etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
1-455: CRD manifest correctly configured for CustomNoUpgrade feature set.The manifest includes appropriate annotations for the CustomNoUpgrade feature set and correctly defines the cluster-scoped PacemakerCluster resource with all required validations.
openapi/openapi.json (3)
9881-9887: LGTM!The enhanced policyType description is clear and accurately documents the allowed values and their semantics.
14013-14068: LGTM! Schema structure is correct.The PacemakerCluster and PacemakerClusterList definitions follow Kubernetes API conventions correctly. The singleton semantics are properly documented, and the compatibility level 4 notice is now correctly present (single occurrence). The $refs and required fields are appropriately configured.
14069-14175: Schema definitions are well-structured.The node status, resource status, and cluster status definitions are properly organized with:
- Correct required field declarations
- Appropriate use of
x-kubernetes-list-type: mapwith proper map keys- Comprehensive condition documentation
The validation constraints (minItems, maxItems, enum) are enforced by the CRD manifests at admission time, though they don't appear in this OpenAPI schema (as noted in prior review). Since OpenAPI generation typically doesn't preserve all CRD validation markers, this is expected behavior for generated output.
etcd/v1alpha1/zz_generated.swagger_doc_generated.go (1)
1-67: LGTM! Swagger documentation is correctly generated.The autogenerated Swagger documentation properly reflects the current type definitions. All five maps (PacemakerCluster, PacemakerClusterList, PacemakerClusterNodeStatus, PacemakerClusterResourceStatus, PacemakerClusterStatus) document only the fields that exist in the actual Go types, with no stale or orphaned documentation.
etcd/v1alpha1/zz_generated.deepcopy.go (1)
1-164: LGTM! DeepCopy implementations are correct.All autogenerated deepcopy methods properly handle nested structures:
- Conditions slices are deep copied element-by-element at all levels (cluster, node, resource)
- Nested slices (Nodes, Resources, IPAddresses) are correctly allocated and copied
- No orphaned types or stale code present
The implementations follow Kubernetes deepcopy conventions and will ensure proper value semantics for these API types.
etcd/v1alpha1/register.go (1)
1-39: LGTM! API registration follows Kubernetes conventions.The registration scaffolding is correctly implemented:
- GroupName and GroupVersion properly defined for etcd.openshift.io/v1alpha1
- schemeBuilder initialized with addKnownTypes function
- Install function exported for scheme registration
- Both PacemakerCluster and PacemakerClusterList registered in addKnownTypes
- Deprecated compatibility exports (SchemeGroupVersion, AddToScheme) included per conventions
- Resource helper properly constructs GroupResource values
This aligns with standard Kubernetes API registration patterns.
jaypoulz
force-pushed
the
OCPEDGE-2084
branch
from
de2dc2c to
0492c73
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (8)
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (1)
100-106: Inconsistent enum quoting for condition status values.The
statusenum uses mixed quoting:"True"and"False"are quoted whileUnknownis not. This inconsistency appears throughout the file (also at lines 208-211, 352-355). While this may be functionally equivalent in YAML, consistent quoting improves readability.status: description: status of the condition, one of True, False, Unknown. enum: - "True" - "False" - - Unknown + - "Unknown" type: stringetcd/README.md (1)
1-142: Consider consolidating with v1alpha1 README.A past reviewer noted this README may duplicate content from
etcd/v1alpha1/README.md. To reduce maintenance drift, consider keeping detailed API documentation in one canonical location (e.g.,etcd/v1alpha1/) and having this file provide a brief overview with a link to the detailed docs.etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
257-260: Fix CEL validation syntax in the source type definition.The source type definition at
etcd/v1alpha1/types_pacemakercluster.go:509contains invalid CEL syntax. The rule usesip.isCanonical(self), but the correct CEL syntax isip(self).isCanonical()—ipis a function that parses a string to an IP object, not an IP value itself. Once the source is corrected, regenerate the CRD manifests.-rule="isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast()" +rule="isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()"payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
257-260: Same CEL IP validation syntax error.This manifest has the same
ip.isCanonical(self)issue as the DevPreviewNoUpgrade variant. The fix should be applied via regeneration from the Go type definitions to ensure consistency across all CRD manifests.etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (1)
257-260: Same CEL IP validation syntax error.This manifest has the same
ip.isCanonical(self)issue. Despite past review comments indicating fixes, the current code still shows the incorrect syntax. Regenerate all CRD manifests from the Go type definitions after fixing the kubebuilder marker intypes_pacemakercluster.go.etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (1)
257-260: CEL IP validation rule uses incorrect syntax.The rule
ip.isCanonical(self)is invalid. Per the Kubernetes CEL IP library,isCanonical()must be called as a member function on an IP object returned byip(). The correct form isip(self).isCanonical().Apply this diff:
x-kubernetes-validations: - message: must be a valid global unicast IPv4 or IPv6 address in canonical form - rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()Regenerate from Go type definitions to apply this fix consistently across all CRD manifest variants.
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (1)
246-264: Fix CEL IP validation: useip(self).isCanonical()instead ofip.isCanonical(self).Per JoelSpeed's earlier comment,
ip.isCanonical(self)is a bug in the IP library. The correct syntax isip(self).isCanonical(), which matches the pattern used forip(self).isGlobalUnicast()on the same line. Theip()function returns an IP object, andisCanonical()is a method on that object.Apply this diff to fix the CEL validation:
x-kubernetes-validations: - message: must be a valid global unicast IPv4 or IPv6 address in canonical form - rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()Based on past review comments.
etcd/v1alpha1/types_pacemakercluster.go (1)
509-509: Critical: Fix CEL syntax error in IP validation.The CEL expression mixes correct and incorrect IP library call syntax on the same line:
- ✓
isIP(self)- correct- ✗
ip.isCanonical(self)- incorrect, should beip(self).isCanonical()- ✓
ip(self).isGlobalUnicast()- correctAccording to Kubernetes CEL IP library documentation, the pattern is
ip(<string>).<method>(), notip.<method>(<string>). This inconsistency will cause CRD validation to fail.Apply this diff to fix the syntax:
- // +kubebuilder:validation:items:XValidation:rule="isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast()",message="must be a valid global unicast IPv4 or IPv6 address in canonical form" + // +kubebuilder:validation:items:XValidation:rule="isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()",message="must be a valid global unicast IPv4 or IPv6 address in canonical form"After fixing this, regenerate the CRD manifests to ensure the corrected validation is applied.
🧹 Nitpick comments (1)
etcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml (1)
1-1192: Comprehensive test coverage for PacemakerCluster validation.The test fixture thoroughly exercises the CRD validation rules including:
- Singleton name enforcement (
cluster)- Full healthy status path with all required conditions
- IP address validation (invalid format, loopback rejection)
- Node name RFC1123 validation
- Required conditions and resources enforcement
Consider adding a positive test case with a valid IPv6 address (e.g.,
2001:db8::1) to complement the existing IPv4 tests, ensuring IPv6 addresses pass validation as expected.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (22)
etcd/README.md(1 hunks)etcd/install.go(1 hunks)etcd/v1alpha1/Makefile(1 hunks)etcd/v1alpha1/doc.go(1 hunks)etcd/v1alpha1/register.go(1 hunks)etcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml(1 hunks)etcd/v1alpha1/types_pacemakercluster.go(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/doc.go(1 hunks)etcd/v1alpha1/zz_generated.deepcopy.go(1 hunks)etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yaml(1 hunks)etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml(1 hunks)etcd/v1alpha1/zz_generated.swagger_doc_generated.go(1 hunks)hack/update-payload-crds.sh(1 hunks)install.go(2 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(2 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- install.go
- etcd/v1alpha1/zz_generated.crd-manifests/doc.go
- etcd/v1alpha1/doc.go
- etcd/install.go
- etcd/v1alpha1/zz_generated.swagger_doc_generated.go
- etcd/v1alpha1/register.go
- etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yaml
- etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
hack/update-payload-crds.shetcd/v1alpha1/zz_generated.deepcopy.goetcd/v1alpha1/Makefilepayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yamletcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yamletcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yamletcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yamlopenapi/openapi.jsonetcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yamletcd/README.mdpayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yamlopenapi/generated_openapi/zz_generated.openapi.goetcd/v1alpha1/types_pacemakercluster.go
🧬 Code graph analysis (2)
etcd/v1alpha1/zz_generated.deepcopy.go (2)
etcd/v1alpha1/types_pacemakercluster.go (5)
PacemakerCluster(396-409)PacemakerClusterStatus(416-457)PacemakerClusterList(585-594)PacemakerClusterNodeStatus(461-527)PacemakerClusterResourceStatus(536-576)etcd/v1alpha1/zz_generated.swagger_doc_generated.go (5)
PacemakerCluster(20-22)PacemakerClusterStatus(63-65)PacemakerClusterList(30-32)PacemakerClusterNodeStatus(42-44)PacemakerClusterResourceStatus(52-54)
etcd/v1alpha1/types_pacemakercluster.go (1)
etcd/v1alpha1/zz_generated.swagger_doc_generated.go (3)
PacemakerClusterNodeStatus(42-44)PacemakerClusterResourceStatus(52-54)PacemakerCluster(20-22)
🪛 checkmake (0.2.2)
etcd/v1alpha1/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
🔇 Additional comments (14)
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (1)
1-455: Well-structured CRD with comprehensive validations.The CRD design follows Kubernetes API conventions properly:
- Singleton enforcement via CEL validation
- Monotonic timestamp validation for
lastUpdated- Map-type lists with appropriate keys for conditions, nodes, and resources
- Required condition type validations ensuring data integrity
hack/update-payload-crds.sh (1)
33-33: LGTM!The glob pattern follows the existing conventions in the file and correctly includes the new PacemakerCluster CRD manifests in the payload.
etcd/v1alpha1/Makefile (1)
1-3: LGTM!The Makefile provides the requested test target for running v1alpha1-specific tests. The static analysis warnings about missing
allandcleantargets are false positives—this minimal Makefile appropriately serves only to run focused tests, which is consistent with the project's patterns.etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
1-455: CRD structure is consistent with DevPreviewNoUpgrade variant.The manifest correctly applies the
CustomNoUpgradefeature-set annotation and maintains structural consistency with other variants. Apart from the IP validation syntax issue (flagged separately), the schema and validations are well-defined.openapi/openapi.json (2)
9884-9884: LGTM!The
policyTypedescription is clear, comprehensive, and accurately documents all three allowed values and their usage contexts.
14013-14175: LGTM! Excellent API definitions with comprehensive documentation.The new PacemakerCluster API schema definitions are well-structured and thoroughly documented:
- All five schema definitions follow standard Kubernetes API conventions with proper metadata, type references, and list handling
- Descriptions are exceptionally detailed, explaining singleton patterns, compatibility levels, validation requirements, and field semantics
- Required fields are appropriately marked (metadata-only for top-level objects, specific fields for status types)
- List types use proper
x-kubernetes-list-typeandx-kubernetes-list-map-keysannotations- Status fields correctly follow the convention of being optional with clear explanations
- Past review issues (typos, duplicate text) have been addressed
The API surface is suitable for the two-node fencing use case and provides CEO consumers with clear guidance on expected structure and constraints.
etcd/v1alpha1/zz_generated.deepcopy.go (1)
1-164: Generated deepcopy code correctly aligns with type definitions.The autogenerated DeepCopy implementations properly handle:
- Pointer fields (
Status *PacemakerClusterStatus)- Slice fields with per-element deep copying (
Conditions,Nodes,Resources,IPAddresses)- Embedded
metav1.TimeviaDeepCopyIntoNo orphaned types or missing field handling detected.
openapi/generated_openapi/zz_generated.openapi.go (2)
523-527: LGTM! Schema registrations are correct.The five PacemakerCluster-related type registrations in the GetOpenAPIDefinitions map follow the established pattern and are properly placed in alphabetical order. Each entry correctly maps the fully-qualified type name to its corresponding schema function.
25298-25587: Validation constraints have been successfully implemented.The kubebuilder validation markers referenced in the previous review have been properly added to the source types (
etcd/v1alpha1/types_pacemakercluster.go):
- MinItems constraints for all required condition arrays (3, 7, and 8 items respectively)
- Enum constraint for the resource name field (Kubelet, Etcd, FencingAgent)
- Additional XValidation rules for cluster naming and timestamp immutability
The generated CRD manifests have been successfully regenerated with these constraints enforced. The OpenAPI schema file (
zz_generated.openapi.go) does not include these constraint properties in its SchemaProps because OpenAPI schema generation and Kubernetes CRD validation are separate concerns—runtime validation is properly enforced through the CRD manifests, while OpenAPI schemas serve as external API documentation.etcd/v1alpha1/types_pacemakercluster.go (5)
1-73: LGTM: Well-structured cluster-level conditions.The cluster-level condition types and reasons are clearly defined with comprehensive documentation. The aggregate nature of the Healthy condition is properly documented.
75-203: LGTM: Comprehensive node-level health tracking.The node-level conditions cover all necessary aspects of node health in a pacemaker cluster. The documentation clearly distinguishes between normal operating states and unexpected states.
205-378: LGTM: Complete resource health model.The resource-level conditions and enum definition properly model pacemaker resource states. The distinction between temporary states (e.g., stopped during quorum recovery) and unexpected states is well-documented.
380-457: LGTM: Properly enforced singleton semantics and status structure.The PacemakerCluster type correctly enforces singleton semantics with the name validation. The PacemakerClusterStatus structure appropriately uses conditions-based health reporting with required condition type enforcement and lastUpdated immutability guarantees.
459-508: LGTM: Node and resource status structures are well-designed.The PacemakerClusterNodeStatus and PacemakerClusterResourceStatus types properly model per-node and per-resource health with comprehensive condition requirements. The RFC 1123 subdomain validation for node names and the enum-based resource naming ensure data integrity. The list type follows standard Kubernetes conventions.
Also applies to: 510-594
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml
Show resolved
Hide resolved
|
/retest-required |
jaypoulz
force-pushed
the
OCPEDGE-2084
branch
from
0492c73 to
0938acb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
etcd/README.md (1)
1-148: Comprehensive documentation for the PacemakerCluster API.The README thoroughly documents:
- Feature gate and component information
- Status structure with all condition types
- Validation rules for naming, IPs, and timestamps
- Usage context for cluster-etcd-operator
A past reviewer noted this may duplicate content in the v1alpha1 directory. Consider consolidating to a single location to reduce maintenance drift, though this is not a blocking concern for the PR.
etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
257-260: Same CEL syntax error as TechPreviewNoUpgrade variant.The
ip.isCanonical(self)syntax is invalid. This should beip(self).isCanonical().Since this is a generated file, ensure the source type definitions or CRD generation templates are fixed so all variants are corrected together.
etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (1)
257-260: CEL validation syntax error for ipAddress -ip.isCanonical(self)is invalid.The CEL rule uses incorrect syntax. According to the Kubernetes CEL IP library,
ip()is a function that converts a string to an IP object, andisCanonical()is a method on that object. The current rule inconsistently mixes correct and incorrect syntax:ip(self).isGlobalUnicast()is correct, butip.isCanonical(self)is incorrect and will cause runtime validation errors.x-kubernetes-validations: - message: must be a valid global unicast IPv4 or IPv6 address in canonical form - rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (1)
257-260: Fix CEL IP validation: useip(self).isCanonical()instead ofip.isCanonical(self).The rule incorrectly calls
ip.isCanonical(self). Per the Kubernetes CEL IP library pattern and as confirmed by reviewer JoelSpeed, the correct syntax isip(self).isCanonical()(parsing the string to an IP object first, then calling the method). The current mixed usage indicates a bug that slipped through during pattern migration.- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (22)
etcd/README.md(1 hunks)etcd/install.go(1 hunks)etcd/v1alpha1/Makefile(1 hunks)etcd/v1alpha1/doc.go(1 hunks)etcd/v1alpha1/register.go(1 hunks)etcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml(1 hunks)etcd/v1alpha1/types_pacemakercluster.go(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml(1 hunks)etcd/v1alpha1/zz_generated.crd-manifests/doc.go(1 hunks)etcd/v1alpha1/zz_generated.deepcopy.go(1 hunks)etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yaml(1 hunks)etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml(1 hunks)etcd/v1alpha1/zz_generated.swagger_doc_generated.go(1 hunks)hack/update-payload-crds.sh(1 hunks)install.go(2 hunks)openapi/generated_openapi/zz_generated.openapi.go(2 hunks)openapi/openapi.json(2 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml(1 hunks)payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml
- etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yaml
- etcd/install.go
- etcd/v1alpha1/register.go
- hack/update-payload-crds.sh
- etcd/v1alpha1/types_pacemakercluster.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
etcd/v1alpha1/Makefileetcd/v1alpha1/doc.goetcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yamletcd/v1alpha1/zz_generated.crd-manifests/doc.goetcd/v1alpha1/zz_generated.swagger_doc_generated.goetcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yamletcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yamletcd/README.mdpayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yamlinstall.goetcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yamlopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonetcd/v1alpha1/zz_generated.deepcopy.go
🧬 Code graph analysis (2)
install.go (2)
etcd/install.go (1)
Install(17-17)etcd/v1alpha1/register.go (1)
Install(14-14)
etcd/v1alpha1/zz_generated.deepcopy.go (2)
etcd/v1alpha1/types_pacemakercluster.go (5)
PacemakerCluster(396-409)PacemakerClusterStatus(416-457)PacemakerClusterList(585-594)PacemakerClusterNodeStatus(461-527)PacemakerClusterResourceStatus(536-576)etcd/v1alpha1/zz_generated.swagger_doc_generated.go (5)
PacemakerCluster(20-22)PacemakerClusterStatus(63-65)PacemakerClusterList(30-32)PacemakerClusterNodeStatus(42-44)PacemakerClusterResourceStatus(52-54)
🪛 checkmake (0.2.2)
etcd/v1alpha1/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
🔇 Additional comments (16)
install.go (1)
57-57: LGTM!The etcd API group import and registration follow the established pattern used by other OpenShift API groups. The import is correctly placed in alphabetical order, and
etcd.Installis properly added to the scheme builder.Also applies to: 93-93
etcd/v1alpha1/doc.go (1)
1-6: LGTM!The code generation annotations follow the standard pattern for OpenShift API types. The
+groupName=etcd.openshift.iocorrectly identifies the API group.etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (1)
1-455: Well-structured CRD with comprehensive validations.The CRD schema is thorough with:
- Appropriate MinItems/MaxItems constraints
- CEL validations ensuring required conditions are present
- Transition rules preventing
lastUpdatedremoval or backward timestamps- Singleton naming enforcement
The only issue requiring attention is the CEL syntax error noted above for ipAddress validation.
etcd/v1alpha1/zz_generated.crd-manifests/doc.go (1)
1-1: LGTM!Standard package declaration for the generated CRD manifests directory.
etcd/v1alpha1/Makefile (1)
1-3: LGTM!The test target correctly focuses on the etcd.openshift.io/v1alpha1 API tests. The static analysis warnings about missing
allandcleantargets are not applicable here—this is a minimal Makefile for test execution, consistent with the pattern used by other API packages.etcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml (2)
1-25: Comprehensive test coverage for PacemakerCluster CRD.The test suite covers:
- Creation with valid/invalid names (singleton enforcement)
- Full healthy cluster status with all required conditions
- IP address validation (invalid, loopback rejection)
- Node name validation (RFC 1123 subdomain)
- Missing required conditions and resources
This provides good confidence in the CRD validation behavior.
413-610: The CEL validation rule syntax in the CRD is correct and requires no changes. Bothip.isCanonical(self)(standalone function form) andip(self).isGlobalUnicast()(parse-then-method form) are valid Kubernetes CEL syntax according to the CEL library documentation. There is no syntax error to fix, and therefore no changes or verification needed for this test file.Likely an incorrect or invalid review comment.
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
1-455: LGTM - Well-structured CRD manifest.The CustomNoUpgrade variant is correctly structured with:
- Appropriate feature-set annotation
- Singleton enforcement via CEL validation (
self.metadata.name == 'cluster')- Monotonic timestamp validation for
lastUpdated- Comprehensive condition type validations at cluster, node, and resource levels
- Proper IP address validation using CEL
etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (1)
1-455: Generated CRD is consistent with source types.The DevPreviewNoUpgrade variant correctly mirrors the schema defined in
types_pacemakercluster.gowith appropriate feature-set annotation. The validation rules and structure are consistent across CRD variants.etcd/v1alpha1/zz_generated.swagger_doc_generated.go (1)
1-67: Generated swagger documentation is correctly aligned with type definitions.The SwaggerDoc maps accurately document all fields from the corresponding Go types:
PacemakerClusterStatus: conditions, lastUpdated, nodesPacemakerClusterNodeStatus: conditions, name, ipAddresses, resourcesPacemakerClusterResourceStatus: conditions, nameetcd/v1alpha1/zz_generated.deepcopy.go (1)
1-164: Generated deepcopy implementations are correct and complete.The deepcopy functions properly handle:
- Pointer fields (Status in PacemakerCluster)
- Slice fields with nested structs (Conditions, Items, Nodes, Resources)
- metav1.Time fields (LastUpdated)
All implementations align with the type definitions in
types_pacemakercluster.go.payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (1)
1-455: Payload CRD manifest is well-structured and consistent.The DevPreviewNoUpgrade payload manifest correctly implements:
- Singleton semantics via
self.metadata.name == 'cluster'validation- Monotonic timestamp constraints for
lastUpdated- Required condition type validations at all levels
- Proper list-map semantics with appropriate keys
The schema is consistent with the generated CRD in
etcd/v1alpha1/zz_generated.crd-manifests/.payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (1)
1-455: LGTM: Comprehensive CRD schema with thorough validations.The CRD is well-structured with extensive validation rules covering:
- Singleton enforcement via CEL (metadata.name == 'cluster')
- Required condition types at cluster, node, and resource levels
- Immutability constraints on lastUpdated timestamp
- Proper list configurations with map keys
- Clear, detailed field descriptions
All previously identified issues (singleton validation, lastUpdated semantics) have been addressed.
openapi/generated_openapi/zz_generated.openapi.go (2)
523-527: LGTM! Schema registrations correctly integrated.The five new PacemakerCluster API schemas are properly registered in GetOpenAPIDefinitions with correct package paths and function references, maintaining alphabetical order.
25298-25587: LGTM! Schema definitions are correctly generated.The five OpenAPI schema functions for PacemakerCluster resources are well-structured and follow Kubernetes API conventions. The schemas include comprehensive descriptions documenting validation constraints (MinItems requirements for conditions, valid enum values for resource names, IP address format requirements, timestamp immutability). These constraints are typically enforced in the CRD manifests rather than OpenAPI schemas, which this PR includes per the summary mentioning "strict status validations."
openapi/openapi.json (1)
14013-14175: LGTM! Well-structured PacemakerCluster API definitions.The new etcd.openshift.io/v1alpha1 PacemakerCluster schema definitions follow Kubernetes conventions correctly:
- Proper singleton resource pattern with cluster scope and "cluster" instance name
- Appropriate x-kubernetes-list-type annotations (map with type/name keys, set for IP addresses)
- Comprehensive descriptions explaining pacemaker concepts and Two Node OpenShift with Fencing usage
- Previous review issues (duplicate text, validation constraints, typos) have been addressed
| > **Note on optional fields:** Per Kubernetes API conventions, all status fields are marked `+optional` in the Go | ||
| > type definitions to allow for partial updates and backward compatibility. However, CEL validation rules enforce | ||
| > that certain fields (like conditions and lastUpdated) cannot be removed once set. This means these fields are | ||
| > "optional on initial creation, but required once present." | ||
| > See: [kube-api-linter statusoptional](https://github.com/kubernetes-sigs/kube-api-linter/blob/main/pkg/analysis/statusoptional/doc.go) |
There was a problem hiding this comment.
We are dropping this requirement, it's better to be able to actually make the fields required
There was a problem hiding this comment.
Does this mean I should bump the linter in the go.mod?
There was a problem hiding this comment.
We should make that bump yes, if you have time to do it, go for it, else I'll try get to it in the next few days
| - type: Healthy | ||
| - type: InService | ||
| - type: NodeCountAsExpected | ||
| lastUpdated: <timestamp> # When status was last updated (required once set, cannot decrease) |
There was a problem hiding this comment.
Is there a way to be confident that things are still working without a clock that will cause writes every 30 seconds?
Node's in Kube have similar properties that haven't aged well, mainly because of scale reasons which I guess is less of an issue here, but still, wonder how else we could be confident things were still good without a ticking clock
There was a problem hiding this comment.
I asked Claude to scan through the existing APIs and review our options for this. The best it could come up with was moving the staleness check into the health checker:
// In healthcheck controller
if cr.ResourceVersion == c.lastSeenResourceVersion {
timeSinceLastChange := time.Since(c.lastResourceVersionTime)
if timeSinceLastChange > threshold {
// Status is stale
}
}
But this still has the problem that if the status is healthy / stable for > 5 minutes, we don't record a heartbeat, so the status becomes stale.
The core objective of the API is to prevent users from entering a dangerous state without a warning provided in the cluster. I think the only way to achieve this is with some kind of heartbeat. The lastUpdated mechanism is simplest I could come up with.
There was a problem hiding this comment.
And the lastUpdated will tick every time you reconcile, not every time there is a particular change, right?
| lastUpdated: <timestamp> # When status was last updated (required once set, cannot decrease) | ||
| nodes: # Per-node status (0-32 nodes, expects 2) | ||
| - name: <hostname> # RFC 1123 subdomain name | ||
| ipAddresses: # List of global unicast IPv4 or IPv6 addresses in canonical form |
There was a problem hiding this comment.
In Nodes, these are just addresses, maybe we want to be consistent
Also, maybe we want to copy the structure where we can add details to addresses later. Would mean making this a list of struct
addresses:
- address: <ip>
type: ??? # Future expansion of the API? Other metadata?
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (9)
etcd/README.md (1)
1-148: Address documentation duplication concern from past review.A past reviewer noted this README duplicates content in the v1alpha1 directory and suggested it's not needed in both locations. Consider consolidating to a brief summary here with a pointer to the canonical v1alpha1 documentation to avoid maintenance drift.
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
257-260: Fix CEL syntax error in IP validation.The CEL expression incorrectly calls
ip.isCanonical(self). Theipfunction must be invoked with the string parameter to parse it and return an IP object before calling theisCanonical()method.🔎 Proposed fix
- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()Note: This same fix must be applied to all PacemakerCluster CRD variants (DevPreviewNoUpgrade, TechPreviewNoUpgrade, and zz_generated CRDs). Regenerate from source types to ensure consistency across all manifests.
openapi/openapi.json (4)
10161-10161: Grammar error still present despite previous fix attempt.The description still contains "is a required field specifies" which should be "is a required field that specifies".
Since this is a generated file, fix the source Go struct field comment (likely in the config API types) and regenerate this file by running the project's OpenAPI generation command.
🔎 Proposed fix
- "description": "policyType is a required field specifies the type of the policy for verification. This field must correspond to how the policy was generated. Allowed values are \"PublicKey\", \"FulcioCAWithRekor\", and \"PKI\". When set to \"PublicKey\", the policy relies on a sigstore publicKey and may optionally use a Rekor verification. When set to \"FulcioCAWithRekor\", the policy is based on the Fulcio certification and incorporates a Rekor verification. When set to \"PKI\", the policy is based on the certificates from Bring Your Own Public Key Infrastructure (BYOPKI).", + "description": "policyType is a required field that specifies the type of the policy for verification. This field must correspond to how the policy was generated. Allowed values are \"PublicKey\", \"FulcioCAWithRekor\", and \"PKI\". When set to \"PublicKey\", the policy relies on a sigstore publicKey and may optionally use a Rekor verification. When set to \"FulcioCAWithRekor\", the policy is based on the Fulcio certification and incorporates a Rekor verification. When set to \"PKI\", the policy is based on the certificates from Bring Your Own Public Key Infrastructure (BYOPKI).",
14346-14393: Missing validation constraints and typo in PacemakerClusterNodeStatus schema.Despite being marked as addressed in previous reviews, the following issues are still present:
- Line 14368: Typo - "is a list IPv4" should be "is a list of IPv4"
- Lines 14355-14366:
conditionsarray is missingminItems: 7constraint (documented in description)- Lines 14367-14375:
ipAddressesarray is missingminItems: 1,maxItems: 8, and pattern validation for canonical IPv4/IPv6 format- Lines 14380-14391:
resourcesarray is missingminItems: 3,maxItems: 8constraintsThese constraints are documented in the descriptions and should be present in the source CRD manifests. Fix the source Go type comments for the typo and ensure the OpenAPI generation includes the validation constraints from the CRD.
14394-14418: Missing validation constraints in PacemakerClusterResourceStatus schema.Despite being marked as addressed in previous reviews, the following constraints are still missing:
- Lines 14401-14412:
conditionsarray is missingminItems: 8,maxItems: 16constraints (documented in description)- Lines 14413-14416:
namefield is missingenum: ["Kubelet", "Etcd", "FencingAgent"]constraint (valid values documented in description)Ensure the OpenAPI generation process includes these validation constraints from the source CRD manifests.
14419-14452: Missing validation constraints in PacemakerClusterStatus schema.Despite being marked as addressed in previous reviews, the following constraints are still missing:
- Lines 14423-14434:
conditionsarray is missingminItems: 3,maxItems: 8constraints (documented in description)- Lines 14439-14450:
nodesarray is missingmaxItems: 32constraint (documented in description)Ensure the OpenAPI generation process includes these validation constraints from the source CRD manifests.
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (1)
100-106: Inconsistent enum value quoting for condition status.The
statusenum mixes quoted ("True","False") and unquoted (Unknown) values. For consistency and clarity in YAML schema definitions, all string enum values should be quoted uniformly.🔎 Suggested fix to quote all enum values consistently
status: description: status of the condition, one of True, False, Unknown. enum: - "True" - "False" - - Unknown + - "Unknown" type: stringApply this same fix at:
- Line 105 (cluster-level conditions)
- Line 211 (node-level conditions)
- Line 355 (resource-level conditions)
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (1)
100-106: Inconsistent enum value quoting for condition status.The
statusenum mixes quoted ("True","False") and unquoted (Unknown) values. For consistency and clarity in YAML schema definitions, all string enum values should be quoted uniformly.🔎 Suggested fix to quote all enum values consistently
status: description: status of the condition, one of True, False, Unknown. enum: - "True" - "False" - - Unknown + - "Unknown" type: stringApply this same fix at:
- Line 105 (cluster-level conditions)
- Line 211 (node-level conditions)
- Line 355 (resource-level conditions)
etcd/v1alpha1/types_pacemakercluster.go (1)
509-509: Critical: Fix CEL syntax error in IP address validation.The CEL expression has incorrect syntax:
ip.isCanonical(self)should beip(self).isCanonical(). According to the Kubernetes CEL IP library documentation, the correct pattern isip(<string>).<method>().The validation rule should be:
isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()This syntax error will cause CRD validation to fail when manifests are applied.
🔎 Proposed fix
- // +kubebuilder:validation:items:XValidation:rule="isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast()",message="must be a valid global unicast IPv4 or IPv6 address in canonical form" + // +kubebuilder:validation:items:XValidation:rule="isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()",message="must be a valid global unicast IPv4 or IPv6 address in canonical form"After fixing this, regenerate the CRD manifests to ensure the corrected validation is applied.
🧹 Nitpick comments (1)
etcd/v1alpha1/Makefile (1)
1-3: LGTM! Test target is correctly implemented.The test target properly invokes the centralized test suite with the appropriate Ginkgo focus filter for this API group.
Optional: Add trailing newline.
A previous review requested a trailing newline at the end of the file to maintain POSIX compliance and prevent editors from automatically adding it. This remains unaddressed.
🔎 Suggested fix
.PHONY: test test: make -C ../../tests test GINKGO_EXTRA_ARGS=--focus="etcd.openshift.io/v1alpha1" +Note on static analysis warnings: The checkmake warnings about missing "all" and "clean" targets can likely be ignored—this is a specialized test-only Makefile in an API version subdirectory, and those targets are not typically required at this level.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (5)
etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*etcd/v1alpha1/zz_generated.crd-manifests/doc.gois excluded by!**/zz_generated.crd-manifests/*etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (17)
etcd/README.mdetcd/install.goetcd/v1alpha1/Makefileetcd/v1alpha1/doc.goetcd/v1alpha1/register.goetcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yamletcd/v1alpha1/types_pacemakercluster.goetcd/v1alpha1/zz_generated.deepcopy.goetcd/v1alpha1/zz_generated.featuregated-crd-manifests.yamletcd/v1alpha1/zz_generated.swagger_doc_generated.gohack/update-payload-crds.shinstall.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonpayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- etcd/v1alpha1/zz_generated.deepcopy.go
- etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yaml
- etcd/v1alpha1/doc.go
- etcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml
🧰 Additional context used
🧬 Code graph analysis (4)
etcd/v1alpha1/register.go (1)
etcd/install.go (2)
GroupName(11-11)Resource(20-22)
etcd/v1alpha1/zz_generated.swagger_doc_generated.go (1)
etcd/v1alpha1/types_pacemakercluster.go (5)
PacemakerCluster(396-409)PacemakerClusterList(585-594)PacemakerClusterNodeStatus(461-527)PacemakerClusterResourceStatus(536-576)PacemakerClusterStatus(416-457)
install.go (4)
etcd/install.go (1)
Install(17-17)etcd/v1alpha1/register.go (1)
Install(14-14)apiextensions/install.go (1)
Install(17-17)apiextensions/v1alpha1/register.go (1)
Install(14-14)
etcd/v1alpha1/types_pacemakercluster.go (1)
etcd/v1alpha1/zz_generated.swagger_doc_generated.go (4)
PacemakerClusterStatus(63-65)PacemakerClusterNodeStatus(42-44)PacemakerClusterResourceStatus(52-54)PacemakerCluster(20-22)
🪛 checkmake (0.2.2)
etcd/v1alpha1/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
🔇 Additional comments (26)
hack/update-payload-crds.sh (1)
33-33: LGTM!The glob pattern correctly follows the established convention and will properly copy the PacemakerCluster CRD manifests to the payload directory.
install.go (1)
58-58: LGTM!The etcd API group integration follows the established pattern used by other OpenShift API groups. The import and schemeBuilder registration are correct.
Also applies to: 95-95
etcd/README.md (2)
45-45: Verify the design decision regarding timestamp-based status updates.A past reviewer raised concerns about the
lastUpdatedfield requiring writes every 30 seconds, noting that similar patterns in Node API "haven't aged well, mainly because of scale reasons." While scale is less of an issue with the two-node constraint, consider whether:
- The periodic update pattern is necessary for detecting stale status, or
- Alternative signals (conditions, heartbeats, or external monitoring) could confirm health without constant writes.
Could you clarify the design rationale and whether alternatives were considered?
48-49: Consider consistency with Kubernetes Node API for addresses structure.A past reviewer suggested using a structure similar to Kubernetes Nodes where addresses are objects with a type field to allow future expansion:
addresses: - address: <ip> type: ??? # Future expansionversus the current flat list of IP strings. Was this design choice intentional? If so, documenting the rationale (e.g., Pacemaker-specific constraints) would help future maintainers understand why the simpler structure was chosen.
etcd/install.go (1)
1-26: LGTM!The etcd package scaffolding follows Kubernetes API conventions and the established OpenShift pattern. The GroupName, schemeBuilder initialization, Install function, and helper constructors are all correctly implemented.
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
440-445: LGTM!The timestamp validation correctly enforces that
lastUpdatedcannot decrease. The use of>=allows idempotent updates (same timestamp), which is appropriate, and the validation message accurately describes the constraint.etcd/v1alpha1/zz_generated.swagger_doc_generated.go (1)
1-67: LGTM! Generated Swagger documentation is consistent with type definitions.The auto-generated Swagger documentation correctly documents all five PacemakerCluster-related types and their fields. The previous issue with phantom fields has been successfully addressed, and the current documentation maps accurately reflect the actual type definitions from
types_pacemakercluster.go.etcd/v1alpha1/register.go (1)
1-39: LGTM! Standard API registration scaffolding.The registration code follows established Kubernetes API patterns correctly:
- Proper GroupName and GroupVersion definitions
- Standard schemeBuilder initialization with addKnownTypes
- Correct deprecated aliases for backward compatibility with old generators
- Both PacemakerCluster and PacemakerClusterList are registered in the correct order
openapi/openapi.json (1)
14290-14345: PacemakerCluster and PacemakerClusterList definitions look good.The duplicate text issue flagged in previous reviews has been successfully fixed. The descriptions are clear, accurate, and follow Kubernetes API conventions for status-only resources.
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (4)
1-10: LGTM: Metadata and annotations are properly configured.The CRD metadata correctly declares the API approval, feature-gate merging, release inclusion, and DevPreviewNoUpgrade feature-set.
11-46: LGTM: CRD spec and standard properties are correctly defined.The API group, resource names, cluster scope, and standard properties follow Kubernetes conventions.
449-451: LGTM: Singleton name validation correctly enforces the "cluster" instance name.The CEL validation rule properly restricts PacemakerCluster instances to the singleton name "cluster" as documented.
47-448: LGTM: Comprehensive status schema with robust validations.The status-only schema is well-designed with:
- Required condition types enforced via CEL validations at cluster, node, and resource levels
- Proper list-map semantics with appropriate keys
- Immutability rules for
lastUpdatedtimestamp- Detailed field constraints (maxItems, minItems, maxLength, patterns)
- Clear documentation of pacemaker concepts and expected values
The extensive CEL validations will help ensure data integrity and provide clear error messages to consumers.
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (3)
1-10: LGTM: Metadata correctly configured for TechPreviewNoUpgrade feature-set.The CRD metadata properly declares the TechPreviewNoUpgrade feature-set with appropriate annotations.
449-451: LGTM: Singleton name validation and overall schema structure.The CEL validation correctly enforces the singleton "cluster" name, and the overall CRD structure mirrors the DevPreviewNoUpgrade variant appropriately with comprehensive status validations.
47-455: LGTM: Consistent CRD structure across feature-set variants.This TechPreviewNoUpgrade variant correctly mirrors the DevPreviewNoUpgrade CRD structure with the same comprehensive status schema, CEL validations, and documentation. The parallel structure ensures consistent API surface across feature-sets.
openapi/generated_openapi/zz_generated.openapi.go (4)
534-538: LGTM!The registration of the new etcd/v1alpha1 types follows the established pattern and maintains alphabetical ordering.
25794-25835: Well-structured status-only resource schema.The PacemakerCluster schema correctly implements the status-only pattern (no spec field) as intended for this informational CRD. The compatibility level 4 documentation clearly sets expectations for API stability.
26049-26054: The monotonic timestamp constraint onlastUpdatedis already enforced through CEL validation rules defined in the source types file (etcd/v1alpha1/types_pacemakercluster.golines 414-415). These kubebuilder validation annotations enforce both that the field cannot be removed once set and that it cannot be set to an earlier timestamp than the current value. No action required.
25888-25973: MinItems constraints for PacemakerClusterNodeStatus are properly enforced.Verification confirms:
- Kubebuilder markers are present in source types:
MinItems=7for conditions andMinItems=3for resources- Constraints are reflected in the CRD manifest: lines show
minItems: 7andminItems: 3in the correct schema contexts- Validation is enforced at the Kubernetes API level through the CRD definition
etcd/v1alpha1/types_pacemakercluster.go (6)
7-360: LGTM: Comprehensive condition type definitions.The condition types and reasons are well-structured across three levels (cluster, node, resource) with clear documentation explaining normal vs unexpected states and aggregation relationships. The naming conventions are consistent and the documentation effectively explains when each condition is True/False.
361-378: LGTM: Resource name enum is properly defined.The
PacemakerClusterResourceNameenum with kubebuilder validation correctly constrains the allowed resource names to the three expected values (Kubelet, Etcd, FencingAgent), and each constant is well-documented.
380-409: LGTM: PacemakerCluster type is properly defined.The main type definition includes appropriate annotations for a cluster-scoped singleton resource, with CEL validation enforcing the singleton name constraint, proper feature gating, and compatibility level marking. The status-only semantics are correctly represented.
411-457: LGTM: Status structure with robust validation.The status structure properly enforces invariants like non-removable/non-decreasing
lastUpdatedand required condition types via CEL validation. The field optionality follows Kubernetes API conventions while MinItems validations ensure completeness once populated.
529-576: LGTM: Resource status properly structured.The resource status structure correctly enforces all 8 required condition types via CEL validation and uses the enum type for the resource name. The documentation clearly explains what pacemaker resources represent in this context.
578-594: LGTM: List type follows Kubernetes conventions.The
PacemakerClusterListtype is properly defined following standard Kubernetes API conventions. The documentation appropriately notes that this list type exists only for API compliance since PacemakerCluster is a singleton.
| apiVersion: apiextensions.k8s.io/v1 | ||
| kind: CustomResourceDefinition | ||
| metadata: | ||
| annotations: | ||
| api-approved.openshift.io: https://github.com/openshift/api/pull/2544 | ||
| api.openshift.io/merged-by-featuregates: "true" | ||
| include.release.openshift.io/ibm-cloud-managed: "true" | ||
| include.release.openshift.io/self-managed-high-availability: "true" | ||
| release.openshift.io/feature-set: CustomNoUpgrade | ||
| name: pacemakerclusters.etcd.openshift.io | ||
| spec: | ||
| group: etcd.openshift.io | ||
| names: | ||
| kind: PacemakerCluster | ||
| listKind: PacemakerClusterList | ||
| plural: pacemakerclusters | ||
| singular: pacemakercluster | ||
| scope: Cluster | ||
| versions: | ||
| - name: v1alpha1 | ||
| schema: | ||
| openAPIV3Schema: | ||
| description: |- | ||
| PacemakerCluster represents the current state of the pacemaker cluster as reported by the pcs status command. | ||
| PacemakerCluster is a cluster-scoped singleton resource. The name of this instance is "cluster". This | ||
| resource provides a view into the health and status of a pacemaker-managed cluster in Two Node OpenShift with Fencing deployments. | ||
| Compatibility level 4: No compatibility is provided, the API can change at any point for any reason. These capabilities should not be used by applications needing long term support. | ||
| properties: | ||
| apiVersion: | ||
| description: |- | ||
| APIVersion defines the versioned schema of this representation of an object. | ||
| Servers should convert recognized schemas to the latest internal value, and | ||
| may reject unrecognized values. | ||
| More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources | ||
| type: string | ||
| kind: | ||
| description: |- | ||
| Kind is a string value representing the REST resource this object represents. | ||
| Servers may infer this from the endpoint the client submits requests to. | ||
| Cannot be updated. | ||
| In CamelCase. | ||
| More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds | ||
| type: string | ||
| metadata: | ||
| type: object | ||
| status: | ||
| description: |- | ||
| status contains the actual pacemaker cluster status information collected from the cluster. | ||
| The goal of this status is to be able to quickly identify if pacemaker is in a healthy state. | ||
| In Two Node OpenShift with Fencing, a healthy pacemaker cluster has 2 nodes, both of which have healthy kubelet, etcd, and fencing resources. | ||
| properties: | ||
| conditions: | ||
| description: |- | ||
| conditions represent the observations of the pacemaker cluster's current state. | ||
| Known condition types are: "Healthy", "InService", "NodeCountAsExpected". | ||
| The "Healthy" condition is an aggregate that tracks the overall health of the cluster. | ||
| The "InService" condition tracks whether the cluster is in service (not in maintenance mode). | ||
| The "NodeCountAsExpected" condition tracks whether the expected number of nodes are present. | ||
| Each of these conditions is required, so the array must contain at least 3 items. | ||
| Conditions is marked optional per Kubernetes API conventions because controllers populate conditions | ||
| after observing state. The MinItems validation ensures conditions are present once the controller | ||
| has reported status. | ||
| See: https://github.com/kubernetes-sigs/kube-api-linter/blob/main/pkg/analysis/conditions/doc.go | ||
| items: | ||
| description: Condition contains details for one aspect of the current | ||
| state of this API Resource. | ||
| properties: | ||
| lastTransitionTime: | ||
| description: |- | ||
| lastTransitionTime is the last time the condition transitioned from one status to another. | ||
| This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. | ||
| format: date-time | ||
| type: string | ||
| message: | ||
| description: |- | ||
| message is a human readable message indicating details about the transition. | ||
| This may be an empty string. | ||
| maxLength: 32768 | ||
| type: string | ||
| observedGeneration: | ||
| description: |- | ||
| observedGeneration represents the .metadata.generation that the condition was set based upon. | ||
| For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date | ||
| with respect to the current state of the instance. | ||
| format: int64 | ||
| minimum: 0 | ||
| type: integer | ||
| reason: | ||
| description: |- | ||
| reason contains a programmatic identifier indicating the reason for the condition's last transition. | ||
| Producers of specific condition types may define expected values and meanings for this field, | ||
| and whether the values are considered a guaranteed API. | ||
| The value should be a CamelCase string. | ||
| This field may not be empty. | ||
| maxLength: 1024 | ||
| minLength: 1 | ||
| pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ | ||
| type: string | ||
| status: | ||
| description: status of the condition, one of True, False, Unknown. | ||
| enum: | ||
| - "True" | ||
| - "False" | ||
| - Unknown | ||
| type: string | ||
| type: | ||
| description: type of condition in CamelCase or in foo.example.com/CamelCase. | ||
| maxLength: 316 | ||
| pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ | ||
| type: string | ||
| required: | ||
| - lastTransitionTime | ||
| - message | ||
| - reason | ||
| - status | ||
| - type | ||
| type: object | ||
| maxItems: 8 | ||
| minItems: 3 | ||
| type: array | ||
| x-kubernetes-list-map-keys: | ||
| - type | ||
| x-kubernetes-list-type: map | ||
| x-kubernetes-validations: | ||
| - message: conditions must contain a condition of type Healthy | ||
| rule: self.exists(c, c.type == 'Healthy') | ||
| - message: conditions must contain a condition of type InService | ||
| rule: self.exists(c, c.type == 'InService') | ||
| - message: conditions must contain a condition of type NodeCountAsExpected | ||
| rule: self.exists(c, c.type == 'NodeCountAsExpected') | ||
| lastUpdated: | ||
| description: |- | ||
| lastUpdated is the timestamp when this status was last updated. This is useful for identifying | ||
| stale status reports. It must be a valid timestamp in RFC3339 format. Once set, this field cannot | ||
| be removed and cannot be set to an earlier timestamp than the current value. | ||
| This field is marked optional because all status fields must be optional to allow for partial updates | ||
| and backward compatibility. However, once set it must always be present. | ||
| See: https://github.com/kubernetes-sigs/kube-api-linter/blob/main/pkg/analysis/statusoptional/doc.go | ||
| format: date-time | ||
| type: string | ||
| nodes: | ||
| description: |- | ||
| nodes provides detailed information about each node in the cluster including per-node resource health. | ||
| Each node entry includes the node's name, IP address, conditions, and resource status. | ||
| The list can contain up to 32 nodes (the upper limit imposed by pacemaker). | ||
| For Two Node OpenShift with Fencing, exactly 2 nodes are expected in a healthy cluster. An empty list indicates no nodes | ||
| are currently reporting status. | ||
| items: | ||
| description: |- | ||
| PacemakerClusterNodeStatus represents the status of a single node in the pacemaker cluster including | ||
| the node's conditions and the health of critical resources running on that node. | ||
| properties: | ||
| conditions: | ||
| description: |- | ||
| conditions represent the observations of the node's current state. | ||
| Known condition types are: "Healthy", "Online", "InService", "Active", "Ready", "Clean", "Member". | ||
| The "Healthy" condition is an aggregate that tracks the overall health of the node. | ||
| The "Online" condition tracks whether the node is online. | ||
| The "InService" condition tracks whether the node is in service (not in maintenance mode). | ||
| The "Active" condition tracks whether the node is active (not in standby mode). | ||
| The "Ready" condition tracks whether the node is ready (not in a pending state). | ||
| The "Clean" condition tracks whether the node is in a clean (status known) state. | ||
| The "Member" condition tracks whether the node is a member of the cluster. | ||
| Each of these conditions is required, so the array must contain at least 7 items. | ||
| Conditions is marked optional per Kubernetes API conventions because controllers populate conditions | ||
| after observing state. The MinItems validation ensures conditions are present once the controller | ||
| has reported status. | ||
| See: https://github.com/kubernetes-sigs/kube-api-linter/blob/main/pkg/analysis/conditions/doc.go | ||
| items: | ||
| description: Condition contains details for one aspect of | ||
| the current state of this API Resource. | ||
| properties: | ||
| lastTransitionTime: | ||
| description: |- | ||
| lastTransitionTime is the last time the condition transitioned from one status to another. | ||
| This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. | ||
| format: date-time | ||
| type: string | ||
| message: | ||
| description: |- | ||
| message is a human readable message indicating details about the transition. | ||
| This may be an empty string. | ||
| maxLength: 32768 | ||
| type: string | ||
| observedGeneration: | ||
| description: |- | ||
| observedGeneration represents the .metadata.generation that the condition was set based upon. | ||
| For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date | ||
| with respect to the current state of the instance. | ||
| format: int64 | ||
| minimum: 0 | ||
| type: integer | ||
| reason: | ||
| description: |- | ||
| reason contains a programmatic identifier indicating the reason for the condition's last transition. | ||
| Producers of specific condition types may define expected values and meanings for this field, | ||
| and whether the values are considered a guaranteed API. | ||
| The value should be a CamelCase string. | ||
| This field may not be empty. | ||
| maxLength: 1024 | ||
| minLength: 1 | ||
| pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ | ||
| type: string | ||
| status: | ||
| description: status of the condition, one of True, False, | ||
| Unknown. | ||
| enum: | ||
| - "True" | ||
| - "False" | ||
| - Unknown | ||
| type: string | ||
| type: | ||
| description: type of condition in CamelCase or in foo.example.com/CamelCase. | ||
| maxLength: 316 | ||
| pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ | ||
| type: string | ||
| required: | ||
| - lastTransitionTime | ||
| - message | ||
| - reason | ||
| - status | ||
| - type | ||
| type: object | ||
| maxItems: 16 | ||
| minItems: 7 | ||
| type: array | ||
| x-kubernetes-list-map-keys: | ||
| - type | ||
| x-kubernetes-list-type: map | ||
| x-kubernetes-validations: | ||
| - message: conditions must contain a condition of type Healthy | ||
| rule: self.exists(c, c.type == 'Healthy') | ||
| - message: conditions must contain a condition of type Online | ||
| rule: self.exists(c, c.type == 'Online') | ||
| - message: conditions must contain a condition of type InService | ||
| rule: self.exists(c, c.type == 'InService') | ||
| - message: conditions must contain a condition of type Active | ||
| rule: self.exists(c, c.type == 'Active') | ||
| - message: conditions must contain a condition of type Ready | ||
| rule: self.exists(c, c.type == 'Ready') | ||
| - message: conditions must contain a condition of type Clean | ||
| rule: self.exists(c, c.type == 'Clean') | ||
| - message: conditions must contain a condition of type Member | ||
| rule: self.exists(c, c.type == 'Member') | ||
| ipAddresses: | ||
| description: |- | ||
| ipAddresses is a list of IPv4 or IPv6 addresses for the node in canonical form. | ||
| Pacemaker allows multiple IP addresses for Corosync communication between nodes. | ||
| The first address in this list is used for IP-based peer URLs for etcd membership. | ||
| Each address must be in canonical form (e.g., "192.168.1.1" not "192.168.001.001", or "2001:db8::1" not "2001:0db8::1"). | ||
| Each address must be a valid global unicast IPv4 or IPv6 address (including private/RFC1918 addresses). | ||
| This excludes special addresses like unspecified, loopback, link-local, and multicast. | ||
| items: | ||
| maxLength: 39 | ||
| type: string | ||
| x-kubernetes-validations: | ||
| - message: must be a valid global unicast IPv4 or IPv6 address | ||
| in canonical form | ||
| rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() | ||
| maxItems: 8 | ||
| minItems: 1 | ||
| type: array | ||
| x-kubernetes-list-type: set | ||
| name: | ||
| description: |- | ||
| name is the name of the node. This is expected to match the Kubernetes node's name, which must be a lowercase | ||
| RFC 1123 subdomain consisting of lowercase alphanumeric characters, '-' or '.', starting and ending with | ||
| an alphanumeric character, and be at most 253 characters in length. | ||
| maxLength: 253 | ||
| minLength: 1 | ||
| type: string | ||
| x-kubernetes-validations: | ||
| - message: name must be a lowercase RFC 1123 subdomain consisting | ||
| of lowercase alphanumeric characters, '-' or '.', and must | ||
| start and end with an alphanumeric character | ||
| rule: '!format.dns1123Subdomain().validate(self).hasValue()' | ||
| resources: | ||
| description: |- | ||
| resources contains the status of pacemaker resources on this node. | ||
| Each resource entry includes the resource name and its health conditions. | ||
| For Two Node OpenShift with Fencing, we manage three resources: Kubelet, Etcd, and FencingAgent. | ||
| All three resources are required to be present, so the array must contain at least 3 items. | ||
| Valid resource names are "Kubelet", "Etcd", and "FencingAgent". | ||
| items: | ||
| description: |- | ||
| PacemakerClusterResourceStatus represents the status of a pacemaker resource on a node. | ||
| A pacemaker resource is a unit of work managed by pacemaker. In pacemaker terminology, resources are services or | ||
| applications that pacemaker monitors, starts, stops, and moves between nodes to maintain high availability. | ||
| For Two Node OpenShift with Fencing, we manage three resources: | ||
| - Kubelet (the Kubernetes node agent and a prerequisite for etcd), | ||
| - Etcd (the distributed key-value store) | ||
| - FencingAgent (used to isolate failed nodes during a quorum loss event) | ||
| properties: | ||
| conditions: | ||
| description: |- | ||
| conditions represent the observations of the resource's current state. | ||
| Known condition types are: "Healthy", "InService", "Managed", "Enabled", "Operational", | ||
| "Active", "Started", "Schedulable". | ||
| The "Healthy" condition is an aggregate that tracks the overall health of the resource. | ||
| The "InService" condition tracks whether the resource is in service (not in maintenance mode). | ||
| The "Managed" condition tracks whether the resource is managed by pacemaker. | ||
| The "Enabled" condition tracks whether the resource is enabled. | ||
| The "Operational" condition tracks whether the resource is operational (not failed). | ||
| The "Active" condition tracks whether the resource is active (available to be used). | ||
| The "Started" condition tracks whether the resource is started. | ||
| The "Schedulable" condition tracks whether the resource is schedulable (not blocked). | ||
| Each of these conditions is required, so the array must contain at least 8 items. | ||
| Conditions is idiomatically the first field in status structs per Kubernetes API conventions. | ||
| Conditions is marked optional per Kubernetes API conventions because controllers populate conditions | ||
| after observing state. The MinItems validation ensures conditions are present once the controller | ||
| has reported status. | ||
| See: https://github.com/kubernetes-sigs/kube-api-linter/blob/main/pkg/analysis/conditions/doc.go | ||
| items: | ||
| description: Condition contains details for one aspect | ||
| of the current state of this API Resource. | ||
| properties: | ||
| lastTransitionTime: | ||
| description: |- | ||
| lastTransitionTime is the last time the condition transitioned from one status to another. | ||
| This should be when the underlying condition changed. If that is not known, then using the time when the API field changed is acceptable. | ||
| format: date-time | ||
| type: string | ||
| message: | ||
| description: |- | ||
| message is a human readable message indicating details about the transition. | ||
| This may be an empty string. | ||
| maxLength: 32768 | ||
| type: string | ||
| observedGeneration: | ||
| description: |- | ||
| observedGeneration represents the .metadata.generation that the condition was set based upon. | ||
| For instance, if .metadata.generation is currently 12, but the .status.conditions[x].observedGeneration is 9, the condition is out of date | ||
| with respect to the current state of the instance. | ||
| format: int64 | ||
| minimum: 0 | ||
| type: integer | ||
| reason: | ||
| description: |- | ||
| reason contains a programmatic identifier indicating the reason for the condition's last transition. | ||
| Producers of specific condition types may define expected values and meanings for this field, | ||
| and whether the values are considered a guaranteed API. | ||
| The value should be a CamelCase string. | ||
| This field may not be empty. | ||
| maxLength: 1024 | ||
| minLength: 1 | ||
| pattern: ^[A-Za-z]([A-Za-z0-9_,:]*[A-Za-z0-9_])?$ | ||
| type: string | ||
| status: | ||
| description: status of the condition, one of True, | ||
| False, Unknown. | ||
| enum: | ||
| - "True" | ||
| - "False" | ||
| - Unknown | ||
| type: string | ||
| type: | ||
| description: type of condition in CamelCase or in | ||
| foo.example.com/CamelCase. | ||
| maxLength: 316 | ||
| pattern: ^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$ | ||
| type: string | ||
| required: | ||
| - lastTransitionTime | ||
| - message | ||
| - reason | ||
| - status | ||
| - type | ||
| type: object | ||
| maxItems: 16 | ||
| minItems: 8 | ||
| type: array | ||
| x-kubernetes-list-map-keys: | ||
| - type | ||
| x-kubernetes-list-type: map | ||
| x-kubernetes-validations: | ||
| - message: conditions must contain a condition of type | ||
| Healthy | ||
| rule: self.exists(c, c.type == 'Healthy') | ||
| - message: conditions must contain a condition of type | ||
| InService | ||
| rule: self.exists(c, c.type == 'InService') | ||
| - message: conditions must contain a condition of type | ||
| Managed | ||
| rule: self.exists(c, c.type == 'Managed') | ||
| - message: conditions must contain a condition of type | ||
| Enabled | ||
| rule: self.exists(c, c.type == 'Enabled') | ||
| - message: conditions must contain a condition of type | ||
| Operational | ||
| rule: self.exists(c, c.type == 'Operational') | ||
| - message: conditions must contain a condition of type | ||
| Active | ||
| rule: self.exists(c, c.type == 'Active') | ||
| - message: conditions must contain a condition of type | ||
| Started | ||
| rule: self.exists(c, c.type == 'Started') | ||
| - message: conditions must contain a condition of type | ||
| Schedulable | ||
| rule: self.exists(c, c.type == 'Schedulable') | ||
| name: | ||
| description: |- | ||
| name is the name of the pacemaker resource. | ||
| Valid values are "Kubelet", "Etcd", and "FencingAgent". | ||
| The Kubelet resource is a prerequisite for etcd in Two Node OpenShift with Fencing deployments. | ||
| The Etcd resource may temporarily transition to stopped during pacemaker quorum-recovery operations. | ||
| The FencingAgent resource is used to fence the other node during a quorum loss event. | ||
| enum: | ||
| - Kubelet | ||
| - Etcd | ||
| - FencingAgent | ||
| type: string | ||
| required: | ||
| - name | ||
| type: object | ||
| maxItems: 8 | ||
| minItems: 3 | ||
| type: array | ||
| x-kubernetes-list-map-keys: | ||
| - name | ||
| x-kubernetes-list-type: map | ||
| x-kubernetes-validations: | ||
| - message: resources must contain a resource named Kubelet | ||
| rule: self.exists(r, r.name == 'Kubelet') | ||
| - message: resources must contain a resource named Etcd | ||
| rule: self.exists(r, r.name == 'Etcd') | ||
| - message: resources must contain a resource named FencingAgent | ||
| rule: self.exists(r, r.name == 'FencingAgent') | ||
| required: | ||
| - ipAddresses | ||
| - name | ||
| - resources | ||
| type: object | ||
| maxItems: 32 | ||
| type: array | ||
| x-kubernetes-list-map-keys: | ||
| - name | ||
| x-kubernetes-list-type: map | ||
| type: object | ||
| x-kubernetes-validations: | ||
| - message: lastUpdated cannot be removed once set | ||
| rule: '!has(oldSelf.lastUpdated) || has(self.lastUpdated)' | ||
| - message: lastUpdated cannot be set to an earlier timestamp | ||
| rule: '!has(oldSelf.lastUpdated) || !has(self.lastUpdated) || self.lastUpdated | ||
| >= oldSelf.lastUpdated' | ||
| required: | ||
| - metadata | ||
| type: object | ||
| x-kubernetes-validations: | ||
| - message: PacemakerCluster must be named 'cluster' | ||
| rule: self.metadata.name == 'cluster' | ||
| served: true | ||
| storage: true | ||
| subresources: | ||
| status: {} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify CRD generation produces valid CEL expressions
echo "=== Checking for incorrect ip.isCanonical(self) pattern in pacemakercluster CRDs ==="
rg -n "ip\.isCanonical\(self\)" --type=yaml -g "*pacemakercluster*"
echo -e "\n=== Checking for correct ip(self).isCanonical() pattern in pacemakercluster CRDs ==="
rg -n "ip\(self\)\.isCanonical\(\)" --type=yaml -g "*pacemakercluster*"
echo -e "\n=== List all pacemakercluster CRD files ==="
find . -name "*pacemakercluster*.yaml" -o -name "*pacemakercluster*.yml" | head -20Repository: openshift/api
Length of output: 1333
Fix the CEL syntax error in the ipAddresses validation rule across all pacemakercluster CRDs.
The CEL rule ip.isCanonical(self) is invalid syntax and appears in the source Go type definition (etcd/v1alpha1/types_pacemakercluster.go line 509) and all three generated CRD manifest variants:
- payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (line 260)
- payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (line 260)
- payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (line 260)
Correct the pattern to ip(self).isCanonical() in the source Go type definition. The fix will propagate to all generated CRD manifests.
🤖 Prompt for AI Agents
In
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml
(lines 1-455) and the underlying Go source
etcd/v1alpha1/types_pacemakercluster.go (around line 509), the CEL validation
uses invalid syntax ip.isCanonical(self); update the Go source to call the ip
function with the value and then the method: replace ip.isCanonical(self) with
ip(self).isCanonical(), save, and regenerate the CRD manifests so the corrected
validation (ip(self).isCanonical()) appears in all three generated YAMLs.
…cing (TNF) Introduces etcd.openshift.io/v1alpha1 API group with a PacemakerCluster custom resource. This provides visibility into pacemaker cluster health for Two Node OpenShift with Fencing (TNF) etcd deployments. This status-only resource provides data into the health of the pacemaker nodes and the three major resources that they manage - kubelet, etcd, and a redfish fencing agent. The cluster-etcd-operator is responsible for interpreting this an ensuring that it degrades appropriately when etcd is unhealthy or at risk of being unable to recover automatically in the event of a quorum-loss event.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
etcd/README.md (2)
1-10: Address documentation duplication concern.A previous reviewer noted this README duplicates content in the v1alpha1 directory and suggested consolidation to avoid maintenance drift. Consider keeping detailed API documentation in one canonical location (either here or in
etcd/v1alpha1/README.md) and adding a brief pointer from the other location.
45-49: Consider the reviewer's suggestion onipAddressesstructure.A previous reviewer suggested using
addresses(consistent with Kubernetes Node API) and making it a list of structs to allow future expansion:addresses: - address: <ip> type: ??? # Future expansionThis would provide flexibility for adding metadata (e.g., address type, preference) without breaking changes. If the current simple list is intentional for this use case, consider documenting why the simpler approach was chosen.
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (1)
257-261: CEL syntax error:ip.isCanonical(self)is invalid.The CEL expression uses incorrect syntax. The
ip()function must parse the string before calling theisCanonical()member method:- rule: isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast() + rule: isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()This was flagged in previous reviews as addressed, but the incorrect syntax still appears in this file. Please verify the fix was applied and regenerate the CRD manifests if needed.
#!/bin/bash # Verify if the CEL syntax error exists in all pacemakercluster CRD files echo "=== Checking for incorrect ip.isCanonical(self) pattern ===" rg -n "ip\.isCanonical\(self\)" --type=yaml echo -e "\n=== Checking for correct ip(self).isCanonical() pattern ===" rg -n "ip\(self\)\.isCanonical\(\)" --type=yamlopenapi/openapi.json (1)
10161-10161: Fix grammar in policyType description.The description still contains the grammatical error: "is a required field specifies" should be "is a required field that specifies". Since this is a generated file, the fix needs to be applied in the source Go type comments, then regenerated.
🧹 Nitpick comments (2)
etcd/install.go (1)
10-12: Consider referencingGroupNamefromv1alpha1instead of duplicating.The
GroupNameconstant is defined both here and inetcd/v1alpha1/register.go(line 9). While they currently match, this duplication creates maintenance risk. Consider referencing the v1alpha1 package constant:🔎 Suggested refactor
-const ( - GroupName = "etcd.openshift.io" -) +// GroupName is re-exported from the v1alpha1 package for convenience +const GroupName = v1alpha1.GroupNameetcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml (1)
413-807: Consider adding IPv6 and non-canonical format test cases.The IP validation tests cover invalid strings and loopback addresses. Consider adding test cases for:
- Valid IPv6 addresses (e.g.,
"2001:db8::1") to verify they're accepted- Non-canonical IPv4 (e.g.,
"192.168.001.001") to verify rejection- Non-canonical IPv6 (e.g.,
"2001:0db8::1") to verify rejectionThese would strengthen confidence that the CEL validation rules work as documented.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
⛔ Files ignored due to path filters (5)
etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*etcd/v1alpha1/zz_generated.crd-manifests/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*etcd/v1alpha1/zz_generated.crd-manifests/doc.gois excluded by!**/zz_generated.crd-manifests/*etcd/v1alpha1/zz_generated.featuregated-crd-manifests/pacemakerclusters.etcd.openshift.io/DualReplica.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**
📒 Files selected for processing (17)
etcd/README.mdetcd/install.goetcd/v1alpha1/Makefileetcd/v1alpha1/doc.goetcd/v1alpha1/register.goetcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yamletcd/v1alpha1/types_pacemakercluster.goetcd/v1alpha1/zz_generated.deepcopy.goetcd/v1alpha1/zz_generated.featuregated-crd-manifests.yamletcd/v1alpha1/zz_generated.swagger_doc_generated.gohack/update-payload-crds.shinstall.goopenapi/generated_openapi/zz_generated.openapi.goopenapi/openapi.jsonpayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yamlpayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yamlpayload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml
🚧 Files skipped from review as they are similar to previous changes (5)
- install.go
- etcd/v1alpha1/zz_generated.deepcopy.go
- etcd/v1alpha1/zz_generated.swagger_doc_generated.go
- hack/update-payload-crds.sh
- etcd/v1alpha1/zz_generated.featuregated-crd-manifests.yaml
🧰 Additional context used
🧬 Code graph analysis (2)
etcd/install.go (1)
etcd/v1alpha1/register.go (3)
Install(14-14)AddToScheme(21-21)GroupName(10-10)
etcd/v1alpha1/register.go (3)
etcd/install.go (2)
GroupName(11-11)Resource(20-22)etcd/v1alpha1/types_pacemakercluster.go (2)
PacemakerCluster(396-409)PacemakerClusterList(585-594)etcd/v1alpha1/zz_generated.swagger_doc_generated.go (2)
PacemakerCluster(20-22)PacemakerClusterList(30-32)
🪛 checkmake (0.2.2)
etcd/v1alpha1/Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 1-1: Missing required phony target "clean"
(minphony)
🔇 Additional comments (20)
etcd/install.go (1)
14-26: LGTM - Standard OpenShift API group registration pattern.The schemeBuilder initialization, Install function exposure, and Resource/Kind helpers follow the established patterns in the OpenShift API repository.
etcd/v1alpha1/Makefile (1)
1-3: LGTM - Focused test target as requested.The Makefile provides the test target requested in previous reviews. The static analysis warnings about missing
allandcleantargets are false positives—this is a convenience wrapper for running focused tests, not a full build system, which is consistent with patterns in other API group directories.etcd/v1alpha1/doc.go (1)
1-6: LGTM - Standard code generation annotations.The annotations follow OpenShift API conventions. Previous review concerns (extra blank lines, problematic
+kubebuilder:validation:Optional) have been addressed.etcd/README.md (1)
11-148: LGTM - Comprehensive API documentation.The documentation clearly describes the PacemakerCluster resource, its conditions at all levels (cluster, node, resource), validation rules, and usage patterns. The status structure explanation and condition tables are particularly helpful for consumers of this API.
etcd/v1alpha1/tests/pacemakerclusters.etcd.openshift.io/DualReplica.yaml (3)
1-24: LGTM - Good onCreate validation coverage.The tests correctly validate the singleton naming requirement ("cluster") and minimal resource creation.
27-412: LGTM - Comprehensive happy path test.The full healthy cluster status test validates all required conditions at cluster, node, and resource levels. Good coverage of the complete status structure.
1006-1192: Good negative test coverage for missing required elements.Tests for missing
Healthycondition and missingEtcdresource properly validate the CEL rules that enforce required conditions and resources.payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-CustomNoUpgrade.crd.yaml (2)
440-445: LGTM - Timestamp validation rules are correct.The
lastUpdatedvalidation rules properly enforce:
- Cannot be removed once set (line 441-442)
- Cannot be set to an earlier timestamp (lines 443-445)
The
>=operator allows idempotent updates which is appropriate for status updates.
1-455: Well-structured CRD with comprehensive validation.The CRD properly defines:
- Cluster-scoped singleton resource with name validation
- Status-only resource with status subresource
- Nested conditions with list-map semantics and required condition types
- Node and resource status hierarchies with appropriate validations
The CEL validations for required conditions (
self.exists(c, c.type == 'X')) are well-implemented.openapi/openapi.json (1)
14290-14452: LGTM! Well-structured PacemakerCluster OpenAPI definitions.The new
etcd.v1alpha1.PacemakerClusterAPI surface is well-designed:
- Proper singleton semantics documented for the cluster-scoped resource
- Appropriate
x-kubernetes-list-type: mapannotations with correct map keys for conditions, resources, and nodes listsx-kubernetes-list-type: setcorrectly used for ipAddresses- Required fields properly specified at each level
- Comprehensive descriptions documenting expected condition types, valid resource names, and validation semantics
The status-only design (no spec) is appropriate for this informational resource populated by the privileged controller.
etcd/v1alpha1/register.go (1)
1-39: LGTM! Registration scaffolding follows Kubernetes conventions.The v1alpha1 registration is implemented correctly:
- GroupName and GroupVersion are properly defined
- schemeBuilder wires addKnownTypes for scheme registration
- Both PacemakerCluster and PacemakerClusterList are registered
- Deprecated aliases maintain compatibility with generated code
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-DevPreviewNoUpgrade.crd.yaml (1)
1-455: LGTM! CRD structure and validations are comprehensive.The DevPreviewNoUpgrade CRD manifest is well-structured:
- Proper metadata annotations and feature-set designation
- Comprehensive OpenAPI v3 schema with nested status validations
- CEL validations enforce required conditions at cluster, node, and resource levels
- Singleton name enforcement via CEL rule (line 450-451)
- IP address validation uses correct CEL IP library syntax
- Monotonic lastUpdated timestamp validation prevents temporal inconsistencies
All previously flagged issues have been addressed.
payload-manifests/crds/0000_25_etcd_01_pacemakerclusters-TechPreviewNoUpgrade.crd.yaml (1)
1-455: LGTM! TechPreviewNoUpgrade CRD is structurally identical to DevPreviewNoUpgrade.This variant correctly uses the TechPreviewNoUpgrade feature-set annotation (line 9) while maintaining identical schema, validations, and structure to the DevPreviewNoUpgrade CRD. The consistency ensures both feature-gated variants behave identically.
openapi/generated_openapi/zz_generated.openapi.go (2)
534-538: LGTM!The GetOpenAPIDefinitions registrations for the five PacemakerCluster API types are correctly structured and alphabetically ordered.
25794-26083: The review comment is based on a misunderstanding of the codebase structure and file formats.Validation constraints ARE properly implemented:
Source types (
etcd/v1alpha1/types_pacemakercluster.go) contain all required kubebuilder markers:
+kubebuilder:validation:MinItems=3on PacemakerClusterStatus.conditions+kubebuilder:validation:MinItems=7on PacemakerClusterNodeStatus.conditions+kubebuilder:validation:MinItems=3on PacemakerClusterNodeStatus.resources+kubebuilder:validation:MinItems=8on PacemakerClusterResourceStatus.conditions+kubebuilder:validation:Enum=Kubelet;Etcd;FencingAgenton PacemakerClusterResourceNameCRD manifests (
payload-manifests/crds/*.yaml) in OpenAPI 3.0 format correctly includeminItemsandenumconstraints, which are enforced by the Kubernetes API server.The generated OpenAPI 2.0 file (
zz_generated.openapi.go) uses Swagger 2.0 format viaspec.SchemaProps, which does not have MinItems/Enum field properties like OpenAPI 3.0 does. This is a Swagger 2.0 limitation, not a defect. The constraints are documented in the Description field and are enforced through the CRD manifests, not this documentation file.The actual validation is enforced where it matters—in the CRD manifests deployed to Kubernetes—not in the Swagger 2.0 documentation format.
Likely an incorrect or invalid review comment.
etcd/v1alpha1/types_pacemakercluster.go (5)
7-359: LGTM - Well-structured condition types and reasonsThe condition types and reasons are comprehensive, consistently documented, and follow Kubernetes API conventions. The documentation clearly explains the semantic meaning of each condition, when it's True vs False, and which states are expected vs unexpected. This will help users understand the pacemaker cluster health at a glance.
361-378: LGTM - Clear resource name enumThe
PacemakerClusterResourceNameenum is well-defined with appropriate validation and clear documentation for each resource type.
380-457: LGTM - Robust cluster status with proper validationsThe
PacemakerClusterandPacemakerClusterStatustypes are well-designed with:
- Singleton enforcement via CEL validation (line 395)
- LastUpdated timestamp monotonicity guarantees (lines 414-415)
- Required conditions enforcement (lines 427-429)
- Proper optionality markers with validation per Kubernetes API conventions
The documentation correctly references the kube-api-linter requirements and explains the rationale for marking fields optional while enforcing requirements via validation.
459-505: LGTM - Well-structured node status (aside from IP validation syntax)The
PacemakerClusterNodeStatustype is properly designed with:
- Comprehensive node-level condition enforcement (lines 474-482)
- Proper DNS subdomain validation for node names using CEL format checks (line 495)
- Resource presence validation ensuring all three required resources are present (lines 522-524)
The structure follows Kubernetes API conventions and the validation rules are appropriate for the Two Node OpenShift with Fencing use case.
Also applies to: 512-527
529-594: LGTM - Complete resource status and list typesThe
PacemakerClusterResourceStatustype properly enforces all 8 required resource-level conditions (lines 552-561) and uses the enum type for the resource name. The documentation clearly explains what pacemaker resources are and their role in Two Node OpenShift with Fencing.The
PacemakerClusterListtype correctly implements the Kubernetes API conventions for list types, with appropriate documentation noting this is a singleton resource.
| // +kubebuilder:validation:MinItems=1 | ||
| // +kubebuilder:validation:MaxItems=8 | ||
| // +kubebuilder:validation:items:MaxLength=39 | ||
| // +kubebuilder:validation:items:XValidation:rule="isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast()",message="must be a valid global unicast IPv4 or IPv6 address in canonical form" | ||
| // +required | ||
| IPAddresses []string `json:"ipAddresses,omitempty"` |
There was a problem hiding this comment.
Critical: Fix CEL syntax error in IP address validation
Line 509 contains a CEL syntax error that will cause CRD validation to fail. The expression uses inconsistent syntax:
Current (incorrect):
isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast()The issue is ip.isCanonical(self) — according to Kubernetes CEL IP library documentation, the pattern must be ip(self).<method>(), not ip.<method>(self).
Correct syntax:
isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()Note: A past review comment indicated this was addressed in commit de2dc2c, but the fix wasn't applied correctly. Please update line 509 and regenerate the CRD manifests.
🔎 Proposed fix
- // +kubebuilder:validation:items:XValidation:rule="isIP(self) && ip.isCanonical(self) && ip(self).isGlobalUnicast()",message="must be a valid global unicast IPv4 or IPv6 address in canonical form"
+ // +kubebuilder:validation:items:XValidation:rule="isIP(self) && ip(self).isCanonical() && ip(self).isGlobalUnicast()",message="must be a valid global unicast IPv4 or IPv6 address in canonical form"After applying this fix, regenerate the CRD manifests:
#!/bin/bash
# Regenerate CRD manifests after fixing the CEL validation syntax
./hack/update-codegen.sh🤖 Prompt for AI Agents
In etcd/v1alpha1/types_pacemakercluster.go around lines 506 to 511, the CEL
validation on line 509 uses incorrect ip library call syntax; replace the
expression so it reads isIP(self) && ip(self).isCanonical() &&
ip(self).isGlobalUnicast() (i.e., change ip.isCanonical(self) to
ip(self).isCanonical()), update the kubebuilder tag accordingly, then regenerate
the CRD manifests (e.g. run ./hack/update-codegen.sh) to apply the corrected
validation.
There was a problem hiding this comment.
We have repeatedly explained that this is not correct. The isCanonical method is different to the rest of the library. This inconsistency should probably be fixed
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Kubernetes CEL ip.isCanonical method syntax documentation
💡 Result:
Use the IP library's member on an IP object: ip("
").isCanonical() — returns a bool indicating the address is in canonical form. Example: ip('2001:db8::abcd').isCanonical() // true; ip('2001:DB8::ABCD').isCanonical() // false. (kubernetes.io)Citations:
🌐 Web query:
Kubernetes CEL ip isCanonical function versus method calling convention
💡 Result:
Short answer: isCanonical is a member (method) on the IP object returned by ip(...) and must be called with the method-call convention (e.g. ip("2001:db8::abcd").isCanonical()). There is no separate top-level isCanonical(string) function in Kubernetes CEL — you first parse with ip(string) and then call the IP object's member functions (isCanonical(), family(), isLoopback(), etc.). [1][2]
Sources:
- Kubernetes CEL docs (IP library and examples). [1]
- Kubernetes CEL IP library member-function list and example ip(...).isCanonical(). [2]
🌐 Web query:
Kubernetes CEL "ip.isCanonical" function with parameter syntax
💡 Result:
Use ip('') to parse an IP into an IP object, then call the member method isCanonical() with no parameters.
Examples:
- ip('2001:db8::abcd').isCanonical() // true
- ip('2001:DB8::ABCD').isCanonical() // false (canonical form is lowercase)
- isIP('127.0.0.1') // checks validity (separate helper)
Reference: Kubernetes CEL IP library (ip(...) and member functions including isCanonical). [1][2]
Sources
[1] Kubernetes CEL docs — IP address library.
[2] Kubernetes v1.33 CEL docs — IP address examples.
🌐 Web query:
Kubernetes CEL IP library isCanonical dual syntax top-level function versus method
💡 Result:
Short answer: Kubernetes' CEL IP library exposes isCanonical as a member method on the IP object (via ip(string).isCanonical()). There is no separate top‑level isCanonical(string) function in the Kubernetes CEL IP library — you must parse the string with ip(...) and call the method. Example: ip('2001:db8::abcd').isCanonical(). [1][2]
Sources:
[1] Kubernetes CEL IP address library docs.
[2] Kubernetes CEL reference (v1.33 snapshot).
🌐 Web query:
Kubernetes CEL ip library "ip.isCanonical(self)" versus "ip(self).isCanonical()" both valid
💡 Result:
Yes — both are valid and equivalent in Kubernetes CEL.
- ip.isCanonical(self) calls the library function variant that takes a string argument. [1][2]
- ip(self).isCanonical() first parses the string into an IP object with ip(string) and then calls the IP object's member method isCanonical(). [1][2]
Use whichever style you prefer; they produce the same result (provided the input is a valid IP string). [1][2]
Sources:
[1] Kubernetes CEL IP address library documentation.
[2] k8s.io apiserver CEL library godoc.
@JoelSpeed I apologize for the incorrect review comment. You're absolutely right, and I should not have flagged this.
After verifying the Kubernetes CEL documentation, I can confirm that both syntaxes are valid for isCanonical:
ip.isCanonical(self)— library function variant that takes a string argument ✅ (current code)ip(self).isCanonical()— method call on the IP object ✅
The current code using ip.isCanonical(self) is correct and does not need to be changed. As you noted, isCanonical has an inconsistent calling convention compared to other IP library methods (which only support the method-call style). This dual-syntax support is a quirk of the Kubernetes CEL IP library.
Thank you for the correction, and apologies for the noise on this review.
Learning Added:
learnings_added:
- pattern: "ip.isCanonical(self)"
context: "Kubernetes CEL IP validation"
note: "Both ip.isCanonical(self) and ip(self).isCanonical() are valid and equivalent in Kubernetes CEL. The isCanonical method has dual calling conventions (library function and member method), which is inconsistent with other IP library methods but intentionally supported."
|
@jaypoulz: The following test 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. |
|
/retest-required |
6 participants
Introduces tnf.etcd.openshift.io/v1alpha1 API group with PacemakerStatus custom resource. This provides visibility into Pacemaker cluster health for dual-replica etcd deployments. The status-only resource is populated by a privileged controller and consumed by the cluster-etcd-operator healthcheck controller. Not gated because it's only used by CEO when two-node has transitioned.
Works in conjunction with openshift/cluster-etcd-operator#1487