search

SUSE / caasp-salt

A collection of salt states used to provision a kubernetes cluster
Apache License 2.0
64 stars 29 forks source link

Don't always reenable the transaction-update timer on update #756

Closed mssola closed 5 years ago

mssola commented 5 years ago

When executing the update orchestration, it's not always desired to reenable the transactional-update. Moreover, sometimes the timer was on a weird state if the update failed. This commit refines the update orchestration following this logic:

  1. Check first which nodes have explicitely disabled the timer, so they are not reenabled in the end.
  2. Disable the timer at the very beginning.
  3. If this is a migration, reenable it always.
  4. Otherwise, reenable the timer only for these nodes which did not explicitely disable it before the update.

Last but not least, I've also tweaked the transactional-update/init.sls file so the timer is not touched by this file when updating.

bsc#1113518

Signed-off-by: Miquel Sabaté Solà msabate@suse.com

MalloZup commented 5 years ago

@mssola about point 3) If this is a migration, reenable it always. since Caasp v4 doesn't support if migration from v3 to v4, i'm thinking we should simplify it here and add that logic in v3

mssola commented 5 years ago

@mssola about point 3) If this is a migration, reenable it always. since Caasp v4 doesn't support if migration from v3 to v4, i'm thinking we should simplify it here and add that logic in v3

I'm not sure I understand. This was already in place before my change, actually. My change tries to preserve the past behavior (point 3), and adds points 1, 2 and 4.

MalloZup commented 5 years ago

@mssola about point 3) If this is a migration, reenable it always. since Caasp v4 doesn't support if migration from v3 to v4, i'm thinking we should simplify it here and add that logic in v3

I'm not sure I understand. This was already in place before my change, actually. My change tries to preserve the past behavior (point 3), and adds points 1, 2 and 4.

ah ok i was thinking you added (point 3) ok thx for comment. ( i could also not found the point 3 in the pr :smile: ) thx

mssola commented 5 years ago

This PR will also need to be backported into 3.0 once this is merged (since the bug originally targets 3.0)