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

✨Allow diskSetup to include partition layout #11634

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

miltalex
Copy link

@miltalex miltalex commented Jan 2, 2025

What this PR does / why we need it:

This PR enhances disk partitioning capabilities by introducing support for custom partition layouts through the new DiskLayout field. Previously, disk partitioning was limited to a boolean option that could only create a single partition for the entire device. This enhancement allows users to define multiple partitions with specific size percentages and partition types.
The changes align with cloud_init logic's disk setup specification, which supports both boolean and list-based partition layouts. The existing layout boolean field is maintained for backward compatibility, while the new diskLayout field provides advanced partitioning capabilities when needed.

Examples

Before (Single Partition):

partitions:
  - device: /dev/sda
    layout: true  # Creates a single partition using entire disk

After (Multiple Partitions):

partitions:
  - device: /dev/sda
    diskLayout:
      - percentage: 33
      - percentage: 33
        partitionType: 82
      - percentage: 33

Generated cloud-init output:

disk_setup:
  /dev/sda:
    layout:
      - 33
      - [33, 82]
      - 33

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #8524

Signed-off-by: Miltiadis Alexis <alexmiltiadis@gmail.com>
Copy link

linux-foundation-easycla bot commented Jan 2, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jan 2, 2025
@k8s-ci-robot k8s-ci-robot added the do-not-merge/needs-area PR is missing an area label label Jan 2, 2025
@k8s-ci-robot
Copy link
Contributor

This PR is currently missing an area label, which is used to identify the modified component when generating release notes.

Area labels can be added by org members by writing /area ${COMPONENT} in a comment

Please see the labels list for possible areas.

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.

@k8s-ci-robot
Copy link
Contributor

[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 justinsb 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

@k8s-ci-robot
Copy link
Contributor

Welcome @miltalex!

It looks like this is your first PR to kubernetes-sigs/cluster-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 2, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @miltalex. 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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jan 2, 2025
Signed-off-by: Miltiadis Alexis <alexmiltiadis@gmail.com>
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial thoughts on the API changes within this PR

// Percentage of disk that partition will take (1-100)
// +kubebuilder:validation:Minimum=1
// +kubebuilder:validation:Maximum=100
Percentage int32 `json:"percentage"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should include a required marker to show explicitly that this is required


// PartitionType is the numerical value of the partition type (optional)
// +optional
PartitionType *int32 `json:"partitionType,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the valid values for this value? And how would I, as an end user, understand those?

Is there a limit on this? Is 0 a valid value? Are negative values valid?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I can use a string here to reflect the correct linux partition types. It was an attempt to purely map the disk layout as described in the cloud init docs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a choice to make here. Either you can allow all of the valid values there, in which case this should be a 2 character lowercase hex value. Or, you can allow specific, common types, and do that by creating an Enum here and creating PascalCase aliases to the formats we actually want to support.

IMO, the latter provides a better UX

@@ -675,19 +675,42 @@ type DiskSetup struct {
Filesystems []Filesystem `json:"filesystems,omitempty"`
}

// DiskLayout represents an array of partition specifications
type DiskLayout []PartitionSpec
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would avoid a type alias to a slice, it makes it harder to understand the API when looking at the go types (it looks like a struct for example if I look at the Partition usage)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will convert it to a struct

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just meant you could inline this slice, rather than using an alias, I don't think it needs to be a struct does it?


// PartitionSpec defines the size and optional type for a partition
type PartitionSpec struct {
// Percentage of disk that partition will take (1-100)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Godoc must start with the serialised version of the field name

Suggested change
// Percentage of disk that partition will take (1-100)
// percentage of disk that partition will take (1-100)

// +kubebuilder:validation:Maximum=100
Percentage int32 `json:"percentage"`

// PartitionType is the numerical value of the partition type (optional)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// PartitionType is the numerical value of the partition type (optional)
// partitionType is the numerical value of the partition type (optional)

// layout specifies the device layout.
// If it is true, a single partition will be created for the entire device.
// When layout is false, it means don't partition or ignore existing partitioning.
Layout bool `json:"layout"`

// diskLayout specifies the percentage of disk space and partition types.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this list ordered?

Can you validate that the percentages sum to 100? Are they allowed to sum to less than 100?

Signed-off-by: Miltiadis Alexis <alexmiltiadis@gmail.com>
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/needs-area PR is missing an area label needs-ok-to-test Indicates a PR that requires an org member to verify it 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.

Allow diskSetup to include partition layout
3 participants