Repository navigation
Conversation
| self.region = str(region or self.config.get("region")).lower() | ||
| zone = zone or self.config.get("zone") or f"{self.region}-1" | ||
| self.zone = str(zone).lower() | ||
| configured_zone = zone or self.config.get("zone") |
There was a problem hiding this comment.
@blackboxsw I went back and forth on preserving the existing behavior where, without an explicit zone, pycloudlib selects the first available subnet. My current proposed change here would preserve existing behavior when no zone is explicitly provided by the user.
But I'm having second thoughts. I am leaning toward always filtering subnets based on self.zone. A subnet in a different zone cannot be used to create an instance in any case. When no zone is configured, pycloudlib already defaults self.zone to {region}-1; selecting a subnet in that zone seems more correct than selecting the first subnet returned by the API.
This would let us remove the _zone_provided / configured_zone workaround. What do you think?
There was a problem hiding this comment.
@Gisaldjo thank you for this discussion. I think your second proposal makes more sense. Tracking whether or not _zone_provided does add complexity and more 'magic' as to how subnets are filtered or created. Let's define the zone selection based on self.zone and default to {region}-1 if unset. The _select_subnet_by_zone parameter does feel like it makes the magic zone selection a bit more complicated and makes call signatures more complex than they need to be. I think just passing in self.zone which may default to {region}-1 if unset would make this logic simpler throughout. If we have overrides to provide during _Subnet.discover in the future, we can provide a specific zone value to that method to get alternative discovery needs met.
There was a problem hiding this comment.
If we need to break from existing magic detection behavior, I'm happy with us bumping the major version to account for that behavior change. As it is I don't think integration tests in cloud-init or ubuntu-advantage-client are specific enough to care. Our pycloudlib.toml files for both projects define a zone value, so changing how default selection behaves in absence of a configured zone shouldn't break any consumers I am aware of.
There was a problem hiding this comment.
One thing to confirm CPC-side is whether there are any supported test harnesses that expect to avoid providing a configured "zone" for pycloudlib test setup. In either case, if a major bump in version from 11 -> 12 is just a number if we want to be safe about behavior changes here with unspecified values.
There was a problem hiding this comment.
@blackboxsw I’ve updated the PR to always select a subnet in the instance’s resolved zone, including the existing -1 default.
I tested the pycloudlib changes against our test harness and validated that they work both in our current usage (default vpc, try instance launch on all zones) and in the case that would fail due to subnet selection (custom vpc, try on all zones).
Happy to do a major bump to keep it extra safe but it's likely not needed.
Subnet discovery for a custom VPC picked the first subnet in the VPC, so instance creation failed when that subnet was in another zone. Select a subnet in the instance zone (explicit, configured, or <region>-1) instead, creating <vpc>-<zone>-subnet if none exists.
f7635a7 to
ce1a398
Compare
blackboxsw
left a comment
blackboxsw
left a comment
There was a problem hiding this comment.
Thank you for this update @Gisaldjo. this looks good, one nit is that we aren't cleaning up the created subnet when we are using pre-existing custom VPCs where we had to create a subnet in the right zone. Other than that, this looks very good.
| subnet = _Subnet.discover(client, vpc_id=vpc["id"]) | ||
| subnet = _Subnet.discover(client, vpc_id=vpc["id"], zone=zone) | ||
| except IBMException: | ||
| subnet = _Subnet.create( |
There was a problem hiding this comment.
In cases where we have a pre-existing custom VPC, and we don't discover a subnet in the proper zone, we have created a new _Subnet, but we don't track self.subnets_created to ensure that pycloudlib-created resources are removed during "clean". We should probably track those created subnets that were created in the proper zone due to VPC.from_existing so that we can loop through any created subnets because pycloudlib isn't going to call vpc.delete() on pre-existing VPCs.
There was a problem hiding this comment.
Nice catch! I updated the PR to track the created subnets for existing VPCs.
When a custom VPC has no subnet in the instance zone, one is created in it. That VPC is not tracked for deletion, so the subnet was left behind after clean(). Track these subnets in created_subnets and delete them in clean(), leaving the VPC and its other subnets intact.
3 participants
Description
Custom IBM VPC discovery previously selected the first subnet in the VPC, regardless of the instance zone. Instance creation failed when the zones differed.
Select a subnet in the instance's current zone, creating one there if none exists. Initial zone resolution is unchanged: explicit argument, configuration, then
<region>-1.Default-VPC selection is unchanged and was already zone-aware.
Additional Context and Relevant Issues
Subnets created in existing VPCs use
<vpc>-<zone>-subnet. This longer name can exceed IBM's subnet-name limit for VPC names whose previous<vpc>-subnetname fit. Name truncation is not addressed in this PR.Creating a missing subnet requires subnet-creation permissions and available address space.
Updates IBM documentation and bumps the version to
1!11.1.9.Test Steps