OCPEDGE-1512: feat: add dualreplica featuregate #2196

eggfoobar · 2025-02-06T15:28:13Z

add feature gate to support DualReplica control plante topology as a featuregate for two node openshift deployments

openshift-ci-robot · 2025-02-06T15:28:19Z

@eggfoobar: This pull request references OCPEDGE-1512 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

add feature gate to support DualReplica control plante topology as a featuregate for two node openshift deployments

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2025-02-06T15:29:11Z

Hello @eggfoobar! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

JoelSpeed · 2025-02-06T15:43:09Z

Skimming this, the generation looks out of date to me, can you try a PROTO_OPTIONAL=1 make update please

eggfoobar · 2025-02-06T16:23:37Z

Skimming this, the generation looks out of date to me, can you try a PROTO_OPTIONAL=1 make update please

Hmm, so I didn't see anything change, I had forgotten to save a test file but other than that, nothing more got altered.

The verify-crd-schema is failing but when checking against the HighlyAvailableArbiter addition, which is similar, I didn't notice much difference that would trigger the failure with

�[0m�[91m		ConditionsMustHaveProperSSATags: 
�[0m�[91m			crd/controllerconfigs.machineconfiguration.openshift.io version/v1 field/^.status.condition must define valid condition properties: observedGeneration attribute is missing
�[0m�[91m		NoMaps: 
�[0m�[91m			crd/controllerconfigs.machineconfiguration.openshift.io version/v1 field/^.spec.dns.spec.privateZone.tags may not be a map
�[0m�[91m		NoMaps: 
�[0m�[91m			crd/controllerconfigs.machineconfiguration.openshift.io version/v1 field/^.spec.dns.spec.publicZone.tags may not be a map
�[0m�[91m		NoMaps: 
�[0m�[91m			crd/controllerconfigs.machineconfiguration.openshift.io version/v1 field/^.spec.images may not be a map

JoelSpeed · 2025-02-06T17:33:14Z

This could be an issue with the tooling, but if you check the generated CRDs, your updated enum values are not being applied to the merged schemas, will need to look into this 🤔

eggfoobar · 2025-02-07T05:53:05Z

Yeah the way I had the tags was wrong, the tooling syntax parsing for the tags expects the FeatureGateAwareEnum:featureGate= to contain multiple values instead of it being multiple lines.

This might be something we might want to alter, since currently enabling one featureSet enables the other, unless using rules validation would be the preferable way to enforce this scenario. Just not sure how that would work since we would need to know the featureSet value.

JoelSpeed · 2025-02-07T11:47:42Z

Yeah the way I had the tags was wrong, the tooling syntax parsing for the tags expects the FeatureGateAwareEnum:featureGate= to contain multiple values instead of it being multiple lines.

I think what you have now will work, but won't let us promote the two features independently. I'll need to look into this because I don't think we've had this scenario before, and the tooling will likely need an update to be able to handle this

eggfoobar · 2025-02-07T13:56:31Z

I think that's fine, the two features are similar enough where that shouldn't be a problem.

eggfoobar · 2025-02-07T14:24:52Z

Just to clarify your comment @JoelSpeed, Arbiter hits GA before DualReplica, and we remove it from the featureGate tag and just make it a Default, will that be a problem for Arbiter since DualReplica will more than likely stay in TP a release longer?

JoelSpeed · 2025-02-10T14:43:31Z

That's correct. We need to teach the tooling to be able to understand what to do when one of the gates is enabled, but the other is not, I will try to find some time to look into that problem this week, unless you'd like to?

eggfoobar · 2025-02-11T06:03:05Z

That's correct. We need to teach the tooling to be able to understand what to do when one of the gates is enabled, but the other is not, I will try to find some time to look into that problem this week, unless you'd like to?

Yeah, I can give it a shot, been meaning to become more familiar with the tooling

eggfoobar · 2025-02-25T06:07:47Z

Hey @JoelSpeed got sometime to look at this today.

I think this might be a bit more difficult than I had originally thought, or more likely I don't have more context on this pattern. As it currently stands this can behave as expected if, for example, when we go GA with Arbiter by adding it to the Default featureSet, changing this featureGate="",enum=HighlyAvailable;SingleReplica;External to featureGate="",enum=HighlyAvailable;HighlyAvailableArbiter;SingleReplica;External and featureGate=HighlyAvailableArbiter;DualReplica to featureGate=DualReplica, then things seem to generate correctly.

However, if we wish the behavior to be that featureGate=HighlyAvailableArbiter;DualReplica stands as is and we just add Arbiter to the Default featureSet, then this seems to be a bit trickier. We'll probably need to define a key/value to split them apart when we generate the Default CRD.

Ideally, it would be better if we had featureGate=HighlyAvailableArbiter and featureGate=DualReplica, as their own separate values, but this seems like a tougher change since it works correctly for the most part until we get to this part in the code, we delegate to the upstream to merge the CRDs here, it seems that since the enum is a value it doesn't join them, it just overrides them with the last item in the sorted FeatureGate list, which happens to be HighlyAvailableArbiter

What do you think, any other avenues come to mind on how to approach this?

JoelSpeed · 2025-02-25T17:27:52Z

It's hack and hustle this friday, I'll see if I can dig in then.

But your analysis is correct.

I think the only away around this is to try and work out the combinations, so, if a file exists that matches more gates (ie HighlyAvailable+DualReplica) then it should ignore HighlyAvailable and DualReplica files.

Then from the perspective of what does this look like in the API, we would have to have 4 entries

The enum for the fone for each of "", HighliyAvailable, DualReplica and HighlyAvailable;DualReplica

We could then tell it what behaviour we want in each combination, and have the correct promotion behaviour

config/v1/types_infrastructure.go

eggfoobar · 2025-03-03T16:01:42Z

Discussed in slack, added a TODO comment and created a Jira ticket to keep track of the issue discussed here. The HighlyAvailableArbiter or DualReplica will not go GA until the problem discussed here is resolved. This is being moved along to unblock other work for TechPreview of DualReplica.

jaypoulz · 2025-03-03T19:46:44Z

/lgtm

The core changes are what we need, but I'm concerned with the error reported in by the verify-crd-schema job since it doesn't look related. do something need to be regenerated?

eggfoobar · 2025-03-03T20:14:37Z

/retest

I don't think we can do much more on the verify-crd-schema failure, since the fields are the same as the arbiter addition, in that PR we overrode this check, but Joel would know better on the next course of action.

JoelSpeed · 2025-03-04T11:53:48Z

/override ci/prow/verify-crd-schema

Existing failures in a partial CRD manifest

JoelSpeed · 2025-03-04T17:21:47Z

/override ci/prow/verify-crd-schema

Existing failures in a partial CRD manifest

openshift-ci · 2025-03-04T17:22:07Z

@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-crd-schema

In response to this:

/override ci/prow/verify-crd-schema

Existing failures in a partial CRD manifest

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.

add feature gate to support DualReplica control plante topology as a featuregate for two node openshift deployments. added todo to make sure we do not go GA with out resolving the annotation issue. Signed-off-by: ehila <[email protected]>

eggfoobar · 2025-03-04T20:49:04Z

Dang, got caught by another conflict from anther PR being merged in, rebased.

eggfoobar · 2025-03-04T20:55:21Z

/override ci/prow/verify-crd-schema

Existing failures in a partial CRD manifest

openshift-ci · 2025-03-04T20:55:38Z

@eggfoobar: eggfoobar unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:openshift: openshift-release-oversight openshift-staff-engineers.

In response to this:

/override ci/prow/verify-crd-schema

Existing failures in a partial CRD manifest

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.

jaypoulz · 2025-03-04T20:56:07Z

/lgtm

openshift-ci · 2025-03-04T20:57:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eggfoobar, jaypoulz, JoelSpeed

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~~ [JoelSpeed]

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

eggfoobar · 2025-03-04T20:58:02Z

Tried to override since nothing changed and we just needed to update due to conflict from another PR. @jerpeter1 Would you mind overriding verify-crd /override ci/prow/verify-crd-schema ?

openshift-ci-robot · 2025-03-04T20:59:13Z

/retest-required

Remaining retests: 0 against base HEAD 422203b and 2 for PR HEAD 8fb0163 in total

jerpeter1 · 2025-03-04T21:00:15Z

/override ci/prow/verify-crd-schema

openshift-ci · 2025-03-04T21:00:58Z

@jerpeter1: Overrode contexts on behalf of jerpeter1: ci/prow/verify-crd-schema

In response to this:

/override ci/prow/verify-crd-schema

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.

eggfoobar · 2025-03-05T00:50:51Z

/retest-required

openshift-ci-robot · 2025-03-05T02:02:45Z

/retest-required

Remaining retests: 0 against base HEAD 422203b and 2 for PR HEAD 8fb0163 in total

eggfoobar · 2025-03-05T04:41:12Z

/test e2e-aws-ovn-hypershift

qJkee · 2025-03-05T08:30:37Z

/override ci/prow/verify-crd-schema

openshift-ci · 2025-03-05T08:31:27Z

@qJkee: qJkee unauthorized: /override is restricted to Repo administrators, approvers in top level OWNERS file, and the following github teams:openshift: openshift-release-oversight openshift-staff-engineers.

In response to this:

/override ci/prow/verify-crd-schema

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.

JoelSpeed · 2025-03-05T09:23:23Z

/override ci/prow/verify-crd-schema

Existing failures in a partial CRD manifest

openshift-ci · 2025-03-05T09:24:59Z

@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-crd-schema

In response to this:

/override ci/prow/verify-crd-schema

Existing failures in a partial CRD manifest

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.

openshift-ci · 2025-03-05T09:25:03Z

@eggfoobar: 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.

openshift-bot · 2025-03-05T11:04:28Z

[ART PR BUILD NOTIFIER]

Distgit: ose-cluster-config-api
This PR has been included in build ose-cluster-config-api-container-v4.19.0-202503051041.p0.g1cd9f24.assembly.stream.el9.
All builds following this will include this PR.

xueqzhan · 2025-03-12T20:17:48Z

/payload-job periodic-ci-openshift-release-master-ci-4.19-e2e-gcp-ovn-techpreview-serial
/payload-job periodic-ci-openshift-release-master-ci-4.19-e2e-gcp-ovn-techpreview-serial
/payload-job periodic-ci-openshift-release-master-ci-4.19-e2e-gcp-ovn-techpreview-serial
/payload-job periodic-ci-openshift-release-master-ci-4.19-e2e-gcp-ovn-techpreview-serial

Please ignore for now. We are testing some payload failures

openshift-ci · 2025-03-12T20:17:53Z

@xueqzhan: trigger 4 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command

periodic-ci-openshift-release-master-ci-4.19-e2e-gcp-ovn-techpreview-serial
periodic-ci-openshift-release-master-ci-4.19-e2e-gcp-ovn-techpreview-serial
periodic-ci-openshift-release-master-ci-4.19-e2e-gcp-ovn-techpreview-serial
periodic-ci-openshift-release-master-ci-4.19-e2e-gcp-ovn-techpreview-serial

See details on https://pr-payload-tests.ci.openshift.org/runs/ci/119f0b60-ff7f-11ef-8cc1-b869ab0caa60-0

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

openshift-ci bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Feb 6, 2025

eggfoobar force-pushed the add-dualreplica-featuregate branch from 4645129 to 80ff76b Compare February 6, 2025 15:34

openshift-ci bot requested review from cgwalters and JoelSpeed February 6, 2025 15:36

eggfoobar force-pushed the add-dualreplica-featuregate branch from 80ff76b to b3d1ab0 Compare February 6, 2025 16:17

eggfoobar force-pushed the add-dualreplica-featuregate branch from b3d1ab0 to 9a92c12 Compare February 7, 2025 05:41

jaypoulz mentioned this pull request Feb 19, 2025

OCPEDGE-1500: Added scaling strategies for TNF openshift/cluster-etcd-operator#1396

Merged

eggfoobar force-pushed the add-dualreplica-featuregate branch from 9a92c12 to 8475947 Compare February 25, 2025 06:07

JoelSpeed reviewed Mar 3, 2025

View reviewed changes

config/v1/types_infrastructure.go Show resolved Hide resolved

eggfoobar force-pushed the add-dualreplica-featuregate branch from 8475947 to 7caf83f Compare March 3, 2025 15:57

openshift-ci bot assigned jaypoulz Mar 3, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 3, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2025

feat: add dualreplica featuregate

8fb0163

add feature gate to support DualReplica control plante topology as a featuregate for two node openshift deployments. added todo to make sure we do not go GA with out resolving the annotation issue. Signed-off-by: ehila <[email protected]>

eggfoobar force-pushed the add-dualreplica-featuregate branch from b7593dd to 8fb0163 Compare March 4, 2025 20:48

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2025

openshift-merge-bot bot merged commit 1cd9f24 into openshift:master Mar 5, 2025
23 checks passed

eggfoobar deleted the add-dualreplica-featuregate branch March 5, 2025 12:34

eggfoobar mentioned this pull request Apr 16, 2025

OCPEDGE-1775: Enable separation of conflicting enum values, and drop DualReplica to DevPreview #2283

Merged

OCPEDGE-1512: feat: add dualreplica featuregate #2196

OCPEDGE-1512: feat: add dualreplica featuregate #2196

Uh oh!

Conversation

eggfoobar commented Feb 6, 2025

Uh oh!

openshift-ci-robot commented Feb 6, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Feb 6, 2025

Uh oh!

JoelSpeed commented Feb 6, 2025

Uh oh!

eggfoobar commented Feb 6, 2025

Uh oh!

JoelSpeed commented Feb 6, 2025

Uh oh!

eggfoobar commented Feb 7, 2025

Uh oh!

JoelSpeed commented Feb 7, 2025

Uh oh!

eggfoobar commented Feb 7, 2025

Uh oh!

eggfoobar commented Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoelSpeed commented Feb 10, 2025

Uh oh!

eggfoobar commented Feb 11, 2025

Uh oh!

eggfoobar commented Feb 25, 2025

Uh oh!

JoelSpeed commented Feb 25, 2025

Uh oh!

Uh oh!

eggfoobar commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jaypoulz commented Mar 3, 2025

Uh oh!

eggfoobar commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JoelSpeed commented Mar 4, 2025

Uh oh!

JoelSpeed commented Mar 4, 2025

Uh oh!

openshift-ci bot commented Mar 4, 2025

Uh oh!

eggfoobar commented Mar 4, 2025

Uh oh!

eggfoobar commented Mar 4, 2025

Uh oh!

openshift-ci bot commented Mar 4, 2025

Uh oh!

jaypoulz commented Mar 4, 2025

Uh oh!

openshift-ci bot commented Mar 4, 2025

Uh oh!

eggfoobar commented Mar 4, 2025

Uh oh!

openshift-ci-robot commented Mar 4, 2025

Uh oh!

jerpeter1 commented Mar 4, 2025

Uh oh!

openshift-ci bot commented Mar 4, 2025

Uh oh!

eggfoobar commented Mar 5, 2025

Uh oh!

openshift-ci-robot commented Mar 5, 2025

Uh oh!

eggfoobar commented Mar 5, 2025

Uh oh!

qJkee commented Mar 5, 2025

Uh oh!

openshift-ci bot commented Mar 5, 2025

Uh oh!

JoelSpeed commented Mar 5, 2025

Uh oh!

openshift-ci bot commented Mar 5, 2025

openshift-ci-robot commented Feb 6, 2025 •

edited by openshift-ci bot

Loading

eggfoobar commented Feb 7, 2025 •

edited

Loading

eggfoobar commented Mar 3, 2025 •

edited

Loading

eggfoobar commented Mar 3, 2025 •

edited

Loading