Skip to content

Conversation

sivchari
Copy link
Member

@sivchari sivchari commented Oct 7, 2025

What this PR does / why we need it:

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 #

Part of #12720

/area clusterclass

@k8s-ci-robot k8s-ci-robot added area/clusterclass Issues or PRs related to clusterclass cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Oct 7, 2025
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 7, 2025
@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 enxebre for approval. For more information see the 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

@sivchari sivchari force-pushed the add-generate-upgrade-plan-types-and-hooks branch from 637a583 to 06c7d6f Compare October 7, 2025 06:44
Comment on lines +34 to +36
// cluster is the cluster ofject the lifecycle hook correspods to.
// +required
Cluster clusterv1beta1.Cluster `json:"cluster,omitempty,omitzero"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// cluster is the cluster ofject the lifecycle hook correspods to.
// +required
Cluster clusterv1beta1.Cluster `json:"cluster,omitempty,omitzero"`
// cluster is the cluster object the GenerateUpgradePlan request corresponds to.
// +required
Cluster clusterv1.Cluster `json:"cluster,omitempty,omitzero"`

Let's go with v1beta2. I know the proposal says v1beta1, but we already decided for the in-place implementation to just directly go with v1beta2 to avoid conversion issues. I would do the same here

Copy link
Member

Choose a reason for hiding this comment

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

+1 on using v1beta2 (sorry I forgot to update the comment with implementation details for this PR 😅)


// fromControlPlaneKubernetesVersion is the current Kubernetes version of the control plane.
// +required
// +kubebuilder:validation:MinLength=1
Copy link
Member

Choose a reason for hiding this comment

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

Not sure about all the kubebuilder:validation: + listType markers. None of these are used anywhere as these are not CRDs so nobody implements the corresponding validation.
I would probably remove them (but let's definitely keep optional + required, this seems like useful info in general)

@fabriziopandini WDYT?

Copy link
Member

@fabriziopandini fabriziopandini Oct 7, 2025

Choose a reason for hiding this comment

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

For the request I think it is ok to have both +required and MinLength, because CAPI will always send a value for the fields that are currently marked as required (and the field that is marked optional will have lenght > 1 when set)

For the response I think it makes sense to keep markers only what we are going enforcing when we are validating the response in

func DefaultAndValidateUpgradePlans(desiredVersion string, controlPlaneVersion string, minWorkersVersion string, controlPlaneUpgradePlan []string, workersUpgradePlan []string) ([]string, error) {
.
Accordingly, let's drop both listType and Min/MaxItems for both arrays.

Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!

Comment on lines +64 to +65
// controlPlaneUpgrades is the list of version upgrade steps for the control plane.
// Each entry represents an intermediate version that must be applied in sequence.
Copy link
Member

Choose a reason for hiding this comment

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

Let's add some more details on the rules that applies to the upgrade plan

Suggested change
// controlPlaneUpgrades is the list of version upgrade steps for the control plane.
// Each entry represents an intermediate version that must be applied in sequence.
// controlPlaneUpgrades is the list of version upgrade steps for the control plane.
// Each entry represents an intermediate version that must be applied in sequence.
// The following rules apply:
// - there should be at least one version for every minor between fromControlPlaneKubernetesVersion (excluded) and ToKubernetesVersion (included).
// - each version must be:
// - greater than fromControlPlaneKubernetesVersion (or with a different build number)
// - greater than the previous version in the list (or with a different build number)
// - less or equal to ToKubernetesVersion (or with a different build number)
// - the last version in the plan must be equal to ToKubernetesVersion

Comment on lines +72 to +73
// workersUpgrades is the list of version upgrade steps for the workers.
// Each entry represents an intermediate version that must be applied in sequence.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// workersUpgrades is the list of version upgrade steps for the workers.
// Each entry represents an intermediate version that must be applied in sequence.
// workersUpgrades is the list of version upgrade steps for the workers.
// Each entry represents an intermediate version that must be applied in sequence.
//
// In case the upgrade plan for workers will be left to empty, the system will automatically
// determine the minimal number of workers upgrade steps, thus minimizing impact on workloads and reducing
// the overall upgrade time.
//
// If instead for any reason a custom upgrade path for workers is required, the following rules apply:
// - each version must be:
// - equal to FromControlPlaneKubernetesVersion or to one of the versions in the control plane upgrade plan.
// - greater than FromWorkersKubernetesVersion (or with a different build number)
// - greater than the previous version in the list (or with a different build number)
// - less or equal to the ToKubernetesVersion (or with a different build number)
// - in case of versions with the same major/minor/patch version but different build number, also the order
// of those versions must be the same for control plane and worker upgrade plan.
// - the last version in the plan must be equal to ToKubernetesVersion
// - the upgrade plane must have all the intermediate version which workers must go through to avoid breaking rules
// defining the max version skew between control plane and workers.

// +kubebuilder:validation:MinLength=1
FromControlPlaneKubernetesVersion string `json:"fromControlPlaneKubernetesVersion,omitempty"`

// fromWorkersKubernetesVersion is the current Kubernetes version of the workers.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// fromWorkersKubernetesVersion is the current Kubernetes version of the workers.
// fromWorkersKubernetesVersion is the min Kubernetes version of the workers (MachineDeployments and MachinePools).

"v1.31.0, and v1.32.0, the hook should return these intermediate versions in the response.\n" +
"\n" +
"Notes:\n" +
"- The response may include separate upgrade paths for control plane and workers\n" +
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"- The response may include separate upgrade paths for control plane and workers\n" +
"- The response may include separate upgrade paths for control plane and workers\n" +
"- The upgrade plan for workers is optional; if missing the system will automatically\n\"" +
" determine the minimal number of workers upgrade steps according to Kubernetes version skew rules.\n" +

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/clusterclass Issues or PRs related to clusterclass cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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.

4 participants