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.

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

✨ Add EphemeralDisk to InstanceSpec #1640

Closed
wants to merge 1 commit into from

Conversation

EmilienM
Copy link
Contributor

@EmilienM EmilienM commented Aug 9, 2023

What this PR does / why we need it:

An additional block device can be attached to the instance. It won't be bootable and only optional.
It can be added aside of a RootVolume or not.

apiVersion: infrastructure.cluster.x-k8s.io/v1alpha7
kind: OpenStackMachineTemplate
metadata:
  name: <cluster-name>-controlplane
  namespace: <cluster-name>
spec:
...
  ephemeralDisk:
     format: "ext4"
     size: 10
...

The local disk will be deleted when the instance is removed.

A use case is when having etcd running on machines that boot from volumes, you can now have a dedicated local disk with higher I/O for it.

@netlify
Copy link

netlify bot commented Aug 9, 2023

Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

Name Link
🔨 Latest commit bbed0ad
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/64e791e2dd7ac8000884ed89
😎 Deploy Preview https://deploy-preview-1640--kubernetes-sigs-cluster-api-openstack.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 9, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @EmilienM. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: EmilienM
Once this PR has been reviewed and has the lgtm label, please assign fabriziopandini for approval. 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:

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

@EmilienM EmilienM changed the title Add EphemeralDisk to OpenStackMachineSpec ✨ Add EphemeralDisk to OpenStackMachineSpec Aug 9, 2023
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 9, 2023
@EmilienM
Copy link
Contributor Author

EmilienM commented Aug 9, 2023

This is just a prototype for now. Also for some reasons I can't make the conversion working yet.

@EmilienM EmilienM changed the title ✨ Add EphemeralDisk to OpenStackMachineSpec ✨ Add EphemeralDisk to OpenStackMachineSpec Aug 9, 2023
@pierreprinetti
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 11, 2023
@EmilienM EmilienM force-pushed the nmstate branch 7 times, most recently from d4e1f62 to 665ef6b Compare August 12, 2023 03:23
@EmilienM
Copy link
Contributor Author

/uncc seanschneeweiss jichenjc

@EmilienM EmilienM changed the title ✨ Add EphemeralDisk to OpenStackMachineSpec ✨ Add EphemeralDisk to Instance Aug 12, 2023
@EmilienM
Copy link
Contributor Author

/cc pierreprinetti mdbooth dulek

@EmilienM
Copy link
Contributor Author

struggling a bit with the conversion & their tests...

@EmilienM EmilienM changed the title ✨ Add EphemeralDisk to Instance ✨ Add EphemeralDisk to InstanceSpec Aug 12, 2023
@ching-kuo
Copy link
Contributor

You might need to restore the value during up conversion like this PR #1517

@jichenjc
Copy link
Contributor

for new spec I suggest create an issue to track the proposed change and create release notes for it... @EmilienM

@mdbooth
Copy link
Contributor

mdbooth commented Aug 16, 2023

Couple of things to think about:

There's prior art here: https://github.com/kubernetes-sigs/cluster-api-provider-azure/blob/e7773fe017f3493be65c3368808d726a6343911e/api/v1beta1/azuremachine_types.go#L77-L82

It might be worth considering using the same terminology unless there's a good reason to do otherwise.

Also consider: how do I specify that I want a secondary disk and it's the nova local disk, vs it's a cinder disk? I suppose it might be neater to give the local disk a separate field in the API because there can only be one of them, vs any number of cinder disks.

I believe the CAPZ folks have done the whole thing end-to-end, i.e. they're also able to mount the disk. Might be worth looking into how they did that.

@mdbooth
Copy link
Contributor

mdbooth commented Aug 17, 2023

It occurs to me that to use a local disk as a data disk it could just be a flag. I'm not sure we need to specify the size of the local disk. We can just take whatever we're given by the flavor. It is possible to specify a smaller disk than the one the flavor gave you, but:

  • We would probably be the only people using this feature, so we'd be the ones finding all the bugs1
  • IIRC from an accounting POV Nova still assumes you used the whole disk, so you might as well use it

If we added to machine spec something like LocalDiskIsData (or something better: that's a terrible name), then:

  • It would be an error if there was no root volume
  • We'd construct an explicit BDM2 with the local disk second
  • Later we could add a separate field for DataDisks provided by Cinder, which could be added to the BDM after the local disk.

Footnotes

  1. I used to be a maintainer of this code

  2. Block Device Mapping

@EmilienM
Copy link
Contributor Author

EmilienM commented Aug 22, 2023

@mdbooth thanks, I'm looking into this
@jichenjc I'll do it, once we settle on the API needs, based on Matt's comments. I'll come up with something and create an issue.

An additional block device can be attached to the instance. It won't be bootable and only optional.
It can be added aside of a `RootVolume` or not.

```yaml
apiVersion: infrastructure.cluster.x-k8s.io/v1alpha7
kind: OpenStackMachineTemplate
metadata:
  name: <cluster-name>-controlplane
  namespace: <cluster-name>
spec:
...
  ephemeralDisk:
     format: "ext4"
     size: 10
...
```

The local disk will be deleted when the instance is removed.
@EmilienM
Copy link
Contributor Author

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 12, 2023
@k8s-ci-robot
Copy link
Contributor

PR needs rebase.

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.

@EmilienM EmilienM closed this Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants