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

[release/v2.21] Anexia: allow extended disk configuration in AnexiaNodeSpec #10916

Merged

Conversation

LittleFox94
Copy link
Contributor

@LittleFox94 LittleFox94 commented Sep 2, 2022

What this PR does / why we need it:

This extends apiv1.AnexiaNodeSpec to allow extended disk configuration introduced in machine-controller with kubermatic/machine-controller#1402.

Without this, Kubermatic API returns a 500 when trying to do any MachineDeployment operation on a cluster which has a MachineDeployment with the new disks attribute (and thus without the old diskSize attribute).

Backport of #10816

What type of PR is this?
/kind bug
/kind feature
/kind api-change

It's a bug (500 error) because of a new feature, which causes and api-change

Special notes for your reviewer:

Comments on the machine-controller PRs:

Does this PR introduce a user-facing change? Then add your Release Note here:

Extend disk configuration for provider Anexia

Documentation:


This extends apiv1.AnexiaNodeSpec to allow extended disk configuration
introduced in machine-controller with pull request 1402
(kubermatic/machine-controller#1402).

Without this, Kubermatic API returns a 500 when trying to do any
MachineDeployment operation on a cluster which has a MachineDeployment
with the new disks attribute (and thus without the old diskSize
attribute).

Signed-off-by: Mara Sophie Grosch <mgrosch@anexia-it.com>
Signed-off-by: Mara Sophie Grosch <mgrosch@anexia-it.com>
Signed-off-by: Mara Sophie Grosch <mgrosch@anexia-it.com>
Setting the OperatingSystemSpec on the NodeSpec given into
machine.GetAnexiaProviderConfig() does not actually do anything with the
OS spec, instead, when creating the MachineDeployment, another one is
created - completely ignoring us having set the ProvisioningUtility to
CloudInit.

This fixes that problem by copying the relevant code from the
baseScenario.createMachineDeployment method and modifying the OS spec
given to the createMachineDeployment() function.

Signed-off-by: Mara Sophie Grosch <mgrosch@anexia-it.com>
OperatingSystemSpec directly contains the config attributes for the
given operating system, but was wrong encoded as
{"$operatingSystem": {$OperatingSystemSpec...}}

This fixes that issue by adding another switch-case over all known
operating systems. Should probably be done in a better way, but need
this done already..

Signed-off-by: Mara Sophie Grosch <mgrosch@anexia-it.com>
@kubermatic-bot kubermatic-bot added kind/bug Categorizes issue or PR as related to a bug. release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. kind/feature Categorizes issue or PR as related to a new feature. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API sig/api Denotes a PR or issue as being assigned to SIG API. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. 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. labels Sep 2, 2022
@kubermatic-bot
Copy link
Contributor

Hi @LittleFox94. Thanks for your PR.

I'm waiting for a kubermatic 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.

@LittleFox94 LittleFox94 changed the title [release/v2.21] allow extended disk configuration in AnexiaNodeSpec [release/v2.21] Anexia: allow extended disk configuration in AnexiaNodeSpec Sep 2, 2022
@ahmedwaleedmalik
Copy link
Member

/ok-to-test

@kubermatic-bot kubermatic-bot 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 Sep 2, 2022
@LittleFox94
Copy link
Contributor Author

/retest

@LittleFox94
Copy link
Contributor Author

/retry

@LittleFox94
Copy link
Contributor Author

/retest

... oops

@ahmedwaleedmalik
Copy link
Member

/retest

1 similar comment
@LittleFox94
Copy link
Contributor Author

/retest

@LittleFox94
Copy link
Contributor Author

If I read the failure correctly, it needs #10918 backported to release/v2.21 and this branch rebased on that backport

I could cherry-pick that commit into this branch here, but it's very unrelated changes and I'd like to avoid that - but it's your repo :)

@ahmedwaleedmalik
Copy link
Member

@LittleFox94 Ha seems like your PRs have been having a really bad time due to our vSphere infrastructure. I'm taking care of backporting #10918. I'll re-run the failing tests on your PRs once the backports are in.

@ahmedwaleedmalik ahmedwaleedmalik self-assigned this Sep 6, 2022
@ahmedwaleedmalik
Copy link
Member

/retest

Copy link
Member

@ahmedwaleedmalik ahmedwaleedmalik left a comment

Choose a reason for hiding this comment

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

/approve

@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2022
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 5cd08e5d3de66653deea981bcf735ed9f6e89f8a

@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahmedwaleedmalik, LittleFox94

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

@kubermatic-bot kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 6, 2022
@kubermatic-bot kubermatic-bot merged commit 4c83dab into kubermatic:release/v2.21 Sep 6, 2022
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. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API kind/bug Categorizes issue or PR as related to a bug. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/api Denotes a PR or issue as being assigned to SIG API. sig/cluster-management Denotes a PR or issue as being assigned to SIG Cluster Management. 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.

None yet

3 participants