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

Fix Linode plan selection #47810

Merged
merged 11 commits into from May 25, 2018

Conversation

Projects
None yet
4 participants
@rmcintosh
Contributor

rmcintosh commented May 23, 2018

What does this PR do?

Fixes Linode plan selection in the Linode Salt Cloud module.

What issues does this PR fix or reference?

Linode plan labels used to look like this: Linode 2048
Now, they look like this: Linode 2GB

Additionally, 1GB Linodes are labeled Nanode 1GB.

This has broken, presumably, all existing Linode cloud profiles.

Previous Behavior

Only accepting plan labels in the form of Linode 2048.

New Behavior

Accepting labels in either form (Linode 2048 vs. Linode 2GB). Also, a 1GB Linode can be deployed via the "Linode" or "Nanode" label, for backwards compatibility.

Tests written?

No

Commits signed with GPG?

Yes

@salt-jenkins salt-jenkins requested a review from saltstack/team-cloud May 23, 2018

@rmcintosh rmcintosh force-pushed the rmcintosh:fix-linode-plan-selection branch 2 times, most recently from 91b99b3 to 104e539 May 23, 2018

@rallytime

This comment has been minimized.

Contributor

rallytime commented May 23, 2018

Rather than doing this programmatically, wouldn't it just make sense to log an error and then people just need to update their cloud profiles?

@rmcintosh

This comment has been minimized.

Contributor

rmcintosh commented May 23, 2018

@rallytime I'm not necessarily opposed to doing so.

My thought was that, given how many cloud profiles this likely will have broken, it would be better to create backwards compatibility than to require every single user of the Linode module to modify all of their Linode cloud profiles (an easy enough sed for a few profiles, but if I have 300 cloud profiles
stored across 25 different machines, maybe less so...)

I can see your argument too, though. If you have a preference for that approach, and letting the user sort things out, I'm happy to adjust.

@rallytime

This comment has been minimized.

Contributor

rallytime commented May 23, 2018

@rmcintosh Yeah, I can definitely see how that would be helpful for people. I also am thinking about maintaining this moving forward and what that should look like. I updated the docs for the Linode driver in #47781, but I can see how editing many cloud profiles would be a chore.

Maybe a good compromise would be to keep this helpful change, but also warn the user that they're using an old size?

@saltstack/team-cloud Anyone else have a preference here?

@rmcintosh

This comment has been minimized.

Contributor

rmcintosh commented May 23, 2018

I like the compromise idea a great deal 👍

I'm not sure what plans you all have with regards to future maintenance of the module (hopefully, we'll move it from using v3 of the API to v4, at some point 🙂) but I'd be glad to assist in any way that I can

@rallytime

This comment has been minimized.

Contributor

rallytime commented May 23, 2018

@rmcintosh 👍

There are not any active plans to move from v3 to v4 that I know of. If you're willing to update the driver, that would be great!

A change like that should definitely go into develop and be run through the cloud tests. As long as v4 can effectively support the same functionality that is currently here, then that should be good. The API connection doesn't have any package dependencies, so that makes upgrading support easier.

Are the connection/authentication parameters the same? (I have not looked.) If they are, that would be most excellent so we wouldn't need to worry about supporting both the v3 and v4 versions at once.

@rmcintosh

This comment has been minimized.

Contributor

rmcintosh commented May 23, 2018

v4 uses the "Authorization: Bearer $token" scheme vs. v3 which only accepts api_key as a query param (or using HTTP Basic Authentication, but this module doesn't do that, I don't think)

Alone, that isn't too troublesome to work around but at least presently, the two token types are not inter-compatible - so one cannot use a v3 token in the v4 API (or vice versa). So, your concern about maintaining both is well-founded. Offhand, I don't have a great solution to the problem... but I'll ponder that

@rmcintosh rmcintosh force-pushed the rmcintosh:fix-linode-plan-selection branch from 5c04b9c to 69f10a5 May 23, 2018

@gtmanfred

Just one nitpick change, everything else looks good to me.

@@ -993,6 +993,58 @@ def get_password(vm_):
)
def decode_linode_plan_label(label):

This comment has been minimized.

@gtmanfred

gtmanfred May 24, 2018

Contributor

this should probably be a private functions, _decode_linode_plan_label

This comment has been minimized.

@rmcintosh

rmcintosh May 24, 2018

Contributor

good call 👍

@cachedout

This comment has been minimized.

Contributor

cachedout commented May 24, 2018

@rallytime We'll probably want this backported.

@rallytime

I have a couple of requests - mostly very picky ones.

Thanks again for this fix. :)

@@ -993,6 +993,58 @@ def get_password(vm_):
)
def _decode_linode_plan_label(label):
"""

This comment has been minimized.

@rallytime

rallytime May 24, 2018

Contributor

I know this is picky, but can you change these to single quotes instead of double? That's what we prefer according to the style guide.

plan_type = plan[0]
try:
plan_size = int(plan[1])
except Exception as e:

This comment has been minimized.

@rallytime

rallytime May 24, 2018

Contributor

Can you change this to a more specific exception? We typically try to stay away from broad exceptions like this. You also don't need the as e here since you're not doing anything with it.

plan_size = 0
if plan_type == "Linode" and plan_size == 1024:
plan_type = "Nanode"

This comment has been minimized.

@rallytime

rallytime May 24, 2018

Contributor

👍

Nice catch. I saw that they changed the name of the 1GB instances like that the other day.

@@ -1020,7 +1072,9 @@ def get_plan_id(kwargs=None, call=None):
'The get_plan_id function requires a \'label\'.'
)
return avail_sizes()[label]['PLANID']
label = decode_linode_plan_label(label)

This comment has been minimized.

@rallytime

rallytime May 24, 2018

Contributor

This should be _decode_linode_plan_label.

@rallytime

This comment has been minimized.

Contributor

rallytime commented May 24, 2018

Alone, that isn't too troublesome to work around but at least presently, the two token types are not inter-compatible - so one cannot use a v3 token in the v4 API (or vice versa). So, your concern about maintaining both is well-founded. Offhand, I don't have a great solution to the problem... but I'll ponder that

One thing we have done in the past was create a new file/driver for new API versions. We had to do that with the DigitalOcean driver a couple of years ago when they changed their API versions significantly.

I see that Linode has marked the v3 API as deprecated, so it's a good idea to build out a v4 version now. Is that something you'd like to work on @rmcintosh? If not, we can start working on it.

Once the v4 version is complete, we can put the old driver on a deprecation path to be eventually replaced completely by v4. Do you know how long Linode is planning on supporting v3?

The same idea of maintaining functionality would apply (at least as best as possible) to the new driver. Then the v4 version will already be in place/tested before v3 support is removed on Linode's end.

rmcintosh added some commits May 24, 2018

@rmcintosh

This comment has been minimized.

Contributor

rmcintosh commented May 24, 2018

@rallytime Thanks! I think I got to each of those - but please let me know if I overlooked anything

One thing we have done in the past was create a new file/driver for new API versions. We had to do that with the DigitalOcean driver a couple of years ago when they changed their API versions significantly.

I see that Linode has marked the v3 API as deprecated, so it's a good idea to build out a v4 version now. Is that something you'd like to work on @rmcintosh? If not, we can start working on it.

I'd be happy to start working on it! And I think creating a new driver for the v4 version would be wise - I had pondered whether that would be a possible solution but wasn't sure if you all were amenable to that. Good to know 👍

Do you know how long Linode is planning on supporting v3?

There's no set date, presently. We anticipate that it will be quite some time before we're able to decommission it completely (1 year or longer, at least, in all likelihood).

The same idea of maintaining functionality would apply (at least as best as possible) to the new driver. Then the v4 version will already be in place/tested before v3 support is removed on Linode's end.

This sounds reasonable to me 👍

better debug message
also, give Cloud Profile its proper capitalization

and some minor formatting consistency updates
@rallytime

This looks great! Thank you @rmcintosh!

@rallytime

This comment has been minimized.

Contributor

rallytime commented May 25, 2018

I'd be happy to start working on it!

Thank you! That would be wonderful. Definitely reach out if you have any questions/comments.

@rallytime rallytime merged commit c69a9a2 into saltstack:develop May 25, 2018

4 of 10 checks passed

jenkins/PR/salt-pr-linode-cent7-py3 Pull Requests » Salt PR - Linode CentOS 7 - PY3 #5268 — ABORTED
Details
jenkins/PR/salt-pr-linode-ubuntu16-py3 Pull Requests » Salt PR - Linode Ubuntu16.04 - PY3 #10239 — ABORTED
Details
jenkins/PR/salt-pr-rs-cent7-n Pull Requests » Salt PR - RS CentOS 7 #19324 — ABORTED
Details
codeclimate 2 issues to fix
Details
default Build finished.
Details
jenkins/PR/salt-pr-linode-ubuntu14-n Pull Requests » Salt PR - Linode Ubuntu14.04 #23202 — FAILURE
Details
WIP ready for review
Details
jenkins/PR/salt-pr-clone Pull Requests » Salt PR - Clone #25458 — SUCCESS
Details
jenkins/PR/salt-pr-docs-n Pull Requests » Salt PR - Docs #17545 — SUCCESS
Details
jenkins/PR/salt-pr-lint-n Pull Requests » Salt PR - Code Lint #22175 — SUCCESS
Details

rallytime added a commit that referenced this pull request May 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment