Re: [PATCH] Core 152: the script "network-hotplug-bridges" now reads the variable ${ZONE}_STP from /var/ipfire/ethernet/settings so that STP can be turned on and off for each bridge

[Jonatan Schlag <jonatan.schlag@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 15135 bytes --]


Hi,

I vote for option one: two separate tables. There will be definitely
more options to come, even if we might not see them now. I also see no
direct connection between STP (or any other option) and which ports are
connected to a zone. 
Another option would be like in the network of ipfire 3.x a table per
zone. (like network zone net0 status). 

Greetings Jonatan

Am Dienstag, den 01.12.2020, 16:27 +0000 schrieb Michael Tremer:
> Hello Leo,
> 
> Thanks for putting all this leg work in.
> 
> I would as well vote for option two: one big table.
> 
> I do not expect that we will add too much more, and splitting this
> into two tables pulls two things that belong together apart.
> 
> Until when do we want to keep the voting open?
> 
> Best,
> -Michael
> 
> > On 28 Nov 2020, at 13:24, Adolf Belka <ahb.ipfire(a)gmail.com> wrote:
> > 
> > Hi Leo,
> > 
> > I prefer the one big table. There are just a couple of extra rows
> > and boxes in each of the zone headings, so I don't think it is
> > overly busy and easier to scan if you are making changes.
> > 
> > At the end of the day, if the two table option was preferred by
> > more people I could also live with that.
> > 
> > My 2p worth.
> > 
> > Regards,
> > 
> > Adolf.
> > 
> > 
> > On 28/11/2020 13:06, Leo Hofmann wrote:
> > > Hi,
> > > once again, thanks for your feedback! I spent some time and
> > > created two more detailed UI drafts. I hope that I have
> > > incorporated all your ideas:
> > > 1: Two tables, zone options on the top, NIC assign matrix
> > > (without any unrelated options) on the bottom
> > > 2: One big table, STP options inside NIC selection
> > > Your thoughts?
> > > Regards
> > > Leo
> > > Am 27.11.2020 um 11:59 schrieb Michael Tremer:
> > > > Hello,
> > > > 
> > > > > On 27 Nov 2020, at 08:34, Jonatan Schlag <
> > > > > jonatan.schlag(a)ipfire.org> wrote:
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > Sorry for jumping late into this conversation, time is rare
> > > > > in these days ....
> > > > > I will try to bring in my thoughts, but maybe they are not
> > > > > well structured.
> > > > > 
> > > > > > Am 26.11.2020 um 15:47 schrieb Daniel Weismüller <
> > > > > > daniel.weismueller(a)ipfire.org>:
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > there is one thing that we didn't talked about...
> > > > > > 
> > > > > > STP and priority must only be activatable if the zone is in
> > > > > > bridge mode otherwise it must be grayed out.
> > > > > > 
> > > > > Shouldn’t we just not display these fields, if they do not
> > > > > matter?
> > > > > > -
> > > > > > 
> > > > > > Daniel
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > > Am 25.11.20 um 21:57 schrieb Michael Tremer:
> > > > > > > Hello,
> > > > > > > 
> > > > > > > > > On 25 Nov 2020, at 17:00, Leo Hofmann <
> > > > > > > > > hofmann(a)leo-andres.de> wrote:
> > > > > > > > Hi Daniel,
> > > > > > > > 
> > > > > > > > thank you very much for the draft & the explanation!
> > > > > > > > 
> > > > > > > > Do you happen to know if there are any other zone-
> > > > > > > > related options that might be added in the future?
> > > > > > > > If this is a possibility, I think we should add a
> > > > > > > > second table. So we don't clutter the NIC assignment
> > > > > > > > with unrelated options.
> > > > > Did I mention that it is very nice, when you wrote what I
> > > > > think so I don’t have to write it again 😉. +1 For a secon
> > > > > d structure
> > > > > > > Good question. On one hand it is good to have things that
> > > > > > > go together in one place. On the other hand, this whole
> > > > > > > page is becoming longer and longer and that simply makes
> > > > > > > it complicated.
> > > > > Definitely to complicated. Already right now.
> > > > > > > The only thing I can think of is MTU. We currently have
> > > > > > > no UI to set that, but it has never been asked for. We
> > > > > > > set it automatically on some of the cloud providers, but
> > > > > > > that is it.
> > > > > > > 
> > > > > > > > I took up your and Michael's suggestions and created a
> > > > > > > > quick HTML demo.
> > > > > > > Looks good :)
> > > > > > > 
> > > > > Here I don’t think so, because more zones (4) will make it
> > > > > impossible to display this on very small displays. Why not
> > > > > creating a table for every zone and putting them among each
> > > > > other.
> > > > I think this definitely has some upsides, but it also has some
> > > > downsides:
> > > > 
> > > > It would be good to have more space for each zone and put all
> > > > settings for one zone together.
> > > > 
> > > > The biggest problem that I see is that it is no longer obvious
> > > > which ports are now available and configured to other zones and
> > > > that makes this part a little bit more complicated. People
> > > > would have to scroll up and down or hit Save and see an error
> > > > message that tells you that you did something wrong.
> > > > 
> > > > What do we think about this?
> > > > 
> > > > It is a bit of extra work - and should be considered a step two
> > > > after STP has been implemented - but I do not think that
> > > > someone will spend a week on implementing this.
> > > > 
> > > > > > > > This new table could be placed below the NIC assignment
> > > > > > > > table. What do you think?
> > > > > > > Call the checkboxes “Enable”, because that is what they
> > > > > > > do.
> > > > > > > 
> > > > > > > I would also suggest to have the labels (e.g. “Priority”)
> > > > > > > on the left so that it is only wasting space once. With
> > > > > > > plenty of zones the table just becomes unnecessarily wide
> > > > > > > then.
> > > > > > > 
> > > > > > > > @Michael: I would like to base this new feature on my
> > > > > > > > recently patched zoneconf.cgi. Is this somehow a bad
> > > > > > > > idea?
> > > > > > > Well, good question. I have no idea why I didn’t merge it
> > > > > > > yet. I didn’t realise it was ready. I will check if there
> > > > > > > is enough testing feedback already.
> > > > > > > 
> > > > > > > Best,
> > > > > > > -Michael
> > > > > > > 
> > > > > > > > Regards
> > > > > > > > Leo
> > > > > > > > 
> > > > > > > > Am 23.11.2020 um 16:13 schrieb Daniel Weismüller:
> > > > > > > > > Hi Leo,
> > > > > > > > > 
> > > > > > > > > that pleases me to hear and I gladly accept your
> > > > > > > > > offer. ;-)
> > > > > > > > > 
> > > > > > > > > I quickly made a draft and attached it. As I said it
> > > > > > > > > is only a draft so there is still plenty of room for
> > > > > > > > > improvement.
> > > > > > > > > 
> > > > > > > > > The checkbox switches the variable named ${ZONE}_STP
> > > > > > > > > to 0 or 1.
> > > > > > > > > The input field fills the variable named
> > > > > > > > > ${ZONE}_STP_PRIORITY.
> > > > > > > > > Here must a number between 1 and 65535 inserted.
> > > > > > > > > 
> > > > > > > > > -
> > > > > > > > > 
> > > > > > > > > Daniel
> > > > > > > > > 
> > > > > > > > > Am 21.11.20 um 17:39 schrieb Leo Hofmann:
> > > > > > > > > > Hi Daniel,
> > > > > > > > > > 
> > > > > > > > > > a few days ago I finally submitted my patches for
> > > > > > > > > > zoneconf.cgi and I
> > > > > > > > > > would now have time to work on this as well.
> > > > > Thank you for submitting these patches. It is enjoyable to
> > > > > read good code.
> > > > > > > > > > (By the way, I almost forgot, thanks @Michael for
> > > > > > > > > > reviewing my patches!)
> > > > > > > > > > 
> > > > > > > > > > If you want me to take this on, it would be very
> > > > > > > > > > helpful if you could
> > > > > > > > > > summarize how this should work. For example, which
> > > > > > > > > > config parameters
> > > > > > > > > > need to be modified. Perhaps you could even paint a
> > > > > > > > > > simple GUI mock-up
> > > > > > > > > > like you did last time?
> > > > > > > > > > 
> > > > > > > > > > Regards,
> > > > > > > > > > Leo
> > > > > > > > > > 
> > > > > > > > > > Am 20.11.2020 um 19:31 schrieb Daniel Weismüller:
> > > > > > > > > > > OK. ;-)
> > > > > > > > > > > 
> > > > > > > > > > > The first step will be the introduction of the
> > > > > > > > > > > possibility to enable STP.
> > > > > > > > > > > 
> > > > > > > > > > > The next step will be the implementation in the
> > > > > > > > > > > webif.
> > > > > > > > > > > 
> > > > > > > > > > > I hope I find someone who can do that.
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > -
> > > > > > > > > > > Daniel
> > > > > > > > > > > 
> > > > > > > > > > > Am 20.11.2020 um 16:18 schrieb Kienker, Fred:
> > > > > > > > > > > > I'm with Michael on this one. If it deserves to
> > > > > > > > > > > > be in IPFire, it
> > > > > > > > > > > > deserves to be on the web interface. Don't
> > > > > > > > > > > > created exceptions which are
> > > > > > > > > > > > only available from a command line.
> > > > > > > > > > > > 
> > > > > > > > > > > > Best regards,
> > > > > > > > > > > > Fred
> > > > > > > > > > > > 
> > > > > > > > > > > > -----Original Message-----
> > > > > > > > > > > > From: Michael Tremer <michael.tremer(a)ipfire.org
> > > > > > > > > > > > >
> > > > > > > > > > > > Sent: Friday, November 20, 2020 5:55 AM
> > > > > > > > > > > > To: Daniel Weismüller <
> > > > > > > > > > > > daniel.weismueller(a)ipfire.org>
> > > > > > > > > > > > Cc: development(a)lists.ipfire.org
> > > > > > > > > > > > Subject: Re: [PATCH] Core 152: the script
> > > > > > > > > > > > "network-hotplug-bridges" now
> > > > > > > > > > > > reads the variable ${ZONE}_STP from
> > > > > > > > > > > > /var/ipfire/ethernet/settings so
> > > > > > > > > > > > that STP can be turned on and off for each
> > > > > > > > > > > > bridge
> > > > > > > > > > > > 
> > > > > > > > > > > > Hi,
> > > > > > > > > > > > 
> > > > > > > > > > > > > On 20 Nov 2020, at 06:58, Daniel Weismüller
> > > > > > > > > > > > <daniel.weismueller(a)ipfire.org> wrote:
> > > > > > > > > > > > > Hello,
> > > > > > > > > > > > > 
> > > > > > > > > > > > > In my opinion it is sufficient to be able to
> > > > > > > > > > > > > set these parameters via
> > > > > > > > > > > > command line.
> > > > > > > > > > > > 
> > > > > > > > > > > > Why is that? IPFire is a distribution that is
> > > > > > > > > > > > supposed to be managed
> > > > > > > > > > > > entirely by the web user interface.
> > > > > And here I was wondering a lot. A lot of options are only
> > > > > available via command line. The setup command is entirely
> > > > > based on the command line. So where do we draw a border what
> > > > > should be available from the webinterface?  These and some
> > > > > other questions I have belonging to the webinterface, are
> > > > > some how fundamental, so that I like to discuss these in the
> > > > > telco ...
> > > > The setup is a CLI tool because the web UI is not set up yet,
> > > > when it is being launched.
> > > > 
> > > > We must have everything that is possible on the web UI and only
> > > > what is necessary on the CLI.
> > > > 
> > > > > Best Jonatan
> > > > > > > > > > > > > It should only be made sure that the settings
> > > > > > > > > > > > > are persitend and not
> > > > > > > > > > > > overwritten by a reboot or the webif.
> > > > > > > > > > > > 
> > > > > > > > > > > > They wont be as they are in
> > > > > > > > > > > > /var/ipfire/ethernet/settings.
> > > > > > > > > > > > 
> > > > > > > > > > > > Best,
> > > > > > > > > > > > -Michael
> > > > > > > > > > > > 
> > > > > > > > > > > > > -
> > > > > > > > > > > > > Daniel
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Am 19.11.2020 um 15:56 schrieb Michael
> > > > > > > > > > > > > Tremer:
> > > > > > > > > > > > > > Hello Daniel,
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > This patch looks good to me.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > I had assumed that we automatically enabled
> > > > > > > > > > > > > > STP on all bridges, but
> > > > > > > > > > > > apparently we do not.
> > > > > > > > > > > > > > How do we process with this?
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > I suppose it is not the most user-friendly
> > > > > > > > > > > > > > way to ask the user to
> > > > > > > > > > > > edit the configuration file. This either must
> > > > > > > > > > > > be documented somewhere or
> > > > > > > > > > > > the zoneconfig.cgi script needs to be extended
> > > > > > > > > > > > to allow enabling STP.
> > > > > > > > > > > > > > Does anyone want to be able to change any
> > > > > > > > > > > > > > STP parameters like
> > > > > > > > > > > > priority or cost of the ports?
> > > > > > > > > > > > > > Best,
> > > > > > > > > > > > > > -Michael
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > On 19 Nov 2020, at 13:18, Daniel
> > > > > > > > > > > > > > > Weismüller
> > > > > > > > > > > > <daniel.weismueller(a)ipfire.org> wrote:
> > > > > > > > > > > > > > > Signed-off-by: Daniel Weismüller <
> > > > > > > > > > > > > > > daniel.weismueller(a)ipfire.org>
> > > > > > > > > > > > > > > ---
> > > > > > > > > > > > > > > config/udev/network-hotplug-bridges | 4
> > > > > > > > > > > > > > > +++-
> > > > > > > > > > > > > > > 1 file changed, 3 insertions(+), 1
> > > > > > > > > > > > > > > deletion(-)
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > diff --git a/config/udev/network-hotplug-
> > > > > > > > > > > > > > > bridges
> > > > > > > > > > > > b/config/udev/network-hotplug-bridges
> > > > > > > > > > > > > > > index 33d6d65ba..7431377bb 100644
> > > > > > > > > > > > > > > --- a/config/udev/network-hotplug-bridges
> > > > > > > > > > > > > > > +++ b/config/udev/network-hotplug-bridges
> > > > > > > > > > > > > > > @@ -81,6 +81,7 @@ MODE="$(get_value
> > > > > > > > > > > > > > > "${ZONE}_MODE")"
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > # The name of the virtual bridge
> > > > > > > > > > > > > > > BRIDGE="$(get_value "${ZONE}_DEV")"
> > > > > > > > > > > > > > > +STP="$(get_value "${ZONE}_STP")"
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > case "${MODE}" in
> > > > > > > > > > > > > > >     bridge)
> > > > > > > > > > > > > > > @@ -89,7 +90,8 @@ case "${MODE}" in
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > >         # We need to create the bridge if
> > > > > > > > > > > > > > > it doesn't exist, yet
> > > > > > > > > > > > > > >         if [ ! -d
> > > > > > > > > > > > > > > "/sys/class/net/${BRIDGE}" ]; then
> > > > > > > > > > > > > > > -            ip link add "${BRIDGE}"
> > > > > > > > > > > > > > > address "${ADDRESS}" type
> > > > > > > > > > > > bridge
> > > > > > > > > > > > > > > +            ip link add "${BRIDGE}"
> > > > > > > > > > > > > > > address "${ADDRESS}" type
> > > > > > > > > > > > bridge \
> > > > > > > > > > > > > > > +                $([ "${STP}" = "on" ] &&
> > > > > > > > > > > > > > > echo "stp_state 1")
> > > > > > > > > > > > > > >             #ip link set "${BRIDGE}" up
> > > > > > > > > > > > > > >         fi
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > -- 
> > > > > > > > > > > > > > > 2.28.0
> > > > > > > > > > > > > > > 
> > > > > > > > <zoneconf-stp.png>