Implement TemplateNodeInfo for magnum cloudprovider

[b0e]

What type of PR is this?

/kind feature

What this PR does / why we need it:

Implement TempalteNodeInfo() for the cluster-autoscaler magnum cloud provider

Which issue(s) this PR fixes:

Haven't found any.

Special notes for your reviewer:

Scaling up a nodegroup from zero didn't work because TemplateNodeInfo was not implemented.

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: b0e (a249ca9)

[k8s-ci-robot]

Welcome @b0e!

It looks like this is your first PR to kubernetes/autoscaler 🎉. 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/autoscaler 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]

Hi @b0e. Thanks for your PR.

I'm waiting for a kubernetes 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-sigs/prow repository.

[Shubham82]

@b0e you have to sign the CLA before the PR can be reviewed.
See the following document to sign the CLA: Signing Contributor License Agreements(CLA)

[Shubham82]

To check EasyCLA

/easycla

[modzilla99]

Hi @BigDarkClown @x13n,
sorry for pinging you, but would you please have a look at this PR?

Fixes #6018

Thank you!

[x13n]

Cloud provider specific changes should be reviewed by dedicated OWNERS. @tghartland can you take a look?

/assign tghartland

[k8s-ci-robot]

@x13n: GitHub didn't allow me to assign the following users: tghartland.

Note that only kubernetes members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

Cloud provider specific changes should be reviewed by dedicated OWNERS. @tghartland can you take a look?

/assign tghartland

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-sigs/prow repository.

[modzilla99]

@tghartland would you please take a look at this?

[tghartland]

Hi, I think I was away for a while when this came in, I must have missed it when I got back.

Sorry for the long delay, I'll take a look at it this week.

[tghartland]

I had some problems getting a working test environment going, but I've tested scaling up from 0 and that works and gives the expected number of nodes for the resources of pending pods and nodegroup flavor size.

While scaling down I saw some odd issues (master nodegroup going to a bad state) though it might just be more problems from my test environment. I want to take another look at that, but I am travelling from today until Tuesday so it will have to wait.

The diff looks fine, I don't see anything in there that would be causing the scale down issues but I need the time to double check.

[modzilla99]

Thanks for the quick response. We've been using the patchset for quite a while now and haven't encountered problems with the master NG. But to be fair, we don't use the main heat stack for scaling and always create another NG. But yeah heat does some weird stuff sometimes.

[modzilla99]

While scaling down I saw some odd issues (master nodegroup going to a bad state) though it might just be more problems from my test environment.

What we've encountered though is that on scale down one node will be removed at a time and not in a batch.

[tghartland]

I've managed to get my devstack environment set to the stable magnum version, and that doesn't have the master nodegroup issue I saw when scaling down.
Everything is working as expected in my testing now.

About your point on batching, the core autoscaler makes multiple requests for single nodes when doing a scale down.

I1103 13:16:01.052902       1 magnum_manager_impl.go:390] manager deleting node: k8s-cluster-3cwfpqnz4qll-node-4
I1103 13:16:01.052947       1 magnum_manager_impl.go:400] resizeOpts: node_group=f5cff265-6baf-4cc1-93fd-803ec976e6cf, node_count=1, remove=[282037cd-4e08-4d65-920d-f4b22b0f3987]
I1103 13:16:06.157309       1 magnum_manager_impl.go:390] manager deleting node: k8s-cluster-3cwfpqnz4qll-node-3
I1103 13:16:06.157314       1 magnum_manager_impl.go:400] resizeOpts: node_group=f5cff265-6baf-4cc1-93fd-803ec976e6cf, node_count=0, remove=[fed68301-d2d8-4b69-a629-5a33d59a07ae]

And magnum is able to handle those requests in parallel, I see it deleting both nodes at the same time:

LAST SEEN   TYPE      REASON                    OBJECT                                 MESSAGE
44m         Normal    DeletingNode              node/k8s-cluster-3cwfpqnz4qll-node-3   Deleting node k8s-cluster-3cwfpqnz4qll-node-3 because it does not exist in the cloud provider
44m         Normal    DeletingNode              node/k8s-cluster-3cwfpqnz4qll-node-4   Deleting node k8s-cluster-3cwfpqnz4qll-node-4 because it does not exist in the cloud provider

In the first implementation of the magnum provider, it did wait and try to batch up those into one request to openstack, but that was before the magnum resize API was available. Once magnum could handle multiple requests at the same time, it was a lot easier to let it handle that as the code in the provider was very awkward.

[tghartland]

Thanks for your contribution!

/lgtm
/approve

[k8s-ci-robot]

@tghartland: changing LGTM is restricted to collaborators

In response to this:

Thanks for your contribution!

/lgtm
/approve

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-sigs/prow repository.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: b0e, tghartland
Once this PR has been reviewed and has the lgtm label, please assign aleksandra-malinowska for approval. For more information see the Kubernetes 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:

• cluster-autoscaler/OWNERS

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

[tghartland]

@x13n the CI bot never likes me, could you set this PR as ready to merge?

[Shubham82]

/ok-to-test

[Shubham82]

Thanks for your contribution!

/lgtm /approve

giving LGTM as the Owner approves PR, but labels are not applied.
/lgtm

[Shubham82]

@x13n the CI bot never likes me, could you set this PR as ready to merge?

@tghartland, IMO, it is because you are not in k8s org.

cc @x13n please approve this PR.
Thanks!

[Shubham82]

@b0e, please resolve the merge conflict.

[k8s-ci-robot]

New changes are detected. LGTM label has been removed.

[Shubham82]

cc @tghartland
PTAL (if all is good after rebasing, then we can merge this)

Thanks!

[Shubham82]

As the owner of Magnum cloud provider in CA, it would be good if you could join k8s Org (become a member) so that you can easily approve the PR related to Magnum.