Incremental version checks during migration #2399

Zash · 2025-01-17T13:31:14Z

Warning

This is a public repository, ensure not to disclose:

personal data beyond what is necessary for interacting with this pull request, nor
business confidential information, such as customer names.

What kind of PR is this?

Required: Mark one of the following that is applicable:

Optional: Mark one or more of the following that are applicable:

Important

Breaking changes should be marked kind/admin-change or kind/dev-change depending on type
Critical security fixes should be marked with kind/security

kind/admin-change
kind/dev-change
kind/security
[kind/adr](set-me)

What does this PR do / why do we need this PR?

Ensure that ck8s upgrade prepare is run before ck8s upgrade apply
Ensure that ck8s upgrade apply is run before ck8s apply TODO

digraph "ck8s upgrade" {
	init [label="ck8s init"]
	edit [label="edit config"]
	apply [label="ck8s apply"]
	uprep [label="ck8s upgrade prepare"]
	upgrade [label="ck8s upgrade apply"]

	init -> edit -> apply -> edit
	edit -> uprep -> upgrade -> apply

# init -> uprep [label="NO!"]
# edit -> init [label="NO!"]
}

This uses a pair of ConfigMaps to keep track of progress during apps upgrades
~~in order to enforce order (prepare first, then apply, and for the same version)~~
and atomicity ~~(only allow prepare once)~~ per step.

apps-meta stores the current version of apps
- .data.version the version number itself
apps-upgrade stores details about an in-progress upgrade, and is deleted to signify a completed migration
- ~~.data.prepare records that ck8s upgrade ... prepare has been started~~
- ~~.data.prepared records that ck8s upgrade ... prepare has completed~~
- .data.prepared records that ck8s upgrade ... apply has started
- .data.last_step records the last snippet run by ck8s upgrade ...

Fixes [3] Add applied apps version to cluster state #2315

Information to reviewers

This is intended to behave as a state machine, going from start ~~to prepare~~ to
apply (for each step) to done, at which point the upgraded-to version is
recorded.

How to test: I applied the changes on top of the release-0.42 branch and upgraded from 0.41.x to 0.42.1, then again to 0.43 by creating a local tag (not to be published).

Checklist

aarnq · 2025-01-22T12:13:27Z

What is the motivation for keeping the phase of prepare in-cluster?

Isn't it just critical to track the apply phase?

Zash · 2025-01-22T13:20:06Z

What is the motivation for keeping the phase of prepare in-cluster?

Seemed a natural starting point for these checks.

Isn't it just critical to track the apply phase?

Yes.

Zash · 2025-01-22T13:31:50Z

#2315 (comment) is probably where I got that from, to track the whole upgrade process from the start.

simonklb

This LGTM!

Regarding the tracking of the prepare phase, I like that we are able to make sure that the config has been properly updated and get rid of the ck8sVersion from the config on the other hand if there is no Kubernetes cluster to verify against then we still have to rely on something external.

No strong opinion here though. It doesn't hurt but might add some extra complexity.

bin/upgrade.bash

simonklb · 2025-01-23T12:17:50Z

bin/upgrade.bash

+    prepared_version="$(get_prepared_version sc || true)"
+    if [ -z "${prepared_version}" ]; then
+      log_fatal "'prepare' step does not appear to have been run, do so first"
+    fi
+
+    if [[ "${prepared_version}" != "${CK8S_TARGET_VERSION}" ]]; then
+      log_fatal "'prepare' step in sc appears to have been run for version ${prepared_version}, not ${CK8S_TARGET_VERSION}"
+    fi


suggestion: Extract into a separate function and reuse for both sc and wc.

scripts/migration/lib.sh

simonklb

Still approved after removing the prepare phase tracking and keep relying on ck8sVersion in the config file. However, I still do think that getting rid of the ck8sVersion config value and only rely on the apps-meta config map would have been neat.

aarnq · 2025-01-24T09:48:59Z

Regarding the tracking of the prepare phase, I like that we are able to make sure that the config has been properly updated and get rid of the ck8sVersion from the config on the other hand if there is no Kubernetes cluster to verify against then we still have to rely on something external.

But if it is in cluster it doesn't say anything, as the config isn't attached to it. Then you must apply some sort of checksumming to assert that it is that config you mean.
Or we have to move the entire config in cluster.
Else if someone has done prepare, then someone else without that config can run apply.

bin/upgrade.bash

aarnq · 2025-01-24T09:52:01Z

bin/upgrade.bash

+  # Create a configmap. This fails if done twice.
+  if [[ "${CK8S_CLUSTER:-}" =~ ^(sc|both)$ ]]; then
+    if ! record_migration_begin sc; then
+      log_fatal "prepare already started in sc (kubectl delete -n kube-system configmap apps-upgrade to try again)"


Suggested change

log_fatal "prepare already started in sc (kubectl delete -n kube-system configmap apps-upgrade to try again)"

log_fatal "apply already started in sc (kubectl delete -n kube-system configmap apps-upgrade to try again)"

Also, would it make sense to add a ck8s upgrade <prefix> unlock-apply to do this?
(With some warning+confirm that you probably shouldn't unless you know what you are doing.)

I pushed a thing with --force, would a separate sub-command be better?

I think so, because else some people might just always run with --force and that would defeat the purpose.

Pushed a ck8s reset-upgrade command to bypass some of the argument related logic that ck8s upgrade ... does. How's it look?

bin/upgrade.bash

simonklb · 2025-01-24T15:56:21Z

Regarding the tracking of the prepare phase, I like that we are able to make sure that the config has been properly updated and get rid of the ck8sVersion from the config on the other hand if there is no Kubernetes cluster to verify against then we still have to rely on something external.

But if it is in cluster it doesn't say anything, as the config isn't attached to it. Then you must apply some sort of checksumming to assert that it is that config you mean. Or we have to move the entire config in cluster. Else if someone has done prepare, then someone else without that config can run apply.

Yes you're right, the inherent issue of working with distributed configuration files still applies. At least now we can make sure that the cluster has been migrated to a new version which is a big win.

I think my main concern with the ck8sVersion config is that it leads to a false sense of security and a bad habit of thinking that just running ck8s init and you're good to go. I've seen it countless of times where the configuration validation succeeds and apply breaks because the migration was never run.

scripts/migration/lib.sh

viktor-f · 2025-01-31T08:03:24Z

bin/ck8s

+version)
+  shift
+  case "${1:-both}" in
+  both)
+    log_info "sc version:" "$(get_version "sc")"
+    log_info "wc version:" "$(get_version "wc")"
+    ;;
+  sc | wc)
+    log_info "${1} version:" "$(get_version "${1}")"
+    ;;
+  esac
+  ;;


Nice to have this command. I think it could be even better if it also printed the version in the config, so you can easily see if they are the same. Does that sound good to you as well?

Sounds good.

Should just be one version in defaults/common-config.yaml, or are there cases where wc and sc may differ?

I think that the only logical place to override the version is in common. It might be technically possible to have it in sc or wc, but that is not intended.

So just comparing with common should be fine, but doing the regular merge of sc with common and wc with common might be safest.

viktor-f · 2025-01-31T08:20:19Z

scripts/migration/lib.sh

+  cluster_version="$(get_apps_version "${1}" >/dev/null 2>&1 || true)"
+  if [ -n "${cluster_version}" ]; then
+    log_info "Currently running ${cluster_version} according to cluster"
+    if [ "${common_override}" != "null" ] && [ "${cluster_version%.*}" != "${common_override}" ]; then
+      log_warn "Version mismatch, cluster ${cluster_version}, config ${common_override}"
+    fi
+  fi
+


Question: Could you elaborate on why we have this block? If I understand it correctly then it will warn if there is a version override in the common config and that is not the same as the applied version. But it will not exit and is not part of a prompt, so it would just be informational when looking at what is happening.

Could it perhaps make sense to merge this check into the prompt above it? Then you would still get this information, but it can be used as more context to decide if you want to abort or not. What do you think of that?

Hm, probably missed that it's the override config it checks against. The intent was probably to check the pre-upgrade config version (defaults/common-config.yaml → .global.ck8sVersion) against the in-cluster version (if available). E.g. if you try to upgrade to v0.43 and your config says v0.42 but your cluster says v0.41 or similar. This gets complicated and hard to think about since the config is changed by ck8s init.

Maybe just drop this, or what do you think is the way forward?

I'm not sure what the best way forward is.
Merging it with the block above might still be best, so that you get a chance to abort. Because I think that there is some value to having this check.

I moved the block above the abort check but removed the mismatch check since it would be comparing the to-be-upgraded-to version recorded by ck8s init to the currently running version, which wasn't the intent and it would always mismatch unless try to migrate a second time.

aarnq · 2025-02-12T09:06:23Z

Regarding the tracking of the prepare phase, I like that we are able to make sure that the config has been properly updated and get rid of the ck8sVersion from the config on the other hand if there is no Kubernetes cluster to verify against then we still have to rely on something external.

But if it is in cluster it doesn't say anything, as the config isn't attached to it. Then you must apply some sort of checksumming to assert that it is that config you mean. Or we have to move the entire config in cluster. Else if someone has done prepare, then someone else without that config can run apply.

Yes you're right, the inherent issue of working with distributed configuration files still applies. At least now we can make sure that the cluster has been migrated to a new version which is a big win.

I think my main concern with the ck8sVersion config is that it leads to a false sense of security and a bad habit of thinking that just running ck8s init and you're good to go. I've seen it countless of times where the configuration validation succeeds and apply breaks because the migration was never run.

If that is the concern then we should change how ck8sVersion is handled, because I do agree that this is an issue because init is not supposed to be the prepare step.

This is meant to atomically record the state transition into a migration (by creation) and then out of it (by deletion). The general apps version is recorded at the end, and compared to the config version on next upgrade.

This reverts commit 53ac173. Decided to go with the way where it starts tracking at the prepare stage.

should apply have one too?

This reverts commit b90a9b3.

separate command to discourage always running with --force

going trough ck8s upgrade invokes a number of behaviors that are not needed

did not mean to commit this

one of the config checks were wrong, better to only have one

it's going to mismatch every you upgrade because it compares the version you have with the version you are upgrading to since ck8s init changes it

Zash · 2025-02-19T13:54:47Z

I think my main concern with the ck8sVersion config is that it leads to a false sense of security and a bad habit of thinking that just running ck8s init and you're good to go. I've seen it countless of times where the configuration validation succeeds and apply breaks because the migration was never run.

If that is the concern then we should change how ck8sVersion is handled, because I do agree that this is an issue because init is not supposed to be the prepare step.

Would it be enough to add something here that checks in the apply steps that config-version == cluster version, or do we need to rethink migrations from ck8s init onwards?

aarnq · 2025-02-24T07:16:51Z

I think my main concern with the ck8sVersion config is that it leads to a false sense of security and a bad habit of thinking that just running ck8s init and you're good to go. I've seen it countless of times where the configuration validation succeeds and apply breaks because the migration was never run.

If that is the concern then we should change how ck8sVersion is handled, because I do agree that this is an issue because init is not supposed to be the prepare step.

Would it be enough to add something here that checks in the apply steps that config-version == cluster version, or do we need to rethink migrations from ck8s init onwards?

Or just move the update of ck8sVersion to be done as a final step when upgrade prepare completes.
Init should still be able to set it in a new config though.

aarnq · 2025-02-24T07:19:41Z

bin/ck8s

@@ -35,7 +35,9 @@ usage() {
  echo "  update-ips <wc|sc|both> <apply|dry-run>         Automatically fetches and applies the IPs for network policies" 1>&2
  echo "  upgrade <wc|sc|both> <vX.Y> apply               runs all apply steps upgrading the environment" 1>&2
  echo "  upgrade <wc|sc|both> <vX.Y> prepare             runs all prepare steps upgrading the configuration" 1>&2
+  echo "  unlock-upgrade <wc|sc|both>                     reset a migration to allow trying again" 1>&2


Nit, could this be upgrade-unlock instead? Makes similar commands stay together.
Or perhaps upgrade <wc|sc|both> unlock to be more similar in structure to the rest of the upgrade commands.

Nit, could this be upgrade-unlock instead?

Sure

Or perhaps upgrade <wc|sc|both> unlock

Finding a good way to bypass the existing path that requires a version number was a bit awkward.

Zash · 2025-02-24T12:19:49Z

bin/ck8s

+  [[ "${2}" =~ ^(wc|sc|both)$ ]] || usage
+  export CK8S_CLUSTER="${2}"
+  "${here}/upgrade.bash" "unlock"
+  ;;


echo " upgrade <wc|sc|both> unlock reset a migration to allow trying again" 1>&2 #### upgrade) [[ "${2}" =~ ^(wc|sc|both)$ ]] || usage if [[ "${3}" == "unlock" ]]; then export CK8S_CLUSTER="${2}" "${here}/upgrade.bash" "unlock" exit 0 fi [[ "${3}" =~ ^(v[0-9]+\.[0-9]+)$ ]] || usage [[ "${4}" =~ ^(prepare|apply)$ ]] || usage check_tools export CK8S_CLUSTER="${2}" "${here}/upgrade.bash" "${3}" "${4}" ;;

I guess this would work, tho feels a bit awkward to jump out in the middle here. @aarnq ?

Depends on how you write it, you don't have to jump in the middle.

[[ "${2}" =~ ^(wc|sc|both)$ ]] || usage [[ "${3}" =~ ^(v[0-9]+\.[0-9]+|unlock)$ ]] || usage if [[ "${3}" != "unlock" ]]; then [[ "${4}" =~ ^(prepare|apply)$ ]] || usage fi check_tools export CK8S_CLUSTER="${2}" "${here}/upgrade.bash" "${3}" "${4:-}"

Zash force-pushed the ka/in-cluster-version branch 8 times, most recently from c3dca19 to 21653d6 Compare January 22, 2025 10:58

Zash marked this pull request as ready for review January 22, 2025 10:58

Zash requested review from a team as code owners January 22, 2025 10:58

simonklb approved these changes Jan 23, 2025

View reviewed changes

Zash requested a review from simonklb January 23, 2025 13:06

Zash force-pushed the ka/in-cluster-version branch from 3162bf7 to 53ac173 Compare January 23, 2025 13:08

simonklb approved these changes Jan 23, 2025

View reviewed changes

Zash commented Jan 24, 2025

View reviewed changes

bin/upgrade.bash Show resolved Hide resolved

aarnq reviewed Jan 24, 2025

View reviewed changes

Zash requested review from viktor-f and Elias-elastisys January 27, 2025 13:54

simonklb reviewed Jan 30, 2025

View reviewed changes

scripts/migration/lib.sh Outdated Show resolved Hide resolved

viktor-f reviewed Jan 31, 2025

View reviewed changes

Zash added 3 commits February 19, 2025 14:28

migration: Track apps version and migration steps

ea62b43

This is meant to atomically record the state transition into a migration (by creation) and then out of it (by deletion). The general apps version is recorded at the end, and compared to the config version on next upgrade.

migration: only track the 'apply' step

1092329

Revert "migration: only track the 'apply' step"

259ddc1

This reverts commit 53ac173. Decided to go with the way where it starts tracking at the prepare stage.

Zash added 21 commits February 19, 2025 14:28

add a --force retry upgrade

10061a1

should apply have one too?

bin: Include version from config

ec88579

don't discard kubectl stderr

c1bc592

Revert "add a --force retry upgrade"

35a8933

This reverts commit b90a9b3.

migration unlock

2db025a

separate command to discourage always running with --force

fixup

5ccc5a3

unlock fixup

3a9c817

stderr fixup

06c1df2

fixup

71ab7ef

take three of upgrade unlock

f7202c1

going trough ck8s upgrade invokes a number of behaviors that are not needed

fixup

267c82b

fixup

90427fb

fixup stdout

7edf075

fixup

432da13

record beginning of upgrade-apply

7f0738b

undo

2a6269c

did not mean to commit this

bin: restructure ck8s version

dd3b6d9

one of the config checks were wrong, better to only have one

test

d65c8f0

expand tests

0ea728f

move and use the parsed and sc/wc aware version

bfcb372

remove version mismatch check

4150a95

it's going to mismatch every you upgrade because it compares the version you have with the version you are upgrading to since ck8s init changes it

Zash force-pushed the ka/in-cluster-version branch from 608960d to 4150a95 Compare February 19, 2025 13:28

aarnq reviewed Feb 24, 2025

View reviewed changes

Zash added 2 commits February 24, 2025 13:11

factor out into function

1877e26

rename unlock-upgrade -> upgrade-unlock per suggestion

28e93bd

Zash commented Feb 24, 2025

View reviewed changes

fixup

d5327ca

Zash marked this pull request as draft February 24, 2025 13:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Incremental version checks during migration #2399

Incremental version checks during migration #2399

Zash commented Jan 17, 2025 •

edited

Loading

aarnq commented Jan 22, 2025 •

edited

Loading

Zash commented Jan 22, 2025

Zash commented Jan 22, 2025

simonklb left a comment

simonklb Jan 23, 2025

Zash Feb 24, 2025

simonklb left a comment

aarnq commented Jan 24, 2025

aarnq Jan 24, 2025

Zash Jan 30, 2025

aarnq Jan 31, 2025

Zash Feb 11, 2025

simonklb commented Jan 24, 2025 •

edited

Loading

viktor-f Jan 31, 2025

Zash Feb 10, 2025

viktor-f Feb 12, 2025

viktor-f Jan 31, 2025

Zash Feb 11, 2025

viktor-f Feb 12, 2025

Zash Feb 19, 2025

aarnq commented Feb 12, 2025

Zash commented Feb 19, 2025

aarnq commented Feb 24, 2025

aarnq Feb 24, 2025

Zash Feb 24, 2025

Zash Feb 24, 2025

aarnq Feb 24, 2025

	log_fatal "prepare already started in sc (kubectl delete -n kube-system configmap apps-upgrade to try again)"
	log_fatal "apply already started in sc (kubectl delete -n kube-system configmap apps-upgrade to try again)"

Incremental version checks during migration #2399

Are you sure you want to change the base?

Incremental version checks during migration #2399

Conversation

Zash commented Jan 17, 2025 • edited Loading

What kind of PR is this?

What does this PR do / why do we need this PR?

Information to reviewers

Checklist

aarnq commented Jan 22, 2025 • edited Loading

Zash commented Jan 22, 2025

Zash commented Jan 22, 2025

simonklb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonklb left a comment

Choose a reason for hiding this comment

aarnq commented Jan 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonklb commented Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarnq commented Feb 12, 2025

Zash commented Feb 19, 2025

aarnq commented Feb 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zash commented Jan 17, 2025 •

edited

Loading

aarnq commented Jan 22, 2025 •

edited

Loading

simonklb commented Jan 24, 2025 •

edited

Loading