Define mechanism for updating template version, and hence infrastructure of deployed workspaces, workspaces services and user resources

[martinpeck]

No description provided.

[marrobi]

I suggest we should support upgrades of minor versions of bundles, but if major version changes an error should be returned.

[tamirkamara]

• templateVersion for an existing resource can be updated. No validation except for the target version to exist.
• Templates include upgrade-able from version section to check if the request upgrade is valid

[martinpeck]

Templates include upgrade-able from version section to check if the request upgrade is valid

What does this mean? Why isn't the versioning enough to say whether something is "upgradable from"?

[marrobi]

Templates include upgrade-able from version section to check if the request upgrade is valid

What does this mean? Why isn't the versioning enough to say whether something is "upgradable from"?

@martinpeck @tamirkamara this was my initial thought as per #1744 (comment)

[martinpeck]

Yep. It's not clear why the versioning/SemVer can't determine this for us.
Wouldn't a backward-looking "upgradable from" have to list all of the versions it's upgradable from?

[tamirkamara]

@marrobi implementing upgrades for just minor updates isn't that good IMO. It also makes it possible to break plus, it doesn't allow major versions. At any case, all our versions are minor and below

[tamirkamara]

It's not clear why the versioning/SemVer can't determine this for us.

@martinpeck can you explain?

Wouldn't a backward-looking "upgradable from" have to list all of the versions it's upgradable from?

If we want, or we can decide to list just the ones we tested, like last released version, etc.

[martinpeck]

Version 5.0.0 can't upgrade versions 4.*
Version 4.9.1 can upgrade things that are versions 4., but can't upgrade 3.

I guess it's up to us to determine if it's the major, minor, or patch number that determines this upgrade-ability, but doesn't this make things easier?

This doesn't fix the testing thing you mention. You're right that we won't test upgrading from all 4.x versions to higher 4.x versions. However, does that matter? What we're signalling here is that we don't expect any problems.

[tamirkamara]

My suggestion was not to get into this now at all and trust the admin to read the docs and upgrade only if instructed.
Checking versions implies we have a good way to decide on those which we currently don't...

[martinpeck]

Yeah....that's fair. So, your suggest is that an admin should do the same check (look at major/minor/path versioning, plus read the docs) and make a decision to upgrade....or not?

[tamirkamara]

For now yes - it's all up to the admin to make the decision.

[marrobi]

My challenge is how do they know they need to do that? Also although unlikely it's not just admin's that can patch resources but users too?

[tamirkamara]

I didn't think this would be a general purpose way to upgrade any resource.
So the idea was that release notes / breaking changes would include instructions to do the patch. Another option would be that the migration command (make) would initiate the upgrade (with logic around the version) or as now instruct one to preform it.

Patch is allowed by researchers for workspace services (not sure why) and user resources.

[guybartal]

Proposed workflow after our internal discussion:

1. User patches a resource with a new template version
2. Check authorization (same as today)
   2.1. shared service: TRE admin
   2.2. workspace: workspace owner
   2.3. workspace service: workspace owner
   2.4. user resource: workspace owner, researcher, or airlock manager
3. Get resource object from cosmos db
4. Get target template by version from cosmos db
5. Validate current resource version >= "minimum_upgradable_version" of the target template
   5.1. If yes or minimum_upgradable_version is missing, then update resource in cosmos db with the target template version.
   5.2 If not, return validation error.

for example:

if template from version 0.3.0 has property minimum_upgradable_version == 0.1.5 then

• Resources with version from 0.0.1 to 0.1.4 can't upgrade to 0.3.0.
• Resources with version 0.1.5 to 0.2.* can upgrade to 0.3.0

Flow chart:

[marrobi]

@guybartal so if I was building a bundle that had breaking changes - say storage account name - and can not be an upgrade - how do I specify that?

[martinpeck]

I'm not sure I 100% follow, so maybe I've misunderstood something...but...

I have a couple of problems with the "minimum_upgradable_version" approach.

Firstly, have we seen this done elsewhere? Is this a pattern we've come up with, or is this a way of marking upgrades/compatibility that we've seen used (and seen work) in other situations?

Second, if this is the approach we take what do the version numbers we're using mean? In the example given...

If template from version 0.3.0 has property minimum_upgradable_version == 0.1.5 then

Resources with version from 0.0.1 to 0.1.4 can't upgrade to 0.3.0.
Resources with version 0.1.5 to 0.2.* can upgrade to 0.3.0

... and assuming that the version numbers are MAJOR.MINOR.BUILD in format, we have a component where some BUILD releases are upgradable but others are not, while some MINOR releases are upgradable across MINOR releases, but others are not. Presumably (although not illustrated here) some MAJOR releases will be upgradable, but some are not.

This, I think, highlights a problem with the way we're designating versions for components. My feeling is that fixing that problem might make this problem go away (or become easier to deal with, at the very least). If we made a decision about which part of the SemVer determines upgradability (e.g. we decide "a break in upgradability is illustrated by bumping the MAJOR") then you'd have a mechanism that's used elsewhere and only requires inspection of the version.

I guess what we're trying to do here is say "we've tested upgrading from X to Y", but I'm still not sure that's something we should encode in the components in this way.

[guybartal]

I agree with you @martinpeck ,

I think we need to decide first if we want to hard block breaking changes (which may lead to data loss),
or simply popup a confirm button in the UI and let the user take responsibility after reading the release notes.
because if we can't upgrade a breaking change (upgrade is blocked), what does it mean? should the user deploy a new resource and delete the old one to take benefit of the newer version?

If we take the idea behind semantic versioning and apply it here, it may simplify things (although mainly it helps solving dependency hell between components as I understand, here are dealing with each component upgradability as stand alone component) :
MAJOR = breaking change (potential data loss, resources which can't be purged / renamed as @marrobi presented above) - always block in API (or UI level need to decide)
MINOR = upgrade (and by the way may be even downgrade if that make sense) should always work and was tested by the template developer.
BUILD/PATCH - not sure how we should use this here, because as long as there is no code change in the template the docker image should be identical between builds, am I missing something?

plus validate that target template registered in cosmos db first.

BTW, minimum_upgradable_version is just something we came up with which mean that we have tested upgradability of all versions in between. but using SemVer simplify things.

Another idea which I've presented during the internal discussion is to display a list of available template versions so the user can pick one to upgrade the resource to (can be defined in a different story).

[marrobi]

We need to prevent upgrade to a version that we know is going to say cause data loss.

I'd be happy to say only upgrade for minor version increments if that is easier to implement, as if you are making a breaking change, then upgrade the major version. If an admin desperately wants to upgrade a major version they could do it via cosmos? Not an ideal user experience but would provide a mechanism to prevent upgrades that will cause data loss which is my primary concern.

[guybartal]

OK, the mechanism will block the upgrade in the API for major version, and allow updates only for minor versions, while an admin can always take the risk and override by updating the version via cosmos db.

Open question:
Do we need to support minor version downgrade in the API or should it block it? Just in case something went wrong and the resource is not functioning properly after upgrade, should we allow to rollback?

[tamirkamara]

1. If we go down this route it implies we always test upgrades from each minor version to a newer one
2. Need to remember that we're not the only ones creating bundles. Will customers understand this and act accordingly?
3. BTW, I don't see specifying the min version as something new as many apps that come to upgrade an earlier version of themselves will check if they can and a version is just one dimension of their validation procedure.

[martinpeck]

1. If we go down this route it implies we always test upgrades from each minor version to a newer one

No. It implies that we definitely haven't tested (and don't support) other types of upgrades. If we're not going to test an upgrade, we bump something other than minor.

2. Need to remember that we're not the only ones creating bundles. Will customers understand this and act accordingly?

No, but then again I don't think others will understand/follow the proposed mechanism either. I'd argue that the proposed mechanism is harder to explain than one where we say "if you're sure it works with previous versions then it's a minor update. If not, it's a major update"

I think my main concern is that we are working around a bigger issue.

When should I change the MAJOR, MINOR, or BUILD version number, and what does it mean if I only change BUILD or I change MINOR?

I don't think we're being consistent in the way we change those values, and we need to address that first. Or am I mistaken? Once we a better understanding of when these values change we can then determine if we need a way to describe our testing strategy. If we do, this additional property might be useful. However, I'd still expect the version number to play a part in whether something is upgradable.

For example, I'd expect all versions that differ only by BUILD to be upgradable, regardless of the minimum_upgradable_version

[guybartal]

We had another internal discussion about this issue, we have decided (for now) to implement the mechanism in the following way, and might change it in the future as we see fit.

Template developers will be required to follow this rule when bumping a template version in PRs.

Version format: MAJOR.MINOR.BUILD

• MAJOR = breaking change (potential data loss, resources which can't get purged / renamed etc.) - Always block in API with another parameter for "Force update" to override.
• MINOR = small changes in which we can certainly say that an upgrade should always work and tested by the template developer (at least from the previous version).
• BUILD/PATCH - fixing typos, bugs - update should always work for that.

This rule should be clearly documented in Azure TRE docs, and in PR template as a reminder.

[tamirkamara]

I don't like us saying a major version is just for a breaking change - if a big piece of functionality was changed/added you'd want to say it's a major version, but if no potential data loss exist (since there isn't one or you handled the situation internally) then you'd say it's minor version. That doesn't seem right...
@guybartal @martinpeck

[guybartal]

If we want to follow SemVer, major means breaking changes.

[martinpeck]

I also don't think "major is just for breaking changes".

Breaking changes require a major version number change. That's true. And, most of the time, that's when we'll want to change the major version number. But...the scenario you mention of a BIG change can still be a major version number change.

What we're saying is that we won't allow an automatic upgrade across major version numbers because we're taking a "better safe than sorry" approach to upgrading. When the major version number changes we assume something significant has changed and in this situation someone should review what has changed and then take some appropriate action. In many cases this will be because of a breaking change, or a change that doesn't easily/automatically upgrade. In other cases it might be that the functionality of the component has significantly changed (and user education/training/whatever may be required).

This, again, highlights that we need to have a better understanding of when MAJOR, MINOR, or BUILD would change, but I think the rule "we don't automatically upgrade across MAJOR" has enough up-sides that the down-side described is one we can probably live with.

[guybartal]

Tasks:

1. Support resource version upgrades inside PATCH operation for all types of resources (block major and downgrade versions)
2. Add relevant unit tests
3. Test upgrades for deployed resources
4. Update documentation
5. Update PR template with version guidance
6. Add support for force update major versions and downgrades