Fix AzureMachinePool/AzureMachine spot instances max price failing to serialize when using the client #1157

axbarsan · 2021-02-02T14:30:45Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

Previously, when trying to deploy a AzureMachinePool (or AzureMachine) resource via the Go client, with spot instances enabled, and a set maximum price, you would get a validation error (please check the issue for more information).

This PR changes the type of the AzureMachinePool.SpotVMOptions.MaxPrice property to the apimachinery/pkg/resource *Quantity type.

By using this type we are able to not lose precision when serializing float values, and also able to deploy the resource by applying the CR template, or by using the Go client. You can read more about the type here.

If we're applying a CR template, we can either specify the maxPrice property by using a stringified float value ("0.1"), an integer (1), or by using resource.Quantity's serialization format (100m).

The only drawback is that if we try to specify a float value in the YAML template, we need to put the value in between quotes.

Which issue(s) this PR fixes:
Fixes #1151

Special notes for your reviewer:

Please let me know if there's a better way to achieve this. Thank you.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Refactor `AzureMachinePool.Spec.Template.SpotVMOptions.MaxPrice` and `AzureMachine.Spec.SpotVMOptions.MaxPrice` types to accept float values using the Go client. Action required: if upgrading a cluster to this version, and you're using spot instances with a set maximum price, you have to manually update the `azuremachinepools.spec.template.spotVMOptions.maxPrice` and `azuremachines.spec.spotVMOptions.maxPrice` fields. Wrapping the value in quotes will do the trick.

…otVMOptions.MaxPrice property By using a quantity type, which is a fixed-point representation of a number, we are able to not lose precision when serializing floats. Also, when applying CRs directly, we have the liberty to use stringified floats, as well as integers or the quantity serialization format.

k8s-ci-robot · 2021-02-02T14:30:48Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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. I understand the commands that are listed here.

k8s-ci-robot · 2021-02-02T14:30:54Z

Welcome @axbarsan!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-azure 🎉. 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-provider-azure 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 · 2021-02-02T14:30:54Z

Hi @axbarsan. 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.

axbarsan · 2021-02-02T16:24:26Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.

If you signed the CLA as a corporation, please sign in with your organization's credentials at identity.linuxfoundation.org/projects/cncf to be authorized.

If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: support.linuxfoundation.org

Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

I signed it!

CecileRobertMichon

/ok-to-test
/cc @JoelSpeed

axbarsan · 2021-02-02T17:07:18Z

apidiff:

sigs.k8s.io/cluster-api-provider-azure/api/v1alpha3
  Incompatible changes:
  - SpotVMOptions.MaxPrice: changed from *string to *k8s.io/apimachinery/pkg/api/resource.Quantity

axbarsan · 2021-02-02T17:11:46Z

/retest

axbarsan · 2021-02-02T17:15:08Z

^ Not sure what to do at this point 😅

Is this fine?

CecileRobertMichon · 2021-02-02T17:35:02Z

@axbarsan yes, the apidiff test is non-blocking. It helps reviewers catch breaking changes. In this case it's a change we want. I wonder if we should mark it as breaking in the release notes however. What is the user facing impact and does it break existing resources?

CecileRobertMichon · 2021-02-02T18:03:00Z

IPV6 LB flake

/retest

axbarsan · 2021-02-02T18:04:10Z

@axbarsan yes, the apidiff test is non-blocking. It helps reviewers catch breaking changes. In this case it's a change we want. I wonder if we should mark it as breaking in the release notes however. What is the user facing impact and does it break existing resources?

This is 100% a breaking change. I marked it in the notes.
Existing resources will fail to reconcile if the field is not updated manually.

CecileRobertMichon · 2021-02-02T18:05:00Z

@axbarsan this PR also changes the AzureMachine SpotVMOptions, let's include that in the commit message/PR title description/action required so it's clear that this change does not only affect machine pools

axbarsan · 2021-02-02T18:09:35Z

@axbarsan this PR also changes the AzureMachine SpotVMOptions, let's include that in the commit message/PR title description/action required so it's clear that this change does not only affect machine pools

I updated it, hope everything's ok 👍🏻

CecileRobertMichon · 2021-02-02T18:37:35Z

@devigned @nader-ziada should we accept this breaking change in v1alpha3 and backport it to 0.4 since the feature it is "breaking" is already broken (and not many users must be using it, if any), or should we stick to protocol and move the change to v1alpha4 (possibly with conversion)?

nader-ziada · 2021-02-02T18:47:07Z

@devigned @nader-ziada should we accept this breaking change in v1alpha3 and backport it to 0.4 since the feature it is "breaking" is already broken (and not many users must be using it, if any), or should we stick to protocol and move the change to v1alpha4 (possibly with conversion)?

I think since it broken we should accept it in v1alpha3

CecileRobertMichon · 2021-02-02T18:53:23Z

/retest

k8s-ci-robot · 2021-02-02T18:54:44Z

@axbarsan: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-provider-azure-apidiff	`3985d63`	link	`/test pull-cluster-api-provider-azure-apidiff`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

CecileRobertMichon · 2021-02-02T20:05:53Z

/lgtm
/assign @devigned

devigned

/lgtm
/approve

Thank you!

k8s-ci-robot · 2021-02-02T20:24:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: devigned

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:

~~OWNERS~~ [devigned]

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

axbarsan · 2021-02-03T07:47:32Z

Thank you for taking care of this so quickly! 🚀

CecileRobertMichon · 2021-02-03T17:53:49Z

@axbarsan thanks for the PR! We will backport to 0.4 once the other bug fix (#356) merges.

JoelSpeed · 2021-02-04T12:29:46Z

config/crd/bases/exp.infrastructure.cluster.x-k8s.io_azuremachinepools.yaml

                        description: MaxPrice defines the maximum price the user is
                          willing to pay for Spot VM instances
-                        type: number
+                        pattern: ^(\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))(([KMGTPE]i)|[numkMGTPE]|([eE](\+|-)?(([0-9]+(\.[0-9]*)?)|(\.[0-9]+))))?$


Sorry I'm late to the party, but, I wonder if there's any way we can change this so that it only accepts numbers and doesn't allow the textual suffixes?

@axbarsan any reason you chose to allow textual suffixes?

The validation was generated automatically after I changed the type to *resource.Quantity. I believe the reason for alowing textual suffixes is to also allow writing numbers in the "quantity" way, for not losing precision (e.g. 1.5 => 1500m)

More info here:
https://github.com/kubernetes/apimachinery/blob/c93b0f84892eb6bcbc80b312ae70729d8168bc7e/pkg/api/resource/quantity.go#L69

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Feb 2, 2021

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 2, 2021

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/provider/azure Issues or PRs related to azure provider labels Feb 2, 2021

k8s-ci-robot requested review from awesomenix and nader-ziada February 2, 2021 14:31

k8s-ci-robot added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 2, 2021

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 2, 2021

CecileRobertMichon reviewed Feb 2, 2021

View reviewed changes

k8s-ci-robot requested a review from JoelSpeed February 2, 2021 16:56

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 Feb 2, 2021

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Feb 2, 2021

k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Feb 2, 2021

axbarsan changed the title ~~Fix AzureMachinePool.SpotVMOptions.MaxPrice failing to serialize when using the client~~ Fix AzureMachinePool/AzureMachine .SpotVMOptions.MaxPrice failing to serialize when using the client Feb 2, 2021

axbarsan changed the title ~~Fix AzureMachinePool/AzureMachine .SpotVMOptions.MaxPrice failing to serialize when using the client~~ Fix AzureMachinePool/AzureMachine spot instances max price failing to serialize when using the client Feb 2, 2021

k8s-ci-robot assigned devigned and CecileRobertMichon Feb 2, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2021

devigned approved these changes Feb 2, 2021

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2021

k8s-ci-robot merged commit 8df2692 into kubernetes-sigs:master Feb 2, 2021

k8s-ci-robot added this to the v0.5.0 milestone Feb 2, 2021

axbarsan mentioned this pull request Feb 3, 2021

Backport spot instances fix from upstream giantswarm/cluster-api-provider-azure#8

Merged

JoelSpeed reviewed Feb 4, 2021

View reviewed changes

CecileRobertMichon modified the milestones: v0.5.0, v0.4.12 Feb 5, 2021

CecileRobertMichon mentioned this pull request Feb 8, 2021

Use apimachinery 'resource.Quantity' type for the AzureMachinePool.SpotVMOptions.MaxPrice property #1167

Merged

3 tasks

alexander-demicev mentioned this pull request Feb 16, 2021

[OCPCLOUD-1108] Fix spot instances price type openshift/cluster-api-provider-azure#200

Merged

dependabot bot mentioned this pull request Mar 15, 2021

Bump sigs.k8s.io/cluster-api-provider-azure from 0.4.11 to 0.4.13 giantswarm/azure-operator#1397

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix AzureMachinePool/AzureMachine spot instances max price failing to serialize when using the client #1157

Fix AzureMachinePool/AzureMachine spot instances max price failing to serialize when using the client #1157

axbarsan commented Feb 2, 2021 •

edited

Loading

k8s-ci-robot commented Feb 2, 2021

k8s-ci-robot commented Feb 2, 2021

k8s-ci-robot commented Feb 2, 2021

axbarsan commented Feb 2, 2021 •

edited

Loading

CecileRobertMichon left a comment

axbarsan commented Feb 2, 2021 •

edited

Loading

axbarsan commented Feb 2, 2021

axbarsan commented Feb 2, 2021

CecileRobertMichon commented Feb 2, 2021

CecileRobertMichon commented Feb 2, 2021

axbarsan commented Feb 2, 2021

CecileRobertMichon commented Feb 2, 2021

axbarsan commented Feb 2, 2021

CecileRobertMichon commented Feb 2, 2021

nader-ziada commented Feb 2, 2021

CecileRobertMichon commented Feb 2, 2021

k8s-ci-robot commented Feb 2, 2021 •

edited

Loading

CecileRobertMichon commented Feb 2, 2021

devigned left a comment

k8s-ci-robot commented Feb 2, 2021

axbarsan commented Feb 3, 2021

CecileRobertMichon commented Feb 3, 2021

JoelSpeed Feb 4, 2021

CecileRobertMichon Feb 5, 2021

axbarsan Feb 8, 2021

Fix AzureMachinePool/AzureMachine spot instances max price failing to serialize when using the client #1157

Fix AzureMachinePool/AzureMachine spot instances max price failing to serialize when using the client #1157

Conversation

axbarsan commented Feb 2, 2021 • edited Loading

k8s-ci-robot commented Feb 2, 2021

k8s-ci-robot commented Feb 2, 2021

k8s-ci-robot commented Feb 2, 2021

axbarsan commented Feb 2, 2021 • edited Loading

CecileRobertMichon left a comment

Choose a reason for hiding this comment

axbarsan commented Feb 2, 2021 • edited Loading

axbarsan commented Feb 2, 2021

axbarsan commented Feb 2, 2021

CecileRobertMichon commented Feb 2, 2021

CecileRobertMichon commented Feb 2, 2021

axbarsan commented Feb 2, 2021

CecileRobertMichon commented Feb 2, 2021

axbarsan commented Feb 2, 2021

CecileRobertMichon commented Feb 2, 2021

nader-ziada commented Feb 2, 2021

CecileRobertMichon commented Feb 2, 2021

k8s-ci-robot commented Feb 2, 2021 • edited Loading

CecileRobertMichon commented Feb 2, 2021

devigned left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 2, 2021

axbarsan commented Feb 3, 2021

CecileRobertMichon commented Feb 3, 2021

JoelSpeed Feb 4, 2021

Choose a reason for hiding this comment

CecileRobertMichon Feb 5, 2021

Choose a reason for hiding this comment

axbarsan Feb 8, 2021

Choose a reason for hiding this comment

axbarsan commented Feb 2, 2021 •

edited

Loading

axbarsan commented Feb 2, 2021 •

edited

Loading

axbarsan commented Feb 2, 2021 •

edited

Loading

k8s-ci-robot commented Feb 2, 2021 •

edited

Loading