Add HighAvailabilityMode field to 'infrastructure' CRD #827

ravidbro · 2020-12-20T19:38:57Z

Implements enhancement openshift/enhancements#555

Signed-off-by: Ravid Brown [email protected]

config/v1/types_infrastructure.go

romfreiman · 2021-01-04T19:20:10Z

sttts · 2021-01-05T08:52:50Z

config/v1/0000_10_config-operator_01_infrastructure.crd.yaml

+              type: string
+              enum:
+              - ""
+              - Full


how about something along ThreeNode ?

I think in the enhancement we want to have either full or non-ha. How ThreeNodes will help?

is there half HA ? APIs should be clear when you read them. Full and None have no meaning.

https://github.com/openshift/enhancements/pull/555/files#diff-47d36bb8151b04437b398146ca27efc44639a74bb22faef61f0c54bc6f193505R82

Thanks for the link. Let's move the bigshedding there. I should have guessed that this field has been bikeshedded to death already ;-)

romfreiman · 2021-01-12T18:57:56Z

@ravidbro once the enhancement is merged, lets update this PR.

ravidbro · 2021-01-14T09:44:12Z

@ravidbro once the enhancement is merged, lets update this PR.

Updated according to the merged enhancement

sttts · 2021-01-14T12:07:27Z

config/v1/0000_10_config-operator_01_infrastructure.crd.yaml

+                the operand for highly-available operation
+              type: string
+              enum:
+              - ""


why do we need empty? Can't we default to HighlyAvailable?

#827 (comment)

that comment does not explain why we don't just use a default, making the API easier to use and observe.

@staebler can you elaborate on why do you think we should have the empty value and not a default?

The empty option was removed

sttts · 2021-01-14T12:08:07Z

config/v1/types_infrastructure.go

+	// The default is 'HighlyAvailable', which represents the behavior operators have in a \"normal\" cluster.
+	// The 'SingleReplica' mode will be used in single-node deployments (developer and production) for example,
+	// and the operators should not configure the operand for highly-available operation
+	ControlPlaneTopology TopologyMode `json:"controlPlaneTopology"`


// +default=HighlyAvailable

this field is only under the 'status' and not under the 'spec'.
does it need to have a default in the definition here?

It does not have to. But a default value shows up automatically if not set otherwise. What should operators do if this field is not set?

I see your point, I will add it

In the enhancement we said that if the field wasn't set operators should assume HighlyAvailable as a default. If setting the default in the CRD definition ensures that a value is provided for clusters upgraded from 4.7 to 4.8 (when the field is added), then maybe we didn't need to worry about that?

I checked it, and I can't use a default value here since this CRD is apiextensions.k8s.io/v1beta1, and default values aren't supported yet.

I checked it, and I can't use a default value here since this CRD is apiextensions.k8s.io/v1beta1, and default values aren't supported yet.

Switch it to v1. We have done that with many CRDs in openshift/api already. No reason not to, just time.

@sttts not sure we have extra time to invest into it at this point. do we understand how much effort is it?

#834 merged. It's v1 CRD now.

Updated the default here and tested the upgrade from an old version. The fields were added to the existing 'cluster' CR with the default value.

config/v1/types_infrastructure.go

wking · 2021-01-14T17:42:08Z

config/v1/types_infrastructure.go

+	// +kubebuilder:default=HighlyAvailable
+	ControlPlaneTopology TopologyMode `json:"controlPlaneTopology"`
+
+	// InfrastructureTopology expresses the expectations for operands that normally run on infrastructure nodes.


"infrastructure nodes" are an OSD thing, not an OCP thing, and this is an OCP config structure. I realize that the enhancement discussed this addition, and am sorry I missed this in review there. Thoughts about revisiting in the enhancement, or discussing here, or...? I guess this API change will not land before 4.7 splits off master anyway, so we have some time...

Where else would we put the new field?

This was added at @derekwaynecarr's suggestion for the external control plane work, which makes me think the idea of infrastructure nodes is coming to OCP?

@derekwaynecarr proposing a control-plane/infra split here, with an eye to future External control plane settings or some such. That seems cross-cutty to me. As long as the only two values are HighlyAvailable and SingleReplica, I can't think of a situation where folks would set one value for ControlPlaneTopology and another for InfrastructureTopology. If we eventually grow External, the distinction would be:

ControlPlaneTopology stops being a way to tell control-plane components how available to be, and becomes a way to tell the remaining components that the control plane components are not local.

I'd rather have addressed that with a cluster-profile knob, since we're using the cluster-profile knob to actually remove the control-plane components. As it stands, which way would, say, the registry operator go? It tolerates the control-plane nodes, but also stays local, even in the IBM/ROKS case where the bulk of the control-plane moves to external hosting. Maybe all the components that fall into that category won't care about this setting anyway (anything managed by the CVO, which includes the registry-operator manifest, will not care about these settings). But it seems like "second-level operators should consume InfrastructureTopology when they are running, and look to ControlPlaneTopology in the future if it grows External"? That makes the names a bit wonky, but seems like the logic folks were going for?

IIUC the goal for the registry operator is to leverage the cluster profiles to support the IBM/ROKS use case:
openshift/cluster-image-registry-operator#643
We (monitoring team) have a similar WIP PR from @csrwng to move the cluster monitoring operator from the control plane nodes to the worker nodes.

@wking @simonpasquier I think we can draw a line between what cluster profiles do and what these fields are meant to accomplish. Cluster profiles affect the manifests that get laid down for operators by the CVO. In the cases that @simonpasquier pointed out, there is no opportunity to change the node selectors via operator logic because they will be laid down directly by the CVO, so if we need alternate node selectors we have to have the CVO handle it.

Cluster profiles could also be used to drive logic in the operator such as adding an environment variable or some other change to an operator manifest to indicate that it should work in HA/non-HA mode. But IMHO it would be less than ideal because then each operator gets to decide how this is communicated to them and they may not agree on what each mode means. My understanding is that these fields are a consistent way to communicate to operators the desired availability mode.

For possible combinations see openshift/enhancements#555 (comment)

@derekwaynecarr suggested the split. I'm not 100% clear on what goes on each side of the line myself. We should probably go back and document that better in the enhancement (and in comments here).

@wking this is not to be read as literal as role=infra nodes. it should be understood as components that provide supporting infrastructure services for applications on the cluster but are not run on control plane hosts by default. for example, it would include components like the registry, router, and monitoring. today those components can be selected to run on node with role=infra but they default to role=worker. what this field is intending to capture is if those supporting services should run highly available or not. it could be read by additional add-ons as a hint in the future for kubevirt, serverless, mesh, pipelines, or any other supporting infrastructure service to understand what availability posture their supporting solution should provide by default.

a very simple use case is a cluster whose control plane is externalized, but whose infrastructure services do not require HA. for example, an IBM ROKS/Satellite cluster with a single worker. this would require a router to not error when its not running HA, which this flag would signal as the desired state.

so maybe update text as follows:

InfrastructureTopology expresses the expectations for operands that provide supporting infrastructure services for workloads on the cluster, but do not run on control hosts by default. If an operator API provides a node selector for placement of its operands, the operator should respect this API for expressing its availability absent any local override.

an alternative scenario is regular standalone highly-available topology (3 control plane hosts) with a single worker. in that model, i may desire HA control plane topology, and SingleReplica Infrastructure Topology. it is acceptible in that instance to run a single replica of the registry for example.

I've proposed an update to the original API enhancement to clarify this. See openshift/enhancements#605

I updated the comment in the code accordingly.

romfreiman · 2021-01-18T19:42:21Z

@wking @csrwng @derekwaynecarr how do you suggest to proceed regrading the "infrastructure nodes" nodes discussion?
#827 (comment)
Are we good as it is now? Or extra infa/changes are required?
Here is an example how it should be used in ingress:
https://github.com/openshift/cluster-ingress-operator/pull/532/files#diff-99c0290799daf9abc6240df64063e20bfaf67b371577b67ac7eec6f4725622ffR189

csrwng · 2021-01-18T21:20:41Z

@romfreiman for me, it's fine as it is

romfreiman · 2021-01-18T22:05:32Z

@wking what about you?

dhellmann · 2021-01-26T15:09:10Z

config/v1/0000_10_config-operator_01_infrastructure.crd.yaml

+                description: InfrastructureTopology expresses the expectations for
+                  infrastructure services that do not run on control plane nodes,
+                  usually indicated by a node selector for a `role` value other than
+                  `master`. This includes, but is not necessarily limited to, the


I think we can stop with the period after "master" here, and leave the next 2 sentences out of the CRD definition.

dhellmann · 2021-01-26T16:31:06Z

config/v1/0000_10_config-operator_01_infrastructure.crd.yaml

+                description: InfrastructureTopology expresses the expectations for
+                  infrastructure services that do not run on control plane nodes,
+                  usually indicated by a node selector for a `role` value other than
+                  `master`.


Do we need to duplicate the descriptions of the 2 modes from above?

I think it makes sense also here, and I copied it.

dhellmann · 2021-01-26T16:31:40Z

config/v1/0000_10_config-operator_01_infrastructure.crd.yaml

+                  that normally run on control nodes. The default is 'HighlyAvailable',
+                  which represents the behavior operators have in a "normal" cluster.
+                  The 'SingleReplica' mode will be used in single-node deployments
+                  (developer and production) for example, and the operators should


Since this is user-facing, we don't need to differentiate between the types of deployments.

Suggested change

(developer and production) for example, and the operators should

and the operators should

dhellmann

The current draft looks good to me.

/approve

sttts · 2021-01-28T09:45:25Z

config/v1/0000_10_config-operator_01_infrastructure.crd.yaml

                  the Kubernetes API.
                type: string
+              controlPlaneTopology:
+                description: ControlPlaneTopology expresses the expectations for operands


sttts · 2021-01-28T09:45:32Z

config/v1/0000_10_config-operator_01_infrastructure.crd.yaml

                  of max length 27 and must have only alphanumeric or hyphen characters.
                type: string
+              infrastructureTopology:
+                description: InfrastructureTopology expresses the expectations for


sttts · 2021-01-28T09:46:56Z

LGTM. nit: lower-case the field name in descriptions

/approve

sttts · 2021-01-28T09:49:24Z

/hold

until master opens.

Implements enhancement openshift/enhancements#555 Signed-off-by: Ravid Brown <[email protected]>

romfreiman · 2021-01-28T09:58:55Z

/lgtm

openshift-ci-robot · 2021-01-28T09:59:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann, ravidbro, romfreiman, sttts

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [sttts]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

romfreiman · 2021-01-28T10:03:11Z

/retest

ravidbro · 2021-01-28T13:33:29Z

/retest

romfreiman · 2021-02-04T10:24:58Z

/hold cancel

ravidbro · 2021-02-07T07:38:52Z

/retest

ravidbro · 2021-02-08T17:29:29Z

/bugzilla refresh

openshift-ci-robot · 2021-02-08T17:29:30Z

@ravidbro: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

Details

In response to this:

/bugzilla refresh

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 kubernetes/test-infra repository.

simonpasquier

redacted (wrong tab)

openshift-ci-robot requested review from bparees and knobunc December 20, 2020 19:39

ravidbro force-pushed the add_ha_mode branch 2 times, most recently from ecdb126 to d982d5f Compare December 21, 2020 15:37

ravidbro mentioned this pull request Dec 21, 2020

Add controlPlaneTopology to infrastructure manifest openshift/installer#4497

Merged

staebler reviewed Dec 21, 2020

View reviewed changes

config/v1/types_infrastructure.go Outdated Show resolved Hide resolved

ravidbro force-pushed the add_ha_mode branch from d982d5f to ff6d957 Compare December 22, 2020 08:13

Miciah mentioned this pull request Jan 4, 2021

NE-508 Support cluster HA mode openshift/cluster-ingress-operator#519

Merged

1 task

sttts reviewed Jan 5, 2021

View reviewed changes

markmc mentioned this pull request Jan 12, 2021

Cluster High-availability Mode API openshift/enhancements#555

Merged

ravidbro force-pushed the add_ha_mode branch from ff6d957 to 4382cda Compare January 14, 2021 09:39

sttts reviewed Jan 14, 2021

View reviewed changes

config/v1/types_infrastructure.go Outdated Show resolved Hide resolved

sttts reviewed Jan 14, 2021

View reviewed changes

config/v1/types_infrastructure.go Outdated Show resolved Hide resolved

sttts reviewed Jan 14, 2021

View reviewed changes

config/v1/types_infrastructure.go Outdated Show resolved Hide resolved

ravidbro force-pushed the add_ha_mode branch 3 times, most recently from 7c43428 to 24a7b5b Compare January 14, 2021 14:33

wking reviewed Jan 14, 2021

View reviewed changes

ravidbro force-pushed the add_ha_mode branch 4 times, most recently from c44d842 to 68bddd8 Compare January 18, 2021 13:15

openshift-ci-robot assigned smarterclayton Jan 20, 2021

ravidbro force-pushed the add_ha_mode branch from 68bddd8 to bd95d15 Compare January 26, 2021 10:35

dhellmann reviewed Jan 26, 2021

View reviewed changes

ravidbro force-pushed the add_ha_mode branch from bd95d15 to 53a4706 Compare January 26, 2021 15:47

dhellmann reviewed Jan 26, 2021

View reviewed changes

ravidbro force-pushed the add_ha_mode branch from 53a4706 to f1ae31a Compare January 27, 2021 08:38

dhellmann reviewed Jan 27, 2021

View reviewed changes

sttts reviewed Jan 28, 2021

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 28, 2021

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 28, 2021

Add HighAvailabilityMode field to 'infrastructure' CRD

56a849f

Implements enhancement openshift/enhancements#555 Signed-off-by: Ravid Brown <[email protected]>

ravidbro force-pushed the add_ha_mode branch from f1ae31a to 56a849f Compare January 28, 2021 09:55

openshift-ci-robot assigned romfreiman Jan 28, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 28, 2021

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 4, 2021

openshift-merge-robot merged commit 7c8b5fd into openshift:master Feb 8, 2021

jhadvig mentioned this pull request Feb 9, 2021

CONSOLE-2757: Bump github.com/openshift/api openshift/console-operator#506

Merged

simonpasquier reviewed Mar 1, 2021

View reviewed changes

simonpasquier mentioned this pull request Mar 1, 2021

WIP: enhancements/monitoring: Describe single replica topology mode openshift/enhancements#671

Closed

	(developer and production) for example, and the operators should
	and the operators should

Add HighAvailabilityMode field to 'infrastructure' CRD #827

Add HighAvailabilityMode field to 'infrastructure' CRD #827

Uh oh!

Conversation

ravidbro commented Dec 20, 2020

Uh oh!

Uh oh!

romfreiman commented Jan 4, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romfreiman commented Jan 12, 2021

Uh oh!

ravidbro commented Jan 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sttts Jan 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wking Jan 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

derekwaynecarr Jan 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sttts Jan 18, 2021 •

edited

Loading

wking Jan 14, 2021 •

edited

Loading

derekwaynecarr Jan 25, 2021 •

edited

Loading

romfreiman commented Jan 18, 2021 •

edited

Loading