-
Notifications
You must be signed in to change notification settings - Fork 509
feat(etcd): implement multi-shard etcd support #7375
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
Changes from all commits
1c9d031
4aeeeeb
e413cd5
18d9289
a83140a
d437c79
81a4749
9808425
f1c9211
b3987f5
d24c75f
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,43 @@ | ||
| package v1beta1 | ||
|
|
||
| // EffectiveShards returns the effective shard configuration for managed etcd. | ||
| // If shards are not explicitly configured, returns a default single shard. | ||
| func (m *ManagedEtcdSpec) EffectiveShards(hcp *HostedControlPlane) []ManagedEtcdShardSpec { | ||
| if len(m.Shards) > 0 { | ||
| return m.Shards | ||
| } | ||
|
|
||
| replicas := int32(1) | ||
| if hcp.Spec.ControllerAvailabilityPolicy == HighlyAvailable { | ||
| replicas = 3 | ||
| } | ||
|
|
||
| return []ManagedEtcdShardSpec{ | ||
| { | ||
| Name: "default", | ||
| ResourcePrefixes: []string{"/"}, | ||
| Priority: EtcdShardPriorityCritical, | ||
| Replicas: &replicas, | ||
| BackupSchedule: "*/30 * * * *", | ||
| }, | ||
| } | ||
| } | ||
|
|
||
| // EffectiveShards returns the effective shard configuration for unmanaged etcd. | ||
| // If shards are not explicitly configured, returns a default single shard using | ||
| // the legacy endpoint and tls fields. | ||
| func (u *UnmanagedEtcdSpec) EffectiveShards() []UnmanagedEtcdShardSpec { | ||
| if len(u.Shards) > 0 { | ||
| return u.Shards | ||
| } | ||
|
|
||
| return []UnmanagedEtcdShardSpec{ | ||
| { | ||
| Name: "default", | ||
| ResourcePrefixes: []string{"/"}, | ||
| Priority: EtcdShardPriorityCritical, | ||
| Endpoint: u.Endpoint, | ||
| TLS: u.TLS, | ||
| }, | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -1900,6 +1900,8 @@ type EtcdSpec struct { | |||||||||||||||||
| // HyperShift. | ||||||||||||||||||
| type ManagedEtcdSpec struct { | ||||||||||||||||||
| // storage specifies how etcd data is persisted. | ||||||||||||||||||
| // When shards are specified, this serves as the default for all shards | ||||||||||||||||||
| // unless overridden per-shard. | ||||||||||||||||||
| // +required | ||||||||||||||||||
| // +kubebuilder:validation:XValidation:rule="has(self.restoreSnapshotURL) == has(oldSelf.restoreSnapshotURL)",message="restoreSnapshotURL cannot be added or removed after creation" | ||||||||||||||||||
| Storage ManagedEtcdStorageSpec `json:"storage"` | ||||||||||||||||||
|
|
@@ -1910,6 +1912,81 @@ type ManagedEtcdSpec struct { | |||||||||||||||||
| // +optional | ||||||||||||||||||
| // +openshift:enable:FeatureGate=HCPEtcdBackup | ||||||||||||||||||
| Backup HCPEtcdBackupConfig `json:"backup,omitzero"` | ||||||||||||||||||
|
|
||||||||||||||||||
| // shards configures etcd sharding by Kubernetes resource kind. | ||||||||||||||||||
| // When not specified, a default single shard accepting all prefixes is used. | ||||||||||||||||||
| // When specified, exactly one shard must have "/" in its resourcePrefixes. | ||||||||||||||||||
| // +optional | ||||||||||||||||||
| // +kubebuilder:validation:MinItems=1 | ||||||||||||||||||
| // +kubebuilder:validation:MaxItems=10 | ||||||||||||||||||
|
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. Any particular decision process to get to 10 as a maximum? Should document in the godoc that this list accepts at most 10 items |
||||||||||||||||||
| // +listType=map | ||||||||||||||||||
| // +listMapKey=name | ||||||||||||||||||
| // +kubebuilder:validation:XValidation:rule="self.exists(s, '/' in s.resourcePrefixes)",message="exactly one shard must have '/' prefix" | ||||||||||||||||||
|
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. The rule itself checks that at least 1 item has
Suggested change
|
||||||||||||||||||
| // +kubebuilder:validation:XValidation:rule="self.all(s, s.resourcePrefixes.all(p, p == '/' || p.endsWith('#')))",message="non-default prefixes must end with '#'" | ||||||||||||||||||
|
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. Why is this rule here vs on the resourcePrefixes field itself? |
||||||||||||||||||
| Shards []ManagedEtcdShardSpec `json:"shards,omitempty"` | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // EtcdShardPriority defines the operational priority of an etcd shard | ||||||||||||||||||
| // +kubebuilder:validation:Enum=Critical;High;Medium;Low | ||||||||||||||||||
| type EtcdShardPriority string | ||||||||||||||||||
|
|
||||||||||||||||||
| const ( | ||||||||||||||||||
| EtcdShardPriorityCritical EtcdShardPriority = "Critical" | ||||||||||||||||||
| EtcdShardPriorityHigh EtcdShardPriority = "High" | ||||||||||||||||||
| EtcdShardPriorityMedium EtcdShardPriority = "Medium" | ||||||||||||||||||
| EtcdShardPriorityLow EtcdShardPriority = "Low" | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
| // ManagedEtcdShardSpec defines configuration for a single managed etcd shard | ||||||||||||||||||
| type ManagedEtcdShardSpec struct { | ||||||||||||||||||
| // name is the unique identifier for this shard | ||||||||||||||||||
| // Must be DNS-1035 compliant (lowercase alphanumeric + hyphens) | ||||||||||||||||||
| // Used for resource naming: etcd-{name}, etcd-{name}-client, etc. | ||||||||||||||||||
|
Comment on lines
+1942
to
+1944
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. For consistency with how we describe this validation on other APIs
Suggested change
|
||||||||||||||||||
| // +required | ||||||||||||||||||
| // +kubebuilder:validation:MinLength=1 | ||||||||||||||||||
| // +kubebuilder:validation:XValidation:rule="self.matches('^[a-z]([-a-z0-9]*[a-z0-9])?$')",message="name must be DNS-1035 compliant" | ||||||||||||||||||
| // +kubebuilder:validation:MaxLength=15 | ||||||||||||||||||
|
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. Why 15? DNS1035 is typically 63 capped. Looks like we are prefixing this, is it always
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. Should add |
||||||||||||||||||
| Name string `json:"name,omitempty"` | ||||||||||||||||||
|
|
||||||||||||||||||
| // resourcePrefixes specifies which Kubernetes resources are stored in this shard | ||||||||||||||||||
| // Format: "group/resource#" or "/" for default (catch-all) | ||||||||||||||||||
| // Examples: "/events#", "/coordination.k8s.io/leases#", "/" | ||||||||||||||||||
| // Exactly one shard must have "/" as a prefix | ||||||||||||||||||
|
Comment on lines
+1951
to
+1954
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. Formatting here won't be preserved when this is generated into documentation. Prefer to use complete sentences. |
||||||||||||||||||
| // +required | ||||||||||||||||||
| // +kubebuilder:validation:MinItems=1 | ||||||||||||||||||
| // +kubebuilder:validation:MaxItems=50 | ||||||||||||||||||
| // +kubebuilder:validation:items:MinLength=1 | ||||||||||||||||||
| // +kubebuilder:validation:items:MaxLength=255 | ||||||||||||||||||
| // +listType=set | ||||||||||||||||||
| ResourcePrefixes []string `json:"resourcePrefixes,omitempty"` | ||||||||||||||||||
|
|
||||||||||||||||||
| // priority determines operational importance and default backup frequency | ||||||||||||||||||
| // Critical: Default backup every 30 minutes | ||||||||||||||||||
| // High: Default backup hourly | ||||||||||||||||||
| // Medium/Low: Default backup disabled | ||||||||||||||||||
| // +optional | ||||||||||||||||||
| // +default="Medium" | ||||||||||||||||||
| Priority EtcdShardPriority `json:"priority,omitempty"` | ||||||||||||||||||
|
Comment on lines
+1963
to
+1969
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. Is this implemented? Do we actually care about this as a feature? |
||||||||||||||||||
|
|
||||||||||||||||||
| // storage specifies storage configuration for this shard | ||||||||||||||||||
| // If not specified, inherits from ManagedEtcdSpec.Storage | ||||||||||||||||||
| // +optional | ||||||||||||||||||
| Storage ManagedEtcdStorageSpec `json:"storage,omitzero"` | ||||||||||||||||||
|
|
||||||||||||||||||
| // replicas is the number of etcd replicas for this shard | ||||||||||||||||||
| // Must be 1 or 3. If not specified, defaults based on cluster's | ||||||||||||||||||
| // ControllerAvailabilityPolicy (1 for SingleReplica, 3 for HighlyAvailable) | ||||||||||||||||||
| // +optional | ||||||||||||||||||
| // +kubebuilder:validation:Enum=1;3 | ||||||||||||||||||
| Replicas *int32 `json:"replicas,omitempty"` | ||||||||||||||||||
|
Comment on lines
+1976
to
+1981
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. Defaulting based on another resource is typically considered bad practice. I think this is what those utils were for, where/when is the defaulting called? |
||||||||||||||||||
|
|
||||||||||||||||||
| // backupSchedule is the cron schedule for backups (standard cron format) | ||||||||||||||||||
| // If empty, uses priority-based default or disables backups | ||||||||||||||||||
|
Comment on lines
+1983
to
+1984
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. AFAICT there is no way to have the priority field empty currently (there's a default) so I don't know how I would actually represent "please disable backups" in this API 🤔 |
||||||||||||||||||
| // Examples: "*/30 * * * *" (every 30 min), "0 * * * *" (hourly) | ||||||||||||||||||
| // +optional | ||||||||||||||||||
| // +kubebuilder:validation:MinLength=1 | ||||||||||||||||||
| // +kubebuilder:validation:MaxLength=100 | ||||||||||||||||||
| BackupSchedule string `json:"backupSchedule,omitempty"` | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // ManagedEtcdStorageType is a storage type for an etcd cluster. | ||||||||||||||||||
|
|
@@ -1981,20 +2058,71 @@ type PersistentVolumeEtcdStorageSpec struct { | |||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| // UnmanagedEtcdSpec specifies configuration which enables the control plane to | ||||||||||||||||||
| // integrate with an eternally managed etcd cluster. | ||||||||||||||||||
| // integrate with an externally managed etcd cluster. | ||||||||||||||||||
| type UnmanagedEtcdSpec struct { | ||||||||||||||||||
| // endpoint is the full etcd cluster client endpoint URL. For example: | ||||||||||||||||||
| // | ||||||||||||||||||
| // https://etcd-client:2379 | ||||||||||||||||||
| // | ||||||||||||||||||
| // If the URL uses an HTTPS scheme, the TLS field is required. | ||||||||||||||||||
| // | ||||||||||||||||||
| // +kubebuilder:validation:Pattern=`^https://` | ||||||||||||||||||
| // endpoint is the full etcd cluster client endpoint URL. | ||||||||||||||||||
| // Used only when shards is not specified (legacy single-etcd mode). | ||||||||||||||||||
| // When shards are specified, this field is ignored. | ||||||||||||||||||
|
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. Would be better to add an XValidation to prevent them from both being set at the same time? |
||||||||||||||||||
| // +optional | ||||||||||||||||||
| // +kubebuilder:validation:MinLength=1 | ||||||||||||||||||
| // +kubebuilder:validation:XValidation:rule="self.startsWith('https://')",message="endpoint must start with https://" | ||||||||||||||||||
| // +kubebuilder:validation:MaxLength=255 | ||||||||||||||||||
| Endpoint string `json:"endpoint,omitempty"` | ||||||||||||||||||
|
|
||||||||||||||||||
| // tls specifies TLS configuration for HTTPS etcd client endpoints. | ||||||||||||||||||
| // Used only when shards is not specified (legacy single-etcd mode). | ||||||||||||||||||
| // When shards are specified, this field is ignored. | ||||||||||||||||||
| // +optional | ||||||||||||||||||
| TLS EtcdTLSConfig `json:"tls,omitzero"` | ||||||||||||||||||
|
|
||||||||||||||||||
| // shards configures etcd sharding by Kubernetes resource kind. | ||||||||||||||||||
| // When not specified, uses endpoint and tls fields (legacy single-etcd mode). | ||||||||||||||||||
| // When specified, exactly one shard must have "/" in its resourcePrefixes. | ||||||||||||||||||
| // +optional | ||||||||||||||||||
| // +kubebuilder:validation:MinItems=1 | ||||||||||||||||||
| // +kubebuilder:validation:MaxItems=10 | ||||||||||||||||||
| // +listType=map | ||||||||||||||||||
| // +listMapKey=name | ||||||||||||||||||
| // +kubebuilder:validation:XValidation:rule="self.exists(s, '/' in s.resourcePrefixes)",message="exactly one shard must have '/' prefix" | ||||||||||||||||||
| // +kubebuilder:validation:XValidation:rule="self.all(s, s.resourcePrefixes.all(p, p == '/' || p.endsWith('#')))",message="non-default prefixes must end with '#'" | ||||||||||||||||||
| Shards []UnmanagedEtcdShardSpec `json:"shards,omitempty"` | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+2061
to
+2089
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. Validation gap allows invalid UnmanagedEtcdSpec state. The Consider adding an XValidation rule: // +kubebuilder:validation:XValidation:rule="has(self.shards) || (self.endpoint != '' && has(self.tls))",message="either shards must be specified, or both endpoint and tls are required"The mutual exclusivity validation (preventing both shards and endpoint/tls from being set simultaneously) was also flagged in a previous review. 🤖 Prompt for AI Agents |
||||||||||||||||||
|
|
||||||||||||||||||
| // UnmanagedEtcdShardSpec defines configuration for a single unmanaged etcd shard | ||||||||||||||||||
| type UnmanagedEtcdShardSpec struct { | ||||||||||||||||||
| // name is the unique identifier for this shard | ||||||||||||||||||
| // Must be DNS-1035 compliant (lowercase alphanumeric + hyphens) | ||||||||||||||||||
| // +required | ||||||||||||||||||
| // +kubebuilder:validation:MinLength=1 | ||||||||||||||||||
| // +kubebuilder:validation:XValidation:rule="self.matches('^[a-z]([-a-z0-9]*[a-z0-9])?$')",message="name must be DNS-1035 compliant" | ||||||||||||||||||
| // +kubebuilder:validation:MaxLength=15 | ||||||||||||||||||
| Name string `json:"name,omitempty"` | ||||||||||||||||||
|
|
||||||||||||||||||
| // resourcePrefixes specifies which Kubernetes resources are stored in this shard | ||||||||||||||||||
| // Format: "group/resource#" or "/" for default (catch-all) | ||||||||||||||||||
| // Examples: "/events#", "/coordination.k8s.io/leases#", "/" | ||||||||||||||||||
| // Exactly one shard must have "/" as a prefix | ||||||||||||||||||
| // +required | ||||||||||||||||||
| // +kubebuilder:validation:MinItems=1 | ||||||||||||||||||
| // +kubebuilder:validation:MaxItems=50 | ||||||||||||||||||
| // +kubebuilder:validation:items:MinLength=1 | ||||||||||||||||||
| // +kubebuilder:validation:items:MaxLength=255 | ||||||||||||||||||
| // +listType=set | ||||||||||||||||||
| ResourcePrefixes []string `json:"resourcePrefixes,omitempty"` | ||||||||||||||||||
|
|
||||||||||||||||||
| // priority determines operational importance | ||||||||||||||||||
| // +optional | ||||||||||||||||||
| // +default="Medium" | ||||||||||||||||||
| Priority EtcdShardPriority `json:"priority,omitempty"` | ||||||||||||||||||
|
|
||||||||||||||||||
| // endpoint is the full etcd shard client endpoint URL | ||||||||||||||||||
| // Example: https://etcd-events-client:2379 | ||||||||||||||||||
| // +required | ||||||||||||||||||
| // +kubebuilder:validation:Pattern=`^https://` | ||||||||||||||||||
| // +kubebuilder:validation:MaxLength=255 | ||||||||||||||||||
| Endpoint string `json:"endpoint"` | ||||||||||||||||||
|
|
||||||||||||||||||
| // tls specifies TLS configuration for HTTPS etcd client endpoints. | ||||||||||||||||||
| // tls specifies TLS configuration for this shard's HTTPS endpoint | ||||||||||||||||||
| // +required | ||||||||||||||||||
| TLS EtcdTLSConfig `json:"tls"` | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
Could also make it so that this field cannot be set, forcing a choice in each shard to be explicit