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

Anexia Provider: Utilize Creating state instead of blocking Create call #1483

Merged
merged 1 commit into from
Nov 21, 2022

Conversation

marioreggiori
Copy link
Contributor

@marioreggiori marioreggiori commented Nov 15, 2022

What this PR does / why we need it:

The machine-controller calls the providers Create method sequentially and only starts with the provisioning of another instance after Create returned without any errors. This PR changes the

  • Create method to not block until the VM provisioning has completed
  • Get method to check via the instance status whether InstanceID and ProvisioningID are set

and utilizes the Creating state until the VM is ready.

This reduces the provisioning duration of node pools with more than one replica significantly as there are now only a couple of seconds (~10s) between the provisioning starts.

Special notes for your reviewer:

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

Anexia: Refactor code to reduce provisioning time for node pools with multiple replicas.

Documentation:

NONE

@kubermatic-bot kubermatic-bot added dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. do-not-merge/docs-needed Indicates that a PR should not merge because it's missing one of the documentation labels. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. sig/osm Denotes a PR or issue as being assigned to SIG OSM. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 15, 2022
@kubermatic-bot
Copy link
Contributor

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

@kubermatic-bot kubermatic-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. docs/none Denotes a PR that doesn't need documentation (changes). release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/docs-needed Indicates that a PR should not merge because it's missing one of the documentation labels. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Nov 15, 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 Nov 15, 2022
@ahmedwaleedmalik
Copy link
Member

Hey @marioreggiori, can you please fix the liniting errors from pull-machine-controller-lint?

level=info msg="[runner] linters took 36.340750268s with stages: goanalysis_metalinter: 36.290770629s, structcheck: 16.344µs, wastedassign: 12.051µs"
pkg/cloudprovider/provider/anexia/helper_test.go:67:6: `createSearchHandler` is unused (deadcode)
func createSearchHandler(t *testing.T, iterations int) http.HandlerFunc {

You can ignore the rest of the CI jobs.

@ahmedwaleedmalik
Copy link
Member

/test pull-machine-controller-e2e-anexia

…` call

Signed-off-by: Mario Schäfer <mschaefer@anexia-it.com>
@kubermatic-bot kubermatic-bot 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 Nov 16, 2022
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
/retest

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

LGTM label has been added.

Git tree hash: 5cd4eebda8f307872a586b34c640eeeb6d7b55dc

@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Nov 16, 2022
@kubermatic-triage-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs

Review the full test history

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

1 similar comment
@kubermatic-triage-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs

Review the full test history

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@ahmedwaleedmalik
Copy link
Member

/hold to avoid retries.
@xrstf can you please override pull-machine-controller-e2e-vsphere

@kubermatic-bot kubermatic-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 18, 2022
@xrstf
Copy link
Contributor

xrstf commented Nov 21, 2022

/override pull-machine-controller-e2e-vsphere
/unhold

@kubermatic-bot kubermatic-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 21, 2022
@kubermatic-bot
Copy link
Contributor

@xrstf: Overrode contexts on behalf of xrstf: pull-machine-controller-e2e-vsphere

In response to this:

/override pull-machine-controller-e2e-vsphere
/unhold

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.

@kubermatic-bot kubermatic-bot merged commit cdf5b1a into kubermatic:main Nov 21, 2022
@LittleFox94
Copy link
Contributor

/cherry-pick release/v1.54

@kubermatic-bot
Copy link
Contributor

@LittleFox94: only kubermatic org members may request cherry picks. You can still do the cherry-pick manually.

In response to this:

/cherry-pick release/v1.54

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
Copy link
Contributor

ok, was worth a try ^^

LittleFox94 pushed a commit to anexia-it/machine-controller that referenced this pull request Dec 7, 2022
…` call (kubermatic#1483)

Signed-off-by: Mario Schäfer <mschaefer@anexia-it.com>

Signed-off-by: Mario Schäfer <mschaefer@anexia-it.com>
@ahmedwaleedmalik
Copy link
Member

/cherry-pick release/v1.55

@kubermatic-bot
Copy link
Contributor

@ahmedwaleedmalik: cannot checkout release/v1.55: error checking out release/v1.55: exit status 1. output: error: pathspec 'release/v1.55' did not match any file(s) known to git

In response to this:

/cherry-pick release/v1.55

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.

@ahmedwaleedmalik
Copy link
Member

/cherry-pick release/v1.55

@kubermatic-bot
Copy link
Contributor

@ahmedwaleedmalik: new pull request created: #1510

In response to this:

/cherry-pick release/v1.55

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.

ahmedwaleedmalik pushed a commit that referenced this pull request Dec 7, 2022
…` call (#1483) (#1509)

Signed-off-by: Mario Schäfer <mschaefer@anexia-it.com>

Signed-off-by: Mario Schäfer <mschaefer@anexia-it.com>

Signed-off-by: Mario Schäfer <mschaefer@anexia-it.com>
Co-authored-by: Mario Reggiori <git@marioreggiori.com>
@ahmedwaleedmalik
Copy link
Member

/cherry-pick release/v1.45

@kubermatic-bot
Copy link
Contributor

@ahmedwaleedmalik: #1483 failed to apply on top of branch "release/v1.45":

Applying: Anexia Provider: Utilize `Creating` state instead of blocking `Create` call
Using index info to reconstruct a base tree...
M	pkg/cloudprovider/provider/anexia/helper_test.go
M	pkg/cloudprovider/provider/anexia/instance.go
M	pkg/cloudprovider/provider/anexia/provider.go
M	pkg/cloudprovider/provider/anexia/provider_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/cloudprovider/provider/anexia/provider_test.go
CONFLICT (content): Merge conflict in pkg/cloudprovider/provider/anexia/provider_test.go
Auto-merging pkg/cloudprovider/provider/anexia/provider.go
CONFLICT (content): Merge conflict in pkg/cloudprovider/provider/anexia/provider.go
Auto-merging pkg/cloudprovider/provider/anexia/instance.go
Auto-merging pkg/cloudprovider/provider/anexia/helper_test.go
CONFLICT (content): Merge conflict in pkg/cloudprovider/provider/anexia/helper_test.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Anexia Provider: Utilize `Creating` state instead of blocking `Create` call
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release/v1.45

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
Copy link
Contributor

oh noes, looks like manual PR?
@marioreggiori can you take care of this?

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. docs/none Denotes a PR that doesn't need documentation (changes). 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/osm Denotes a PR or issue as being assigned to SIG OSM. 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

7 participants