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

[DPE-2242] Add kafka upgrade #120

Merged
merged 14 commits into from
Aug 21, 2023
4 changes: 2 additions & 2 deletions lib/charms/data_platform_libs/v0/upgrade.py
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ def restart(self, event) -> None:

# Increment this PATCH version before using `charmcraft publish-lib` or reset
# to 0 if you are raising the major API version
LIBPATCH = 9
LIBPATCH = 10

PYDEPS = ["pydantic>=1.10,<2"]

Expand Down Expand Up @@ -817,7 +817,7 @@ def idle(self) -> Optional[bool]:
Returns:
True if all application units in idle state. Otherwise False
"""
return self.cluster_state == "idle"
return set(self.unit_states) == {"idle"}

@abstractmethod
def pre_upgrade_check(self) -> None:
Expand Down
14 changes: 13 additions & 1 deletion src/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,18 @@ def log_dirs(self) -> str:
[os.fspath(storage.location) for storage in self.charm.model.storages["data"]]
)

@property
def inter_broker_protocol_version(self) -> str:
"""Creates the protocol version from the kafka version.

Returns:
string with the `major.minor` version
zmraul marked this conversation as resolved.
Show resolved Hide resolved
"""
# Remove patch number from full vervion.
major_minor = self.charm.upgrade.current_version.split(".", maxsplit=2)
major_minor.pop()
return ".".join(major_minor)
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: Could we use indexing here?

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Then we could do major_minor[:2] no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

major_minor is a list though right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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


@property
def rack_properties(self) -> List[str]:
"""Builds all properties related to rack awareness configuration.
Expand Down Expand Up @@ -512,7 +524,7 @@ def server_properties(self) -> List[str]:
f"listeners={','.join(listeners_repr)}",
f"advertised.listeners={','.join(advertised_listeners)}",
f"inter.broker.listener.name={self.internal_listener.name}",
f"inter.broker.protocol.version={self.charm.upgrade.current_version}",
f"inter.broker.protocol.version={self.inter_broker_protocol_version}",
]
+ self.config_properties
+ self.scram_properties
Expand Down
22 changes: 22 additions & 0 deletions tests/unit/test_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from literals import (
ADMIN_USER,
CHARM_KEY,
DEPENDENCIES,
INTER_BROKER_USER,
INTERNAL_USERS,
JMX_EXPORTER_PORT,
Expand Down Expand Up @@ -402,6 +403,27 @@ def test_rack_properties(harness: Harness):
assert "broker.rack=gondor-west" in harness.charm.kafka_config.server_properties


def test_inter_broker_protocol_version(harness):
"""Checks that rack properties are added to server properties."""
harness.add_relation(PEER, CHARM_KEY)
zk_relation_id = harness.add_relation(ZK, CHARM_KEY)
harness.update_relation_data(
zk_relation_id,
harness.charm.app.name,
{
"chroot": "/kafka",
"username": "moria",
"password": "mellon",
"endpoints": "1.1.1.1,2.2.2.2",
"uris": "1.1.1.1:2181/kafka,2.2.2.2:2181/kafka",
"tls": "disabled",
},
)
assert len(DEPENDENCIES["kafka_service"]["version"].split(".")) == 3

assert f"inter.broker.protocol.version=3.3" in harness.charm.kafka_config.server_properties


def test_super_users(harness):
"""Checks super-users property is updated for new admin clients."""
peer_relation_id = harness.add_relation(PEER, CHARM_KEY)
Expand Down
Loading