Add external topology mode #940
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -82,6 +82,8 @@ type InfrastructureStatus struct { | |
| // The default is 'HighlyAvailable', which represents the behavior operators have in a "normal" cluster. | ||
| // The 'SingleReplica' mode will be used in single-node deployments | ||
| // and the operators should not configure the operand for highly-available operation | ||
| // The 'External' mode indicates that the control plane is hosted externally to the cluster and that | ||
| // its components are not visible within the cluster. | ||
| // +kubebuilder:default=HighlyAvailable | ||
| ControlPlaneTopology TopologyMode `json:"controlPlaneTopology"` | ||
|
|
||
|
|
@@ -96,7 +98,7 @@ type InfrastructureStatus struct { | |
| } | ||
|
|
||
| // TopologyMode defines the topology mode of the control/infra nodes. | ||
| // +kubebuilder:validation:Enum=HighlyAvailable;SingleReplica;External | ||
|
There was a problem hiding this comment. If we move this declaration up to the fields above, we can provide different enum values and prevent the infra topology from being set to external. We would want a comment here to explain why we're doing it that way, too.
Contributor
Author
There was a problem hiding this comment. @dhellmann I like this suggestion. We can then make sure we don't set an invalid value for infra topology. Will submit a follow up PR |
||
| type TopologyMode string | ||
|
|
||
| const ( | ||
|
|
@@ -105,6 +107,12 @@ const ( | |
|
|
||
| // "SingleReplica" is for operators to avoid spending resources for high-availability purpose. | ||
| SingleReplicaTopologyMode TopologyMode = "SingleReplica" | ||
|
|
||
| // "External" indicates that the component is running externally to the cluster. When specified | ||
| // as the control plane topology, operators should avoid scheduling workloads to masters or assume | ||
| // that any of the control plane components such as kubernetes API server or etcd are visible within | ||
| // the cluster. | ||
| ExternalTopologyMode TopologyMode = "External" | ||
| ) | ||
|
|
||
| // PlatformType is a specific supported infrastructure provider. | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
are we actually consistent about infra?
in hypershift we have some infra components that run external (olm) and some that run on cluster (registry) right?
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.
That is correct. From an operator point of view, I don't think the External value makes sense for infra topology (Unless we really externalize all of infra). It is meant to apply only to control plane topology for now. However, not sure that we can express that through the API as it is.
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.
So "external" will not be a valid value for status.infrastructureTopology ?
I guess can understand not creating another Type to split them, so i guess we can live with this but it is somewhat unfortunate that it confusingly implies it is valid, even if practice we'd never actually set it to that value.