Skip to content
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.

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

Fix priority changing on Bookworm #3093

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

Conversation

Williangalvani
Copy link
Member

@Williangalvani Williangalvani commented Jan 24, 2025

to test the routing, use watch ip route, then change it in the UI.

this is now also always using the settings file as a source of truth, instead of randomly accepting the current state and saving it to the settings file.

fix #3086

tested on bookworm and bullseye

Summary by Sourcery

Fix interface priority issues on Debian Bookworm.

Bug Fixes:

  • Fix priority changing on Bookworm.

Enhancements:

  • Always use the settings file as the source of truth for network configuration, rather than randomly accepting the current state and saving it.

Tests:

  • Tested on Bookworm, needs testing on Bullseye.

@Williangalvani Williangalvani marked this pull request as draft January 24, 2025 14:26
@Williangalvani Williangalvani force-pushed the priority_fix branch 2 times, most recently from 6f892b9 to 0a9cf5c Compare January 24, 2025 16:23
@Williangalvani Williangalvani changed the title Fix priority changing on Bookworm, add watchdog Fix priority changing on Bookworm Jan 24, 2025
@Williangalvani Williangalvani force-pushed the priority_fix branch 2 times, most recently from b6f4352 to 2840d3f Compare February 5, 2025 15:42
@Williangalvani Williangalvani marked this pull request as ready for review February 7, 2025 19:48
Copy link

sourcery-ai bot commented Feb 7, 2025

Reviewer's Guide by Sourcery

This pull request refactors how network interface priorities are managed on Bookworm by removing legacy implementations and standardizing on using the settings file as the source of truth. The changes include the introduction of a new method for updating default routes with retries, removal of redundant and legacy priority functions, updates to the network manager’s configuration handling, and corresponding adjustments on the UI side to accommodate the new priority-setting logic.

Sequence diagram for setting network interface priority

sequenceDiagram
    actor User
    participant UI as NetworkInterfacePriorityMenu (UI)
    participant API as Cable Guy Manager (API)
    participant Handler as NetworkHandler (BookwormHandler / DHCPCD)
    participant OS as Operating System (IPRoute)

    User->>UI: Clicks "Set Highest Interface" button
    UI->>API: POST /set_interfaces_priority (with new priorities)
    API->>API: Validate interface names and priorities
    API->>Handler: set_interfaces_priority(interfaces)
    Handler->>Handler: set_interfaces_priority_using_ipr(interfaces)
    loop For each interface
      Handler->>OS: ipr.link_lookup(ifname) and ipr.get_routes(...)
      loop For each default route
        Handler->>Handler: _update_route(interface_name, interface_index, route, priority)
        alt Route requires update
          Handler->>OS: ipr.route(del, ...)
          loop Retry up to 3 times
            Handler->>OS: ipr.route(add, priority + attempt, ...)
            Note over OS: Returns success or error
          end
        else No update needed
          Note over Handler: Skip route
        end
      end
    end
    Handler-->>API: Returns after processing routes
    API->>API: Save updated settings (using settings file as source of truth)
    API->>UI: Return success response
    UI->>User: Notify updated interface priorities
Loading

File-Level Changes

Change Details Files
Refactored network interface priority management in the network setup service.
  • Introduced a new helper method (_update_route) to update default routes with new priority values including retries.
  • Added set_interfaces_priority_using_ipr method to encapsulate the updated route priority logic.
  • Removed legacy get_interfaces_priority implementation from NetworkManager and DHCPCD classes.
  • Ensured that the settings file is now always used as the source of truth for interface priorities.
core/services/cable_guy/networksetup.py
Updated network manager API to handle interface configurations using the new priority logic.
  • Changed the handling of saved interfaces; if an interface is missing in settings, a new entry is created.
  • Updated methods to use get_interfaces instead of deprecated get_ethernet_interfaces.
  • Modified priorities_mismatch to return a list of mismatched interfaces for more granular error handling.
  • Adjusted removal of DHCP server and interface refresh logic to account for missing saved interface scenarios.
core/services/cable_guy/api/manager.py
Adjusted the frontend component to align with the updated interface priority mechanism.
  • Updated the NetworkInterfacePriorityMenu.vue to send an array of interface priorities with explicit numeric values.
  • Improved sorting logic on the frontend to consider the new priority field, making the order of interfaces consistent.
core/frontend/src/components/app/NetworkInterfacePriorityMenu.vue
Extended type definitions to support the priority field.
  • Added an optional priority property to the NetworkInterface model in cable_guy typedefs.
  • Enforced the priority property in the NetworkInterfaceMetricApi model.
  • Updated frontend Ethernet interface type to include an optional priority field.
core/services/cable_guy/typedefs.py
core/frontend/src/types/ethernet.ts

Assessment against linked issues

Issue Objective Addressed Explanation
#3086 Fix route duplication when changing network interface priority
#3086 Ensure network priority changes replace existing routes instead of duplicating them
#3086 Prevent random acceptance of current network state and ensure settings file is used as source of truth

Possibly linked issues


Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!
  • Generate a plan of action for an issue: Comment @sourcery-ai plan on
    an issue to generate a plan of action for it.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey @Williangalvani - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Consider explicitly comparing the route destination to '0.0.0.0/0' in _update_route for clarity rather than relying on a None check.
  • Avoid clearing the entire interfaces array in setHighestInterface; an optimistic update might reduce UI flicker.
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 Documentation: all looks good

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Co-authored-by: João Mário Lago <58235456+JoaoMario109@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bookworm network priority changes is duplicating routes instead of replacing them
2 participants