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

Upgrade via URL method #178

Closed
wants to merge 1 commit into from
Closed

Upgrade via URL method #178

wants to merge 1 commit into from

Conversation

aj-jester
Copy link

Ok. I spent some time on this today. First there is the bad news, then lots of good news, then some unfortunate caveats.

The bad news is I didn't use camptocamp/archive module. Similar to nanliu/staging it did only some stuff well, like checksums. But getting it to do exactly the right thing could more or less be summed up by the gif below, just replace "css" with "archive" lol.

css

I was just as frustrated as this guy ^^^

That being said, the good news is I implemented all the goodies of camptocamp/archive and fine tuned it to our needs.

  • Installs, upgrades and downgrades, both "core" and UI.
  • Supports multiple checksums; (md5, sha1, sha224, sha256, sha384, sha512).
  • Users can set timeouts, toggle env vars and tmp dir params.
  • Refactored URL method to mimic package install so the end user does not need to know when to use present, installed or absent, they can use it for both URL and package methods.
  • Paves the way for possible "uninstall" option, currently it will just notify as unsupported.
  • The dependencies are down to 1; puppet-stdlib.
  • The URL operation cleans up after itself, nothing is left behind except what it installed, unlike staging and archive.

Caveats:

  • I had to limit the URL package type to only zip. With so many execs it would've been way too complicated. I don't think this would be a problem for majority of the users.
  • Speaking of URLs, I had to change the class param name for the main package (aka core) from package_* to core_package_* and ui_download_* to ui_package_* to match core_package_* params, all for the sake of clarity. These changes will not affect users who use the default parameters. If they use a custom download URL it would break for them unfortunately.
  • Changed ensure for packages from latest to installed since URL doesn't support latest but its the default install method, you see the dilemma 😅.
  • The unit tests are broken, I will fix them once we are ok with the direction of this PR.
A big caveat

Even if the version is the same, when users use this branch for the first time, and only for the very first time, the module will upgrade consul and restart, because it doesn't know the existing version. This is mostly important for masters but I would not want clients restarting either. There are 2 ways to mitigate this:

  1. Users can run the following on each of the masters:
$ echo "Managed by Puppet\nconsul_core_<version>\n" > <data_dir>/.version_core

with the following permissions 0644 and whatever consul user/group is used:

-rw-r--r-- 1 consul consul   36 Aug 24 01:46 .version_core

❗ Be sure to update <version> and <data_dir> with appropriate values.

This prevents puppet from notifying the rest of the install process. I would recommend this to be done on clients as well.

OR

  1. Set restart_on_change to false and it will only replace the existing binary with an identical one, thats it, no restarts.

I don't know if one is better than the other, but I would do option 1. Is there a better way to do this? I'm all ears.

I would appreciate if everyone tests this branch and let me know if there is anything else I missed.

Ok, lets take a breather, here's a few 🍻

Thoughts? Concerns? Fears?

cc @solarkennedy

@aj-jester
Copy link
Author

@solarkennedy This PR addresses these issues as well

#123 (closes)
#118 (closes)
#93 (obsolete)

#116 (fixed)
#103 (fixed)
#79 (progress)

@aj-jester aj-jester force-pushed the AJ-url-upgrade-support branch 2 times, most recently from 56b824e to b27a4a3 Compare August 24, 2015 03:12
@solarkennedy
Copy link
Contributor

Hmm. I'm happy to see this issue fixed, but I'm sad that archive didn't work out. Let me think about this carefully.

@aj-jester
Copy link
Author

Ok thanks.

I forgot to mention this, another issue with archive is their error messages are so darn ambiguous. One of the errors was rm-on-error but it didn't tell what the actual error was, after spelunking through the code it turns out "rm on error" means "error while checking checksum", they could've just said that lol. Under the hood this is doing exactly the same thing as archive but without all the fluff and clearer messages to debug issues.

@solarkennedy
Copy link
Contributor

I can see that the archive module isn't as polished as I hoped. But if anything I would rather invest effort in making that existing codebase before re-implement.

On the other hand, we are going to have more control with this thing and it is nice not to have any other dependencies.

If we did move to archive, we would probably have a similar rough transition, I think the version_core trick is probably inevitable.

Let me think just a little more.

@aj-jester
Copy link
Author

I think the version_core trick is probably inevitable.

Yup, no doubt. I was thinking at first to just submit a PR with them but then figured why not put that effort into writing our own which we can have more control over 😄

Thanks.

@hopperd
Copy link
Contributor

hopperd commented Dec 8, 2015

What did we decide on this particular issue? This will circle around again with the recent release of Consul 0.6.0 so wondering what the agreed upon solution was for this.

@aj-jester
Copy link
Author

I was thinking about this as well but recently I came to know about https://github.com/llinder/deb-consul-template. You can either add the ppa to your apt repos sudo apt-add-repository ppa:bcandrea/consul or you can build your own package.

I think managing upgrades/rollbacks via staging module or any approach (as seen here) won't be pretty 😞

@aj-jester aj-jester closed this Dec 8, 2015
@aj-jester aj-jester deleted the AJ-url-upgrade-support branch December 8, 2015 18:03
@hopperd
Copy link
Contributor

hopperd commented Dec 8, 2015

Yes I would agree they can be clumsy with staging, but take a look at the pull request I just added #202 and see if we think that it can meet the needs of handling upgrading/downgrading via the URL.

@solarkennedy
Copy link
Contributor

Woo!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants