-
Notifications
You must be signed in to change notification settings - Fork 501
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
[JUJU-3969] Allow upgrades without specifying version #15727
[JUJU-3969] Allow upgrades without specifying version #15727
Conversation
@@ -24,7 +24,6 @@ type versionCheckTC struct { | |||
allowed bool | |||
minVers string | |||
err string | |||
patch bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patch was removed, because it was set to true for every test.
ceb3581
to
07df7b5
Compare
@SimonRichardson what has changed since we last talked? If this will be done in pieces, what are they? |
The following allow migrating to a new version of juju without explicitly stating the newer major version (3.x -> 4.x). Juju shouldn't have to know about new major versions, so to fix this, we check that it's not in the version map, before checking if we're moving to a larger major version. This is required as 3.1 has been ear marked as the last LTS before releasing 4.0. The target controller will then validate where the model is coming from, and ensure it's with it's own range before allowing it to migrate.
07df7b5
to
f318d70
Compare
So that others can do the review and approve while I"m on vacation.
The prior code had logic from the 2.9 branch. We can remove the branching logic and just rely on the fact that the current version can never downgrade.
f318d70
to
abfbef7
Compare
This removes the hack to upgrade version, instead just remove all source checks for source requests.
c06f669
to
30ac8d4
Compare
The following removes the target version from source prechecking. The source check should __never__ know about the version it's moving to. The source doesn't care. The target cares. Removing this logic is the first step of cleaning all of this up. Inreality we should send a big set of information for the target controller to validate. Nothing should be checked on the source.
30ac8d4
to
ede7ea7
Compare
) []Validator { | ||
validators := []Validator{ | ||
getCheckTargetVersionForModel(targetVersion, MigrateToAllowed), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is the real meat of it. We no longer check the source for the target version. This is totally up to the target to check for.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can MigrateToAllowed
be deleted?
if targetVersion.Major == 3 { | ||
validators = append(validators, | ||
checkNoWinMachinesForModel, | ||
checkForDeprecatedUbuntuSeriesForModel, | ||
getCheckForLXDVersion(cloudspec), | ||
) | ||
if targetVersion.Minor >= 1 { | ||
validators = append(validators, checkForCharmStoreCharms) | ||
} | ||
} | ||
return validators |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're already at 3.1 so these branches can just be removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a separate PR doing the version validation on the target controller? We would need to do that up front before any model import is done. We'd lose the advantage of not having to bother to quiesce the source model, unless we added a PreCheck() method on the target controller or something like that.
migration/types.go
Outdated
// Copyright 2023 Canonical Ltd. | ||
// Licensed under the AGPLv3, see LICENCE file for details. | ||
|
||
package migration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other packages call this file interface.go
Maybe consistency is best?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
environscloudspec "github.com/juju/juju/environs/cloudspec" | ||
) | ||
|
||
// ValidatorsForModelMigrationSource returns a list of validators to run source controller for model migration, | ||
// ValidatorsForModelMigrationSource returns a list of validators to run source |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... to run on the ...
) []Validator { | ||
validators := []Validator{ | ||
getCheckTargetVersionForModel(targetVersion, MigrateToAllowed), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can MigrateToAllowed
be deleted?
There is always a check on the target controller. https://github.com/juju/juju/blob/3.2/migration/precheck.go#L258
No, because we need to check this in the target precheck for the source-to-target validation. This just removes the code that the target was already checking. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Problem in QA
validators = append(validators, checkForCharmStoreCharms) | ||
} | ||
if targetVersion.Major == 3 && targetVersion.Minor >= 1 { | ||
validators = append(validators, checkForCharmStoreCharms) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not hitting this check. Migration doesn't fail until here:
machine-0: 20:44:19 ERROR juju.worker.migrationmaster.a98b55 model data transfer failed, failed to migrate binaries: cannot upload charms: cannot upload charm: Post https://10.186.60.233:17070/migrate/charms?arch=&name=ubuntu&revision=24&schema=cs&series=&user=: cannot upload charm: unsupported schema "cs"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah because you're not allowed cs charms in 3.1. So this is correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I did some investigation. This is entirely correct for what we have now. If you try and migrate from 2.9 to 3.1 there is no check at all, on the source side.
juju/upgrades/upgradevalidation/migrate.go
Lines 13 to 28 in 02d4986
func ValidatorsForModelMigrationSource( | |
targetVersion version.Number, cloudspec environscloudspec.CloudSpec, | |
) []Validator { | |
validators := []Validator{ | |
getCheckTargetVersionForModel(targetVersion, MigrateToAllowed), | |
getCheckUpgradeSeriesLockForModel(false), | |
} | |
if targetVersion.Major == 3 { | |
validators = append(validators, | |
checkNoWinMachinesForModel, | |
checkForDeprecatedUbuntuSeriesForModel, | |
getCheckForLXDVersion(cloudspec, minLXDVersion), | |
) | |
} | |
return validators | |
} |
The target side will only ever validate from 3.1 onwards
juju/upgrades/upgradevalidation/upgrade.go
Lines 21 to 31 in d3c8991
if targetVersion.Major == 3 { | |
validators = append(validators, | |
checkMongoVersionForControllerModel, | |
checkNoWinMachinesForModel, | |
checkForDeprecatedUbuntuSeriesForModel, | |
getCheckForLXDVersion(cloudspec), | |
) | |
if targetVersion.Minor >= 1 { | |
validators = append(validators, checkForCharmStoreCharms) | |
} | |
} |
So this PR doesn't actually break any existing flow from 2.9 to 3.1. It's currently been this way. There is no way to get a cs charm into 3.1, so we could actually remove the check on the source side as well.
Here are my steps to reproduce exactly the same thing as this comment:
$ snap install juju --classic --channel=2.9/stable
$ /snap/bin/juju bootstrap lxd src29
$ /snap/bin/juju deploy cs:~jameinel/ubuntu-lite
$ git checkout 3.1
$ juju bootstrap lxd target31 --build-agent
$ /snap/bin/juju migrate src29:default target31
@hmlanigan we can fix that issue in a subsequent PR, but not this one, then I'm fine with that.
The following code is just not used anymore because we're no longer checking the source.
8460b58
to
4ad7f57
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can fix the QA fail in a new pr
/merge |
#15896 Forward ports: - #15845 - #15725 - #15852 - #15854 - #15849 - #15855 - #15857 - #15861 - #15862 - #15860 - #15864 - #15825 - #15866 - #15863 - #15870 - #15871 - #15872 - #15873 - #15874 - #15876 - #15875 - #15881 - #15727 - #15883 - #15884 - #15880 - #15879 - #15886 - #15887 - #15877 - #15888 - #15893 - #15894 Conflicts: - cmd/juju/ssh/debugcode_test.go - cmd/juju/ssh/debughooks_test.go - cmd/juju/ssh/scp_unix_test.go - cmd/juju/ssh/ssh_machine.go - cmd/juju/ssh/ssh_machine_test.go - cmd/juju/ssh/ssh_unix_test.go - worker/dbaccessor/worker.go - caas/kubernetes/provider/k8s_test.go - caas/kubernetes/provider/operator_test.go - caas/kubernetes/provider/package_test.go - caas/kubernetes/provider/rbac.go - cmd/juju/application/deployer/bundlehandler.go - cmd/juju/application/deployer/bundlehandler_test.go - cmd/jujud/agent/model/manifolds.go - core/bundle/changes/changes.go - worker/caasapplicationprovisioner/application.go - worker/caasapplicationprovisioner/application_test.go - worker/caasapplicationprovisioner/mock_test.go
The following allows migrating to a new version of juju without explicitly stating the newer major version (3.x -> 4.x). Juju shouldn't have to know about new major versions, so to fix this requires removing the code that was recently added to pass the target controller version to the source pre-checks. In addition to this, I've also refactored the migration precheck context types to not allow target pre-check to explicitly know about model checking. This should prevent any target and source issues.
This is required as 3.1 was earmarked as the last LTS before releasing 4.0. The target controller will then validate where the model is coming from, and ensure it's within its own range before allowing it to migrate.
Checklist
QA steps
We require this PR one to land before we can fully test the workflow.
To ensure we didn't break any backward compatibility: