Skip to content

fix/SafeLabel-Owner-GCP for Issue #6218 #6219

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jimmycgz wants to merge 6 commits into
base: master
Choose a base branch
from
Open

fix/SafeLabel-Owner-GCP for Issue #6218 #6219

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
fa3c44a
fix gcp subnet name from vpc
jimmycgz May 6, 2025
81e50fd
clean up
jimmycgz May 6, 2025
3a5ca48
Merge pull request #2 from jimmycgz/fix-gcp-subnet-name
jimmycgz May 6, 2025
acb1caf
Merge branch 'GoogleCloudPlatform:master' into master
jimmycgz Oct 23, 2025
2440b80
fix: Sanitize owner label to comply with GCP label requirements
jimmycgz Nov 17, 2025
6855c74
revert: remove unrelated gce_network changes per PR review
jimmy-cui-sada Nov 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion perfkitbenchmarker/benchmark_spec.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1253,7 +1253,7 @@ def _GetResourceDict(self, time_format, timeout_minutes=None):
'create_time_utc': now_utc.strftime(time_format),
'benchmark': self.name,
'perfkit_uuid': self.uuid,
'owner': FLAGS.owner,
'owner': self._SafeLabelKeyOrValue(FLAGS.owner),
Copy link
Collaborator

@hubatish hubatish Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice job finding the right place in code for this! Only comment is as below to call this method for all tags & not just owner.

'benchmark_uid': self.uid,
}

Expand Down
34 changes: 22 additions & 12 deletions perfkitbenchmarker/providers/gcp/gce_network.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -925,13 +925,21 @@ def __init__(self, network_spec: GceNetworkSpec):
self.is_existing_network = True
self.subnet_names = []
self.primary_subnet_name = None
if network_spec.subnet_names:
self.subnet_names = network_spec.subnet_names
elif gcp_flags.GCE_NETWORK_NAMES.value:
self.subnet_names = gcp_flags.GCE_NETWORK_NAMES.value
self.network_name = None

# Determine the network name first
if gcp_flags.GCE_NETWORK_NAMES.value:
self.network_name = gcp_flags.GCE_NETWORK_NAMES.value[0]
else:
self.subnet_names = self._MakeGceNetworkName()
self.network_name = self._MakeGceNetworkName()
self.is_existing_network = False

# Then handle subnet names
if network_spec.subnet_names:
self.subnet_names = network_spec.subnet_names
elif not self.is_existing_network:
# If we're creating a new network, use the network name as subnet name by default
self.subnet_names = [self.network_name]
if not isinstance(self.subnet_names, list):
self.subnet_names = [self.subnet_names]
self.primary_subnet_name = self.subnet_names[0]
Expand All @@ -940,28 +948,30 @@ def __init__(self, network_spec: GceNetworkSpec):
self.subnet_resources = []
mode = gcp_flags.GCE_NETWORK_TYPE.value
self.subnet_resource = None
# Create network resources using network_name
if not self.is_existing_network or mode == 'legacy':
for name in self.subnet_names:
Copy link
Collaborator

@hubatish hubatish Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to send this networking code in the same PR? It's not mentioned in the description at all & I'm not sure if we can safely delete subnet_names / replace it with only a singular network_name. I'd have to search through our callers to see if we set multiple anywhere (unless you can convince me the code for this is dead). Either way that's complicated enough it seems like a separate PR.

Copy link
Author

@jimmycgz jimmycgz Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for catching that. I mixed another fix to this PR, so now cleaned up.

self.network_resources.append(
GceNetworkResource(name, mode, self.project, self.mtu)
)
self.network_resources.append(
GceNetworkResource(self.network_name, mode, self.project, self.mtu)
)

# Create subnet resources using subnet_names within the specified network
if (self.is_existing_network and mode != 'legacy') or (mode == 'custom'):
subnet_region = util.GetRegionFromZone(network_spec.zone)
for name in self.subnet_names:
self.subnet_resources.append(
GceSubnetResource(
name, name, subnet_region, self.cidr, self.project
name, self.network_name, subnet_region, self.cidr, self.project
)
)
self.subnet_resource = GceSubnetResource(
self.primary_subnet_name,
self.primary_subnet_name,
self.network_name,
subnet_region,
self.cidr,
self.project,
)
self.network_resource = GceNetworkResource(
self.primary_subnet_name, mode, self.project, self.mtu
self.network_name, mode, self.project, self.mtu
)
# Stage FW rules.
self.all_nets = self._GetNetworksFromSpec(
Expand Down