api icon indicating copy to clipboard operation
api copied to clipboard

SPLAT-1808: Add vSphere multi disk support

Open vr4manta opened this issue 1 year ago • 18 comments

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 feature
  • MAO Changes: https://github.com/openshift/machine-api-operator/pull/1290

vr4manta avatar Sep 16 '24 13:09 vr4manta

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] avatar Sep 16 '24 13:09 openshift-ci[bot]

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.

openshift-ci[bot] avatar Sep 16 '24 13:09 openshift-ci[bot]

/test all

vr4manta avatar Sep 17 '24 13:09 vr4manta

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 avatar Sep 17 '24 20:09 kannon92

/retest

vr4manta avatar Sep 18 '24 11:09 vr4manta

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 avatar Sep 18 '24 11:09 vr4manta

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 avatar Sep 23 '24 14:09 vr4manta

/assign @JoelSpeed Ready for review

vr4manta avatar Sep 23 '24 15:09 vr4manta

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

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 feature

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-robot avatar Sep 23 '24 17:09 openshift-ci-robot

/lgtm

rvanderp3 avatar Sep 23 '24 19:09 rvanderp3

Is this feature currently supported in Cluster API?

JoelSpeed avatar Sep 24 '24 13:09 JoelSpeed

Is this feature currently supported in Cluster API?

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

vr4manta avatar Sep 24 '24 13:09 vr4manta

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

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 feature
  • MAO Changes: https://github.com/openshift/machine-api-operator/pull/1290

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-robot avatar Sep 24 '24 14:09 openshift-ci-robot

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?

JoelSpeed avatar Sep 24 '24 16:09 JoelSpeed

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 avatar Sep 24 '24 16:09 vr4manta

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

vr4manta avatar Oct 02 '24 12:10 vr4manta

@vr4manta How are you getting on with upstream WRT this feature?

JoelSpeed avatar Dec 13 '24 15:12 JoelSpeed

@vr4manta How are you getting on with upstream WRT this feature?

This is now merged. It merged during the holidays. I am updating any code related to this PR and will have ready shortly. Thanks!

vr4manta avatar Jan 06 '25 13:01 vr4manta

/unhold

vr4manta avatar Jan 06 '25 18:01 vr4manta

/lgtm

rvanderp3 avatar Jan 06 '25 18:01 rvanderp3

/retest

vr4manta avatar Jan 07 '25 12:01 vr4manta

@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 story to target the "4.19.0" version, but no target version was set.

In response to this:

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/cluster-control-plane-machine-set-operator/pull/335
  • 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 feature
  • MAO Changes: https://github.com/openshift/machine-api-operator/pull/1290

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-robot avatar Jan 07 '25 14:01 openshift-ci-robot

@vr4manta: 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-ci[bot] avatar Jan 08 '25 16:01 openshift-ci[bot]

/lgtm

JoelSpeed avatar Jan 08 '25 17:01 JoelSpeed

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed, rvanderp3, vr4manta

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[bot] avatar Jan 08 '25 17:01 openshift-ci[bot]

[ART PR BUILD NOTIFIER]

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

openshift-bot avatar Jan 08 '25 19:01 openshift-bot