From 58ded3184832c7162056f3ebfafd01c6267c74e2 Mon Sep 17 00:00:00 2001 From: NohaIhab Date: Tue, 10 Sep 2024 13:21:27 +0000 Subject: [PATCH 01/10] add configurable replicas to gateway deployment --- charms/istio-gateway/config.yaml | 4 ++++ charms/istio-gateway/src/charm.py | 1 + charms/istio-gateway/src/manifest.yaml | 12 ++++++++++++ 3 files changed, 17 insertions(+) diff --git a/charms/istio-gateway/config.yaml b/charms/istio-gateway/config.yaml index 2dadadeb..54fb1aa1 100644 --- a/charms/istio-gateway/config.yaml +++ b/charms/istio-gateway/config.yaml @@ -12,3 +12,7 @@ options: default: 'docker.io/istio/proxyv2:1.22.0' description: Istio Proxy image type: string + replicas: + default: 1 + description: Number of replicas for the istio-ingressgateway pods + type: int diff --git a/charms/istio-gateway/src/charm.py b/charms/istio-gateway/src/charm.py index 3c258b01..22544233 100755 --- a/charms/istio-gateway/src/charm.py +++ b/charms/istio-gateway/src/charm.py @@ -94,6 +94,7 @@ def start(self, event): pilot_host=pilot["service-name"], pilot_port=pilot["service-port"], gateway_service_type=self.model.config["gateway_service_type"], + replicas=self.model.config["replicas"], ) for obj in codecs.load_all_yaml(rendered): diff --git a/charms/istio-gateway/src/manifest.yaml b/charms/istio-gateway/src/manifest.yaml index 5fb11c6c..5bbab2b9 100644 --- a/charms/istio-gateway/src/manifest.yaml +++ b/charms/istio-gateway/src/manifest.yaml @@ -25,6 +25,18 @@ metadata: name: istio-{{ kind }}gateway-workload namespace: {{ namespace }} spec: + replicas: {{ replicas }} +{% if replicas > 1 %} + affinity: + podAntiAffinity: + requiredDuringSchedulingIgnoredDuringExecution: + - labelSelector: + matchExpressions: + - key: app + operator: In + values: + - istio-{{ kind }}gateway +{% endif %} selector: matchLabels: app: istio-{{ kind }}gateway From 556c79d442cdb3633d7a564ed2f1a5aed7486be9 Mon Sep 17 00:00:00 2001 From: NohaIhab Date: Tue, 10 Sep 2024 21:45:02 +0000 Subject: [PATCH 02/10] fix manifest and add topologyKey --- charms/istio-gateway/src/manifest.yaml | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/charms/istio-gateway/src/manifest.yaml b/charms/istio-gateway/src/manifest.yaml index 5bbab2b9..af8b2581 100644 --- a/charms/istio-gateway/src/manifest.yaml +++ b/charms/istio-gateway/src/manifest.yaml @@ -26,17 +26,6 @@ metadata: namespace: {{ namespace }} spec: replicas: {{ replicas }} -{% if replicas > 1 %} - affinity: - podAntiAffinity: - requiredDuringSchedulingIgnoredDuringExecution: - - labelSelector: - matchExpressions: - - key: app - operator: In - values: - - istio-{{ kind }}gateway -{% endif %} selector: matchLabels: app: istio-{{ kind }}gateway @@ -66,6 +55,15 @@ spec: sidecar.istio.io/inject: "false" spec: affinity: + podAntiAffinity: + requiredDuringSchedulingIgnoredDuringExecution: + - labelSelector: + matchExpressions: + - key: app + operator: In + values: + - istio-{{ kind }}gateway + topologyKey: kubernetes.io/hostname nodeAffinity: preferredDuringSchedulingIgnoredDuringExecution: null requiredDuringSchedulingIgnoredDuringExecution: null From 74e6be9273980f12f0ef4f4a03c4bb9dbee27081 Mon Sep 17 00:00:00 2001 From: NohaIhab Date: Tue, 10 Sep 2024 21:45:28 +0000 Subject: [PATCH 03/10] update unit tests data --- .../istio-gateway/tests/unit/data/egress-example.yaml | 10 ++++++++++ .../istio-gateway/tests/unit/data/ingress-example.yaml | 10 ++++++++++ 2 files changed, 20 insertions(+) diff --git a/charms/istio-gateway/tests/unit/data/egress-example.yaml b/charms/istio-gateway/tests/unit/data/egress-example.yaml index a60b4005..4c6a602c 100644 --- a/charms/istio-gateway/tests/unit/data/egress-example.yaml +++ b/charms/istio-gateway/tests/unit/data/egress-example.yaml @@ -24,6 +24,7 @@ metadata: name: istio-egressgateway-workload namespace: None spec: + replicas: 1 selector: matchLabels: app: istio-egressgateway @@ -53,6 +54,15 @@ spec: sidecar.istio.io/inject: "false" spec: affinity: + podAntiAffinity: + requiredDuringSchedulingIgnoredDuringExecution: + - labelSelector: + matchExpressions: + - key: app + operator: In + values: + - istio-egressgateway + topologyKey: kubernetes.io/hostname nodeAffinity: preferredDuringSchedulingIgnoredDuringExecution: null requiredDuringSchedulingIgnoredDuringExecution: null diff --git a/charms/istio-gateway/tests/unit/data/ingress-example.yaml b/charms/istio-gateway/tests/unit/data/ingress-example.yaml index 56b30b0f..2e83fb29 100644 --- a/charms/istio-gateway/tests/unit/data/ingress-example.yaml +++ b/charms/istio-gateway/tests/unit/data/ingress-example.yaml @@ -24,6 +24,7 @@ metadata: name: istio-ingressgateway-workload namespace: None spec: + replicas: 1 selector: matchLabels: app: istio-ingressgateway @@ -53,6 +54,15 @@ spec: sidecar.istio.io/inject: "false" spec: affinity: + podAntiAffinity: + requiredDuringSchedulingIgnoredDuringExecution: + - labelSelector: + matchExpressions: + - key: app + operator: In + values: + - istio-ingressgateway + topologyKey: kubernetes.io/hostname nodeAffinity: preferredDuringSchedulingIgnoredDuringExecution: null requiredDuringSchedulingIgnoredDuringExecution: null From 941a60a722d35b62e2ef7e76f214a71c5edb0cd2 Mon Sep 17 00:00:00 2001 From: NohaIhab Date: Tue, 10 Sep 2024 21:46:08 +0000 Subject: [PATCH 04/10] render with replicas on remove --- charms/istio-gateway/src/charm.py | 1 + 1 file changed, 1 insertion(+) diff --git a/charms/istio-gateway/src/charm.py b/charms/istio-gateway/src/charm.py index 22544233..b1ec9199 100755 --- a/charms/istio-gateway/src/charm.py +++ b/charms/istio-gateway/src/charm.py @@ -114,6 +114,7 @@ def remove(self, event): proxy_image=self.model.config["proxy-image"], pilot_host="foo", pilot_port="foo", + replicas=self.model.config["replicas"], ) try: From be40474241c5bb345a45711c8f1c366595896fe0 Mon Sep 17 00:00:00 2001 From: NohaIhab Date: Tue, 10 Sep 2024 21:46:25 +0000 Subject: [PATCH 05/10] add integration test for replicas --- tests/test_bundle.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/test_bundle.py b/tests/test_bundle.py index dde7bf27..d7adb60f 100644 --- a/tests/test_bundle.py +++ b/tests/test_bundle.py @@ -15,6 +15,7 @@ create_namespaced_resource, load_in_cluster_generic_resources, ) +from lightkube.resources.apps_v1 import Deployment from pytest_operator.plugin import OpsTest log = logging.getLogger(__name__) @@ -332,6 +333,27 @@ async def test_disable_ingress_auth(ops_test: OpsTest): ) +async def test_gateway_replicas_config(ops_test: OpsTest): + """Test the replicas config takes effect in the Deployment.""" + + replicas_value = "2" + await ops_test.model.applications[ISTIO_GATEWAY_APP_NAME].set_config( + {"replicas": replicas_value} + ) + await ops_test.model.wait_for_idle(apps=[ISTIO_GATEWAY_APP_NAME], status="active", timeout=300) + + client = lightkube.Client() + gateway_deployment = client.get( + Deployment, name="istio-ingressgateway-workload", namespace=ops_test.model_name + ) + + assert gateway_deployment.spec.replicas == int(replicas_value) + # TODO: assert antiaffinity contents + # assert gateway_deployment.spec.template.spec.affinity == "expected" + + # TODO: assert second pod is not scheduled + + async def test_charms_removal(ops_test: OpsTest): """Test the istio-operators can be removed without errors.""" # NOTE: the istio-gateway charm has to be removed before istio-pilot since From a75bb9613ae558a7f5af8acf45fec17c3e0f86f0 Mon Sep 17 00:00:00 2001 From: NohaIhab Date: Thu, 12 Sep 2024 09:28:30 +0000 Subject: [PATCH 06/10] add asserts to integration test --- tests/test_bundle.py | 49 +++++++++++++++++++++++++++++++++++++++----- 1 file changed, 44 insertions(+), 5 deletions(-) diff --git a/tests/test_bundle.py b/tests/test_bundle.py index d7adb60f..bfcf00fe 100644 --- a/tests/test_bundle.py +++ b/tests/test_bundle.py @@ -16,6 +16,7 @@ load_in_cluster_generic_resources, ) from lightkube.resources.apps_v1 import Deployment +from lightkube.resources.core_v1 import Pod from pytest_operator.plugin import OpsTest log = logging.getLogger(__name__) @@ -47,6 +48,10 @@ plural="virtualservices", ) +EXPECTED_LABEL_SELECTOR = ( + "LabelSelectorRequirement(key='app', operator='In', values=['istio-ingressgateway'])" +) + @pytest.mark.abort_on_fail async def test_kubectl_access(ops_test: OpsTest): @@ -334,7 +339,11 @@ async def test_disable_ingress_auth(ops_test: OpsTest): async def test_gateway_replicas_config(ops_test: OpsTest): - """Test the replicas config takes effect in the Deployment.""" + """Test changing the replicas config to 2, and then: + 1. Assert the replicas in the Deployment spec went up to 2. + 2. Assert the Deployment has the antiaffinity rule as expected. + 3. Assert the new Pod was not scheduled due to only 1 Node being available. + """ replicas_value = "2" await ops_test.model.applications[ISTIO_GATEWAY_APP_NAME].set_config( @@ -343,15 +352,45 @@ async def test_gateway_replicas_config(ops_test: OpsTest): await ops_test.model.wait_for_idle(apps=[ISTIO_GATEWAY_APP_NAME], status="active", timeout=300) client = lightkube.Client() + + # Get the gateway Deployment gateway_deployment = client.get( Deployment, name="istio-ingressgateway-workload", namespace=ops_test.model_name ) + # Assert the Deployment has the number of replicas from the config assert gateway_deployment.spec.replicas == int(replicas_value) - # TODO: assert antiaffinity contents - # assert gateway_deployment.spec.template.spec.affinity == "expected" - - # TODO: assert second pod is not scheduled + # Assert the Deployment has the correct antiaffinity rule + assert ( + str( + gateway_deployment.spec.template.spec.affinity.podAntiAffinity.requiredDuringSchedulingIgnoredDuringExecution[ # noqa E501 + 0 + ].labelSelector.matchExpressions[ + 0 + ] + ) + == EXPECTED_LABEL_SELECTOR + ) + + # List gateway pods that are in Pending status + pending_gateway_pods = list( + client.list( + Pod, + namespace=ops_test.model_name, + labels={"app": "istio-ingressgateway"}, + fields={"status.phase": "Pending"}, + ) + ) + + # Assert one Pod is in Pending status + assert len(pending_gateway_pods) == 1 + + # Get the status message for the Pending pod + pending_gateway_pod = pending_gateway_pods[0] + message = pending_gateway_pod.status.conditions[0].message + + # Assert the status message is about anti-affinity + assert "didn't match pod anti-affinity rules" in message async def test_charms_removal(ops_test: OpsTest): From 94f8d74dec1ace68dd09fe1d33ddff51fb01b17c Mon Sep 17 00:00:00 2001 From: NohaIhab Date: Tue, 24 Sep 2024 11:51:19 +0000 Subject: [PATCH 07/10] skip: add unit test and offload integration test --- charms/istio-gateway/tests/unit/test_charm.py | 35 +++++++++++++++++++ tests/test_bundle.py | 7 ++-- 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/charms/istio-gateway/tests/unit/test_charm.py b/charms/istio-gateway/tests/unit/test_charm.py index b271f2c0..4764c8e7 100644 --- a/charms/istio-gateway/tests/unit/test_charm.py +++ b/charms/istio-gateway/tests/unit/test_charm.py @@ -158,3 +158,38 @@ def test_metrics(harness): } ], ) + + +def test_manifests_applied_with_replicas_config(configured_harness, mocked_client): + """ + Asserts that the Deployment manifest called by `lightkube_client.apply` + contains the replicas config value. + """ + + # Arrange + # Reset the mock so that the calls list does not include any calls from other hooks + mocked_client.reset_mock() + + # Update the replicas config in the harness + replicas_config_value = 2 + configured_harness.update_config({"replicas": replicas_config_value}) + + # Act + configured_harness.charm.on.install.emit() + + actual_objects = [] + + # Get all the objects called by lightkube client `.apply` + for call in mocked_client.return_value.apply.call_args_list: + # The first (and only) argument to the apply method is the obj + # Convert the object to a dictionary and add it to the list + actual_objects.append(call.args[0].to_dict()) + + # Filter out the objects with Deployment kind + deployments = filter(lambda obj: obj.get("kind") == "Deployment", actual_objects) + # The gateway deployment is the only Deployment object in the manifests + gateway_deployment = list(deployments)[0] + + # Assert + assert gateway_deployment["spec"].get("replicas") == replicas_config_value + assert configured_harness.charm.model.unit.status == ActiveStatus("") diff --git a/tests/test_bundle.py b/tests/test_bundle.py index bfcf00fe..06adbe0d 100644 --- a/tests/test_bundle.py +++ b/tests/test_bundle.py @@ -340,9 +340,8 @@ async def test_disable_ingress_auth(ops_test: OpsTest): async def test_gateway_replicas_config(ops_test: OpsTest): """Test changing the replicas config to 2, and then: - 1. Assert the replicas in the Deployment spec went up to 2. - 2. Assert the Deployment has the antiaffinity rule as expected. - 3. Assert the new Pod was not scheduled due to only 1 Node being available. + 1. Assert the Deployment has the antiaffinity rule as expected. + 2. Assert the new Pod was not scheduled due to only 1 Node being available. """ replicas_value = "2" @@ -358,8 +357,6 @@ async def test_gateway_replicas_config(ops_test: OpsTest): Deployment, name="istio-ingressgateway-workload", namespace=ops_test.model_name ) - # Assert the Deployment has the number of replicas from the config - assert gateway_deployment.spec.replicas == int(replicas_value) # Assert the Deployment has the correct antiaffinity rule assert ( str( From bff115fc48e8992d0978f4dbf50d19272d7ddeea Mon Sep 17 00:00:00 2001 From: NohaIhab Date: Mon, 30 Sep 2024 19:32:45 +0000 Subject: [PATCH 08/10] skip: address comment --- charms/istio-gateway/tests/unit/test_charm.py | 43 +++++++++++++++++++ tests/test_bundle.py | 26 +---------- 2 files changed, 45 insertions(+), 24 deletions(-) diff --git a/charms/istio-gateway/tests/unit/test_charm.py b/charms/istio-gateway/tests/unit/test_charm.py index 4764c8e7..e1806b1a 100644 --- a/charms/istio-gateway/tests/unit/test_charm.py +++ b/charms/istio-gateway/tests/unit/test_charm.py @@ -160,6 +160,49 @@ def test_metrics(harness): ) +def test_manifests_applied_with_anti_affinity(configured_harness, kind, mocked_client): + """ + Asserts that the Deployment manifest called by `lightkube_client.apply` + contains the correct anti-affinity rule + """ + + # Arrange + # Reset the mock so that the calls list does not include any calls from other hooks + mocked_client.reset_mock() + + # Define the expected labelSelector based on the kind + expected_label_selector = {"key": "app", "operator": "In", "values": [f"istio-{kind}gateway"]} + + # Act + configured_harness.charm.on.start.emit() + + actual_objects = [] + + # Get all the objects called by lightkube client `.apply` + for call in mocked_client.return_value.apply.call_args_list: + # The first (and only) argument to the apply method is the obj + # Convert the object to a dictionary and add it to the list + actual_objects.append(call.args[0].to_dict()) + + # Filter out the objects with Deployment kind + deployments = filter(lambda obj: obj.get("kind") == "Deployment", actual_objects) + # The gateway deployment is the only Deployment object in the manifests + gateway_deployment = list(deployments)[0] + + # Assert + assert ( + gateway_deployment["spec"] + .get("template") + .get("spec") + .get("affinity") + .get("podAntiAffinity") + .get("requiredDuringSchedulingIgnoredDuringExecution")[0] + .get("labelSelector") + .get("matchExpressions")[0] + == expected_label_selector + ) + + def test_manifests_applied_with_replicas_config(configured_harness, mocked_client): """ Asserts that the Deployment manifest called by `lightkube_client.apply` diff --git a/tests/test_bundle.py b/tests/test_bundle.py index 06adbe0d..22ec067a 100644 --- a/tests/test_bundle.py +++ b/tests/test_bundle.py @@ -48,10 +48,6 @@ plural="virtualservices", ) -EXPECTED_LABEL_SELECTOR = ( - "LabelSelectorRequirement(key='app', operator='In', values=['istio-ingressgateway'])" -) - @pytest.mark.abort_on_fail async def test_kubectl_access(ops_test: OpsTest): @@ -339,9 +335,8 @@ async def test_disable_ingress_auth(ops_test: OpsTest): async def test_gateway_replicas_config(ops_test: OpsTest): - """Test changing the replicas config to 2, and then: - 1. Assert the Deployment has the antiaffinity rule as expected. - 2. Assert the new Pod was not scheduled due to only 1 Node being available. + """Test changing the replicas config to 2, and Assert the new Pod was not scheduled + due to only 1 Node being available. """ replicas_value = "2" @@ -352,23 +347,6 @@ async def test_gateway_replicas_config(ops_test: OpsTest): client = lightkube.Client() - # Get the gateway Deployment - gateway_deployment = client.get( - Deployment, name="istio-ingressgateway-workload", namespace=ops_test.model_name - ) - - # Assert the Deployment has the correct antiaffinity rule - assert ( - str( - gateway_deployment.spec.template.spec.affinity.podAntiAffinity.requiredDuringSchedulingIgnoredDuringExecution[ # noqa E501 - 0 - ].labelSelector.matchExpressions[ - 0 - ] - ) - == EXPECTED_LABEL_SELECTOR - ) - # List gateway pods that are in Pending status pending_gateway_pods = list( client.list( From ea2a30f4fb9ce63e8202c68a4f953b60c1a00fb0 Mon Sep 17 00:00:00 2001 From: NohaIhab Date: Mon, 30 Sep 2024 19:35:23 +0000 Subject: [PATCH 09/10] skip: lint --- tests/test_bundle.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_bundle.py b/tests/test_bundle.py index 22ec067a..5dacba34 100644 --- a/tests/test_bundle.py +++ b/tests/test_bundle.py @@ -15,7 +15,6 @@ create_namespaced_resource, load_in_cluster_generic_resources, ) -from lightkube.resources.apps_v1 import Deployment from lightkube.resources.core_v1 import Pod from pytest_operator.plugin import OpsTest From 6cc2ca0a46f8af13c5c36eb4760c210feed8ef69 Mon Sep 17 00:00:00 2001 From: NohaIhab Date: Mon, 30 Sep 2024 20:33:49 +0000 Subject: [PATCH 10/10] skip: rename test and add comment --- charms/istio-gateway/tests/unit/test_charm.py | 2 +- tests/test_bundle.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/charms/istio-gateway/tests/unit/test_charm.py b/charms/istio-gateway/tests/unit/test_charm.py index e1806b1a..9d1cac8f 100644 --- a/charms/istio-gateway/tests/unit/test_charm.py +++ b/charms/istio-gateway/tests/unit/test_charm.py @@ -189,7 +189,7 @@ def test_manifests_applied_with_anti_affinity(configured_harness, kind, mocked_c # The gateway deployment is the only Deployment object in the manifests gateway_deployment = list(deployments)[0] - # Assert + # Assert the Deployment has the correct antiaffinity rule assert ( gateway_deployment["spec"] .get("template") diff --git a/tests/test_bundle.py b/tests/test_bundle.py index 5dacba34..e26d550f 100644 --- a/tests/test_bundle.py +++ b/tests/test_bundle.py @@ -333,7 +333,7 @@ async def test_disable_ingress_auth(ops_test: OpsTest): ) -async def test_gateway_replicas_config(ops_test: OpsTest): +async def test_gateway_replicas_config_pod_anti_affinity(ops_test: OpsTest): """Test changing the replicas config to 2, and Assert the new Pod was not scheduled due to only 1 Node being available. """