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

Do not allow downgrades without force flag #699

Merged
merged 2 commits into from
Mar 1, 2023

Conversation

davidcassany
Copy link
Contributor

@davidcassany davidcassany commented Feb 28, 2023

This commit sorts the current OS version with the one we aim to upgrade to. If the OS to be upgraded to is not higher than the current one it assumes we are up to date and the upgrade script exits 0. This causes the upgrade plan to succeed and to be considered as upgraded. This is to fix rancher/elemental-operator#364 (comment)

Alternatively if a downgrade is required it is still possible, but requires a customized Update Group (ManagedOSImage crd) to include the FORCE environment variable within the upgrade script:

apiVersion: elemental.cattle.io/v1beta1
kind: ManagedOSImage
metadata:
  name: upgrade3
  namespace: fleet-default
spec:
  clusterTargets:
    - clusterName: my-cluster
  upgradeContainer:
    command:
      - "/usr/sbin/suc-upgrade"
    envs:
      - name: FORCE
        value: "true"
    image: myimage:mytag

With the FORCE flag no version is taken into account and the image is applied in any case.

elemental-operator logic makes use of the osImage field if no upgradeContainer field is provided to actually create a default ugpradeContainer. However if upgradeContainer is provided then osImage field is ignored. I'd say this logic is not very intuitive, but I'd say this should be handled separately if we consider there is something to change in there.

Fixes rancher/elemental-operator#364

Signed-off-by: David Cassany <dcassany@suse.com>
@davidcassany davidcassany requested a review from a team February 28, 2023 17:25
@davidcassany
Copy link
Contributor Author

@kkaempf @agracey this is the fix I tested to skip downgrades by default. I'd be also fine by not including it and assume the admin has to provide sane update groups and/or try to fix the issue beyond elemental (system-upgrade-controller, fleet, rancher, etc.). IMHO the good solution would be preventing concurrent upgrades at downstream cluster level, this is essentially orchestrated by the fleet and system-upgrade-controller pair.

@agracey
Copy link

agracey commented Feb 28, 2023

I think this makes sense for now. Since there is a workaround to force a downgrade (however unpleasant), I'm comfortable with the limitation

@davidcassany
Copy link
Contributor Author

I think this makes sense for now. Since there is a workaround to force a downgrade (however unpleasant), I'm comfortable with the limitation

Yes, we can improve it by making the ManagedOSImage logic a bit more intuitive. So we could enable something like:

Current common Upgrade Group without force:

apiVersion: elemental.cattle.io/v1beta1
kind: ManagedOSImage
metadata:
  name: upgrade3
  namespace: fleet-default
spec:
  clusterTargets:
    - clusterName: my-cluster
  osImage: myimage:mytag

Upgrade Group with force:

apiVersion: elemental.cattle.io/v1beta1
kind: ManagedOSImage
metadata:
  name: upgrade3
  namespace: fleet-default
spec:
  clusterTargets:
    - clusterName: my-cluster
  osImage: myimage:mytag
  upgradeContainer:
    envs:
    - name: FORCE
      value: "true"

So basically use osImage as upgradeContainer.image even if some other fields of upgradeContainer are provided. Alternatively we could also add a new forceUpgrade field at the level of osImage, but probably I'd wait for that until we have further or more refined feedback on what is required for the upgrades. If we add a new field we will have to support it and I dislike having multiple ways of setting the exact same thing.

Copy link
Member

@fgiudici fgiudici left a comment

Choose a reason for hiding this comment

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

looks good here

Signed-off-by: David Cassany <dcassany@suse.com>
@davidcassany davidcassany merged commit f178106 into rancher:main Mar 1, 2023
@davidcassany davidcassany deleted the no_concurrent_suc-upgrade branch March 1, 2023 15:40
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.

Prevent multiple upgrades at the same time
3 participants