Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

NP-793: SDN live migration #1640

Merged
merged 7 commits into from Nov 28, 2023
Merged

NP-793: SDN live migration #1640

merged 7 commits into from Nov 28, 2023

Conversation

kyrtapz
Copy link
Contributor

@kyrtapz kyrtapz commented Oct 27, 2023 •
edited

Enhancement: openshift/enhancements#1064

Changes include:

  • Add new 'mode' flag to 'spec.Migration' in Networks.operator.openshift.io
  • Add status.conditions to networks.config.openshift.io
  • Restrict the newly added fields to TechPreview

This PR is based on #1546 and #1560 with minor changes.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 27, 2023

Hello @kyrtapz! 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 27, 2023
@kyrtapz kyrtapz marked this pull request as draft October 27, 2023 11:34
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 27, 2023
@kyrtapz
Copy link
Contributor Author

kyrtapz commented Oct 27, 2023

/test verify

@kyrtapz kyrtapz marked this pull request as ready for review October 27, 2023 14:22
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 27, 2023
@kyrtapz
Copy link
Contributor Author

kyrtapz commented Oct 27, 2023

I think that verify-crd-schema is failing on existing fields because I moved the files around to introduce the tech preview fields.
@JoelSpeed is there something more we need to do about that?

@kyrtapz
Copy link
Contributor Author

kyrtapz commented Oct 30, 2023

/cc @pliurh

@openshift-ci openshift-ci bot requested a review from pliurh October 30, 2023 11:37
@kyrtapz kyrtapz changed the title SDN live migration NP-793: SDN live migration Oct 31, 2023
@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Oct 31, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Oct 31, 2023 •
edited by openshift-ci bot

@kyrtapz: This pull request references NP-793 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.15.0" version, but no target version was set.

In response to this:

Enhancement: openshift/enhancements#1064

Changes include:

  • Add new 'mode' flag to 'spec.Migration' in Networks.operator.openshift.io
  • Add status.conditions to networks.config.openshift.io
  • Restrict the newly added fields to TechPreview

This PR is based on #1546 and #1560 with minor changes.

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.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2023
@kyrtapz
Copy link
Contributor Author

kyrtapz commented Nov 2, 2023 •
edited

/hold

Adding status.conditions to networks.config.openshift.io as techpreview is tricky because that CRD is also applied in the installer from a hard-coded path: https://github.com/openshift/installer/blob/master/data/data/manifests/openshift/cluster-networkconfig-crd.yaml.
IIUC this means the installer would have to be modified to selectively pick the CRD based on the feature-set specified in the install config.
An alternative would be to just add the new conditions field to the status without putting it behind a techpreview but I am not sure if this is acceptable. This is a generic field that isn't specific to any feature.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 2, 2023
pliurh
pliurh reviewed Nov 8, 2023
View reviewed changes
config/v1/types_network.go Outdated Show resolved Hide resolved
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 9, 2023
@deads2k
Copy link
Contributor

deads2k commented Nov 13, 2023

as techpreview is tricky because that CRD is also applied in the installer from a hard-coded path: https://github.com/openshift/installer/blob/master/data/data/manifests/openshift/cluster-networkconfig-crd.yaml.
IIUC this means the in

This should be fixed so the correct manifest is selected. This is commonly done during rendering by operators.

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2023
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Nov 24, 2023
pliurh and others added 6 commits November 24, 2023 17:16
@pliurh @kyrtapz
Add new flag to Networks.operator.openshift.io for SDN live migration
58504bb 
Add flag 'mode' to 'spec.Migration' to indicate SDN migration mode.
The supported values are Live, Offline. If unset, the default mode is
Offline.

Signed-off-by: Peng Liu <pliu@redhat.com>
@pliurh @kyrtapz
Add status.conditions to networks.config.openshift.io
458a0f1 
The conditions will be used to represents the status of SDN live
migration.

Signed-off-by: Peng Liu <pliu@redhat.com>
Signed-off-by: Patryk Diak <pdiak@redhat.com>
@pliurh @kyrtapz
Add TechPreview feature gate for SDN live migration
6ea40f8 
This commit adds a feature gate for SDN live Migration feature.

Signed-off-by: Peng Liu <pliu@redhat.com>
@deads2k @kyrtapz
stub files for techpreview CRD schema
fceb81a 
Signed-off-by: Patryk Diak <pdiak@redhat.com>
@deads2k @kyrtapz
restrict network migration mode to techpreview
9ef26be 
Signed-off-by: Patryk Diak <pdiak@redhat.com>
@deads2k @kyrtapz
add networking api tests
5203a1a 
Signed-off-by: Patryk Diak <pdiak@redhat.com>
@kyrtapz
Add validation for simultaneous networkType and mtu migration
9de6d28 
networkType and mtu migrations should not be performed
at the same time unless the migration mode is set to Live.

Signed-off-by: Patryk Diak <pdiak@redhat.com>
@pliurh
Copy link
Contributor

pliurh commented Nov 27, 2023

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 27, 2023
@kyrtapz
Copy link
Contributor Author

kyrtapz commented Nov 27, 2023

/test e2e-aws-serial-techpreview
/test e2e-aws-ovn-techpreview

@deads2k
Copy link
Contributor

deads2k commented Nov 27, 2023

missing test case so CI won't pass. But the gate and gated field look good.

/approve

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 27, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, kyrtapz, pliurh

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:

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 27, 2023
@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 7955d3d and 2 for PR HEAD 9de6d28 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD 6ceea94 and 1 for PR HEAD 9de6d28 in total

@openshift-ci-robot
Copy link

/retest-required

Remaining retests: 0 against base HEAD d83ab5b and 0 for PR HEAD 9de6d28 in total

@openshift-ci-robot
Copy link

/hold

Revision 9de6d28 was retested 3 times: holding

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 28, 2023
@kyrtapz
Copy link
Contributor Author

kyrtapz commented Nov 28, 2023

/test e2e-azure
/test e2e-gcp

@kyrtapz
Copy link
Contributor Author

kyrtapz commented Nov 28, 2023

verify-crd-schema is failing on existing fields because I moved the files around to introduce the tech preview fields.
The fields added in this PR are not flagged so I think we should override the job.

@JoelSpeed
Copy link
Contributor

/override ci/prow/verify-crd-schema

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 28, 2023

@JoelSpeed: Overrode contexts on behalf of JoelSpeed: 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/test-infra repository.

@JoelSpeed
Copy link
Contributor

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 28, 2023
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Nov 28, 2023

@kyrtapz: 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/test-infra repository. I understand the commands that are listed here.

@openshift-merge-bot openshift-merge-bot bot merged commit 436a23f into openshift:master Nov 28, 2023
16 checks passed
@kyrtapz kyrtapz mentioned this pull request Nov 28, 2023
Merged
@openshift-bot
Copy link

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-cluster-config-api-container-v4.15.0-202311282232.p0.g436a23f.assembly.stream for distgit ose-cluster-config-api.
All builds following this will include this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoelSpeed JoelSpeed JoelSpeed left review comments

@pliurh pliurh pliurh left review comments

@adambkaplan adambkaplan Awaiting requested review from adambkaplan

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

@mfojtik mfojtik Awaiting requested review from mfojtik

Assignees

@pliurh pliurh

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@kyrtapz @openshift-ci-robot @deads2k @pliurh @JoelSpeed @openshift-bot @openshift-merge-robot