[DPE-2242] Add kafka upgrade #120

zmraul · 2023-07-24T12:33:48Z

In-place upgrade for Kafka. Based of ZK upgrade.

Changes from ZK upgrade

Kafka charm now needs to check for Zookeeper version on _on_upgrade_granted function. If the requirement is not met, the unit will be set to failed.
- This is done through an API call to srvr command on the zookeeper leader.
- Uses the verify_requirements function from the upgrade lib.

Manual testing

This is the same as in zookeeper, but with the extra relation:

# deploy Zookeeper
juju deploy zookeeper --channel 3/edge

# deploying Kafka 3.3.1 from snap revision 16 
cd /tmp && git clone [email protected]:canonical/kafka-operator && cd kafka-operator
git switch feature/dpe-2242-in-place-upgrade
charmcraft pack && juju deploy ./*.charm -n 3 --force

juju relate kafka zookeeper
# wait for active/idle + stable

# runs checks and ensures quorum leader upgrades last
juju run-action kafka/leader pre-upgrade-check --wait

# modifies charm snap version to Kafka 3.4 from snap revision 17 
sed -i 's/CHARMED_KAFKA_SNAP_REVISION \= 16/CHARMED_KAFKA_SNAP_REVISION \= 17/' src/literals.py
charmcraft pack

# upgrades the charm
juju refresh kafka --path ./*.charm

# wait for active/idle
# checks that all units are up

# checks that the new snap version is being used - revision 17
juju ssh kafka/0 sudo -i 'charmed-kafka.configs --version'

deusebio

Code-wise looks good! Just some small points/nipticks.

I'm actually testing this right now, but we can probably merge this PR even if I did not finish to test, as it may take also part of next week

src/upgrade.py

marcoppenheimer · 2023-08-14T13:52:13Z

src/literals.py

+DEPENDENCIES = {
+    "service": {
+        "dependencies": {"zookeeper": ">3"},
+        "name": "kafka",
+        "upgrade_supported": ">3",
+        "version": "3.3",
+    },
+}


todo: I think here - to be safe - we should use a carets because we don't know what major-level version changes will do to compatibility in the future, and the specific version of ZooKeeper shipped with this Kafka version, AND the specific version of Kafka.

So that would be:

zookeeper ^3.6 (i.e support up to 4.0)

version 3.3.2

Ah, good catch. This was an oversight from testing probably. Thanks!

Having ^3.6 means that exactly 3.6 would fail no? So upgrading back to 3.6 in case of an error would fail requirements.
Saying because I ran the unit tests with mock on zk version:

patch( "upgrade.KafkaUpgrade.zookeeper_current_version", new_callable=PropertyMock, return_value="3.6", ) # leads to the unit test failing: ERROR upgrade:upgrade.py:86 Current ZooKeeper version 3.6 does not meet requirement ^3.6

zookeeper ^3.6 (i.e support up to 4.0)

I'm not sure we should allow zookeeper to be updated to 3.8... Didn't we try this out already and failed to get it working?

@zmraul That might be a bug in the requirement verification!

Gimme a moment to look at it.

On this one

"version": "3.3"

If I have understood the code correctly, this number will go all the way down to be specified in the protocol version field of the properties file, which should only be major.minor. We should either rollback this change or change the logic in the properties such that we strip patch number in the protocol version

You are right, I forgot about this. I'm not sure it is a problem for the properties. The Kafka docs don't say "use exactly major.minor". And they don't advise against having the full version either. I'll check manually to see if there is a problem with it.

I'm not sure I have ever come across any "major.minor.patch" specification for the protocol version. Can we check if we find any (reliable) reference about using major.minor.patch?

@zmraul - That bug has been resolved and merged - canonical/data-platform-libs#86

@deusebio @marcoppenheimer I think both issues have been addressed on the last commit: 4d8172b
Re-requesting review just for that one.

src/literals.py

src/upgrade.py

lib/charms/data_platform_libs/v0/upgrade.py

src/upgrade.py

marcoppenheimer

Great stuff!

src/config.py

marcoppenheimer · 2023-08-18T14:05:18Z

src/config.py

+        major_minor = self.charm.upgrade.current_version.split(".", maxsplit=2)
+        major_minor.pop()
+        return ".".join(major_minor)


nitpick: Could we use indexing here?

major_minor = self.charm.upgrade.current_version.split(".") return ".".join(major_minor[:-1])

I believe it safer the one proposed by Raul as we should rather stick to major.minor, if we ever had a 3.2.1.1, using the other would result in a major.minor.patch version, which we don't want

Then we could do major_minor[:2] no?

Indexing is not so clear directly on the string, because version might be 10.11.2 one day, so [:-1] or [:2] would not work at some point

major_minor is a list though right?

Ah yes sorry. I was thinking about using indexing directly on the string.

deusebio · 2023-08-18T14:14:38Z

LGTM

* add kafka upgrade * add zk check * update client lib

zmraul requested a review from marcoppenheimer July 24, 2023 12:33

zmraul added 2 commits August 4, 2023 10:20

add kafka upgrade

246f82b

add zk check

f8f0fb5

zmraul force-pushed the feature/dpe-2242-in-place-upgrade branch from 646e793 to f8f0fb5 Compare August 4, 2023 09:00

zmraul added 2 commits August 4, 2023 11:02

fix snap revision

8855276

fix unit tests

5606752

zmraul changed the base branch from main to feature/upgrade August 8, 2023 07:51

Merge branch 'feature/upgrade' into feature/dpe-2242-in-place-upgrade

4ba38d2

zmraul marked this pull request as ready for review August 8, 2023 07:53

zmraul added 2 commits August 9, 2023 10:39

add unit tests

428d8b5

update client lib

5d7d3a4

zmraul requested a review from deusebio August 10, 2023 08:55

deusebio approved these changes Aug 10, 2023

View reviewed changes

src/upgrade.py Outdated Show resolved Hide resolved

src/upgrade.py Outdated Show resolved Hide resolved

src/upgrade.py Outdated Show resolved Hide resolved

add pr feedback

fc01c03

marcoppenheimer requested changes Aug 14, 2023

View reviewed changes

add pr feedback

0cc774c

zmraul requested a review from marcoppenheimer August 14, 2023 19:07

marcoppenheimer approved these changes Aug 14, 2023

View reviewed changes

zmraul added 3 commits August 16, 2023 11:27

update upgrade lib

11687f7

fix versioning config string

4d8172b

fix lint

14153e8

zmraul requested review from marcoppenheimer and deusebio August 18, 2023 13:53

marcoppenheimer approved these changes Aug 18, 2023

View reviewed changes

deusebio approved these changes Aug 18, 2023

View reviewed changes

zmraul added 2 commits August 18, 2023 17:02

fix docstring

d8a3b77

improve version handling

82efb8f

zmraul merged commit f86b178 into feature/upgrade Aug 21, 2023
6 checks passed

zmraul deleted the feature/dpe-2242-in-place-upgrade branch August 21, 2023 07:29

zmraul added a commit that referenced this pull request Aug 21, 2023

[DPE-2242] Add kafka upgrade (#120) (#127)

ea09062

* add kafka upgrade * add zk check * update client lib

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DPE-2242] Add kafka upgrade #120

[DPE-2242] Add kafka upgrade #120

zmraul commented Jul 24, 2023 •

edited

Loading

deusebio left a comment

marcoppenheimer Aug 14, 2023

zmraul Aug 14, 2023

zmraul Aug 14, 2023 •

edited

Loading

deusebio Aug 14, 2023

marcoppenheimer Aug 14, 2023

deusebio Aug 14, 2023 •

edited

Loading

zmraul Aug 14, 2023 •

edited

Loading

deusebio Aug 14, 2023

marcoppenheimer Aug 15, 2023

zmraul Aug 18, 2023

marcoppenheimer left a comment

marcoppenheimer Aug 18, 2023

deusebio Aug 18, 2023

marcoppenheimer Aug 18, 2023

zmraul Aug 18, 2023

marcoppenheimer Aug 18, 2023

zmraul Aug 21, 2023

deusebio commented Aug 18, 2023

[DPE-2242] Add kafka upgrade #120

[DPE-2242] Add kafka upgrade #120

Conversation

zmraul commented Jul 24, 2023 • edited Loading

Changes from ZK upgrade

Manual testing

deusebio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zmraul Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deusebio Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

zmraul Aug 14, 2023 • 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

marcoppenheimer left a 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

Choose a reason for hiding this comment

deusebio commented Aug 18, 2023

zmraul commented Jul 24, 2023 •

edited

Loading

zmraul Aug 14, 2023 •

edited

Loading

deusebio Aug 14, 2023 •

edited

Loading

zmraul Aug 14, 2023 •

edited

Loading