-
Notifications
You must be signed in to change notification settings - Fork 578
OCPEDGE-2084: Add PacemakerStatus CRD for two-node fencing #2544
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| swaggerdocs: | ||
| commentPolicy: Warn | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| # etcd.openshift.io API Group | ||
|
|
||
| This API group contains CRDs related to etcd cluster management. Specifically, this is only used for TNF (Two Node Fencing) | ||
| for gathering status updates from the node to ensure the cluster-admin is warned about unhealthy setups. | ||
|
|
||
| ## API Versions | ||
|
|
||
| ### v1alpha1 | ||
|
|
||
| Contains the `PacemakerCluster` custom resource for monitoring Pacemaker cluster health in TNF (Two Node Fencing) deployments. | ||
|
|
||
| #### PacemakerCluster | ||
|
|
||
| - **Feature Gate**: None - this CRD is gated by cluster-etcd-operator start-up. It will only be created once a TNF cluster has transitioned to external etcd. | ||
| - **Component**: `two-node-fencing` | ||
| - **Scope**: Cluster-scoped singleton resource named "cluster" | ||
| - **Resource Path**: `pacemakerclusters.etcd.openshift.io` | ||
|
|
||
| The `PacemakerCluster` resource provides visibility into the health and status of a Pacemaker-managed cluster. It is periodically updated by the cluster-etcd-operator's status collector running as a privileged CronJob. | ||
|
|
||
| **Status Fields:** | ||
| - `lastUpdated` (required): Timestamp when status was last collected - used to detect stale data | ||
| - `summary`: High-level cluster health metrics | ||
| - `pacemakerDaemonState`: Running state (enum: `Running`, `KnownNotRunning`) | ||
| - `quorumStatus`: Quorum state (enum: `Quorate`, `NoQuorum`) | ||
| - `nodesOnline`, `nodesTotal`: Node counts (0-2) | ||
| - `resourcesStarted`, `resourcesTotal`: Resource counts (0-16) | ||
| - `nodes`: Detailed status of each node (1-2 nodes) | ||
| - Name, IPv4/IPv6 addresses, online status (enum), mode (enum: `Active`, `Standby`) | ||
| - `resources`: Detailed status of each resource (1-16 resources) | ||
| - Name, resource agent, role (enum: `Started`, `Stopped`), active status (enum), node assignment | ||
| - `nodeHistory`: Recent operation failures for troubleshooting (up to 16 entries, last 5 minutes) | ||
| - `fencingHistory`: Recent fencing events (up to 16 events, last 24 hours) | ||
| - Target node, action (enum: `reboot`, `off`, `on`), status (enum: `success`, `failed`, `pending`), completion timestamp | ||
| - `collectionError`: Any errors encountered during status collection (max 2KB) | ||
| - `rawXML`: Full XML output from `pcs status xml` for debugging (max 256KB) | ||
|
|
||
| **Design Principles:** | ||
| The API follows "Act on Deterministic Information": | ||
| - All fields except `lastUpdated` are optional | ||
| - Missing data indicates unknown state, not error | ||
| - Operator only acts on definitive information | ||
| - Unknown state preserves the last known health condition | ||
|
|
||
| **Usage:** | ||
| The cluster-etcd-operator healthcheck controller watches this resource and updates operator conditions based on the cluster state. | ||
|
Comment on lines
+12
to
+46
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is duplicating the readme in the v1alpha1, I suspect we don't need it in both locations |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| package etcd | ||
|
|
||
| import ( | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| "k8s.io/apimachinery/pkg/runtime/schema" | ||
|
|
||
| v1alpha1 "github.com/openshift/api/etcd/v1alpha1" | ||
| ) | ||
|
|
||
| const ( | ||
| GroupName = "etcd.openshift.io" | ||
| ) | ||
|
|
||
| var ( | ||
| schemeBuilder = runtime.NewSchemeBuilder(v1alpha1.Install) | ||
| // Install is a function which adds every version of this group to a scheme | ||
| Install = schemeBuilder.AddToScheme | ||
| ) | ||
|
|
||
| func Resource(resource string) schema.GroupResource { | ||
| return schema.GroupResource{Group: GroupName, Resource: resource} | ||
| } | ||
|
|
||
| func Kind(kind string) schema.GroupKind { | ||
| return schema.GroupKind{Group: GroupName, Kind: kind} | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| .PHONY: verify-with-container | ||
| verify-with-container: | ||
| $(MAKE) -f ../../Makefile $@ | ||
|
|
||
| .PHONY: update-with-container | ||
| update-with-container: | ||
| $(MAKE) -f ../../Makefile $@ | ||
|
Comment on lines
+1
to
+7
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this actually work? I wouldn't expect it to since we don't generally maintain the update-with-container targets, and some of them require context of the entire API Also, we would usually have a test target at this level of makefile, can you please include that |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| # etcd.openshift.io/v1alpha1 | ||
|
|
||
| This API group contains types related to two-node fencing for etcd cluster management. | ||
|
|
||
| ## PacemakerCluster | ||
|
|
||
| The `PacemakerCluster` CRD provides visibility into the health and status of Pacemaker-managed clusters in dual-replica (two-node) OpenShift deployments. | ||
|
|
||
| ### Feature Gate | ||
|
|
||
| - **Feature Gate**: None - this CRD is gated by cluster-etcd-operator start-up. It will only be created once a TNF cluster has transitioned to external etcd. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All APIs must start behind a feature gate, even in v1alpha1
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can throw it behind the DualReplica feature gate because it's already blocked by that gate. I don't think it needs its own gate since TNF and this monitor are tightly coupled. This makes more sense now that it'll be TP in 4.21. |
||
| - **Component**: `two-node-fencing` | ||
|
|
||
| ### Usage | ||
|
|
||
| The PacemakerCluster resource is a cluster-scoped, status-only singleton named "cluster". It is periodically updated by a privileged controller that runs `pcs status xml` and parses the output into structured fields for health checking. | ||
|
|
||
| ### Status Fields | ||
|
|
||
| - **LastUpdated** (required): Timestamp when status was last collected | ||
| - **Summary**: High-level cluster state including: | ||
| - `pacemakerDaemonState`: Running state of the pacemaker daemon (enum: `Running`, `KnownNotRunning`) | ||
| - `quorumStatus`: Whether cluster has quorum (enum: `Quorate`, `NoQuorum`) | ||
| - `nodesOnline`, `nodesTotal`: Node counts | ||
| - `resourcesStarted`, `resourcesTotal`: Resource counts | ||
| - **Nodes**: Detailed per-node status (name, IPv4/IPv6 addresses, online status, mode) | ||
| - **Resources**: Detailed per-resource status (name, resource agent type, role enum, active status, node assignment) | ||
| - **NodeHistory**: Recent operation history for troubleshooting (operation failures within last 5 minutes) | ||
| - **FencingHistory**: Recent fencing events (events within last 24 hours) | ||
| - **RawXML**: Complete XML output from `pcs status xml` (for debugging only, max 256KB) | ||
| - **CollectionError**: Any errors encountered during status collection | ||
|
|
||
| ### Design Principles | ||
|
|
||
| The API follows a "Design Principle: Act on Deterministic Information" approach: | ||
| - Almost all fields are optional except `lastUpdated` | ||
| - Missing data means "unknown" not "error" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We generally prefer to populate all data with explicit unknown rather than have it omitted
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean we should default to required for fields wherever possible? |
||
| - The operator only transitions between PacemakerHealthy and PacemakerDegraded states based on deterministic information | ||
| - When information is unavailable, the last known state is preserved | ||
|
|
||
| ### Notes | ||
|
|
||
| The spec field is reserved but unused - all meaningful data is in the status subresource. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| // +k8s:deepcopy-gen=package,register | ||
| // +k8s:defaulter-gen=TypeMeta | ||
| // +k8s:openapi-gen=true | ||
| // +openshift:featuregated-schema-gen=true | ||
|
|
||
| // +kubebuilder:validation:Optional | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't do this (I know this exists on other APIs but it's not right) This changes the default behaviour for optionality of a field and has bitten many people where they thought they were making fields required and weren't |
||
| // +groupName=etcd.openshift.io | ||
| package v1alpha1 | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| package v1alpha1 | ||
|
|
||
| import ( | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/runtime" | ||
| "k8s.io/apimachinery/pkg/runtime/schema" | ||
| ) | ||
|
|
||
| var ( | ||
| GroupName = "etcd.openshift.io" | ||
| GroupVersion = schema.GroupVersion{Group: GroupName, Version: "v1alpha1"} | ||
| schemeBuilder = runtime.NewSchemeBuilder(addKnownTypes) | ||
| // Install is a function which adds this version to a scheme | ||
| Install = schemeBuilder.AddToScheme | ||
|
|
||
| // SchemeGroupVersion generated code relies on this name | ||
| // Deprecated | ||
| SchemeGroupVersion = GroupVersion | ||
| // AddToScheme exists solely to keep the old generators creating valid code | ||
| // DEPRECATED | ||
| AddToScheme = schemeBuilder.AddToScheme | ||
| ) | ||
|
|
||
| // Resource generated code relies on this being here, but it logically belongs to the group | ||
| // DEPRECATED | ||
| func Resource(resource string) schema.GroupResource { | ||
| return schema.GroupResource{Group: GroupName, Resource: resource} | ||
| } | ||
|
|
||
| func addKnownTypes(scheme *runtime.Scheme) error { | ||
| metav1.AddToGroupVersion(scheme, GroupVersion) | ||
|
|
||
| scheme.AddKnownTypes(GroupVersion, | ||
| &PacemakerCluster{}, | ||
| &PacemakerClusterList{}, | ||
| ) | ||
|
|
||
| return nil | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| apiVersion: apiextensions.k8s.io/v1 # Hack because controller-gen complains if we don't have this | ||
| name: "PacemakerCluster" | ||
| crdName: pacemakerclusters.etcd.openshift.io | ||
| tests: | ||
| onCreate: | ||
| - name: Should be able to create a minimal PacemakerCluster | ||
| initial: | | ||
| apiVersion: etcd.openshift.io/v1alpha1 | ||
| kind: PacemakerCluster | ||
| metadata: | ||
| name: cluster | ||
| spec: {} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally we try to avoid allowing an empty spec to be valid. What would this object achieve if it has no spec?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It only exists to reflect status. It's not configuration, not is it trying to modify behavior or configure the cluster in any way. This is one of the reasons I wasn't sure if this really belonged in API. |
||
| expected: | | ||
| apiVersion: etcd.openshift.io/v1alpha1 | ||
| kind: PacemakerCluster | ||
| metadata: | ||
| name: cluster | ||
| spec: {} | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't do this, would rather this were enforced for new API groups