improvement(kms): cleanup AWS KMS aliases only if there are no VMs #8030

vponomaryov · 2024-07-16T19:20:40Z

Testing

[ ]

PR pre-checks (self review)

I added the relevant backport labels
I didn't leave commented-out/debugging code

Reminders

Add New configuration option and document them (in sdcm/sct_config.py)
Add unit tests to cover my changes (under unit-test/ folder)
Update the Readme/doc folder relevant to this change (if needed)

vponomaryov · 2024-07-16T19:26:32Z

keep DB nodes: https://jenkins.scylladb.com/job/scylla-staging/job/valerii/job/vp-longevity-10gb-3h-test/118

./docker/env/hydra.sh --execute-on-runner 13.49.158.148 clean-resources --post-behavior --test-id 37cab0e7-d80e-41c5-a5bc-28ac62647313
...
20:26:31  Post behavior keep for db_nodes. Keep resources running
20:26:31  Post behavior keep for monitor_nodes. Keep resources running
20:26:31  Post behavior keep for loader_nodes. Keep resources running
20:26:31  Post behavior keep for k8s_cluster. Keep resources running
20:26:32  There are no instances to remove in AWS region eu-north-1
20:26:32  No capacity reservations to cancel.
20:26:32  There are no EIPs to remove in AWS region eu-north-1
20:26:33  There are no SGs to remove in AWS region eu-north-1
20:26:33  Skip AWS KMS alias deletion because DB nodes deletion was not scheduled
20:26:33  Cleanup for the {'TestId': '37cab0e7-d80e-41c5-a5bc-28ac62647313'} resources has been finished
20:26:34  Cleaning SSH agent
20:26:34  Agent pid 15491 killed
20:26:34  + echo 'Finished cleaning resources.'
20:26:34  Finished cleaning resources.

destroy DB nodes: https://jenkins.scylladb.com/job/scylla-staging/job/valerii/job/vp-longevity-10gb-3h-test/119

./docker/env/hydra.sh --execute-on-runner 16.170.243.50 clean-resources --post-behavior --test-id 5665e8b2-4723-496d-91f6-f6435c1f98ff
...
20:27:10  Post behavior destroy for db_nodes. Schedule cleanup
20:27:10  Post behavior destroy for monitor_nodes. Schedule cleanup
20:27:10  Post behavior destroy for loader_nodes. Schedule cleanup
20:27:10  Post behavior destroy for k8s_cluster. Schedule cleanup
20:27:10  Going to delete 'i-0b66c842f96a620d3' [name=longevity-10gb-3h-dev-loader-node-5665e8b2-2] 
20:27:11  Going to delete 'i-096ab304e17b7ba9b' [name=longevity-10gb-3h-dev-loader-node-5665e8b2-1] 
20:27:11  Going to delete 'i-072ad393b29600a65' [name=longevity-10gb-3h-dev-db-node-5665e8b2-7] 
20:27:11  Going to delete 'i-068f6c9efbb74dea6' [name=longevity-10gb-3h-dev-db-node-5665e8b2-2] 
20:27:12  Going to delete 'i-0ebe6424a0a4ab2d9' [name=longevity-10gb-3h-dev-db-node-5665e8b2-4] 
20:27:12  Going to delete 'i-05160e80556f3b2a2' [name=longevity-10gb-3h-dev-db-node-5665e8b2-3] 
20:27:12  Going to delete 'i-061822528bad6d4bf' [name=longevity-10gb-3h-dev-db-node-5665e8b2-6] 
20:27:13  Going to delete 'i-0e6c0527dd2358290' [name=longevity-10gb-3h-dev-db-node-5665e8b2-5] 
20:27:13  Going to delete 'i-0222716310d046267' [name=longevity-10gb-3h-dev-monitor-node-5665e8b2-1] 
20:27:14  No capacity reservations to cancel.
20:27:14  There are no EIPs to remove in AWS region eu-north-1
20:27:15  There are no SGs to remove in AWS region eu-north-1
20:27:16  Deleting the 'alias/testid-5665e8b2-4723-496d-91f6-f6435c1f98ff' alias in the KMS
20:27:16  Cleanup for the {'TestId': '5665e8b2-4723-496d-91f6-f6435c1f98ff'} resources has been finished
20:27:17  Cleaning SSH agent
20:27:17  Agent pid 15723 killed
20:27:17  + echo 'Finished cleaning resources.'
20:27:17  Finished cleaning resources.

Attempt to cleanup AWS KMS alias when DB nodes are still kept:

$ ./docker/env/hydra.sh clean-aws-kms-aliases -r eu-north-1 --time-delta-h 1
There is scylladb/hydra:v1.71-scylla-driver-3.26.9 in local cache, using it.
Going to run './sct.py  clean-aws-kms-aliases -r eu-north-1 --time-delta-h 1'...
...
KMS: Search for aliases older than '1' hours
KMS: [region 'eu-north-1'][key '07d2097b-3194-4dd3-a2d1-c9cee571f836'] read aliases
KMS: [region 'eu-north-1'][key '07d2097b-3194-4dd3-a2d1-c9cee571f836'] ignore the 'alias/qa-kms-key-for-rotation-1' alias as not matching
KMS: [region 'eu-north-1'][key '27a13c69-eb54-4db7-bd76-371ce43c4fb7'] read aliases
KMS: [region 'eu-north-1'][key '27a13c69-eb54-4db7-bd76-371ce43c4fb7'] ignore the 'alias/qa-kms-key-for-rotation-2' alias as not matching
KMS: [region 'eu-north-1'][key 'fb8e450f-c075-443b-8d7c-03852249c4d9'] read aliases
KMS: [region 'eu-north-1'][key 'fb8e450f-c075-443b-8d7c-03852249c4d9'] ignore the 'alias/qa-kms-key-for-rotation-3' alias as not matching
KMS: [region 'eu-north-1'][key 'fb8e450f-c075-443b-8d7c-03852249c4d9'] Found old alias -> 'alias/testid-37cab0e7-d80e-41c5-a5bc-28ac62647313' (2024-07-16 15:50:53.632000+00:00). Skip it because related DB nodes still exist.
KMS: finished cleaning up old aliases

Deletion of the alias after a separate deletion of DB nodes:

$ ./docker/env/hydra.sh clean-aws-kms-aliases -r eu-north-1 --time-delta-h 2
There is scylladb/hydra:v1.71-scylla-driver-3.26.9 in local cache, using it.
Going to run './sct.py  clean-aws-kms-aliases -r eu-north-1 --time-delta-h 2'...
...
KMS: Search for aliases older than '2' hours
KMS: [region 'eu-north-1'][key '07d2097b-3194-4dd3-a2d1-c9cee571f836'] read aliases
KMS: [region 'eu-north-1'][key '07d2097b-3194-4dd3-a2d1-c9cee571f836'] ignore the 'alias/qa-kms-key-for-rotation-1' alias as not matching
KMS: [region 'eu-north-1'][key '27a13c69-eb54-4db7-bd76-371ce43c4fb7'] read aliases
KMS: [region 'eu-north-1'][key '27a13c69-eb54-4db7-bd76-371ce43c4fb7'] ignore the 'alias/qa-kms-key-for-rotation-2' alias as not matching
KMS: [region 'eu-north-1'][key 'fb8e450f-c075-443b-8d7c-03852249c4d9'] read aliases
KMS: [region 'eu-north-1'][key 'fb8e450f-c075-443b-8d7c-03852249c4d9'] ignore the 'alias/qa-kms-key-for-rotation-3' alias as not matching
KMS: [region 'eu-north-1'][key 'fb8e450f-c075-443b-8d7c-03852249c4d9'] deleting old alias -> 'alias/testid-37cab0e7-d80e-41c5-a5bc-28ac62647313' (2024-07-16 15:50:53.632000+00:00)
Deleting the 'alias/testid-37cab0e7-d80e-41c5-a5bc-28ac62647313' alias in the KMS
KMS: finished cleaning up old aliases

fruch

refactor is breaking the unittests

It will allow to be more flexible in importing other common utils to avoid circular imports.

Make the AWS KMS alias cleanup logic check VMs presence and skip deletion if some VMs have been found tagged with the same test-id. It will allow to make kept DB clusters be workable during it's lifetime.

Before this change we were deleting AWS kMS alias of a test run even if DB cluster was set to be kept ('keep' tag).

soyacz

LGTM

soyacz · 2024-07-17T06:53:54Z

sdcm/utils/gce_utils.py

+        return self.cleaner.gcloud.run(f"container clusters delete {self.name} --zone {self.zone} --quiet")
+
+
+class GkeCleaner(GcloudContainerMixin):


I understand this was only moved here, but I'll leave comment for the future: Api of this class is very misleading and complicated.
GkeCleaner that doesn't have 'cleaning' methods - mostly listing resources (and e.g. list_gke_clusters returns list of GkeClusterForCleaner - so again mislead as it does not return gke cluster itself).

When introducing classes with simple goals like cleaning, let's make sure names, methods are consistent with this goal.

I agree, but it is out of scope for this PR.
This PR is about fixing AWS KMS alias cleanups and dependency change with this moving of existing functionality.

vponomaryov · 2024-07-17T10:00:44Z

It is needed as a fix only for enterprise branches, but will definitely make sense to backport to 6.0 and 5.4 just for ease of other backports reducing code diffs.
@fruch , @soyacz agree to also mark it for 6.0 and 5.4 backports?

fruch · 2024-07-17T10:45:39Z

It is needed as a fix only for enterprise branches, but will definitely make sense to backport to 6.0 and 5.4 just for ease of other backports reducing code diffs. @fruch , @soyacz agree to also mark it for 6.0 and 5.4 backports?

yes

fruch

LGTM

github-actions bot assigned vponomaryov Jul 16, 2024

vponomaryov added backport/2024.1 Need backport to 2024.1 backport/2024.2 Need backport to 2024.2 Ready for review labels Jul 16, 2024

vponomaryov requested review from fruch and soyacz July 16, 2024 19:21

vponomaryov requested a review from roydahan July 16, 2024 19:26

fruch requested changes Jul 16, 2024

View reviewed changes

vponomaryov force-pushed the fix-aws-kms-alias-cleanup branch from 0b83b35 to d5aa178 Compare July 16, 2024 20:29

vponomaryov added 4 commits July 16, 2024 23:30

refactor(utils): move out 'cleanup' utils to a separate module

50c1a96

It will allow to be more flexible in importing other common utils to avoid circular imports.

improvement(kms): cleanup old AWS KMS aliases only if there are no VMs

3bb00c3

Make the AWS KMS alias cleanup logic check VMs presence and skip deletion if some VMs have been found tagged with the same test-id. It will allow to make kept DB clusters be workable during it's lifetime.

fix(kms): cleanup a test run's AWS KMS alias only with DB nodes

f4dc742

Before this change we were deleting AWS kMS alias of a test run even if DB cluster was set to be kept ('keep' tag).

fix(kms): use provided 'time_delta_h' param in AWS KMS alias cleanups

e2818b1

vponomaryov force-pushed the fix-aws-kms-alias-cleanup branch from d5aa178 to e2818b1 Compare July 16, 2024 20:30

soyacz approved these changes Jul 17, 2024

View reviewed changes

vponomaryov requested a review from fruch July 17, 2024 08:31

vponomaryov mentioned this pull request Jul 17, 2024

Don't clear kept clusters KMS aliases #7909

Closed

2 tasks

fruch approved these changes Jul 17, 2024

View reviewed changes

fruch added backport/5.4 Need backport to 5.4 backport/6.0 backport/2023.1 Need to backport to 2023.1 labels Jul 17, 2024

fruch merged commit a6fe0f7 into scylladb:master Jul 17, 2024
10 checks passed

scylladbbot added the promoted-to-master label Jul 17, 2024

mergify bot mentioned this pull request Jul 17, 2024

[Backport 2024.2] improvement(kms): cleanup AWS KMS aliases only if there are no VMs #8039

Merged

2 tasks

scylladbbot added backport/2024.2-done Commit backported to 2024.2 and removed backport/2024.2 Need backport to 2024.2 labels Jul 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improvement(kms): cleanup AWS KMS aliases only if there are no VMs #8030

improvement(kms): cleanup AWS KMS aliases only if there are no VMs #8030

vponomaryov commented Jul 16, 2024 •

edited

Loading

vponomaryov commented Jul 16, 2024

fruch left a comment

soyacz left a comment

soyacz Jul 17, 2024

vponomaryov Jul 17, 2024

vponomaryov commented Jul 17, 2024 •

edited

Loading

fruch commented Jul 17, 2024

fruch left a comment

		return self.cleaner.gcloud.run(f"container clusters delete {self.name} --zone {self.zone} --quiet")


		class GkeCleaner(GcloudContainerMixin):

improvement(kms): cleanup AWS KMS aliases only if there are no VMs #8030

improvement(kms): cleanup AWS KMS aliases only if there are no VMs #8030

Conversation

vponomaryov commented Jul 16, 2024 • edited Loading

Testing

PR pre-checks (self review)

Reminders

vponomaryov commented Jul 16, 2024

fruch left a comment

Choose a reason for hiding this comment

soyacz left a comment

Choose a reason for hiding this comment

soyacz Jul 17, 2024

Choose a reason for hiding this comment

vponomaryov Jul 17, 2024

Choose a reason for hiding this comment

vponomaryov commented Jul 17, 2024 • edited Loading

fruch commented Jul 17, 2024

fruch left a comment

Choose a reason for hiding this comment

vponomaryov commented Jul 16, 2024 •

edited

Loading

vponomaryov commented Jul 17, 2024 •

edited

Loading