From 526f4a898a894551dbbbb15669a5d66668be1ecf Mon Sep 17 00:00:00 2001
From: Jim Ryan <j.ryan@f5.com>
Date: Fri, 15 Mar 2024 17:32:50 +0000
Subject: [PATCH] further isolate tests

---
 internal/k8s/controller.go                    |  8 +-
 .../app.yaml                                  | 99 +++++++++++++++++++
 .../configmap/nginx-config.yaml               |  4 +-
 .../standard/virtual-server-many.yaml         |  9 ++
 ...tual-server-route-many-splits-initial.yaml |  2 +-
 ...virtual-server-route-many-splits-swap.yaml |  2 +-
 ...v_s_route_weight_changes_without_reload.py |  1 +
 ...ight_changes_without_reload_many_splits.py | 93 ++++++++---------
 8 files changed, 159 insertions(+), 59 deletions(-)
 create mode 100644 tests/data/common/app/weight-changes-without-reload-vsr-many-splits/app.yaml
 create mode 100644 tests/data/virtual-server-route-weight-changes-without-reload/standard/virtual-server-many.yaml

diff --git a/internal/k8s/controller.go b/internal/k8s/controller.go
index 2c5de5148c..0f7a45f61d 100644
--- a/internal/k8s/controller.go
+++ b/internal/k8s/controller.go
@@ -4337,7 +4337,9 @@ func (lbc *LoadBalancerController) processVSRWeightChangesWithoutReload(vsrOld *
 	var weightUpdates []configs.WeightUpdate
 	vs, exists := lbc.getVirtualServerByVirtualServerRoute(vsrNew)
 	if !exists {
-		glog.V(3).Infof("VirtualServerRoute %v does not have a VirtualServer", vsrNew.Name)
+		// If the VirtualServerRoute does not have a VirtualServer (can happen if an invalid state is applied),
+		// we should add it to the sync queue to be processed normally.
+		lbc.AddSyncQueue(vsrNew)
 		return
 	}
 	splitClientsIndex := lbc.getStartingSplitClientsIndex(vsrNew, vs)
@@ -4443,6 +4445,8 @@ func (lbc *LoadBalancerController) getStartingSplitClientsIndex(vsr *conf_v1.Vir
 }
 
 func (lbc *LoadBalancerController) haltIfVSConfigInvalid(vsNew *conf_v1.VirtualServer) bool {
+	lbc.configuration.lock.Lock()
+	defer lbc.configuration.lock.Unlock()
 	key := getResourceKey(&vsNew.ObjectMeta)
 	validationErr := lbc.configuration.virtualServerValidator.ValidateVirtualServer(vsNew)
 
@@ -4467,6 +4471,8 @@ func (lbc *LoadBalancerController) haltIfVSConfigInvalid(vsNew *conf_v1.VirtualS
 }
 
 func (lbc *LoadBalancerController) haltIfVSRConfigInvalid(vsrNew *conf_v1.VirtualServerRoute) bool {
+	lbc.configuration.lock.Lock()
+	defer lbc.configuration.lock.Unlock()
 	key := getResourceKey(&vsrNew.ObjectMeta)
 	validationErr := lbc.configuration.virtualServerValidator.ValidateVirtualServerRoute(vsrNew)
 
diff --git a/tests/data/common/app/weight-changes-without-reload-vsr-many-splits/app.yaml b/tests/data/common/app/weight-changes-without-reload-vsr-many-splits/app.yaml
new file mode 100644
index 0000000000..7d8fa3200f
--- /dev/null
+++ b/tests/data/common/app/weight-changes-without-reload-vsr-many-splits/app.yaml
@@ -0,0 +1,99 @@
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: backend1
+spec:
+  replicas: 2
+  selector:
+    matchLabels:
+      app: backend1
+  template:
+    metadata:
+      labels:
+        app: backend1
+    spec:
+      containers:
+      - name: backend1
+        image: nginxdemos/nginx-hello:plain-text
+        ports:
+        - containerPort: 8080
+---
+apiVersion: v1
+kind: Service
+metadata:
+  name: backend1-svc
+spec:
+  ports:
+  - port: 80
+    targetPort: 8080
+    protocol: TCP
+    name: http
+  selector:
+    app: backend1
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: backend2
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      app: backend2
+  template:
+    metadata:
+      labels:
+        app: backend2
+    spec:
+      containers:
+      - name: backend2
+        image: nginxdemos/nginx-hello:plain-text
+        ports:
+        - containerPort: 8080
+---
+apiVersion: v1
+kind: Service
+metadata:
+  name: backend2-svc
+spec:
+  ports:
+  - port: 80
+    targetPort: 8080
+    protocol: TCP
+    name: http
+  selector:
+    app: backend2
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: backend3
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      app: backend3
+  template:
+    metadata:
+      labels:
+        app: backend3
+    spec:
+      containers:
+      - name: backend3
+        image: nginxdemos/nginx-hello:plain-text
+        ports:
+        - containerPort: 8080
+---
+apiVersion: v1
+kind: Service
+metadata:
+  name: backend3-svc
+spec:
+  ports:
+  - port: 80
+    targetPort: 8080
+    protocol: TCP
+    name: http
+  selector:
+    app: backend3
+---
diff --git a/tests/data/virtual-server-route-weight-changes-without-reload/configmap/nginx-config.yaml b/tests/data/virtual-server-route-weight-changes-without-reload/configmap/nginx-config.yaml
index e4b184c402..bd644b7164 100644
--- a/tests/data/virtual-server-route-weight-changes-without-reload/configmap/nginx-config.yaml
+++ b/tests/data/virtual-server-route-weight-changes-without-reload/configmap/nginx-config.yaml
@@ -6,5 +6,5 @@ metadata:
 data:
   map-hash-bucket-size: "512"
   map-hash-max-size: "8192"
-  variables-hash-bucket-size: "128"
-  variables-hash-max-size: "4096"
+  variables-hash-bucket-size: "256"
+  variables-hash-max-size: "16384"
diff --git a/tests/data/virtual-server-route-weight-changes-without-reload/standard/virtual-server-many.yaml b/tests/data/virtual-server-route-weight-changes-without-reload/standard/virtual-server-many.yaml
new file mode 100644
index 0000000000..71283e8a86
--- /dev/null
+++ b/tests/data/virtual-server-route-weight-changes-without-reload/standard/virtual-server-many.yaml
@@ -0,0 +1,9 @@
+apiVersion: k8s.nginx.org/v1
+kind: VirtualServer
+metadata:
+  name: virtual-server-route-many
+spec:
+  host: virtual-server-route.example.com
+  routes:
+  - path: "/backends"
+    route: backends2-namespace/backendsmany
diff --git a/tests/data/virtual-server-route-weight-changes-without-reload/virtual-server-route-many-splits-initial.yaml b/tests/data/virtual-server-route-weight-changes-without-reload/virtual-server-route-many-splits-initial.yaml
index 796fdb9f94..2ee657fc89 100644
--- a/tests/data/virtual-server-route-weight-changes-without-reload/virtual-server-route-many-splits-initial.yaml
+++ b/tests/data/virtual-server-route-weight-changes-without-reload/virtual-server-route-many-splits-initial.yaml
@@ -1,7 +1,7 @@
 apiVersion: k8s.nginx.org/v1
 kind: VirtualServerRoute
 metadata:
-  name: backends
+  name: backendsmany
 spec:
   host: virtual-server-route.example.com
   upstreams:
diff --git a/tests/data/virtual-server-route-weight-changes-without-reload/virtual-server-route-many-splits-swap.yaml b/tests/data/virtual-server-route-weight-changes-without-reload/virtual-server-route-many-splits-swap.yaml
index 4a9bd33e65..21a3f92ab8 100644
--- a/tests/data/virtual-server-route-weight-changes-without-reload/virtual-server-route-many-splits-swap.yaml
+++ b/tests/data/virtual-server-route-weight-changes-without-reload/virtual-server-route-many-splits-swap.yaml
@@ -1,7 +1,7 @@
 apiVersion: k8s.nginx.org/v1
 kind: VirtualServerRoute
 metadata:
-  name: backends
+  name: backendsmany
 spec:
   host: virtual-server-route.example.com
   upstreams:
diff --git a/tests/suite/test_v_s_route_weight_changes_without_reload.py b/tests/suite/test_v_s_route_weight_changes_without_reload.py
index 15e9f7d972..e1184c78f1 100644
--- a/tests/suite/test_v_s_route_weight_changes_without_reload.py
+++ b/tests/suite/test_v_s_route_weight_changes_without_reload.py
@@ -94,6 +94,7 @@ def fin():
 
 
 @pytest.mark.vsr
+@pytest.mark.weight_changes_without_reload
 @pytest.mark.smok
 @pytest.mark.skip_for_nginx_oss
 @pytest.mark.parametrize(
diff --git a/tests/suite/test_v_s_route_weight_changes_without_reload_many_splits.py b/tests/suite/test_v_s_route_weight_changes_without_reload_many_splits.py
index 90ff8a0720..af5062c992 100644
--- a/tests/suite/test_v_s_route_weight_changes_without_reload_many_splits.py
+++ b/tests/suite/test_v_s_route_weight_changes_without_reload_many_splits.py
@@ -9,6 +9,7 @@
     delete_namespace,
     ensure_response_from_backend,
     get_reload_count,
+    replace_configmap,
     replace_configmap_from_yaml,
     wait_before_test,
     wait_until_all_pods_are_ready,
@@ -63,31 +64,52 @@ def vsr_weight_changes_without_reload_many_splits_setup(
 
     metrics_url = f"http://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.metrics_port}/metrics"
     vs_routes_ns = get_route_namespace_from_vs_yaml(
-        f"{TEST_DATA}/{request.param['example']}/standard/virtual-server.yaml"
+        f"{TEST_DATA}/{request.param['example']}/standard/virtual-server-many.yaml"
     )
     ns_1 = create_namespace_with_name_from_yaml(kube_apis.v1, vs_routes_ns[0], f"{TEST_DATA}/common/ns.yaml")
     print("------------------------- Deploy Virtual Server -----------------------------------")
     vs_name = create_virtual_server_from_yaml(
-        kube_apis.custom_objects, f"{TEST_DATA}/{request.param['example']}/standard/virtual-server.yaml", ns_1
+        kube_apis.custom_objects, f"{TEST_DATA}/{request.param['example']}/standard/virtual-server-many.yaml", ns_1
     )
-    vs_host = get_first_host_from_yaml(f"{TEST_DATA}/{request.param['example']}/standard/virtual-server.yaml")
+    vs_host = get_first_host_from_yaml(f"{TEST_DATA}/{request.param['example']}/standard/virtual-server-many.yaml")
 
     print("------------------------- Deploy Virtual Server Route -----------------------------------")
     vsr_name = create_v_s_route_from_yaml(
-        kube_apis.custom_objects, f"{TEST_DATA}/{request.param['example']}/virtual-server-route-initial.yaml", ns_1
+        kube_apis.custom_objects,
+        f"{TEST_DATA}/{request.param['example']}/virtual-server-route-many-splits-initial.yaml",
+        ns_1,
+    )
+    vsr_paths = get_paths_from_vsr_yaml(
+        f"{TEST_DATA}/{request.param['example']}/virtual-server-route-many-splits-initial.yaml"
     )
-    vsr_paths = get_paths_from_vsr_yaml(f"{TEST_DATA}/{request.param['example']}/virtual-server-route-initial.yaml")
     route = VirtualServerRoute(ns_1, vsr_name, vsr_paths)
-    backends_url = f"http://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.port}{vsr_paths[0]}"
+    backends_url = (
+        f"http://{ingress_controller_endpoint.public_ip}:{ingress_controller_endpoint.port}{vsr_paths[0][:-1]}"
+    )
+
+    print("-----------------------Apply Config Map---------------------------------------------------")
+    config_map_name = ingress_controller_prerequisites.config_map["metadata"]["name"]
+    replace_configmap_from_yaml(
+        kube_apis.v1,
+        config_map_name,
+        ingress_controller_prerequisites.namespace,
+        f"{TEST_DATA}/{request.param['example']}/configmap/nginx-config.yaml",
+    )
 
     print("---------------------- Deploy weight changes without reload vsr app ----------------------------")
-    create_example_app(kube_apis, "weight-changes-without-reload-vsr", ns_1)
+    create_example_app(kube_apis, "weight-changes-without-reload-vsr-many-splits", ns_1)
     wait_until_all_pods_are_ready(kube_apis.v1, ns_1)
 
     def fin():
         if request.config.getoption("--skip-fixture-teardown") == "no":
             print("Delete test namespace")
             delete_namespace(kube_apis.v1, ns_1)
+            replace_configmap(
+                kube_apis.v1,
+                config_map_name,
+                ingress_controller_prerequisites.namespace,
+                ingress_controller_prerequisites.config_map,
+            )
 
     request.addfinalizer(fin)
 
@@ -95,6 +117,7 @@ def fin():
 
 
 @pytest.mark.vsr
+@pytest.mark.weight_changes_without_reload
 @pytest.mark.smok
 @pytest.mark.skip_for_nginx_oss
 @pytest.mark.parametrize(
@@ -115,60 +138,34 @@ def fin():
     ],
     indirect=["crd_ingress_controller", "vsr_weight_changes_without_reload_many_splits_setup"],
 )
-class TestVSRWeightChangesWithReloadManySplits:
+class TestVSRWeightChangesWithoutReloadManySplits:
 
-    def test_vsr_weight_changes_reload_many_splits(
+    def test_vsr_weight_changes_without_reload_many_splits(
         self, kube_apis, crd_ingress_controller, vsr_weight_changes_without_reload_many_splits_setup
     ) -> None:
         """
         This test checks if 32 splits can be created when the following values are specified in the configmap
         map-hash-bucket-size: "512"
         map-hash-max-size: "8192"
-        variables-hash-bucket-size: "128"
-        variables-hash-max-size: "4096"
+        variables-hash-bucket-size: "256"
+        variables-hash-max-size: "16384"
 
         and also that weight-changes-without-reload is set to true
         """
-        initial_weights_config = f"{TEST_DATA}/virtual-server-route-weight-changes-without-reload/virtual-server-route-many-splits-initial.yaml"
         swap_weights_config = (
             f"{TEST_DATA}/virtual-server-route-weight-changes-without-reload/virtual-server-route-many-splits-swap.yaml"
         )
 
-        test_cm_src = f"{TEST_DATA}/virtual-server-route-weight-changes-without-reload/configmap/nginx-config.yaml"
-        config_map_name = "nginx-config"
-        config_map_namespace = "nginx-ingress"
-
-        print("Step 1: Create configmap.")
-        replace_configmap_from_yaml(
-            kube_apis.v1,
-            config_map_name,
-            config_map_namespace,
-            test_cm_src,
-        )
-        print("Step 2: Apply initial config.")
-        patch_v_s_route_from_yaml(
-            kube_apis.custom_objects,
-            vsr_weight_changes_without_reload_many_splits_setup.route.name,
-            initial_weights_config,
-            vsr_weight_changes_without_reload_many_splits_setup.route.namespace,
-        )
-
-        print("Step 3: Get a response from the backend.")
+        print("Step 1: Get a response from the backend.")
         backends32_url = f"{vsr_weight_changes_without_reload_many_splits_setup.backends_url}32"
         wait_and_assert_status_code(200, backends32_url, vsr_weight_changes_without_reload_many_splits_setup.vs_host)
         resp = requests.get(
             backends32_url,
             headers={"host": vsr_weight_changes_without_reload_many_splits_setup.vs_host},
         )
-        assert (
-            "backend1" in resp.text
-        )  # The route /backends/backends32 splits traffic between backend1 and backend2. All traffic goes to backend1 initially
+        assert "backend1" in resp.text
 
-        print("Step 4: Record the initial number of reloads.")
-        count_before = get_reload_count(vsr_weight_changes_without_reload_many_splits_setup.metrics_url)
-        print(f"Reload count before: {count_before}")
-
-        print("Step 5: Apply a configuration that swaps the weights (0 100) to (100 0).")
+        print("Step 3: Apply a configuration that swaps the weights (0 100) to (100 0).")
         patch_v_s_route_from_yaml(
             kube_apis.custom_objects,
             vsr_weight_changes_without_reload_many_splits_setup.route.name,
@@ -176,23 +173,11 @@ def test_vsr_weight_changes_reload_many_splits(
             vsr_weight_changes_without_reload_many_splits_setup.route.namespace,
         )
 
-        wait_before_test(5)
-
-        print("Step 6: Verify hitting the other backend.")
+        print("Step 4: Verify hitting the other backend.")
         ensure_response_from_backend(backends32_url, vsr_weight_changes_without_reload_many_splits_setup.vs_host)
         wait_and_assert_status_code(200, backends32_url, vsr_weight_changes_without_reload_many_splits_setup.vs_host)
         resp = requests.get(
             backends32_url,
             headers={"host": vsr_weight_changes_without_reload_many_splits_setup.vs_host},
         )
-        assert (
-            "backend2" in resp.text
-        )  # After applying the new config /backends/backends/32 should always point to backend2
-
-        print("Step 7: Verify reload behavior")
-        count_after = get_reload_count(vsr_weight_changes_without_reload_many_splits_setup.metrics_url)
-        print(f"Reload count after: {count_after}")
-
-        assert (
-            count_before == count_after
-        ), "The reload count should not change when weights are swapped and weight-changes-without-reload=true."
+        assert "backend2" in resp.text