search

openshift / api

Canonical location of the OpenShift API definition.
http://www.openshift.org
Apache License 2.0
94 stars 517 forks source link

SPLAT-1808: Add vSphere multi disk support #2028

Open vr4manta opened 2 months ago

vr4manta commented 2 months ago

SPLAT-1808

Changes

Blocks

Notes

openshift-ci[bot] commented 2 months ago

Skipping CI for Draft Pull Request. If you want CI signal for your change, please convert it to an actual PR. You can still manually trigger a test run with /test all

openshift-ci[bot] commented 2 months ago

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

vr4manta commented 2 months ago

/test all

kannon92 commented 2 months ago

Do you have a Jira ticket for this work? I'm trying to research disk support in openshift and this is of interest to me.

vr4manta commented 2 months ago

/retest

vr4manta commented 2 months ago

Do you have a Jira ticket for this work? I'm trying to research disk support in openshift and this is of interest to me.

@kannon92 , We are investigating this as well. I am using our spike to track the work and am getting stories / epic information for you. I'll attach the JIRAs to the PR above as well.

Spike: https://issues.redhat.com/browse/SPLAT-1789

vr4manta commented 1 month ago

overall lgtm, i left a couple of comments. also, will there be a need for additional unit tests?

Unit tests are in the webhook for MAO. The CEL is being ignored based on current behavior of provider specs.

vr4manta commented 1 month ago

/assign @JoelSpeed Ready for review

openshift-ci-robot commented 1 month ago

@vr4manta: This pull request references SPLAT-1808 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 task to target the "4.18.0" version, but no target version was set.

In response to [this](https://github.com/openshift/api/pull/2028): >[SPLAT-1808](https://issues.redhat.com/browse/SPLAT-1808) > >### Changes >- Create new feature gate for vSphere multi disk >- Create initial CRD changes for adding additional disks to vSphere machines > >### Notes >- This is being done as part of vSphere work for [this](https://issues.redhat.com/browse/OCPSTRAT-1592) feature Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fapi). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
rvanderp3 commented 1 month ago

/lgtm

openshift-ci[bot] commented 1 month ago

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rvanderp3, vr4manta Once this PR has been reviewed and has the lgtm label, please ask for approval from joelspeed. For more information see the Kubernetes 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: - **[OWNERS](https://github.com/openshift/api/blob/master/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
JoelSpeed commented 1 month ago

Is this feature currently supported in Cluster API?

vr4manta commented 1 month ago

Is this feature currently supported in Cluster API?

No. CAPI only supports additional disks that are created in the template.

openshift-ci-robot commented 1 month ago

@vr4manta: This pull request references SPLAT-1808 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 task to target the "4.18.0" version, but no target version was set.

In response to [this](https://github.com/openshift/api/pull/2028): >[SPLAT-1808](https://issues.redhat.com/browse/SPLAT-1808) > >### Changes >- Create new feature gate for vSphere multi disk >- Create initial CRD changes for adding additional disks to vSphere machines > >### Blocks >- https://github.com/openshift/machine-api-operator/pull/1290 >- https://github.com/openshift/installer/pull/9035 >- https://github.com/openshift/library-go/pull/1797 > >### Notes >- This is being done as part of vSphere work for [this](https://issues.redhat.com/browse/OCPSTRAT-1592) feature >- MAO Changes: https://github.com/openshift/machine-api-operator/pull/1290 Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fapi). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci[bot] commented 1 month ago

New changes are detected. LGTM label has been removed.

JoelSpeed commented 1 month ago

No. CAPI only supports additional disks that are created in the template.

So, if we merge this feature, this will create a feature gap between MAPV and CAPV? How would you suggest we migrate users using this feature?

We likely need to implement this upstream in CAPV before we can do any migration right?

vr4manta commented 1 month ago

No. CAPI only supports additional disks that are created in the template.

So, if we merge this feature, this will create a feature gap between MAPV and CAPV? How would you suggest we migrate users using this feature?

We likely need to implement this upstream in CAPV before we can do any migration right?

Yes. We will need to work with CAPV to see what they have thought about in the past for dynamic disk configs for VMs. The installer enhancement for this feature currently uses the postProvision hook after CAPI machines (control plane) are created and adds the disks to the VMs. I have been trying to follow what some of the other providers have for naming conventions, but each platform seems to use slightly different field names. For vSphere specific, it would be good to see if we can use the current additionalDisks field to add them if they do not exist in the template.

vr4manta commented 1 month ago

/hold Looking into upstream enhancement (CAPV) vs having a a drift in potential configs.

openshift-ci[bot] commented 1 week ago

@vr4manta: 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/e2e-aws-serial-techpreview e09483b43320f2097c37ff52bf1392cfa9bacafc link true /test e2e-aws-serial-techpreview
ci/prow/e2e-aws-serial e09483b43320f2097c37ff52bf1392cfa9bacafc link true /test e2e-aws-serial

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).