[subnet_division] Fix overlapping subnets with different prefix sizes #864#1252
Open
Piyushkhobragade wants to merge 1 commit intoopenwisp:masterfrom
Open
[subnet_division] Fix overlapping subnets with different prefix sizes #864#1252Piyushkhobragade wants to merge 1 commit intoopenwisp:masterfrom
Piyushkhobragade wants to merge 1 commit intoopenwisp:masterfrom
Conversation
Contributor
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReworks subnet-division provisioning to be rule-aware and incremental. get_max_subnet now prefers computing the maximum from subnets already provisioned for the given division_rule (using IPNetwork normalization) and, if none exist, scans the master subnet for the next available subnet (using IPNetwork/IPSet), raising ValidationError when space is exhausted. create_subnets creates only the remaining subnets one-by-one, saving each and creating SubnetDivisionIndex entries. create_ips saves each IpAddress individually before creating corresponding SubnetDivisionIndex entries. Minor imports added (gettext_lazy as _, transaction) and attrgetter is imported inline in get_config. Sequence Diagram(s)sequenceDiagram
participant Rule as DivisionRuleHandler
participant IPTools as IPNetwork/IPSet
participant DB as Database(Subnet, IpAddress, SubnetDivisionIndex)
Note over Rule,DB: Provisioning start
Rule->>DB: query Subnet WHERE division_rule = X
alt existing subnets for rule
DB-->>Rule: return existing subnets
Rule->>IPTools: normalize and compute max/next subnet from returned subnets
else no subnets for rule
Rule->>DB: fetch master subnet for config
DB-->>Rule: return master subnet
Rule->>IPTools: iterate master to find next unused subnets using IPSet
end
loop for each subnet to create
Rule->>DB: create/save Subnet (single)
DB-->>Rule: return Subnet id
Rule->>DB: create SubnetDivisionIndex for subnet
loop per IP to provision in subnet
Rule->>DB: create/save IpAddress (single)
DB-->>Rule: return IpAddress id
Rule->>DB: create SubnetDivisionIndex for IP
end
end
Note over Rule,DB: destroy_provisioned_subnets_ips deletes Subnet rows -> DB cascades IpAddress and Index deletions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/subnet_division/rule_types/base.py`:
- Around line 151-199: The class BaseSubnetDivisionRuleType contains duplicate
method definitions for create_ips and destroy_provisioned_subnets_ips; remove
the earlier (dead) duplicates so each method is defined only once and the later
authoritative implementations remain. Locate the duplicate create_ips and
destroy_provisioned_subnets_ips methods in BaseSubnetDivisionRuleType, delete
the redundant (overridden) versions, and ensure the remaining methods correctly
reference symbols used inside them (create_ips uses IpAddress,
SubnetDivisionIndex and division_rule fields; destroy_provisioned_subnets_ips
uses cls.get_config, rule type string, config.subnetdivisionindex_set and
Subnet) so no imports or references are broken. After removal, run tests/lint to
ensure no missing references and update or consolidate any docstrings/comments
if needed.
- Around line 249-255: The existing_count query is overcounting because the
filter on subnetdivisionindex__rule joins multiple SubnetDivisionIndex rows per
subnet; change the counting query in the block using existing_count (the
Subnet.objects.filter(... subnetdivisionindex__rule=division_rule).count() call)
to count distinct subnets (e.g. add .distinct() or use
.values('id').distinct().count()) so existing_count reflects unique Subnet
instances before comparing to division_rule.number_of_subnets and returning
early; this adjustment ensures create_ips (which adds multiple indexes per
subnet) won't inflate the count.
- Around line 220-225: The current get_max_subnet() uses
existing_rule_subnets.order_by("subnet").last(), which relies on DB text
ordering and is backend-dependent; instead, fetch the matching subnets
(existing_rule_subnets) into Python (e.g., values_list('subnet', flat=True)),
parse them with the ipaddress module (ipaddress.ip_network) and compute the
numeric maximum (max(..., key=lambda n: int(n.network_address))). Return that
network as a string so .next() allocates from the correct numeric maximum;
update the code around existing_rule_subnets and the .order_by("subnet").last()
usage accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
openwisp_controller/subnet_division/rule_types/base.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/subnet_division/rule_types/base.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/subnet_division/rule_types/base.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/subnet_division/rule_types/base.py
🧬 Code graph analysis (1)
openwisp_controller/subnet_division/rule_types/base.py (3)
openwisp_controller/subnet_division/models.py (1)
SubnetDivisionIndex(12-15)openwisp_controller/subnet_division/rule_types/vpn.py (1)
destroy_provisioned_subnets_ips(57-65)openwisp_controller/subnet_division/rule_types/device.py (1)
destroy_provisioned_subnets_ips(39-43)
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
openwisp_controller/subnet_division/rule_types/base.py (1)
151-199:⚠️ Potential issue | 🟠 MajorRemove duplicate method definitions to avoid dead code and drift.
create_ipsanddestroy_provisioned_subnets_ipsare defined here, but they are redefined again at Line 301 and Line 332. Python keeps the later definitions, so this block is unreachable and risky to maintain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/subnet_division/rule_types/base.py` around lines 151 - 199, The file defines duplicate methods create_ips and destroy_provisioned_subnets_ips (the earlier definitions in openwisp_controller/subnet_division/rule_types/base.py) which are shadowed by later redefinitions; remove the redundant earlier definitions (or merge any differing logic) so there is exactly one canonical implementation of create_ips and one of destroy_provisioned_subnets_ips in the class (refer to the methods named create_ips and destroy_provisioned_subnets_ips within the same module/class), update any callers if behavior changed, and run tests to confirm no regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/subnet_division/rule_types/base.py`:
- Around line 273-275: The loop that checks subnet containment currently uses
break and thus can silently return fewer subnets than
division_rule.number_of_subnets; replace the break with raising a clear
exception (e.g., ValueError or a specific SubnetAllocationError) when not
ip_network(str(required_subnet)).subnet_of(master_subnet.subnet) so callers know
allocation failed and include context in the message (requested
division_rule.number_of_subnets, current subnet_id/existing_count, and
master_subnet.subnet) to aid debugging; update any callers/tests to expect this
failure mode if applicable.
- Around line 169-187: The SubnetDivisionIndex objects are being created with
ip_id from unsaved IpAddress instances (IpAddress.id is None) inside create_ips,
breaking the FK; to fix, persist the IpAddress rows first and only then create
SubnetDivisionIndex entries: collect IpAddress instances into generated_ips,
call IpAddress.objects.bulk_create(generated_ips) (or save individually),
refresh their IDs (e.g., reload or use saved instances/transaction.on_commit to
ensure IDs are assigned), then build and persist SubnetDivisionIndex records
using the real ip.id values (refer to the create_ips function, generated_ips
list, IpAddress and SubnetDivisionIndex classes and the existing tasks.py
transaction.on_commit pattern as a reference).
---
Duplicate comments:
In `@openwisp_controller/subnet_division/rule_types/base.py`:
- Around line 151-199: The file defines duplicate methods create_ips and
destroy_provisioned_subnets_ips (the earlier definitions in
openwisp_controller/subnet_division/rule_types/base.py) which are shadowed by
later redefinitions; remove the redundant earlier definitions (or merge any
differing logic) so there is exactly one canonical implementation of create_ips
and one of destroy_provisioned_subnets_ips in the class (refer to the methods
named create_ips and destroy_provisioned_subnets_ips within the same
module/class), update any callers if behavior changed, and run tests to confirm
no regressions.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
openwisp_controller/subnet_division/rule_types/base.py
📜 Review details
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/subnet_division/rule_types/base.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/subnet_division/rule_types/base.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/subnet_division/rule_types/base.py
🔇 Additional comments (2)
openwisp_controller/subnet_division/rule_types/base.py (2)
219-233: Backend-independent max-subnet selection looks correct.Using
values_list(...).distinct()and numericIPNetworkcomparison avoids DB collation/order differences.
257-265: Distinct subnet counting fix is correct.Counting distinct subnet IDs prevents join-multiplication from
SubnetDivisionIndexrows and aligns the guard with actual subnet count.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openwisp_controller/subnet_division/rule_types/base.py (1)
151-199: 🛠️ Refactor suggestion | 🟠 MajorRemove shadowed duplicate methods to avoid dead and divergent logic.
create_ipsanddestroy_provisioned_subnets_ipsare each defined twice (Line 151/190 and Line 303/335). The earlier versions are overridden and never executed, which leaves dead logic in the class and increases regression risk.♻️ Proposed cleanup
- `@staticmethod` - def create_ips(config, division_rule, generated_subnets, generated_indexes): - ... - - `@classmethod` - def destroy_provisioned_subnets_ips(cls, instance, **kwargs): - ... + # Keep only the later authoritative implementations at Line 303+ and Line 335+Also applies to: 303-341
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/subnet_division/rule_types/base.py` around lines 151 - 199, The class defines duplicate methods create_ips and destroy_provisioned_subnets_ips (the early definitions at the top of the class are shadowed by later ones), so remove or consolidate the redundant definitions to avoid dead/divergent logic: keep the correct implementation (choose the later or merged version) and delete the duplicate earlier methods named create_ips and destroy_provisioned_subnets_ips, updating any internal references to use the single canonical method names so only one implementation of each exists.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/subnet_division/rule_types/base.py`:
- Around line 271-277: The loop in create_subnets may pick a required_subnet
that overlaps other already-allocated subnets because it only checks containment
against master_subnet; update the loop that iterates subnet_id to, for each
candidate required_subnet, check against existing allocated subnets (and
generated_subnets) for overlaps (using ip_network(...).overlaps(...) or
equivalent) and skip forward (required_subnet =
IPNetwork(str(required_subnet)).next()) until you find a non-overlapping
candidate or run out of space; after each advance re-check containment against
master_subnet.subnet and raise the same ValidationError if no free space
remains. Ensure you reference required_subnet, generated_subnets,
existing_count, division_rule.number_of_subnets and master_subnet.subnet when
implementing the checks.
---
Duplicate comments:
In `@openwisp_controller/subnet_division/rule_types/base.py`:
- Around line 151-199: The class defines duplicate methods create_ips and
destroy_provisioned_subnets_ips (the early definitions at the top of the class
are shadowed by later ones), so remove or consolidate the redundant definitions
to avoid dead/divergent logic: keep the correct implementation (choose the later
or merged version) and delete the duplicate earlier methods named create_ips and
destroy_provisioned_subnets_ips, updating any internal references to use the
single canonical method names so only one implementation of each exists.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
openwisp_controller/subnet_division/rule_types/base.py
📜 Review details
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/subnet_division/rule_types/base.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/subnet_division/rule_types/base.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/subnet_division/rule_types/base.py
🔇 Additional comments (1)
openwisp_controller/subnet_division/rule_types/base.py (1)
258-266: Distinct subnet counting fix is correct.Using
.values("id").distinct().count()avoids overcounting caused by the join withsubnetdivisionindex__ruleand correctly reflects unique provisioned subnets for the rule.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/subnet_division/rule_types/base.py (1)
116-132: 🛠️ Refactor suggestion | 🟠 MajorConsider wrapping provisioning logic in an atomic transaction.
If
create_subnetspartially succeeds (e.g., creates 2 of 3 subnets) before raisingValidationError, the already-saved subnets persist while thegenerated_indexesbulk_create at line 131 never runs. This leaves orphaned subnets without correspondingSubnetDivisionIndexentries, and a retry could hit theUniqueConstraintonSubnetDivisionIndex.Wrapping the provisioning in
transaction.atomic()would ensure all-or-nothing behavior:Proposed fix
`@classmethod` def create_subnets_ips(cls, instance, division_rule, **kwargs): try: config = cls.get_config(instance) except (AttributeError, ObjectDoesNotExist): return - master_subnet = division_rule.master_subnet - max_subnet = cls.get_max_subnet(master_subnet, division_rule) - generated_indexes = [] - generated_subnets = cls.create_subnets( - config, division_rule, max_subnet, generated_indexes - ) - generated_ips = cls.create_ips( - config, division_rule, generated_subnets, generated_indexes - ) - SubnetDivisionIndex.objects.bulk_create(generated_indexes) - return {"subnets": generated_subnets, "ip_addresses": generated_ips} + with transaction.atomic(): + master_subnet = division_rule.master_subnet + max_subnet = cls.get_max_subnet(master_subnet, division_rule) + generated_indexes = [] + generated_subnets = cls.create_subnets( + config, division_rule, max_subnet, generated_indexes + ) + generated_ips = cls.create_ips( + config, division_rule, generated_subnets, generated_indexes + ) + SubnetDivisionIndex.objects.bulk_create(generated_indexes) + return {"subnets": generated_subnets, "ip_addresses": generated_ips}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_controller/subnet_division/rule_types/base.py` around lines 116 - 132, The create_subnets_ips flow can leave partial DB state if create_subnets or create_ips raise after some writes; wrap the entire provisioning sequence in a single atomic transaction so either all created subnets, generated_indexes (SubnetDivisionIndex.bulk_create) and ips persist or none do. Specifically, surround the logic inside create_subnets_ips (use transaction.atomic()) around calls to cls.create_subnets, cls.create_ips and SubnetDivisionIndex.objects.bulk_create to ensure all-or-nothing commits and rollbacks on exceptions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/subnet_division/rule_types/base.py`:
- Line 5: The inline import of attrgetter inside the function is redundant
because attrgetter is already imported at module top (from operator import
attrgetter); remove the local inline import statement (the one importing
attrgetter around the code that uses attrgetter) and ensure all uses reference
the module-level attrgetter; look for the inline import near the code that calls
attrgetter (around the function/method that performs sorting/filtering) and
delete that local import to avoid duplication.
- Around line 201-203: Move the repeated inline imports of ValidationError and
IPSet to the top of the module: add "from django.core.exceptions import
ValidationError" and "from netaddr import IPSet" to the module-level imports,
then remove the inline imports currently inside the functions/classes (the
duplicates around the uses in functions referencing ValidationError and IPSet in
rule_types.base, e.g. the blocks that import ValidationError at the two
locations and IPSet at the two locations). Ensure all references to
ValidationError and IPSet still work after removing the inline imports.
- Around line 162-163: Remove the redundant inline import of ObjectDoesNotExist
inside the function in openwisp_controller.subnet_division.rule_types.base (the
local "from django.core.exceptions import ObjectDoesNotExist" at ~line 162);
rely on the module-level import already present (imported near the top of the
file), and simply use ObjectDoesNotExist where needed in the function body
without re-importing it.
- Around line 191-193: get_max_subnet currently returns
str(candidate.previous()) relying on wrap-around behavior and subsequent .next()
in create_subnets to get back into the master range; make this explicit and less
error-prone by returning the candidate directly (not candidate.previous()) from
get_max_subnet and update create_subnets to accept that candidate as the
starting subnet and use candidate.next()/candidate as appropriate, or if you
prefer minimal change, add a clear comment in get_max_subnet explaining the
intentional previous()/next() wrap-around behavior and why validation via
subnet_of(master_subnet.subnet) will keep results in bounds; refer to
get_max_subnet, create_subnets, master_network, and the
candidate.previous()/candidate.next() usage when implementing the change.
---
Outside diff comments:
In `@openwisp_controller/subnet_division/rule_types/base.py`:
- Around line 116-132: The create_subnets_ips flow can leave partial DB state if
create_subnets or create_ips raise after some writes; wrap the entire
provisioning sequence in a single atomic transaction so either all created
subnets, generated_indexes (SubnetDivisionIndex.bulk_create) and ips persist or
none do. Specifically, surround the logic inside create_subnets_ips (use
transaction.atomic()) around calls to cls.create_subnets, cls.create_ips and
SubnetDivisionIndex.objects.bulk_create to ensure all-or-nothing commits and
rollbacks on exceptions.
ℹ️ Review info
Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b45a08a4-7a53-4d8a-9e72-7a8d807af4fc
📒 Files selected for processing (1)
openwisp_controller/subnet_division/rule_types/base.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/subnet_division/rule_types/base.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/subnet_division/rule_types/base.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/subnet_division/rule_types/base.py
🔇 Additional comments (3)
openwisp_controller/subnet_division/rule_types/base.py (3)
276-294: LGTM!The fix correctly saves each
IpAddressinstance before creating the correspondingSubnetDivisionIndex, ensuring the foreign key (ip_id) has a valid value. This addresses the previously identified critical FK integrity issue.
297-304: LGTM!The
destroy_provisioned_subnets_ipsmethod correctly uses cascade deletion through theSubnetmodel, which will automatically clean up relatedIpAddressandSubnetDivisionIndexrecords.
223-250: Potential race condition when concurrent requests provision subnets simultaneously.The
consumed_spacesnapshot is computed from a single database query (lines 223-228), but another concurrent request could allocate the same subnet between this check and thesave()call at line 250. Sincetransaction.on_commit()executes the provisioning after the triggering transaction commits, multiple threads can execute this method concurrently with no locking mechanism. If theSubnetmodel in openwisp_ipam lacks a unique constraint on(subnet, master_subnet), duplicate subnet allocations could occur. Even if a constraint exists, an unhandledIntegrityErrorwould crash the operation since there is no error handling.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| for candidate in master_network.subnet(prefixlen=division_rule.size): | ||
| if not consumed_space.intersection(IPSet([candidate])): | ||
| return str(candidate.previous()) |
There was a problem hiding this comment.
CRITICAL: Logic error when master subnet has no existing subnets.
When consumed_space is empty (no subnets exist in the master subnet), the first candidate will be the first available subnet (e.g., 10.0.0.0/28). Calling candidate.previous() returns a subnet outside the master subnet range, which causes create_subnets to fail when it calls IPNetwork(str(max_subnet)).next().
Fix: Return str(candidate) instead of str(candidate.previous()) when the candidate is the first available subnet.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (1 file)
|
CI Failure AnalysisHello @nemesifier, There are two distinct failures in the CI logs:
Remediation
|
Contributor
|
ping @Piyushkhobragade |
Code Style FailuresHello @Piyushkhobragade, The CI has detected several code style issues. Please run Specifically, the following files have line length issues (E501) or complexity issues (E704, E301, E302, W391, E203, F401, E402) that require manual correction:
Please address these manually and ensure all files comply with the project's coding standards. |
…penwisp#864 Refactored the allocation logic to use netaddr.IPSet for mathematical gap-filling instead of database sorting. Added guards for exact-match /32 rules and scoped exhaustion logic per device config to resolve test regressions. Fixes openwisp#864
Piyushkhobragade
force-pushed
the
issues/864-subnet-division-overlap
branch
from
1b255a9 to
511a951
Compare
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_controller/subnet_division/rule_types/base.py`:
- Around line 136-138: The current anchor detection using string containment
("Reserved" in (a_qs.first().name or "")) is fragile; modify the Subnet model to
add a dedicated boolean field (e.g., is_anchor) or a unique, unlikely name
prefix (e.g., "__anchor__") and update the lookup in the SubnetDivision logic to
check that explicit marker instead of substring-matching; specifically, add the
new field and migration on the Subnet model, set it for existing anchor subnets,
and change the check in the method that contains a_qs =
Subnet.objects.filter(subnet=max_s, master_subnet=ms) to test
a_qs.filter(is_anchor=True).exists() (or check name.startswith("__anchor__") if
you choose the prefix approach), and update any other codepaths that currently
rely on the "Reserved" name heuristic.
- Around line 147-165: The master-subnet-exhausted branch in the subnet
provisioning logic (the ip_network(...).subnet_of(ms.subnet) check and the
notify.send block in base.py) currently logs with logger.info and breaks, which
silently returns partial results; change logger.info to logger.error and instead
of break raise a descriptive exception (e.g., RuntimeError or a specific
SubnetProvisionError) that includes ms.name and ms.subnet so callers can detect
failure; keep the notify.send call but replace the silent break with the
exception to surface the error to callers.
- Around line 226-233: The base destroy_provisioned_subnets_ips currently
deletes Subnet rows by only filtering subnetdivisionindex_set by rule__type;
update it to also filter by rule__master_subnet_id using the instance context
(like VpnSubnetDivisionRuleType does) to avoid removing subnets belonging to
other master subnets. In practice, inside destroy_provisioned_subnets_ips (and
using cls.get_config(instance)), retrieve the master_subnet_id from the instance
(or config) and add rule__master_subnet_id=<that id> to the QuerySet filter
alongside rule__type so the Subnet.objects.filter(id__in=s_ids).delete() only
targets subnets for that specific master subnet. Ensure you reference
subnetdivisionindex_set, rule__type, rule__master_subnet_id, get_config, and
Subnet when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 47d21e6a-15b7-4ddc-be4f-5a749ba7a655
📒 Files selected for processing (1)
openwisp_controller/subnet_division/rule_types/base.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}
📄 CodeRabbit inference engine (Custom checks)
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp}: Flag potential security vulnerabilities in code
Avoid unnecessary comments or docstrings for code that is already clear
Code formatting is compact and readable. Do not add excessive blank lines, especially inside function or method bodies
Flag unused or redundant code
Ensure variables, functions, classes, and files have descriptive and consistent names
New code must handle errors properly: log errors that cannot be resolved by the user with error level, log unusual conditions with warning level, log important background actions with info level, and provide user-facing messages for errors that the user can solve autonomously
Files:
openwisp_controller/subnet_division/rule_types/base.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sql}
📄 CodeRabbit inference engine (Custom checks)
Flag obvious performance regressions, such as heavy loops, repeated I/O, or unoptimized queries
Files:
openwisp_controller/subnet_division/rule_types/base.py
**/*.{js,ts,tsx,jsx,py,java,go,cs,rb,php,c,cpp,h,hpp,sh,bash,sql}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_controller/subnet_division/rule_types/base.py
**/*.{py,html}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
openwisp_controller/subnet_division/rule_types/base.py
🧠 Learnings (3)
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/subnet_division/rule_types/base.py
📚 Learning: 2026-02-17T19:13:10.088Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/whois/commands.py:0-0
Timestamp: 2026-02-17T19:13:10.088Z
Learning: In reviews for the openwisp/openwisp-controller repository, do not propose changes based on Ruff warnings. The project does not use Ruff as its linter; ignore Ruff-related suggestions and follow the repository’s established linting and configuration rules. This guidance applies to all Python files under the openwisp_controller directory.
Applied to files:
openwisp_controller/subnet_division/rule_types/base.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/subnet_division/rule_types/base.py
🔇 Additional comments (6)
openwisp_controller/subnet_division/rule_types/base.py (6)
1-18: LGTM!Imports are properly consolidated at the module level. The addition of
IPSetfromnetaddraligns with the PR objective to use mathematical set operations for subnet collision detection.
34-50: LGTM!The refactored
provision_receiverhas cleaner control flow. The exception handling correctly catches cases where the instance doesn't have the expected configuration, andtransaction.on_commitensures provisioning happens after the transaction commits successfully.
82-88: LGTM!The
get_configmethod properly extracts config retrieval logic with an existence check to prevent operations on deleted configs. The implementation aligns with the subclass usage patterns seen inVpnSubnetDivisionRuleType.destroy_provisioned_subnets_ips.
166-183: LGTM!The subnet creation correctly updates the
IPSetafter each allocation (line 174), ensuring subsequent iterations don't allocate overlapping space. This directly addresses the PR objective of preventing overlapping subnets.
185-209: LGTM!The refactored
create_ipscorrectly createsIpAddressobjects viaobjects.create()before referencing their IDs inSubnetDivisionIndex(line 204). This addresses the previous FK relationship issue whereip_idwas captured asNonefrom unsaved instances.
104-116: The anchor creation logic correctly handles overlapping subnets with different prefix sizes through the IPSet intersection check increate_subnets. Multiple rules on the same master subnet are tested, though the test suite lacks coverage for rules with different sizes (e.g., /28 and /29 anchors simultaneously). Since overlaps are properly detected and avoided during allocation, this is not a bug but a test coverage gap if coverage for different rule sizes is desired.
Comment on lines
+136
to
+138
| a_qs = Subnet.objects.filter(subnet=max_s, master_subnet=ms) | ||
| if exist > 0 or (a_qs.exists() and "Reserved" in (a_qs.first().name or "")): | ||
| req = req.next() |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fragile string-based anchor detection.
Line 137 uses "Reserved" in (a_qs.first().name or "") to detect anchor subnets. This is fragile because:
- Admins could rename anchor subnets
- Other subnets might coincidentally contain "Reserved" in their name
- The string is not i18n-safe (the name uses a translated string on line 111)
Consider adding a dedicated boolean field or a specific naming convention with a unique prefix (e.g., __anchor__) that's less likely to conflict.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/subnet_division/rule_types/base.py` around lines 136 -
138, The current anchor detection using string containment ("Reserved" in
(a_qs.first().name or "")) is fragile; modify the Subnet model to add a
dedicated boolean field (e.g., is_anchor) or a unique, unlikely name prefix
(e.g., "__anchor__") and update the lookup in the SubnetDivision logic to check
that explicit marker instead of substring-matching; specifically, add the new
field and migration on the Subnet model, set it for existing anchor subnets, and
change the check in the method that contains a_qs =
Subnet.objects.filter(subnet=max_s, master_subnet=ms) to test
a_qs.filter(is_anchor=True).exists() (or check name.startswith("__anchor__") if
you choose the prefix approach), and update any other codepaths that currently
rely on the "Reserved" name heuristic.
Comment on lines
+147
to
165
| if not ip_network(str(req)).subnet_of(ms.subnet): | ||
| notify.send( | ||
| sender=config, | ||
| type="generic_message", | ||
| target=config.device, | ||
| action_object=master_subnet, | ||
| level="error", | ||
| action_object=ms, | ||
| message=_( | ||
| "Failed to provision subnets for" | ||
| " [{notification.target}]({notification.target_link})" | ||
| "Failed to provision subnets for " | ||
| "[{notification.target}]({notification.target_link})" | ||
| ), | ||
| description=_( | ||
| "The [{notification.action_object}]({notification.action_link})" | ||
| " subnet has run out of space." | ||
| "The [{notification.action_object}]" | ||
| "({notification.action_link}) subnet has run " | ||
| "out of space." | ||
| ), | ||
| ) | ||
| logger.info(f"Cannot create more subnets of {master_subnet}") | ||
| logger.info(f"Cannot create more subnets of {ms.name} {ms.subnet}") | ||
| break |
Contributor
There was a problem hiding this comment.
Use appropriate log level for error condition and consider raising an exception.
Line 164 uses logger.info for a failure condition (master subnet exhausted), but per coding guidelines, errors that cannot be resolved by the user should use logger.error. Additionally, the method silently returns partial results after break instead of signaling failure to the caller.
🔧 Proposed fix
- logger.info(f"Cannot create more subnets of {ms.name} {ms.subnet}")
+ logger.error(
+ f"Cannot create more subnets of {ms.name} ({ms.subnet}): "
+ f"address space exhausted after {len(subs)} of "
+ f"{rule.number_of_subnets} requested subnets."
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/subnet_division/rule_types/base.py` around lines 147 -
165, The master-subnet-exhausted branch in the subnet provisioning logic (the
ip_network(...).subnet_of(ms.subnet) check and the notify.send block in base.py)
currently logs with logger.info and breaks, which silently returns partial
results; change logger.info to logger.error and instead of break raise a
descriptive exception (e.g., RuntimeError or a specific SubnetProvisionError)
that includes ms.name and ms.subnet so callers can detect failure; keep the
notify.send call but replace the silent break with the exception to surface the
error to callers.
Comment on lines
226
to
+233
| @classmethod | ||
| def destroy_provisioned_subnets_ips(cls, instance, **kwargs): | ||
| # Deleting related subnets automatically deletes related IpAddress | ||
| # and SubnetDivisionIndex objects | ||
| config = cls.get_config(instance) | ||
| rule_type = f"{cls.__module__}.{cls.__name__}" | ||
| subnet_ids = config.subnetdivisionindex_set.filter( | ||
| rule__type=rule_type | ||
| ).values_list("subnet_id") | ||
| Subnet.objects.filter(id__in=subnet_ids).delete() | ||
| rt = f"{cls.__module__}.{cls.__name__}" | ||
| s_ids = config.subnetdivisionindex_set.filter(rule__type=rt).values_list( | ||
| "subnet_id", flat=True | ||
| ) | ||
| Subnet.objects.filter(id__in=s_ids).delete() |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if a rule type can be applied to multiple master subnets
rg -n "master_subnet" openwisp_controller/subnet_division/base/models.py -A 3 -B 3
# Check how rules are typically associated
rg -n "subnetdivisionrule_set" openwisp_controller/subnet_division/ --type py -A 2 -B 2Repository: openwisp/openwisp-controller
Length of output: 4359
🏁 Script executed:
# Find the VpnSubnetDivisionRuleType class and its destroy method
fd "vpn.py" openwisp_controller/subnet_division/ --type f | head -5Repository: openwisp/openwisp-controller
Length of output: 125
🏁 Script executed:
# View the destroy_provisioned_subnets_ips method in vpn.py
rg -n "destroy_provisioned_subnets_ips" openwisp_controller/subnet_division/rule_types/vpn.py -A 8 -B 2Repository: openwisp/openwisp-controller
Length of output: 635
🏁 Script executed:
# Check the base method and how it's called
rg -n "destroy_provisioned_subnets_ips" openwisp_controller/subnet_division/rule_types/base.py -A 8 -B 2Repository: openwisp/openwisp-controller
Length of output: 846
🏁 Script executed:
# Check if rules are looked up by both type and master_subnet together
rg -n "filter.*rule__type" openwisp_controller/subnet_division/ --type py -B 3 -A 3Repository: openwisp/openwisp-controller
Length of output: 820
🏁 Script executed:
# Find all classes that inherit from SubnetDivisionRuleType
rg -n "class.*SubnetDivisionRuleType" openwisp_controller/subnet_division/ --type pyRepository: openwisp/openwisp-controller
Length of output: 783
🏁 Script executed:
# Check if destroy_provisioned_subnets_ips is overridden in other subclasses
rg -n "class.*SubnetDivisionRuleType|def destroy_provisioned_subnets_ips" openwisp_controller/subnet_division/rule_types/ --type pyRepository: openwisp/openwisp-controller
Length of output: 899
🏁 Script executed:
# Check how the destroy method is called - via signal handlers or explicit calls
rg -n "destroy_provisioned_subnets_ips|destroyer_receiver" openwisp_controller/subnet_division/ --type py -B 2 -A 2Repository: openwisp/openwisp-controller
Length of output: 2507
🏁 Script executed:
# Check the full implementation of DeviceSubnetDivisionRuleType.destroy_provisioned_subnets_ips
sed -n '37,50p' openwisp_controller/subnet_division/rule_types/device.pyRepository: openwisp/openwisp-controller
Length of output: 672
🏁 Script executed:
# Check if there's context about how different rule types are instantiated
rg -n "rule_class|get_rule_class" openwisp_controller/subnet_division/ --type py -A 2 -B 2Repository: openwisp/openwisp-controller
Length of output: 2519
🏁 Script executed:
# Understand the relationship between Config and SubnetDivisionRule
rg -n "config\.subnetdivisionindex_set" openwisp_controller/ --type py -B 3 -A 3Repository: openwisp/openwisp-controller
Length of output: 6177
🏁 Script executed:
# Check SUBNET_DIVISION_TYPES setting to see which rule types are registered
rg -n "SUBNET_DIVISION_TYPES" openwisp_controller/ --type py -A 5 -B 2Repository: openwisp/openwisp-controller
Length of output: 3186
🏁 Script executed:
# Check if BaseSubnetDivisionRuleType is ever instantiated as a rule type
rg -n "BaseSubnetDivisionRuleType" openwisp_controller/ --type py | grep -v "^.*:(class|import|@)"Repository: openwisp/openwisp-controller
Length of output: 1253
🏁 Script executed:
# Look for any other custom subclasses of BaseSubnetDivisionRuleType outside of rule_types/
fd --type f "\.py$" openwisp_controller/ | xargs grep -l "BaseSubnetDivisionRuleType" | grep -v rule_typesRepository: openwisp/openwisp-controller
Length of output: 126
🏁 Script executed:
# Check if there's any documentation about destroy_provisioned_subnets_ips behavior
rg -n "destroy_provisioned_subnets_ips|master_subnet" openwisp_controller/subnet_division/rule_types/base.py -B 5 -A 10 | head -50Repository: openwisp/openwisp-controller
Length of output: 2093
🏁 Script executed:
# Check the SubnetDivisionIndex model to understand the data model better
rg -n "class SubnetDivisionIndex" openwisp_controller/subnet_division/ --type py -A 15Repository: openwisp/openwisp-controller
Length of output: 478
🏁 Script executed:
# Check the AbstractSubnetDivisionIndex model structure
rg -n "class AbstractSubnetDivisionIndex" openwisp_controller/subnet_division/ --type py -A 20Repository: openwisp/openwisp-controller
Length of output: 1907
Add master_subnet filtering to the base destroy method for consistency and correctness.
The base destroy_provisioned_subnets_ips filters only by rule__type, but VpnSubnetDivisionRuleType overrides it to also filter by rule__master_subnet_id. Since the unique constraint allows the same rule type across multiple master subnets, the base implementation could delete subnets from other master subnets if a custom rule type inherits without overriding this method. The query should filter by both rule__type and rule__master_subnet_id (obtained from the instance context, similar to how VpnSubnetDivisionRuleType does it) to ensure only the intended subnets are deleted.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_controller/subnet_division/rule_types/base.py` around lines 226 -
233, The base destroy_provisioned_subnets_ips currently deletes Subnet rows by
only filtering subnetdivisionindex_set by rule__type; update it to also filter
by rule__master_subnet_id using the instance context (like
VpnSubnetDivisionRuleType does) to avoid removing subnets belonging to other
master subnets. In practice, inside destroy_provisioned_subnets_ips (and using
cls.get_config(instance)), retrieve the master_subnet_id from the instance (or
config) and add rule__master_subnet_id=<that id> to the QuerySet filter
alongside rule__type so the Subnet.objects.filter(id__in=s_ids).delete() only
targets subnets for that specific master subnet. Ensure you reference
subnetdivisionindex_set, rule__type, rule__master_subnet_id, get_config, and
Subnet when making the change.
CI Failures: Code Style and Commit MessageHello @Piyushkhobragade,
For example: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #864.
The test_multiple_subnetdivision_rules_different_sizes test was failing because subnets with varying prefix lengths (like /31 and /29) were physically overlapping.
What was happening:
The old logic relied on database ordering (.last()) to figure out the next available subnet. This doesn't work well in SQLite when evaluating different prefix lengths, causing the IP collisions.
The fix:
Dropped the database-ordering approach in get_max_subnet and create_subnets.
Switched to calculating the next valid block purely mathematically using netaddr.IPSet.
Added a guard to ensure we only create the exact number of subnets required by the rule. Previously, keeping the math contiguous was forcing the creation of placeholder subnets, which caused the AssertionError: 4 != 2 in the test.
Tests are passing locally.