Hello @champtar, thanks for fixing this! This needs to be covered by an integration test to reproduce/demonstrate the original bug and validate the fix. If you would like to give this a try, please have a look at https://github.com/cockpit-project/cockpit/blob/main/test/README.md

Otherwise someone from our team (@mvollmer or me) can help with that. However, we have a meeting week, so nothing will happen until the week afterwards. We are not ignoring you 😉

Hello @martinpitt, not sure I will have time to look at the tests this week either :(

I wanted to learn a bit more about integration tests so tried creating a test for this. ~~~Couldn't get it passing locally though for some reason as it errors with the selection~~~

The patch works, tested it with a VM 🎉

It should create a new bond with bond0 name, one interface added that is made primary, then the bond edited to make sure it is selected as primary

diff --git a/test/verify/check-networkmanager-bond b/test/verify/check-networkmanager-bond
index 8ec09b950..1fdaa9fdc 100755
--- a/test/verify/check-networkmanager-bond
+++ b/test/verify/check-networkmanager-bond
@@ -223,6 +223,38 @@ class TestBonding(netlib.NetworkCase):
         b.wait_visible(f"#network-interface-members tr[data-interface='{iface}']")
         b.wait_in_text("#network-interface .pf-v5-c-card:contains('tbond')", ip)
 
+    def testPrimary(self):
+        b = self.browser
+
+        self.login_and_go("/network")
+
+        b.wait_visible("#networking")
+        iface = "cockpit"
+        self.add_veth(iface, dhcp_cidr="10.111.112.2/20")
+        self.nm_activate_eth(iface)
+        self.wait_for_iface(iface)
+
+        # Create a bond
+        b.click("button:contains('Add bond')")
+        b.wait_visible("#network-bond-settings-dialog")
+        b.set_input_text("#network-bond-settings-interface-name-input", "tbond")
+        # add interface to bond
+        b.set_checked(f"input[data-iface='{iface}']", val=True)
+        # select it as primary
+        b.select_from_dropdown("#network-bond-settings-primary-select", iface)
+        # finish creating the bond
+        b.click("#network-bond-settings-dialog button:contains('Add')")
+        b.wait_not_present("#network-bond-settings-dialog")
+
+        # Navigate to bond and edit it
+        b.click("#networking-interfaces tr[data-interface='tbond'] button")
+        b.wait_visible("#network-interface")
+        b.click("#network-interface #networking-edit-bond")
+
+        # edit bond dialog
+        b.wait_visible("#network-bond-settings-dialog")
+        b.wait_val("#network-bond-settings-primary-select", iface)
+
     def testAmbiguousMember(self):
         b = self.browser
         m = self.machine

It should create a new bond with bond0 name, one interface added that is made primary, then the bond edited to make sure it is selected as primary

Thanks! From the description of the pull request, I think there are a few more cases that could be tested: removing the primary, and automatic removal when switching to a mode that doesn't have the concept of a primary interface.

But we can install the test as it is and run our CI, and see whether our coverage report discovers the missing cases as well. :-)

@champtar, do you want to add @Venefilyn's patch, or do you want one of the Cockpit maintainers to take over?

It should create a new bond with bond0 name, one interface added that is made primary, then the bond edited to make sure it is selected as primary

Thanks! From the description of the pull request, I think there are a few more cases that could be tested: removing the primary, and automatic removal when switching to a mode that doesn't have the concept of a primary interface.

I can think of:

1. When the page loads, the current primary value is correctly displayed (no primary or one of the interfaces)
2. can switch from no primary to one of the interface of the bond
3. can switch primary from one of the interface to another
4. can change primary setting when current primary value is invalid (I managed to set primary=- at some point)
5. can switch to a mode that doesn't support primary

But we can install the test as it is and run our CI, and see whether our coverage report discovers the missing cases as well. :-)

@champtar, do you want to add @Venefilyn's patch, or do you want one of the Cockpit maintainers to take over?

Please take over, maintainers should be able to push in this PR directly

Nice, diff coverage is 100%: https://cockpit-logs.us-east-1.linodeobjects.com/pull-21533-5a3107ef-20250130-150909-fedora-41-devel/Coverage/lcov/github-pr.diff.gcov.html

Do I need to rebase or do anything here ? The failures seem unrelated