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

Network teams #4571

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
2 participants
@mvollmer
Copy link
Member

commented Jun 14, 2016

Needs #4568 for the new dependency.

This doesn't expose any settings. The defaults are assumed to be perfect for Cockpit users.

@mvollmer mvollmer added the blocked label Jun 14, 2016

@mvollmer mvollmer force-pushed the mvollmer:network-teams branch from b96b19d to bd4066c Jun 15, 2016

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Jun 19, 2016

Needs a rebase.

@mvollmer mvollmer changed the title Network teams WIP - Network teams Jun 27, 2016

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2016

This is still work in process. I need to add some settings after all, based on the feedback we got.

@mvollmer mvollmer force-pushed the mvollmer:network-teams branch from bd4066c to 7d1c93d Jun 27, 2016

@mvollmer mvollmer force-pushed the mvollmer:network-teams branch 3 times, most recently from 4da01cf to 3090b57 Aug 2, 2016

@mvollmer mvollmer removed the blocked label Aug 2, 2016

@mvollmer mvollmer changed the title WIP - Network teams Network teams Aug 2, 2016

@mvollmer mvollmer removed the needswork label Aug 2, 2016

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Aug 2, 2016

This should now be "minimally viable", but it still is quite terrible.

For example, to configure which interface is the primary and which is the backup in a "Active Backup" team, one has to put the right priorities into the individual port configurations. This is insultingly undiscoverable, IMO.

@mvollmer mvollmer force-pushed the mvollmer:network-teams branch from 4a83a36 to 75807c2 Aug 3, 2016

@@ -358,6 +358,7 @@ Provides: %{name}-subscriptions = %{version}-%{release}
Requires: subscription-manager >= 1.13
Provides: %{name}-networkmanager = %{version}-%{release}
Requires: NetworkManager
Requires: NetworkManager-team

This comment has been minimized.

Copy link
@stefwalter

stefwalter Aug 8, 2016

Contributor

Out of interest, why isn't this part of the standard NetworkManager code?

In addition, I think this is missing a change further down in the spec file under the %package networkmanager. It should probably have Suggests or Recommends style logic there.

This comment has been minimized.

Copy link
@mvollmer

mvollmer Aug 9, 2016

Author Member

Out of interest, why isn't this part of the standard NetworkManager code?

I don't know, because Linux is about choice? :)

This comment has been minimized.

Copy link
@mvollmer

mvollmer Aug 9, 2016

Author Member

In addition, I think this is missing a change further down in the spec file under the %package networkmanager.

Yes, indeed.

It should probably have Suggests or Recommends style logic there.

And we hide the "Add Team" button when it is not installed?

This comment has been minimized.

Copy link
@mvollmer

mvollmer Aug 9, 2016

Author Member

We actually get nice enough error messages when the plugin is not installed, so I think we are okay.

@@ -80,6 +80,7 @@
<span translatable="yes">Interfaces</span>
<div class="pull-right">
<button translatable="yes" id="networking-add-bond" class="btn btn-default network-privileged">Add Bond</button>
<button translatable="yes" id="networking-add-team" class="btn btn-default network-privileged">Add Team</button>

This comment has been minimized.

Copy link
@stefwalter

stefwalter Aug 8, 2016

Contributor

How should people decide between these two buttons? Is it clear to users?

This comment has been minimized.

Copy link
@mvollmer

mvollmer Aug 9, 2016

Author Member

How should people decide between these two buttons? Is it clear to users?

I think it is not clear. This is the minimal level that allows us to claim the promised "team support" in Cockpit, and it is slightly better than what nmtui and Gnome Control Center have.

return JSON.parse(str);
}
catch (e) {
return { };

This comment has been minimized.

Copy link
@stefwalter

stefwalter Aug 8, 2016

Contributor

In what cases does this happen? Does it happen as a result of an admin editing config files manually? If so, we should probably display an alert instead of an empty config.

This comment has been minimized.

Copy link
@mvollmer

mvollmer Aug 9, 2016

Author Member

Yes, this happens when the sysadmin puts invalid JSON into /etc/sysconfig/network-scripts/ifcfg-foo, maybe via nmtui.

I have added a commit that draws some attention to that.

@stefwalter

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2016

There's a 'rhel-atomic' fail:

error: Failed dependencies:
    NetworkManager-team is needed by cockpit-shell-wip-1.el7.centos.noarch

@stefwalter stefwalter added the needswork label Aug 8, 2016

@mvollmer mvollmer force-pushed the mvollmer:network-teams branch from 75807c2 to e6e51fd Aug 9, 2016

@mvollmer

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2016

There's a 'rhel-atomic' fail:

error: Failed dependencies:
    NetworkManager-team is needed by cockpit-shell-wip-1.el7.centos.noarch

This should fix itself once the team support is optional.

mvollmer added some commits Jun 14, 2016

networkmanager: Clear out port settings when changing slave roles
A team slave must not have bridge port settings, and vice versa.  Not
clearing these settings results in errors from NetworkManager and
confusing configurations.

@mvollmer mvollmer force-pushed the mvollmer:network-teams branch from e6e51fd to fa8cae0 Aug 9, 2016

@mvollmer mvollmer removed the needswork label Aug 9, 2016

stefwalter added a commit that referenced this pull request Aug 11, 2016

networkmanager: Clear out port settings when changing slave roles
A team slave must not have bridge port settings, and vice versa.  Not
clearing these settings results in errors from NetworkManager and
confusing configurations.

Closes #4571
Reviewed-by: Stef Walter <stefw@redhat.com>

stefwalter added a commit that referenced this pull request Aug 11, 2016

networkmanager: Call out unparseable team configuration
Closes #4571
Reviewed-by: Stef Walter <stefw@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.