Skip to content

Commit 09acb31

Browse files
refactor(swap_encryption): use PKB GcloudCommand instead of raw vm_util.IssueCommand
Replace all raw ['gcloud', ...] list + vm_util.IssueCommand calls in swap_encryption_benchmark.py with PKB's existing GcloudCommand infrastructure: - _create_benchmark_node_pool: use cluster._GcloudCommand() + cmd.flags dict + cmd.Issue(timeout=1200); keeps hyperdisk IOPS/throughput flags and --system-config-from-file for kubelet memorySwapBehavior - _delete_default_node_pool: use cluster._GcloudCommand() + cmd.args.append + cmd.Issue(timeout=300) - _attach_swap_disk: use gcp_util.GcloudCommand(_GcpZonalResource) for both disk create and instance attach-disk; always zonal (not regional) to target the correct AZ for node-attached swap disks - _delete_disk_by_name: use gcp_util.GcloudCommand for describe/detach/delete Add _GcpZonalResource shim: minimal resource object that pins a single zone for gcloud compute operations. GkeCluster._GcloudCommand promotes zone to region for multi-zone clusters, which is wrong for compute operations that must target a specific AZ. GcloudCommand auto-injects --project and --zone/--region, handles auth token refresh, and uses --format json consistently -- matching PKB standards.
1 parent 8aa8b23 commit 09acb31

1 file changed

Lines changed: 79 additions & 149 deletions

File tree

perfkitbenchmarker/linux_benchmarks/swap_encryption_benchmark.py

Lines changed: 79 additions & 149 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@
7777
from perfkitbenchmarker import sample
7878
from perfkitbenchmarker import vm_util
7979
from perfkitbenchmarker.resources.container_service import kubectl
80+
from perfkitbenchmarker.providers.gcp import util as gcp_util
8081

8182
FLAGS = flags.FLAGS
8283

@@ -388,6 +389,22 @@
388389
_DEFAULT_NODEPOOL = 'default-pool'
389390

390391

392+
class _GcpZonalResource:
393+
"""Minimal resource shim for gcp_util.GcloudCommand on compute operations.
394+
395+
gcp_util.GcloudCommand auto-injects --project and --zone from the resource
396+
object passed to it. GkeCluster._GcloudCommand() handles container/*
397+
operations correctly but also switches --zone → --region for multi-zone
398+
clusters, which is wrong for gcloud compute commands (--region creates
399+
regional resources, not zonal ones). This shim pins a single zone so all
400+
gcloud compute calls target the correct AZ.
401+
"""
402+
403+
def __init__(self, project: str, zone: str) -> None:
404+
self.project = project
405+
self.zone = zone
406+
407+
391408
def _daemonset_yaml(image: str) -> str:
392409
"""Render the privileged benchmark DaemonSet manifest.
393410
@@ -856,12 +873,6 @@ def _create_benchmark_node_pool(cluster) -> None:
856873
is_lssd = _BENCHMARK_LSSD.value or 'lssd' in machine_type.lower()
857874

858875
# Determine zone/region from the cluster object.
859-
zone_flags: list[str] = []
860-
if getattr(cluster, 'zones', None):
861-
zone_flags = ['--zone', cluster.zones[0]]
862-
elif getattr(cluster, 'region', None):
863-
zone_flags = ['--region', cluster.region]
864-
865876
# LSSD configs only need a small boot disk (OS only; swap is on local NVMe).
866877
# Hyperdisk configs need 500 GiB to hit 80 000 IOPS (the IOPS/GiB ratio on
867878
# hyperdisk-balanced is 1:1 up to the provisioned ceiling, so a 100 GiB disk
@@ -870,53 +881,43 @@ def _create_benchmark_node_pool(cluster) -> None:
870881
disk_size_gb = 100 if is_lssd else _BOOT_DISK_SIZE_GB.value
871882

872883
disk_type = _BOOT_DISK_TYPE.value
873-
cmd = [
874-
'gcloud',
884+
885+
# Use PKB's GcloudCommand wrapper: auto-injects --project, --zone/--region,
886+
# and auth token refresh. GkeCluster._GcloudCommand also handles the
887+
# zone → region promotion for multi-zone / regional clusters.
888+
cmd = cluster._GcloudCommand(
875889
'container',
876890
'node-pools',
877891
'create',
878892
_BENCHMARK_NODEPOOL,
879893
'--cluster',
880894
cluster.name,
881-
'--project',
882-
cluster.project,
883-
'--machine-type',
884-
machine_type,
885-
'--image-type',
886-
_NODE_IMAGE_TYPE.value,
887-
'--disk-type',
888-
disk_type,
889-
'--disk-size',
890-
str(disk_size_gb),
891-
'--num-nodes',
892-
'1',
893-
'--node-labels',
894-
f'pkb_nodepool={_BENCHMARK_NODEPOOL}',
895-
'--no-enable-autoupgrade',
896-
'--no-enable-autorepair',
897-
] + zone_flags
895+
)
896+
cmd.flags['machine-type'] = machine_type
897+
cmd.flags['image-type'] = _NODE_IMAGE_TYPE.value
898+
cmd.flags['disk-type'] = disk_type
899+
cmd.flags['disk-size'] = disk_size_gb
900+
cmd.flags['num-nodes'] = 1
901+
cmd.flags['node-labels'] = f'pkb_nodepool={_BENCHMARK_NODEPOOL}'
902+
cmd.args += ['--no-enable-autoupgrade', '--no-enable-autorepair']
898903

899904
# IOPS and throughput provisioning only applies to hyperdisk-* types AND
900905
# only when the boot disk is also the swap device (non-LSSD configs).
901906
# For LSSD machines the boot disk is OS-only; swap is on local NVMe.
902907
# Provisioning 80k IOPS on a 100 GiB boot disk would exceed the
903908
# hyperdisk-balanced per-GiB cap (80 IOPS/GiB × 100 GiB = 8 000 max).
904909
if disk_type.startswith('hyperdisk') and not is_lssd:
905-
cmd += [
906-
'--boot-disk-provisioned-iops',
907-
str(_BOOT_DISK_IOPS.value),
908-
'--boot-disk-provisioned-throughput',
909-
str(
910-
_valid_hyperdisk_throughput(
911-
_BOOT_DISK_IOPS.value, _BOOT_DISK_THROUGHPUT.value
912-
)
913-
),
914-
]
910+
# Hyperdisk boot-disk IOPS/throughput provisioning — not covered by
911+
# GkeCluster._AddNodeParamsToCmd (which only handles secondary disks).
912+
cmd.flags['boot-disk-provisioned-iops'] = _BOOT_DISK_IOPS.value
913+
cmd.flags['boot-disk-provisioned-throughput'] = _valid_hyperdisk_throughput(
914+
_BOOT_DISK_IOPS.value, _BOOT_DISK_THROUGHPUT.value
915+
)
915916

916917
# For LSSD machines, expose local NVMe as raw block devices so fio/mdadm
917918
# can access them directly (go/gke-swap-lssd uses local-nvme-ssd-block).
918919
if is_lssd:
919-
cmd += ['--local-nvme-ssd-block', f'count={_LSSD_COUNT.value}']
920+
cmd.flags['local-nvme-ssd-block'] = f'count={_LSSD_COUNT.value}'
920921

921922
# ── GKE kubelet swap config ───────────────────────────────────────────────
922923
# Per Ajay's review comment (go/pkb-swap-encryption-pr1): the benchmark
@@ -937,7 +938,7 @@ def _create_benchmark_node_pool(cluster) -> None:
937938
)
938939
system_config_tmp.write(kubelet_yaml)
939940
system_config_tmp.flush()
940-
cmd += ['--system-config-from-file', system_config_tmp.name]
941+
cmd.flags['system-config-from-file'] = system_config_tmp.name
941942
logging.info(
942943
'[swap_encryption] kubeletConfig.memorySwapBehavior=%s (written to %s)',
943944
swap_behavior,
@@ -963,9 +964,7 @@ def _create_benchmark_node_pool(cluster) -> None:
963964
# GKE must also initialise the local NVMe devices before marking nodes Ready.
964965
# 1200 s (20 min) covers observed worst-case times on c4-lssd and n4 configs.
965966
try:
966-
stdout, stderr, rc = vm_util.IssueCommand(
967-
cmd, timeout=1200, raise_on_failure=False
968-
)
967+
_, stderr, rc = cmd.Issue(timeout=1200, raise_on_failure=False)
969968
finally:
970969
if system_config_tmp is not None:
971970
try:
@@ -1102,36 +1101,22 @@ def _attach_swap_disk(cluster) -> None:
11021101
disk_size_gb,
11031102
disk_type,
11041103
)
1105-
create_cmd = [
1106-
'gcloud',
1107-
'compute',
1108-
'disks',
1109-
'create',
1110-
disk_name,
1111-
'--project',
1112-
project,
1113-
'--zone',
1114-
zone,
1115-
'--type',
1116-
disk_type,
1117-
'--size',
1118-
f'{disk_size_gb}GB',
1119-
'--quiet',
1120-
]
1121-
if disk_type.startswith('hyperdisk'):
1122-
create_cmd += [
1123-
'--provisioned-iops',
1124-
str(_BOOT_DISK_IOPS.value),
1125-
'--provisioned-throughput',
1126-
str(
1127-
_valid_hyperdisk_throughput(
1128-
_BOOT_DISK_IOPS.value, _BOOT_DISK_THROUGHPUT.value
1129-
)
1130-
),
1131-
]
1132-
_, stderr, rc = vm_util.IssueCommand(
1133-
create_cmd, timeout=120, raise_on_failure=False
1104+
# Use PKB's GcloudCommand via _GcpZonalResource: auto-injects --project
1105+
# and --zone (always zonal — gcloud compute --region creates regional
1106+
# resources, which is not what we want for a node-attached swap disk).
1107+
gcp_res = _GcpZonalResource(project, zone)
1108+
create_cmd = gcp_util.GcloudCommand(
1109+
gcp_res, 'compute', 'disks', 'create', disk_name
11341110
)
1111+
create_cmd.flags['type'] = disk_type
1112+
create_cmd.flags['size'] = f'{disk_size_gb}GB'
1113+
create_cmd.args.append('--quiet')
1114+
if disk_type.startswith('hyperdisk'):
1115+
create_cmd.flags['provisioned-iops'] = _BOOT_DISK_IOPS.value
1116+
create_cmd.flags['provisioned-throughput'] = _valid_hyperdisk_throughput(
1117+
_BOOT_DISK_IOPS.value, _BOOT_DISK_THROUGHPUT.value
1118+
)
1119+
_, stderr, rc = create_cmd.Issue(timeout=120, raise_on_failure=False)
11351120
if rc != 0:
11361121
raise errors.Benchmarks.RunError(
11371122
f'[swap_encryption] Failed to create swap disk {disk_name}: {stderr}'
@@ -1141,25 +1126,13 @@ def _attach_swap_disk(cluster) -> None:
11411126
logging.info(
11421127
'[swap_encryption] Attaching swap disk %s to %s', disk_name, instance_name
11431128
)
1144-
attach_cmd = [
1145-
'gcloud',
1146-
'compute',
1147-
'instances',
1148-
'attach-disk',
1149-
instance_name,
1150-
'--project',
1151-
project,
1152-
'--zone',
1153-
zone,
1154-
'--disk',
1155-
disk_name,
1156-
'--device-name',
1157-
'pkb-swap',
1158-
'--quiet',
1159-
]
1160-
_, stderr, rc = vm_util.IssueCommand(
1161-
attach_cmd, timeout=120, raise_on_failure=False
1129+
attach_cmd = gcp_util.GcloudCommand(
1130+
gcp_res, 'compute', 'instances', 'attach-disk', instance_name
11621131
)
1132+
attach_cmd.flags['disk'] = disk_name
1133+
attach_cmd.flags['device-name'] = 'pkb-swap'
1134+
attach_cmd.args.append('--quiet')
1135+
_, stderr, rc = attach_cmd.Issue(timeout=120, raise_on_failure=False)
11631136
if rc != 0:
11641137
raise errors.Benchmarks.RunError(
11651138
f'[swap_encryption] Failed to attach swap disk to {instance_name}: '
@@ -1179,22 +1152,12 @@ def _delete_disk_by_name(disk_name: str, project: str, zone: str) -> bool:
11791152
leaked. Returns True if the disk is gone (deleted or already absent).
11801153
"""
11811154
for attempt in range(1, 5):
1182-
users, _, rc = vm_util.IssueCommand(
1183-
[
1184-
'gcloud',
1185-
'compute',
1186-
'disks',
1187-
'describe',
1188-
disk_name,
1189-
'--project',
1190-
project,
1191-
'--zone',
1192-
zone,
1193-
'--format=value(users)',
1194-
],
1195-
timeout=60,
1196-
raise_on_failure=False,
1155+
gcp_res = _GcpZonalResource(project, zone)
1156+
describe_cmd = gcp_util.GcloudCommand(
1157+
gcp_res, 'compute', 'disks', 'describe', disk_name
11971158
)
1159+
describe_cmd.flags['format'] = 'value(users)'
1160+
users, _, rc = describe_cmd.Issue(timeout=60, raise_on_failure=False)
11981161
if rc != 0:
11991162
logging.info(
12001163
'[swap_encryption] Swap disk %s not present — nothing to delete',
@@ -1207,40 +1170,17 @@ def _delete_disk_by_name(disk_name: str, project: str, zone: str) -> bool:
12071170
logging.info(
12081171
'[swap_encryption] Detaching swap disk %s from %s', disk_name, inst
12091172
)
1210-
vm_util.IssueCommand(
1211-
[
1212-
'gcloud',
1213-
'compute',
1214-
'instances',
1215-
'detach-disk',
1216-
inst,
1217-
'--project',
1218-
project,
1219-
'--zone',
1220-
zone,
1221-
'--disk',
1222-
disk_name,
1223-
'--quiet',
1224-
],
1225-
timeout=120,
1226-
raise_on_failure=False,
1173+
detach_cmd = gcp_util.GcloudCommand(
1174+
gcp_res, 'compute', 'instances', 'detach-disk', inst
12271175
)
1228-
_, derr, drc = vm_util.IssueCommand(
1229-
[
1230-
'gcloud',
1231-
'compute',
1232-
'disks',
1233-
'delete',
1234-
disk_name,
1235-
'--project',
1236-
project,
1237-
'--zone',
1238-
zone,
1239-
'--quiet',
1240-
],
1241-
timeout=180,
1242-
raise_on_failure=False,
1176+
detach_cmd.flags['disk'] = disk_name
1177+
detach_cmd.args.append('--quiet')
1178+
detach_cmd.Issue(timeout=120, raise_on_failure=False)
1179+
delete_cmd = gcp_util.GcloudCommand(
1180+
gcp_res, 'compute', 'disks', 'delete', disk_name
12431181
)
1182+
delete_cmd.args.append('--quiet')
1183+
_, derr, drc = delete_cmd.Issue(timeout=180, raise_on_failure=False)
12441184
if drc == 0:
12451185
logging.info('[swap_encryption] Swap disk deleted: %s', disk_name)
12461186
return True
@@ -1281,31 +1221,21 @@ def _delete_default_node_pool(cluster) -> None:
12811221
requirement that a cluster must have at least one nodepool at creation time.
12821222
Removing it stops the clock on its cost immediately.
12831223
"""
1284-
zone_flags: list[str] = []
1285-
if getattr(cluster, 'zones', None):
1286-
zone_flags = ['--zone', cluster.zones[0]]
1287-
elif getattr(cluster, 'region', None):
1288-
zone_flags = ['--region', cluster.region]
1289-
1290-
cmd = [
1291-
'gcloud',
1224+
# Use PKB's GcloudCommand: auto-injects --project, --zone/--region.
1225+
cmd = cluster._GcloudCommand(
12921226
'container',
12931227
'node-pools',
12941228
'delete',
12951229
_DEFAULT_NODEPOOL,
12961230
'--cluster',
12971231
cluster.name,
1298-
'--project',
1299-
cluster.project,
1300-
'--quiet',
1301-
] + zone_flags
1232+
)
1233+
cmd.args.append('--quiet')
13021234

13031235
logging.info(
13041236
'[swap_encryption] Deleting default nodepool: %s', _DEFAULT_NODEPOOL
13051237
)
1306-
stdout, stderr, rc = vm_util.IssueCommand(
1307-
cmd, timeout=300, raise_on_failure=False
1308-
)
1238+
_, stderr, rc = cmd.Issue(timeout=300, raise_on_failure=False)
13091239
if rc != 0:
13101240
logging.warning(
13111241
'[swap_encryption] Could not delete default nodepool (rc=%d): %s',

0 commit comments

Comments
 (0)