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

✨ [e2e framework] Allow cluster upgrade spec to work without MachinePools #5092

Merged
merged 1 commit into from
Aug 16, 2021

Conversation

detiber
Copy link
Member

@detiber detiber commented Aug 13, 2021

What this PR does / why we need it:

Enables providers to use ClusterUpgradeConformanceSpec even if they do not implement MachinePools.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 13, 2021
var clusterTemplateWorkerMachineCount int64 = 2
var workerMachineCount = 2 * clusterTemplateWorkerMachineCount
Copy link
Member Author

Choose a reason for hiding this comment

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

I was working on updating cluster-api-provider-packet to v1alpha4 and adopting the upstream test specs. The only thing preventing me from using ClusterUpgradeConformanceSpec is that it expects that a provider implements MachinePools, since we do not the spec always fails because the machine count does not match.

// also adjust the expected workerMachineCount if we have MachinePools
var workerMachineCount = clusterTemplateWorkerMachineCount
if len(clusterResources.MachinePools) > 0 {
workerMachineCount = 2 * workerMachineCount
Copy link
Member

@sbueringer sbueringer Aug 16, 2021

Choose a reason for hiding this comment

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

Suggested change
workerMachineCount = 2 * workerMachineCount
workerMachineCount += int64(len(clusterResources.MachinePools)) * workerMachineCount

This way it would also work with >1 MachinePools, what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a couple of helper methods to the ApplyClusterTemplateAndWaitResult type to support both multiple MachinePools and multiple MachineDeployments. I expected there to be additional uses of this in other tests, but it looks like this was the only test that did this type of computation, so the updated refactor might be a bit overkill 😂

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.

/lgtm
pending @sbueringer comment

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 16, 2021
@detiber detiber force-pushed the optionalMachinePool branch from da01689 to 7331b2f Compare August 16, 2021 15:08
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 16, 2021
@fabriziopandini
Copy link
Member

/lgtm
Thanks for cleaning up the logic about expected total nodes, It is much more cleaner now

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 16, 2021
@sbueringer
Copy link
Member

Very nice, thank you :)

/lgtm

@fabriziopandini
Copy link
Member

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 16, 2021
@k8s-ci-robot k8s-ci-robot merged commit dffe1ce into kubernetes-sigs:master Aug 16, 2021
@k8s-ci-robot k8s-ci-robot added this to the v0.4 milestone Aug 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants