From 6735d286b4161e512be1daa47db1ae0b8406a0d2 Mon Sep 17 00:00:00 2001 From: Alan Moran Date: Tue, 20 Aug 2024 11:51:43 +0200 Subject: [PATCH 1/5] Improve cleanup_apps function with dynamic app retrieval and conditional undeploy logic --- ci/autoscaler/scripts/common.sh | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/ci/autoscaler/scripts/common.sh b/ci/autoscaler/scripts/common.sh index c98285702b..df326ca7d2 100644 --- a/ci/autoscaler/scripts/common.sh +++ b/ci/autoscaler/scripts/common.sh @@ -81,8 +81,16 @@ function cleanup_credhub(){ } function cleanup_apps(){ + step "cleaning up apps" cf_target "${autoscaler_org}" "${autoscaler_space}" - cf undeploy com.github.cloudfoundry.app-autoscaler-release -f + + local mtar_app="$(curl --header "Authorization: $(cf oauth-token)" "deploy-service.$(SYSTEM_DOMAIN)/api/v2/spaces/$(cf space --guid $AUTOSCALER_SPACE)/mtas" | jq ". | .[] | .metadata | .id" -r)" + + if [ -n "${mtar_app}" ]; then + cf undeploy "${mtar_app}"-f + else + echo "No app to undeploy" + fi if ! cf spaces | grep --quiet --regexp="^${AUTOSCALER_SPACE}$"; then cf delete-space -f "${AUTOSCALER_SPACE}" From 10221e3bca9c688e6e16ea5fa6156d03ba98c2f6 Mon Sep 17 00:00:00 2001 From: Alan Moran Date: Tue, 20 Aug 2024 12:17:17 +0200 Subject: [PATCH 2/5] Move cleanup_apps function call and fix system_domain variable case in cleanup_apps function --- ci/autoscaler/scripts/cleanup-autoscaler.sh | 2 +- ci/autoscaler/scripts/common.sh | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ci/autoscaler/scripts/cleanup-autoscaler.sh b/ci/autoscaler/scripts/cleanup-autoscaler.sh index e31a451f4e..ddc15a98a4 100755 --- a/ci/autoscaler/scripts/cleanup-autoscaler.sh +++ b/ci/autoscaler/scripts/cleanup-autoscaler.sh @@ -10,12 +10,12 @@ function main() { bosh_login cf_login + cleanup_apps cleanup_acceptance_run cleanup_service_broker cleanup_bosh_deployment delete_releases cleanup_credhub - cleanup_apps } [ "${BASH_SOURCE[0]}" == "${0}" ] && main "$@" diff --git a/ci/autoscaler/scripts/common.sh b/ci/autoscaler/scripts/common.sh index df326ca7d2..32c18418ac 100644 --- a/ci/autoscaler/scripts/common.sh +++ b/ci/autoscaler/scripts/common.sh @@ -84,10 +84,10 @@ function cleanup_apps(){ step "cleaning up apps" cf_target "${autoscaler_org}" "${autoscaler_space}" - local mtar_app="$(curl --header "Authorization: $(cf oauth-token)" "deploy-service.$(SYSTEM_DOMAIN)/api/v2/spaces/$(cf space --guid $AUTOSCALER_SPACE)/mtas" | jq ". | .[] | .metadata | .id" -r)" + local mtar_app="$(curl --header "Authorization: $(cf oauth-token)" "deploy-service.${system_domain}/api/v2/spaces/$(cf space --guid $AUTOSCALER_SPACE)/mtas" | jq ". | .[] | .metadata | .id" -r)" if [ -n "${mtar_app}" ]; then - cf undeploy "${mtar_app}"-f + cf undeploy "${mtar_app}" -f else echo "No app to undeploy" fi From 1d8a4269388c3fb6b598c3cf069475c6b2021383 Mon Sep 17 00:00:00 2001 From: Alan Moran Date: Tue, 20 Aug 2024 16:42:21 +0200 Subject: [PATCH 3/5] Fix lint --- ci/autoscaler/scripts/common.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ci/autoscaler/scripts/common.sh b/ci/autoscaler/scripts/common.sh index 32c18418ac..414841b936 100644 --- a/ci/autoscaler/scripts/common.sh +++ b/ci/autoscaler/scripts/common.sh @@ -82,9 +82,11 @@ function cleanup_credhub(){ function cleanup_apps(){ step "cleaning up apps" + local mtar_app + cf_target "${autoscaler_org}" "${autoscaler_space}" - local mtar_app="$(curl --header "Authorization: $(cf oauth-token)" "deploy-service.${system_domain}/api/v2/spaces/$(cf space --guid $AUTOSCALER_SPACE)/mtas" | jq ". | .[] | .metadata | .id" -r)" + mtar_app="$(curl --header "Authorization: $(cf oauth-token)" "deploy-service.${system_domain}/api/v2/spaces/$(cf space --guid ${autoscaler_space})/mtas" | jq ". | .[] | .metadata | .id" -r)" if [ -n "${mtar_app}" ]; then cf undeploy "${mtar_app}" -f From 7c9472ae86729df04e64ed757b0919c91061e373 Mon Sep 17 00:00:00 2001 From: Alan Moran Date: Tue, 20 Aug 2024 17:15:19 +0200 Subject: [PATCH 4/5] Improve variable handling in cleanup_apps function MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit • Store space GUID in a variable for reuse • Update mtar_app assignment to use the new space_guid variable --- ci/autoscaler/scripts/common.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ci/autoscaler/scripts/common.sh b/ci/autoscaler/scripts/common.sh index 414841b936..9a311e822b 100644 --- a/ci/autoscaler/scripts/common.sh +++ b/ci/autoscaler/scripts/common.sh @@ -83,10 +83,12 @@ function cleanup_credhub(){ function cleanup_apps(){ step "cleaning up apps" local mtar_app + local space_guid cf_target "${autoscaler_org}" "${autoscaler_space}" - mtar_app="$(curl --header "Authorization: $(cf oauth-token)" "deploy-service.${system_domain}/api/v2/spaces/$(cf space --guid ${autoscaler_space})/mtas" | jq ". | .[] | .metadata | .id" -r)" + space_guid="$(cf space --guid "${autoscaler_space}")" + mtar_app="$(curl --header "Authorization: $(cf oauth-token)" "deploy-service.${system_domain}/api/v2/spaces/${space_guid}/mtas" | jq ". | .[] | .metadata | .id" -r)" if [ -n "${mtar_app}" ]; then cf undeploy "${mtar_app}" -f From 142773e8a83aee09fbdc4c3bbdcbd5de6ae90dd0 Mon Sep 17 00:00:00 2001 From: Silvestre Zabala Date: Tue, 20 Aug 2024 13:57:27 +0200 Subject: [PATCH 5/5] fix(metricsforwarder): Clear allowed metrics cache once policy is removed. # Issue If an app was bound with a policy using custom metrics, even after the policy being removed the app was still allowed to submit the custom metrics previously used. # Fix Correctly clear the entry in the cache if the policy was removed. --- .../metricsforwarder/manager/policyManager.go | 3 ++ .../manager/policyManager_test.go | 49 +++++++++++++------ 2 files changed, 37 insertions(+), 15 deletions(-) diff --git a/src/autoscaler/metricsforwarder/manager/policyManager.go b/src/autoscaler/metricsforwarder/manager/policyManager.go index 6fd409ff48..2352ab3d65 100644 --- a/src/autoscaler/metricsforwarder/manager/policyManager.go +++ b/src/autoscaler/metricsforwarder/manager/policyManager.go @@ -118,6 +118,9 @@ func (pm *PolicyManager) RefreshAllowedMetricCache(policies map[string]*models.A pm.logger.Error("Error updating allowedMetricCache", err) return err } + } else { + //If the policy is not present in the cache, remove the entry from the cache + pm.allowedMetricCache.Delete(applicationId) } } return nil diff --git a/src/autoscaler/metricsforwarder/manager/policyManager_test.go b/src/autoscaler/metricsforwarder/manager/policyManager_test.go index e7486d0dab..9824f4d21e 100644 --- a/src/autoscaler/metricsforwarder/manager/policyManager_test.go +++ b/src/autoscaler/metricsforwarder/manager/policyManager_test.go @@ -92,7 +92,7 @@ var _ = Describe("PolicyManager", func() { policyManager.Stop() }) - Context("when allowedMetricCache has already filled with metricstype details of the same appilication", func() { + When("allowedMetricCache has already filled with metricstype details of the same appilication", func() { BeforeEach(func() { scalingPolicy = &models.ScalingPolicy{ @@ -114,22 +114,41 @@ var _ = Describe("PolicyManager", func() { Expect(found).To(BeTrue()) Expect(maps).Should(HaveKey("queuelength")) - database.RetrievePoliciesStub = func() ([]*models.PolicyJson, error) { - return []*models.PolicyJson{{AppId: testAppId, PolicyStr: policyStr}}, nil - } - }) - It("should be able refresh allowed metrics cache", func() { - Eventually(database.RetrievePoliciesCallCount).Should(Equal(1)) - clock.Increment(1 * testPolicyPollerInterval) - Eventually(database.RetrievePoliciesCallCount).Should(Equal(2)) - - res, found := allowedMetricCache.Get(testAppId) - maps := res.(map[string]struct{}) - Expect(found).To(BeTrue()) - Expect(maps).Should(HaveKey("test-metric-name")) - Expect(maps).ShouldNot(HaveKey("queuelength")) + When("the policy is updated", func() { + BeforeEach(func() { + database.RetrievePoliciesStub = func() ([]*models.PolicyJson, error) { + return []*models.PolicyJson{{AppId: testAppId, PolicyStr: policyStr}}, nil + } + }) + It("should refresh the allowed metrics cache", func() { + Eventually(database.RetrievePoliciesCallCount).Should(Equal(1)) + clock.Increment(1 * testPolicyPollerInterval) + Eventually(database.RetrievePoliciesCallCount).Should(Equal(2)) + + res, found := allowedMetricCache.Get(testAppId) + maps := res.(map[string]struct{}) + + Expect(found).To(BeTrue()) + Expect(maps).Should(HaveKey("test-metric-name")) + Expect(maps).ShouldNot(HaveKey("queuelength")) + }) + }) + When("the policy is deleted", func() { + BeforeEach(func() { + database.RetrievePoliciesStub = func() ([]*models.PolicyJson, error) { + return []*models.PolicyJson{}, nil + } + }) + It("should refresh the allowed metrics cache", func() { + Eventually(database.RetrievePoliciesCallCount).Should(Equal(1)) + clock.Increment(1 * testPolicyPollerInterval) + Eventually(database.RetrievePoliciesCallCount).Should(Equal(2)) + + _, found := allowedMetricCache.Get(testAppId) + Expect(found).To(BeFalse()) + }) }) }) })