MGMT-19980: TNA clusters #7402

giladravid16 · 2025-03-13T15:21:05Z

Enhancement to define the implementation of installing TNA clusters.
See openshift's enhancement for more details about TNA clusters.

List all the issues related to this PR

What environments does this code impact?

Automation (CI, tools, etc)
Cloud
Operator Managed Deployments
None

How was this code tested?

assisted-test-infra environment
dev-scripts environment
Reviewer's test appreciated
Waiting for CI to do a full test run
Manual (Elaborate on how it was tested)
No tests needed

Checklist

Title and description added to both, commit and PR.
Relevant issues have been associated (see CONTRIBUTING guide)
This change does not require a documentation update (docstring, docs, README, etc)
Does this change include unit-tests (note that code changes require unit-tests)

Reviewers Checklist

Are the title and description (in both PR and commit) meaningful and clear?
Is there a bug required (and linked) for this change?
Should this PR be backported?

openshift-ci-robot · 2025-03-13T15:21:10Z

openshift-ci · 2025-03-13T15:23:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giladravid16

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [giladravid16]

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

giladravid16 · 2025-03-13T15:23:50Z

/cc @carbonin @eranco74

tsorya · 2025-03-18T14:54:13Z

docs/enhancements/arbiter-clusters.md

+  the worker's requirements to avoid issues mid-development).
+- The HostTypeHardwareRequirementsWrapper structs used by the operators.
+
+We will auto-assign the arbiter role to a host if the following are true:


Probably we need to assign it to the host with lowest HW in case all 3 matches HW requirements to be masters?

My initial idea was to check for the lowest HW that pass the requirements, but then I saw that right now for masters we auto-select the first ones we find without comparing it to the other hosts (even if it's a total waste of resources) and I assume it's because it can be difficult to determine which host is smallest (we would need to compare ClusterHostRequirementsDetails).

gamli75 · 2025-03-18T15:02:03Z

docs/enhancements/arbiter-clusters.md

+This enhancement will include the following:
+- In the API we will:
+  - Add a new host_role for arbiter.
+  - Add a new high_availability_mode for TNA.


the high_availability_mode is deprecated, and we will use control_plane_count instead.
I'm not sure if 3 CP == 2 CP with 1 arbiter.
@danmanor WDYT?

The most explicit thing to do is add arbiter_count. I wouldn't use control_plane_count=2 alone for TNA, because one day OpenShift might support a 2-node cluster without an arbiter (this was demoed some time back).

That's why I thought of using high_availability_mode, in openshift-installer they set the ControlPlaneTopology to HighlyAvailableArbiter and InfrastructureTopology to HighlyAvailable for this reason.

As @gamli75 said, high_availability_mode is deprecated in the Assisted API.

I added arbiter_count instead

gamli75 · 2025-03-18T15:03:52Z

docs/enhancements/arbiter-clusters.md

+  - Deprecate the existing parameters and create new ones for hyperthreading
+    and disk encryption (they will be maps of host_role to bool).
+  - Add a new feature_support_level_id for TNA.
+- We will update the clusters' validations and transitions for TNA clusters.


can you add more details about the validations that will be affected by this change?

I go into more details in the Implementation Details

gamli75 · 2025-03-18T15:08:39Z

docs/enhancements/arbiter-clusters.md

+
+Our validations will need to be updated as follows:
+- A host can only be assigned the arbiter role if the cluster is TNA.
+- TNA clusters must have at least 1 arbiter node.


Can we add a host with an arbiter role on day 2? If not, let's add it here

I'd imagine not. Maybe add this as a non-goal.

I checked and it is allowed to add arbiter nodes as day2 nodes, if the cluster was installed as TNA.

If we want to handle it in day2 - let's add it to the enhancement

I added it as a goal

carbonin · 2025-03-18T17:55:06Z

docs/enhancements/arbiter-clusters.md

+
+### User Stories
+
+- As a solutions architect for a retail organization, I want to deploy OpenShift


I'd rather see user stories associated with assisted installer here. I'm not sure how these are helpful for framing how we're going to implement this.

These are good to justify why OpenShift might want to allow this kind of topology, but that's not really relevant here.

@gamli75 are there any user stories of our customers wanting a topology like this?

carbonin · 2025-03-18T17:55:39Z

docs/enhancements/arbiter-clusters.md

+
+Our validations will need to be updated as follows:
+- A host can only be assigned the arbiter role if the cluster is TNA.
+- TNA clusters must have at least 1 arbiter node.


I'd imagine not. Maybe add this as a non-goal.

carbonin · 2025-03-18T17:56:07Z

docs/enhancements/arbiter-clusters.md

+
+Our validations will need to be updated as follows:
+- A host can only be assigned the arbiter role if the cluster is TNA.
+- TNA clusters must have at least 1 arbiter node.


Why at least? Are we expecting more than one?

The openshift-installer does not set a max limit on the number of arbiter nodes, it just has to be at least 1.

docs/enhancements/arbiter-clusters.md

carbonin · 2025-03-18T17:57:04Z

docs/enhancements/arbiter-clusters.md

+Our validations will need to be updated as follows:
+- A host can only be assigned the arbiter role if the cluster is TNA.
+- TNA clusters must have at least 1 arbiter node.
+- TNA clusters can have 2-5 control plane nodes.


Similar question here. Why have an arbiter node if we have 3 or more CP nodes?

The openshift-installer allows it, so I assume we should also allow it.

carbonin · 2025-03-18T18:00:50Z

docs/enhancements/arbiter-clusters.md

+A few drawbacks we have is that we will be creating a new variant of OpenShift
+that implements a new unique way of doing HA for kubernetes. This does mean


This isn't a drawback of our implementation. This sounds like it's talking more about TNA generally.

This enhancement should focus on the changes in assisted installer rather than TNA as an OpenShift topology.

docs/enhancements/arbiter-clusters.md

avishayt · 2025-03-18T19:44:39Z

docs/enhancements/arbiter-clusters.md

+This enhancement will include the following:
+- In the API we will:
+  - Add a new host_role for arbiter.
+  - Add a new high_availability_mode for TNA.


The most explicit thing to do is add arbiter_count. I wouldn't use control_plane_count=2 alone for TNA, because one day OpenShift might support a 2-node cluster without an arbiter (this was demoed some time back).

avishayt · 2025-03-18T19:45:28Z

docs/enhancements/arbiter-clusters.md

+  - Deprecate the existing parameters and create new ones for hyperthreading
+    and disk encryption (they will be maps of host_role to bool).


Currently hyperthreading is one of ['masters', 'workers', 'none', 'all'], I think it's better to define a struct that has a boolean for each role then add more combinations with arbiter.
Disk encryption is similar with it's enable_on field, I would prefer adding a new field.

From what I can tell, hyperthreading will only affect the REST API, so that's easy. Disk encryption would break the k8s API, which we should avoid.

I recommend extending the possibilities to avoid breaking the API. You can add:
'masters,workers'
'masters,arbiter'
'workers,arbiter'
'masters,workers,arbiter'

'all' would now mean 'masters,workers,arbiter'.
Using 'masters,workers,arbiter' would be future-proof in case we add another type later.

Since it's problematic to depracate the disk-encryption, I would rather also not depracate the hyperthreading. Either way the API will have the extra enum values that I tried to avoid.

giladravid16 · 2025-03-26T10:40:30Z

/retest-required

openshift-ci · 2025-03-26T12:45:57Z

@giladravid16: all tests passed!

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

carbonin · 2025-03-26T14:45:59Z

This looks fine to me

@tsorya @avishayt @gamli75 do you want to take another look?

gamli75 · 2025-03-26T15:00:25Z

docs/enhancements/arbiter-clusters.md

+- In the host discovery we need to allow setting a host's role to be arbiter.
+- In the cluster details:
+  - we need to allow setting the arbiter_count, with zero as the default.
+  - We need to allow setting control_plane_count to 2, but then force setting


@carbonin Will this be correct when we have the Two Node OpenShift with Fencing?

When we have TNF we'll need to add another parameter for it, and then the validation will need to check for either TNA or TNF. Right now only TNA is an option.

Right, I think this is fine for now when we only have TNA, but I don't think there's anything better we can do until we also need to handle TNF.

I suppose I take this back based on the conversation in the kickoff.

It might be better to just validate that we have a good combination rather than forcing arbiters to 1 since that's something we'd need to pull out once we have TNF.

carbonin · 2025-04-02T14:19:48Z

docs/enhancements/arbiter-clusters.md

+This enhancement will include the following:
+- In the API we will:
+  - Add a new host_role for arbiter.
+  - Add a new field for clusters named arbiter_count. The default value will


Based on the kickoff it would be better to not require this up front.

We should be able to infer when an arbiter would be required and do autoassign and validations accordingly after discovery.

carbonin · 2025-04-02T14:20:40Z

docs/enhancements/arbiter-clusters.md

+
+### Implementation Details/Notes/Constraints
+
+The enums for cluster's hyperthreading and disk encryption will be:


During kickoff it was raised that we may want to evaluate the usage of the disk encryption feature, but I'm not sure that really affects what we want to handle in this feature.

carbonin · 2025-04-02T14:21:12Z

docs/enhancements/arbiter-clusters.md

+
+### Open Questions
+
+N/A


We should add ABI concerns here. We should either ensure it works or disable it explicitly.

Same with kube-api for the first pass. We should disable this explicitly in the controllers if we're not going to support it on the first pass.

carbonin · 2025-04-02T14:22:05Z

docs/enhancements/arbiter-clusters.md

+at the edge. They can support running 2 node clusters for redundancy but would like 
+the option to deploy a lower cost node as an arbiter to supply the 3 nodes for etcd quorum.
+
+### Goals


Maybe add a goal for tracking this feature usage in metrics.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 13, 2025

openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 13, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 13, 2025

openshift-ci bot requested review from danmanor and rccrdpccl March 13, 2025 15:23

openshift-ci bot requested review from carbonin and eranco74 March 13, 2025 15:23

giladravid16 force-pushed the MGMT-19980 branch from 6bdb5e0 to 8005ed0 Compare March 17, 2025 13:05

tsorya reviewed Mar 18, 2025

View reviewed changes

gamli75 reviewed Mar 18, 2025

View reviewed changes

carbonin reviewed Mar 18, 2025

View reviewed changes

avishayt reviewed Mar 18, 2025

View reviewed changes

giladravid16 force-pushed the MGMT-19980 branch 2 times, most recently from 2571c9e to 4031a3b Compare March 20, 2025 09:38

MGMT-19980: TNA clusters

7ae70b4

giladravid16 force-pushed the MGMT-19980 branch from 4031a3b to 7ae70b4 Compare March 20, 2025 14:51

gamli75 reviewed Mar 26, 2025

View reviewed changes

carbonin reviewed Apr 2, 2025

View reviewed changes


		### User Stories

		- As a solutions architect for a retail organization, I want to deploy OpenShift

		A few drawbacks we have is that we will be creating a new variant of OpenShift
		that implements a new unique way of doing HA for kubernetes. This does mean

		- Deprecate the existing parameters and create new ones for hyperthreading
		and disk encryption (they will be maps of host_role to bool).


		### Implementation Details/Notes/Constraints

		The enums for cluster's hyperthreading and disk encryption will be:

MGMT-19980: TNA clusters #7402

Are you sure you want to change the base?

MGMT-19980: TNA clusters #7402

Conversation

giladravid16 commented Mar 13, 2025

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci-robot commented Mar 13, 2025 • edited by openshift-ci bot Loading

List all the issues related to this PR

What environments does this code impact?

How was this code tested?

Checklist

Reviewers Checklist

openshift-ci bot commented Mar 13, 2025

giladravid16 commented Mar 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giladravid16 Mar 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giladravid16 Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

giladravid16 commented Mar 26, 2025

openshift-ci bot commented Mar 26, 2025

carbonin commented Mar 26, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Mar 13, 2025 •

edited by openshift-ci bot

Loading

giladravid16 Mar 19, 2025 •

edited

Loading

giladravid16 Mar 20, 2025 •

edited

Loading