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

[READY] - Belkin RT3200 support for scale openwrt #611

Merged
merged 14 commits into from
Aug 24, 2023
Merged

Conversation

sarcasticadmin
Copy link
Member

@sarcasticadmin sarcasticadmin commented Aug 8, 2023

Description of PR

Fixes: #367 #532

I wanted to get this to a place where it was workable before it got any larger. But this adds support for a new piece of hardware: Belkin RT3200/linksys (https://openwrt.org/toh/linksys/e8450)

There are some known issues with whats here but Ill clean that up in follow up PRs, mainly:

  • Disable logd
  • Add back the backdoor network interface for the new mt-7622 hardware
  • disable conntrack and netfilter modules
  • Enable MIMO hardware offloading on belkin 3200
  • Supporting new htmode for wifi channel allocation (AX modes specifically)? however, after reviewing the docs we might just wanted to leave this in a general sweet spot: https://openwrt.org/docs/guide-user/network/wifi/basic#htmodewi-fi_channel_width
  • Enable radio3 for wifi6

Each of these items has been spun out as an issue in our backlog

Previous Behavior

  • Only supported 2 APs architectures
  • Weekly build of ar71xx, ipq806x
  • Openwrt configs were built with the default being ar71xx

New Behavior

  • Support for 3 AP architectures: ar71xx, mt-7622, ipq806x (technically 4 with x86_64 as well but we dont run that on hardware)
  • Default openwrt config leverages neutral x86_64 architecture so no architecture/hardware specific packages are pulled in. We will now default to leveraging x64_64 when building common.config for openwrt.
  • Leveraging architecture specific files-<arch> dir to overlay config after files templating. This provides a much clearer way to place architecture specific configuration instead of if, elif, ... in the go templates
  • DSA configuration for mt-7622
  • Weekly build of: ar71xx, mt-7622
  • Move ar71xx specific configuration (wired, wireless) to its own files-ar71xx folder
  • Disable golden tests for ipq806x since we will not be investing any additional effort into that architecture

Tests

  • DSA network configuration was tested on the mgmt interface for both vlan 103 & 503

  • brctl show, confirming STP enabled on all interfaces:

# brctl show
bridge name     bridge id               STP enabled     interfaces
scaleslow-br            7fff.e89f80e7f941       yes             br-lan.500
                                                        br-lan.100
br-lan          7fff.e89f80e7f941       yes             lan4
                                                        lan2
                                                        wan
                                                        lan3
                                                        lan1
mgmt-br         7fff.e89f80e7f941       yes             br-lan.503
                                                        br-lan.103
scalefast-br            7fff.e89f80e7f941       yes             br-lan.501
                                                        br-lan.101

note spanning tree (STP) is enabled

Tested the uptime with real traffic for the last ~4 days. Looks stable:

# uptime
 08:39:56 up 4 days,  3:49,  load average: 0.14, 0.04, 0.01

CI image build results: https://github.com/socallinuxexpo/scale-network/actions/runs/5946721473

@sarcasticadmin sarcasticadmin changed the title [REVIEW] - Belkin 3200 support for scale openwrt [REVIEW] - Belkin RT3200 support for scale openwrt Aug 8, 2023
CONFIG_PACKAGE_openssh-client=y
CONFIG_PACKAGE_openssh-client-utils=y
CONFIG_PACKAGE_openssh-keygen=y
CONFIG_PACKAGE_openssh-server=y
CONFIG_PACKAGE_python3-base=y
Copy link
Member Author

Choose a reason for hiding this comment

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

removing python3 since we dont currently rely on it for anything

Copy link
Collaborator

Choose a reason for hiding this comment

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

Uh, shouldn't we be going the other way... Deprecating Python2 and migrating all our dependencies to Python3?

Copy link
Member Author

Choose a reason for hiding this comment

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

this only pertains to python3 on the openwrt image. I opted to remove it since we dont currently have any python scripts installed on the APs. So we wont have python3 or python2 on the APs so we save the build time and the space on the image

Copy link
Member

Choose a reason for hiding this comment

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

good riddance!

CONFIG_PACKAGE_jq=y
CONFIG_PACKAGE_kmod-ipt-core=y
CONFIG_PACKAGE_kmod-nf-ipt=y
CONFIG_PACKAGE_kmod-usb-serial=y
Copy link
Member Author

Choose a reason for hiding this comment

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

Some of these kernel modules were for serial connectivity, but arent used since we just use the serial pins on the board instead of a dedicated usb device

Copy link
Member

Choose a reason for hiding this comment

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

makes sense

CONFIG_PACKAGE_lldpd=y
# CONFIG_PACKAGE_logd is not set
Copy link
Member Author

Choose a reason for hiding this comment

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

will add this back in a follow up PR

@@ -0,0 +1,111 @@
config interface 'loopback'
Copy link
Member Author

Choose a reason for hiding this comment

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

Im opting to embed the configs for the network and the wireless vs template them. I dont think contorting the existing config is worth it given how static this typically is. Open to ideas here

Copy link
Collaborator

Choose a reason for hiding this comment

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

I really don't understand the need for a ula_prefix in OpenWRT. If we can avoid it, I'd rather do so. We have no use for ULA in our environment. OTOH, it seems that OpenWRT may perhaps break badly without one for reasons utterly passing understanding. I haven't had time to dig into this and/or rant about it upstream, but if you can look at that and provide some feedback, that would be appreciated.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for the catch @owendelong This was just a carrier over from the default auto gen config from openwrt. Removing it doesnt break anything from my testing.

It comes from the uci defaults: https://github.com/openwrt/openwrt/blob/master/package/base-files/files/etc/uci-defaults/12_network-generate-ula but why thats a default I dont know either.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we really want to build x86 targets? For what?

Copy link
Member Author

Choose a reason for hiding this comment

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

Youre right and we wont be using the target on any deployment but I briefly touched on it in the PR description:

Default openwrt config leverages neutral x86_64 architecture so no architecture/hardware specific packages are pulled in. We will now default to leveraging x64_64 when building common.config for openwrt.

In order to do multiple architectures in openwrt I have to start with a base config which needs an architecture. It autoselects one if I dont have any defined. x86 is a neutral architecture so we dont get any specific architecture or model packages. Theres no other way to filter them out either. If anyone else has better ideas how to do this Im all ears.

So instead the idea is to do the following order of operations to generate our configurations across N architectures:

    1. Generate a common.config using the x86 architecture since it omits
       any specific architecture packages. All packages declared here should
       be hardware independent and common across any architecture in scales
       environment.
    2. For new architectures generate a $TARGET-generic.config without
       including any common config.
    3. Combine $TARGET-generic.config + common.config and tweak until it
       includes all desired configuration. Output this the
       $TARGET-generic.config. This will include all CONFIG_TARGET config and
       architecture specific packages

Previously we were diffing the configs based on the prefixes of the build option names. Anything with that included CONFIG_TARGET was target specific, while everything else was common. This no longer works since there are many packages that exist specifically for a given arch/model and are not prefixed with CONFIG_TARGET.

Copy link
Collaborator

@owendelong owendelong left a comment

Choose a reason for hiding this comment

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

I've provided individual comments inline, but generally looks good. Excellent effort, Rob.

Finally getting support for the belkin RT3200
The assumption is that we are using bash by default. For some reason
ubuntu still can inherit sh. Instead of assuming well be explicit about
which SHELL we expect.
@sarcasticadmin sarcasticadmin force-pushed the rh/issue-367 branch 2 times, most recently from a9929ea to bfb3f29 Compare August 9, 2023 20:37
Previously we were diffing the configs based on the prefixes of the
option names. Anything with that included CONFIG_TARGET was target
specific, while everything else was common.

This no longer works since there are many packages that exist
specifically for a given arch and are not prefixed with CONFIG_TARGET.

So instead the idea is to do the following order of operations to
generate our configurations across N architectures:

1. Generate a common.config using the x86 architecture since it omits
   any specific architecture packages. All packages declared here should
   be hardware independent and common across any architecture in scales
   environment.
2. For new architectures generate a $TARGET-generic.config without
   including any common config.
3. Combine $TARGET-generic.config + common.config and tweak until it
   includes all desired configuration. Output this the
   $TARGET-generic.config. This will include all CONFIG_TARGET config and
   architecture specific packages.
Not all architectures have "generic" in their output paths for build
artifacts. Especially the mt7622 builds.
we dont currently use the build for ipq806x so Id prefer to disable this
in favor of the new mt7622 build (belkin RT3200)
Adding notes about new workflow for the openwrt build configs and some
notes about doing it manually. Since you typically have to do this when
adding a new board or cleaning out the existing config files.
@sarcasticadmin
Copy link
Member Author

@owendelong thanks for the review and the feedback! If you need any more clarification you know where to find me!

Copy link
Member

@kylerisse kylerisse left a comment

Choose a reason for hiding this comment

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

@sarcasticadmin this all makes sense logically. As discussed, I didn't run through full test as I more than trust your methodology, and as you stated it's best to get this moving now since its getting beefy, and continue to iterate. Very cool idea to default to the common architecture and leverage arch specific configs. 🚢

cat ./$(BUILD_DIR)/$(IMAGEBUILDER)/.diffconfig | grep -v CONFIG_TARGET > ./configs/common.config

targetconfig: diffconfig
cat ./$(BUILD_DIR)/$(IMAGEBUILDER)/.diffconfig | grep CONFIG_TARGET > ./configs/$(TARGET)-generic.config
comm -23 <(sort ./$(BUILD_DIR)/$(IMAGEBUILDER)/.diffconfig) <(sort ./configs/common.config) > ./configs/$(TARGET)-generic.config
Copy link
Member

Choose a reason for hiding this comment

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

comm, nice!

CONFIG_PACKAGE_jq=y
CONFIG_PACKAGE_kmod-ipt-core=y
CONFIG_PACKAGE_kmod-nf-ipt=y
CONFIG_PACKAGE_kmod-usb-serial=y
Copy link
Member

Choose a reason for hiding this comment

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

makes sense

CONFIG_PACKAGE_openssh-client=y
CONFIG_PACKAGE_openssh-client-utils=y
CONFIG_PACKAGE_openssh-keygen=y
CONFIG_PACKAGE_openssh-server=y
CONFIG_PACKAGE_python3-base=y
Copy link
Member

Choose a reason for hiding this comment

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

good riddance!

@@ -0,0 +1,25 @@
# CONFIG_PACKAGE_kmod-ata-ahci-mtk is not set
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for marking all the not set features. This really helps somebody unfamiliar with all the bells and whistles like me understand what's going on.

```
> Assuming mt7622 is our target arch

Thats it now you should have a generic config and a arch specific config going forward. The majority of these steps are taken care of by the Makefile in the `openwrt` dir but in the cases where
Copy link
Member

Choose a reason for hiding this comment

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

nice, seems like a clean setup

facts/secrets/mt7622-openwrt-example.yaml Outdated Show resolved Hide resolved
@sarcasticadmin
Copy link
Member Author

sarcasticadmin commented Aug 21, 2023

Thanks @kylerisse for your review, Ill make the small tweaks and then report back when its pushed up

@sarcasticadmin
Copy link
Member Author

@owendelong @kylerisse Marking this as ready! Thanks again for the feedback and review. Ive spun out some additional items to address with this hardware after we get this initial set of changes merged.

@sarcasticadmin sarcasticadmin changed the title [REVIEW] - Belkin RT3200 support for scale openwrt [READY] - Belkin RT3200 support for scale openwrt Aug 23, 2023
the ipq806x based AP (tplink) is on its way out. We found the drivers to
unstable after many years of waiting. Well be moving onward to the
mt7622 and belkin/linksys hardware instead.
Copy link
Collaborator

@owendelong owendelong left a comment

Choose a reason for hiding this comment

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

Looks good to me. Excellent work, Rob.

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.

Update openwrt network configs for openwrt dsa conversion
3 participants