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

2.2 force upgrade #7958

Merged
merged 3 commits into from
Oct 25, 2017
Merged

2.2 force upgrade #7958

merged 3 commits into from
Oct 25, 2017

Conversation

jameinel
Copy link
Member

Description of change

This addresses an issue that has come up a few times, where we are asking people to upgrade their Controller, but they are unable to because of some agents that aren't working properly.

It does 3 primary things:

  1. Update the State code to allow an override so we will skip the agent version check.
  2. Update the API to expose it so that new Controllers can be triggered by new Juju clients (juju upgrade-juju --ignore-agent-versions)
  3. Brings in Tim's 'force-upgrade' script as something you can run directly on controllers for when their current version is to old.

QA steps

$ juju bootstrap lxd
$ juju deploy ubuntu
$ juju ssh ubuntu/0 "sudo service jujud-unit-ubuntu-0 stop"
$ juju upgrade-juju
$ juju upgrade-juju # should fail because unit-ubuntu-0 didn't get upgraded
$ juju upgrade-juju --ignore-agent-versions # should succeed

Documentation changes

The flag should be documented, along with when it is appropriate to use it.

Bug reference

lp:1726893

At the State level, allow a force flag that ignores the agent-version
check.  Expose this with a direct client that can trigger an upgrade to
any version you want.
Rename 'force-upgrade' to 'juju-force-upgrade'.
Expose a new value in the API that allows us to pass Force
through to the state code.
Change the flag from 'force' to 'ignore-agent-versions' because it is
more descriptive about what we're actually doing with the flag. And we
don't actually let you force things like models newer than controller,
etc.
@nskaggs
Copy link
Contributor

nskaggs commented Oct 24, 2017

As a drive-by, can you confirm the model version check is accurate? I saw a failure comparing versions with rc in it for instance..

(Basically, can you add a test for versions with strings in them and confirm)

Copy link
Contributor

@anastasiamac anastasiamac left a comment

Choose a reason for hiding this comment

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

Looks great overall \o/
Thank you for a nice doc string explaining the parameter and awesome QA steps on the PR!
I'd love to see the best facade check in CLI.
Also are you sure you don't want to bump a facade version? You have after all changed an input type...
Leaner api layer tests is just a wish but it would be nice if we started cleaning them up as we go rather than fixing & modifying existing full-stack, heavy tests....

@@ -363,7 +369,8 @@ func (c *upgradeJujuCommand) Run(ctx *cmd.Context) (err error) {
return block.ProcessBlockedError(err, block.BlockChange)
}
}
if err := client.SetModelAgentVersion(context.chosen); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reasons why you did not want to use BestFacadeVersion check to determine which call to make?

@@ -527,7 +527,7 @@ func (s *clientSuite) TestSetModelAgentVersionDuringUpgrade(c *gc.C) {
_, err = s.State.EnsureUpgradeInfo(machine.Id(), agentVersion, nextVersion)
c.Assert(err, jc.ErrorIsNil)

err = s.APIState.Client().SetModelAgentVersion(nextVersion)
err = s.APIState.Client().SetModelAgentVersion(nextVersion, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

I appreciate that this is an old, full-stack test. It'd be great to make it leaner \o/ Especially since the only thing we'd want to check here is that all the parameters are passed in.

@nskaggs
Copy link
Contributor

nskaggs commented Oct 25, 2017

@jameinel 2.2.6 is going right now -- can we ship this?

@jameinel
Copy link
Member Author

jameinel commented Oct 25, 2017 via email

@jujubot
Copy link
Collaborator

jujubot commented Oct 25, 2017

Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju

@jujubot jujubot merged commit abaa606 into juju:2.2 Oct 25, 2017
@nskaggs
Copy link
Contributor

nskaggs commented Oct 26, 2017

@pmatulis This needs documented. It shipped in 2.2.6. It's kind of a get of a jail card, if a model is really broken, this can let you fix it. Not sure how we want to highlight it, or the extent to which we do.

@pmatulis
Copy link
Contributor

@jameinel
For doc purposes,

  • Is this flag only to be used in the context of a controller model?
    • If so, is there any problem using it in a non-controller model (user mistake)?
    • If so, what to do about troubled units in other models if an upgrade is not possible?
  • Are there any risks associated with the usage of this flag?

@jameinel
Copy link
Member Author

This is valid for both controller and non controller models. You can still only upgrade non-controller models to agents that are <= the controller agent version.
Before this flag, we would check that all agents are == 'currentVersion' before we would allow you to upgrade. (So when going 2.1.1 => 2.1.3 => 2.2.2, we would require that all agents report 2.1.3 before we would let you upgrade to 2.2.2.)
That is slightly safer, because then we know that you don't end up in a weird skewed system where you have a big spread of all the versions of all the agents. (some are 2.1.1, some 2.1.3, some 2.2.2.)
In practice, what we've actually seen is that the only time agent versions end up being skewed is because specific agents/machines are down/broken. And having to fix those agents before we can upgrade the rest of the agents has generally been prohibitive, so this gives a way out.

The 'juju-force-upgrade' command exists to trigger the same upgrade (still checks versions, still ensures controllers are higher version than individual models, just ignores agent version skew). It
a) Works on controllers that don't support the 'IgnoreAgentVersions' flag.
b) Requires that you run on one of the controller machines (as root so you can read /var/log/juju/agents/machine-*/agent.conf)

Once 2.2.6+ is prevalent enough, I would expect juju-force-upgrade to be deprecated and retired.

@pmatulis
Copy link
Contributor

@jameinel

  1. So if some agents/machines are down/broken the user may end up with version skew?
  2. Where do I find juju-force-upgrade? I cannot find it.
  3. What is the connection between the deprecation of juju-force-upgrade and 2.2.6+?
  4. Similar to the last question, why is juju-force-upgrade only in 2.2(.6) and not in 2.3?

@jameinel
Copy link
Member Author

jameinel commented Oct 30, 2017 via email

@pmatulis
Copy link
Contributor

  1. The upgrade will abort unless all agents are only 1 patch version behind the targeted version. By overriding that we are getting users into a position we don't want them to be in?
  2. The docs will refrain from mentioning juju-force-upgrade until the command is available for regular users.
  3. What is the oldest version that supports the --ignore-agent-versions flag?
  4. Please let the docs team know what happens.

New question:

  1. How does one discover what values for --agent-version can be used (with bootstrap or upgrade-juju)?

@jameinel
Copy link
Member Author

jameinel commented Nov 1, 2017 via email

@pmatulis
Copy link
Contributor

pmatulis commented Nov 2, 2017

  1. For an aborted upgrade, does upgrade-juju give sufficient information for the user to rectify the situation (and retry)?
  2. Similarly, if the flag is used, is the user provided enough information to rectify (and retry with the flag)?

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