Skip to content

Conversation

maiqueb
Copy link
Contributor

@maiqueb maiqueb commented Sep 23, 2025

No description provided.

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 23, 2025
@openshift-ci-robot
Copy link

openshift-ci-robot commented Sep 23, 2025

@maiqueb: This pull request references CORENET-6005 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.21.0" version, but no target version was set.

In response to this:

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.

Copy link
Contributor

openshift-ci bot commented Sep 23, 2025

Hello @maiqueb! 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.

@openshift-ci openshift-ci bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 23, 2025
@openshift-ci openshift-ci bot requested a review from everettraven September 23, 2025 13:35
Copy link
Contributor

openshift-ci bot commented Sep 23, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign everettraven for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot requested a review from JoelSpeed September 23, 2025 13:35
@maiqueb
Copy link
Contributor Author

maiqueb commented Sep 23, 2025

/jira refresh

@openshift-ci-robot
Copy link

openshift-ci-robot commented Sep 23, 2025

@maiqueb: This pull request references CORENET-6005 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 either version "4.21." or "openshift-4.21.", but it targets "openshift-4.20" instead.

In response to this:

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

@maiqueb
Copy link
Contributor Author

maiqueb commented Sep 23, 2025

/jira refresh

@openshift-ci-robot
Copy link

openshift-ci-robot commented Sep 23, 2025

@maiqueb: This pull request references CORENET-6005 which is a valid jira issue.

In response to this:

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

@JoelSpeed
Copy link
Contributor

@maiqueb Is there a reason this is currently only being tested on one variant?

@maiqueb
Copy link
Contributor Author

maiqueb commented Sep 24, 2025

@maiqueb Is there a reason this is currently only being tested on one variant?

Hey @JoelSpeed o/

Yes, and a good one - this feature is only supported on bare-metal. Hence, no point in testing it in other platforms.

@maiqueb
Copy link
Contributor Author

maiqueb commented Sep 24, 2025

/retest verify-feature-promotion

Copy link
Contributor

openshift-ci bot commented Sep 24, 2025

@maiqueb: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test build
/test e2e-aws-ovn
/test e2e-aws-ovn-hypershift
/test e2e-aws-ovn-hypershift-conformance
/test e2e-aws-ovn-techpreview
/test e2e-aws-serial-1of2
/test e2e-aws-serial-2of2
/test e2e-aws-serial-techpreview-1of2
/test e2e-aws-serial-techpreview-2of2
/test e2e-azure
/test e2e-gcp
/test e2e-upgrade
/test e2e-upgrade-out-of-change
/test images
/test integration
/test lint
/test minor-e2e-upgrade-minor
/test minor-images
/test okd-scos-images
/test unit
/test verify
/test verify-client-go
/test verify-crd-schema
/test verify-crdify
/test verify-deps
/test verify-feature-promotion

The following commands are available to trigger optional jobs:

/test okd-scos-e2e-aws-ovn

Use /test all to run the following jobs that were automatically triggered:

pull-ci-openshift-api-master-build
pull-ci-openshift-api-master-images
pull-ci-openshift-api-master-integration
pull-ci-openshift-api-master-lint
pull-ci-openshift-api-master-minor-e2e-upgrade-minor
pull-ci-openshift-api-master-minor-images
pull-ci-openshift-api-master-okd-scos-e2e-aws-ovn
pull-ci-openshift-api-master-okd-scos-images
pull-ci-openshift-api-master-unit
pull-ci-openshift-api-master-verify
pull-ci-openshift-api-master-verify-client-go
pull-ci-openshift-api-master-verify-crd-schema
pull-ci-openshift-api-master-verify-crdify
pull-ci-openshift-api-master-verify-deps
pull-ci-openshift-api-master-verify-feature-promotion

In response to this:

/retest verify-feature-promotion

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.

@maiqueb
Copy link
Contributor Author

maiqueb commented Sep 24, 2025

/test verify-feature-promotion

Copy link
Contributor

openshift-ci bot commented Sep 24, 2025

@maiqueb: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/minor-e2e-upgrade-minor ec8539c link true /test minor-e2e-upgrade-minor
ci/prow/okd-scos-e2e-aws-ovn ec8539c link false /test okd-scos-e2e-aws-ovn
ci/prow/verify-feature-promotion ec8539c link true /test verify-feature-promotion

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.

@JoelSpeed
Copy link
Contributor

@maiqueb And its only supported on dual stack? Or also single?

@maiqueb
Copy link
Contributor Author

maiqueb commented Sep 24, 2025

@maiqueb And its only supported on dual stack? Or also single?

It should be on all networking configurations. We chose to deploy it only on dual-stack since that configuration is more flexible, and we get to check all types of UDNs in it (single stack IPv4 ; single stack IPv6 ; dual-stack).

At the end of the day, that is the "subject" under test.

@JoelSpeed
Copy link
Contributor

Could you please also add testing on the two single stack variants, so we have complete coverage?

@maiqueb
Copy link
Contributor Author

maiqueb commented Sep 24, 2025

Could you please also add testing on the two single stack variants, so we have complete coverage?

@tssurya FYI

@tssurya
Copy link
Contributor

tssurya commented Sep 24, 2025

Could you please also add testing on the two single stack variants, so we have complete coverage?

Our QE @asood-rh should be doing v4 and v6 singlestack and dualstack on their side as well - we were trying to have less duplication between devel and qe on platform support resources wise but adding this might also help QE in some sense. So I don't mind having it in our ocp CI (not just qe CI). Although our baremetal CI resources are always limited so for each networking feature we seem to be adding new lanes for each feature level which is going to blow up at some point:

Example:
for BGP we did:

  1. e2e-metal-ipi-ovn-dualstack-bgp-local-gw
  2. e2e-metal-ipi-ovn-dualstack-bgp

For this feature we did:

  1. e2e-metal-ipi-ovn-bgp-virt-dualstack-techpreview - which will GA
  2. ipv4 variant - todo based on request
  3. ipv6 variant - todo based on request

In the past for persistentIPs we did:

  1. e2e-aws-kubevirt-ovn-techpreview - https://github.com/openshift/release/pull/65092/files
  2. e2e-aws-ovn-virt-periodic - maybe this is same but periodic? - are these jobs still running @maiqueb ?

For L3 live migration @qinqon added a hypershift-kubevirt which always perma fails and not sure if there are periodics for this

So +1 for all stack coverage just want to highlight we might need to perm/combo choose a subset or consolidate all VM features into a single lane for cost-save rather than adding a virt lane per feature OR put all bgp features into a single lane

@qinqon
Copy link

qinqon commented Sep 24, 2025

For L3 live migration @qinqon added a hypershift-kubevirt which always perma fails and not sure if there are periodics for this

@tssurya at hypershift kubevirt we have already periodics running on azure that test L3 live migration.

https://prow.ci.openshift.org/view/gs/test-platform-results/logs/periodic-ci-openshift-hypershift-release-4.21-periodics-e2e-azure-kubevirt-ovn/1965988883430641664

@JoelSpeed
Copy link
Contributor

Our QE @asood-rh should be doing v4 and v6 singlestack and dualstack on their side as well - we were trying to have less duplication between devel and qe on platform support resources

Testing should default to on the public side of things now. There are various reasons we historically had private QE testing, but for new stuff, we should always aim for public testing reporting into sippy. Without data going into sippy we cannot manage regressions across the product.

Although our baremetal CI resources are always limited

Are there ways that the same clusters can be used to test these various features serially, so that we don't spin up a new suite for every feature?

@maiqueb
Copy link
Contributor Author

maiqueb commented Sep 24, 2025

@JoelSpeed so we need 4 new lanes right ? 2 (TP and GA) for single stack ipv4; same for ipv6

@everettraven
Copy link
Contributor

so we need 4 new lanes right ? 2 (TP and GA) for single stack ipv4; same for ipv6

@maiqueb That sounds correct.

@tssurya
Copy link
Contributor

tssurya commented Sep 29, 2025

Our QE @asood-rh should be doing v4 and v6 singlestack and dualstack on their side as well - we were trying to have less duplication between devel and qe on platform support resources

Testing should default to on the public side of things now. There are various reasons we historically had private QE testing, but for new stuff, we should always aim for public testing reporting into sippy. Without data going into sippy we cannot manage regressions across the product.

ack that's good to know! Is this also captured on our FG docs?

Although our baremetal CI resources are always limited

Are there ways that the same clusters can be used to test these various features serially, so that we don't spin up a new suite for every feature?

yes I believe its possible to consolidate instead of doing a new job per feature.

@maiqueb
Copy link
Contributor Author

maiqueb commented Sep 30, 2025

@JoelSpeed in hindsight, we've just realized this feature is not supposed to work for IPv6 single stack clusters.

It is clearly listed in the merged openshift enhancement: https://github.com/openshift/enhancements/pull/1793/files#diff-68747e5623b9c43b40b99e813c2acd3d642f58de610cfac6025699c66f062eeaR106

Thus, I think we should only add single stack IPv4 lanes. Would that be OK ?
Adding @kyrtapz to "back me up", and hoping he can add more motivation.

@kyrtapz
Copy link
Contributor

kyrtapz commented Sep 30, 2025

@JoelSpeed in hindsight, we've just realized this feature is not supposed to work for IPv6 single stack clusters.

It is clearly listed in the merged openshift enhancement: https://github.com/openshift/enhancements/pull/1793/files#diff-68747e5623b9c43b40b99e813c2acd3d642f58de610cfac6025699c66f062eeaR106

Thus, I think we should only add single stack IPv4 lanes. Would that be OK ?

+1
While having coverage for all possible setups would be ideal we don't want to consume too much of the available metal infra so having a dual-stack lane with BGP enabled us to test the the most scenarios. I am cool with adding a single-stack v4 as this would likely be the most popular deployment.

@everettraven
Copy link
Contributor

in hindsight, we've just realized this feature is not supposed to work for IPv6 single stack clusters.

It is clearly listed in the merged openshift enhancement: https://github.com/openshift/enhancements/pull/1793/files#diff-68747e5623b9c43b40b99e813c2acd3d642f58de610cfac6025699c66f062eeaR106

To clarify further - that means the usage of this feature is unsupported in that environment? How are we planning to make customers aware of that? If the feature is attempted to be used in an IPv6 single stack cluster, what happens?

@maiqueb
Copy link
Contributor Author

maiqueb commented Sep 30, 2025

in hindsight, we've just realized this feature is not supposed to work for IPv6 single stack clusters.
It is clearly listed in the merged openshift enhancement: https://github.com/openshift/enhancements/pull/1793/files#diff-68747e5623b9c43b40b99e813c2acd3d642f58de610cfac6025699c66f062eeaR106

To clarify further - that means the usage of this feature is unsupported in that environment? How are we planning to make customers aware of that? If the feature is attempted to be used in an IPv6 single stack cluster, what happens?

It won't work. The feature is initiated from MTV (which imports the VM from the vmware cluster), which is not ipv6 aware.

@everettraven
Copy link
Contributor

It won't work. The feature is initiated from MTV (which imports the VM from the vmware cluster), which is not ipv6 aware.

Sounds fine to me to drop that testing lane if it is known to not work and customers will be made aware of the fact the feature doesn't support the ipv6 single network stack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants