profiles: improve SUPPORTED_DEVICES handling and profile identification#1533
profiles: improve SUPPORTED_DEVICES handling and profile identification#1533map-b wants to merge 3 commits intoopenwrt:mainfrom
Conversation
This reverts commit 1579236.
|
Hard NAK. Is this AI slop code? You've broken event logging, broken a bug fix and removed an important test case. As I've already explained to you several times, the root cause of this issue is incorrect metadata coming from upstream. Attempting to work around the upstream issue in the ASU server does not fix anything and leaves several other things broken (specifically both |
|
In those two reverts, included in this PR, you were mixing totally the translation or the DT compatible strings into actually profiles, that is not sanitize that is lose track of what are the strings sended by ASU clients(if they do not try to translate before sending them) and the actual profiles which are used in ImageBuilders. It is not AI sope code, a bit unrespectfully comment was that. If you do not want to reason, it is over, It is really sad. |
| # Sanitize the profile in case the client did not (bug in older LuCI app). | ||
| build_request.profile = build_request.profile.replace(",", "_") | ||
|
|
||
| add_build_event("requests") |
There was a problem hiding this comment.
This revert d7fd828 made this, I am not sure why. You are right on that.
You've broken event logging
This reverts commit 2e9edfd.
The actual handling does not allow to several devices share a DTS compatible string to allow image compatibility between them. Take the lists as lists and select the profile after checking all possible candidates. Throw a new validation exception when it is not possible to identify a profile which will help to identify inconsistencies. This also can help to make a little bit clearer the difference between "profile" and "DTS compatible string". * There is not need to add [profile] to SUPPORTED_DEVICES list anymore. Before: [1] 'glinet,gl-mt2500': 'glinet_gl-mt2500-airoha', 'glinet,mt2500-emmc': 'glinet_gl-mt2500-airoha', 'glinet,gl-mt2500-airoha': 'glinet_gl-mt2500-airoha', 'glinet_gl-mt2500': 'glinet_gl-mt2500', 'glinet_gl-mt2500-airoha': 'glinet_gl-mt2500-airoha' After: 'glinet_gl-mt2500': ['glinet,gl-mt2500', 'glinet,mt2500-emmc', 'glinet,gl-mt2500-airoha', 'glinet_gl-mt2500'], 'glinet_gl-mt2500-airoha': ['glinet,gl-mt2500-airoha', 'glinet,mt2500-emmc', 'glinet,gl-mt2500', 'glinet_gl-mt2500-airoha'], [1]: openwrt#1525 (comment) Fixes: openwrt#1525 Fixes: openwrt/openwrt#20566
96024c1 to
e8bf73f
Compare
They share the same "compatible" in device tree causing some incompatibilities (ASU profile identification), assign a unique "compatible" and "model" to each variant.[1][2] Commit [3] just avoided ASU profile identification limitation[4] but not fixed the root cause. [1]: openwrt#20632 (comment) [2]: openwrt#20632 (comment) [3]: openwrt@b71f466 [4]: openwrt/asu#1533 Fixes: b71f466 ("mediatek: filogic: fix supported_devices list for gl-mt2500") Fixes: 8d30e07 ("mediatek: filogic: fix for new GL.iNet GL-MT2500/GL-MT2500A hardware revision") Fixes: openwrt#20566 Fixes(partially): openwrt/asu#1525 Signed-off-by: Mario Andrés Pérez <mapb_@outlook.com>
These devices share the same "compatible" in device tree causing some incompatibilities (sysupgrades, ASU profile identification), assign a unique "compatible" and "model" to each variant. Context: Commit [1] added each variant's dts compatible to the SUPPORTED_DEVICES field of the other variant to make easy sysupgrades between these physically indistinguishable devices variants possible. But there were found three issues which does not allow this: - the sysupgrade's stricter check still used in some sysupgrade paths(this check is being replaced(and redundant) with the newer fwtool's SUPPORTED_DEVICES check using the info in images METADATA), this check will fail when sysupgrading from a different board_name(compatible dts) that the image was created for (image profile name).[2] - ASU needs unique "dts compatible" to identify the devices profile. - and an ASU's profile identification limitation when several devices from a common target share SUPPORTED_DEVICES entries.[3] There is a proposal for these issues but not yet implemented [4][3]. Until these issues are fixed we won't allow "easy" sysupgrades between these two device variants. Commit [5] avoided the ASU profile identification limitation but missed the required two unique dts compatibles in order to make the two variants fully work, although not allowing easy sysupgrade between them. [1]: openwrt@8d30e07 [2]: sysupgrade stricter check openwrt#20566 (comment) [3]: ASU proposal openwrt/asu#1533 [4]: allow easy sysupgrade proposal openwrt#20947 [5]: openwrt@b71f466 Fixes: b71f466 ("mediatek: filogic: fix supported_devices list for gl-mt2500") Fixes: 8d30e07 ("mediatek: filogic: fix for new GL.iNet GL-MT2500/GL-MT2500A hardware revision") Fixes: openwrt#20566 Fixes: openwrt/asu#1525 Signed-off-by: Mario Andrés Pérez <mapb_@outlook.com>
These devices share the same "compatible" in device tree causing some incompatibilities (sysupgrades, ASU profile identification), assign a unique "compatible" and "model" to each variant. Context: Commit [1] added each variant's dts compatible to the SUPPORTED_DEVICES field of the other variant to make easy sysupgrades between these physically indistinguishable devices variants possible. But there were found three issues which does not allow this: - the sysupgrade's stricter check still used in some sysupgrade paths(this check is being replaced(and redundant) with the newer fwtool's SUPPORTED_DEVICES check using the info in images METADATA), this check will fail when sysupgrading from a different board_name(compatible dts) that the image was created for (image profile name).[2] - ASU needs unique "dts compatible" to identify the devices profile. - and an ASU's profile identification limitation when several devices from a common target share SUPPORTED_DEVICES entries.[3] There is a proposal for these issues but not yet implemented [4][3]. Until these issues are fixed we won't allow "easy" sysupgrades between these two device variants. Commit [5] avoided the ASU profile identification limitation but missed the required two unique dts compatibles in order to make the two variants fully work, although not allowing easy sysupgrade between them. [1]: openwrt@8d30e07 [2]: sysupgrade stricter check openwrt#20566 (comment) [3]: ASU proposal openwrt/asu#1533 [4]: allow easy sysupgrade proposal openwrt#20947 [5]: openwrt@b71f466 Fixes: b71f466 ("mediatek: filogic: fix supported_devices list for gl-mt2500") Fixes: 8d30e07 ("mediatek: filogic: fix for new GL.iNet GL-MT2500/GL-MT2500A hardware revision") Fixes: openwrt#20566 Fixes: openwrt/asu#1525 Signed-off-by: Mario Andrés Pérez <mapb_@outlook.com> Link: openwrt#21842 Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
These devices share the same "compatible" in device tree causing some incompatibilities (sysupgrades, ASU profile identification), assign a unique "compatible" and "model" to each variant. Context: Commit [1] added each variant's dts compatible to the SUPPORTED_DEVICES field of the other variant to make easy sysupgrades between these physically indistinguishable devices variants possible. But there were found three issues which does not allow this: - the sysupgrade's stricter check still used in some sysupgrade paths(this check is being replaced(and redundant) with the newer fwtool's SUPPORTED_DEVICES check using the info in images METADATA), this check will fail when sysupgrading from a different board_name(compatible dts) that the image was created for (image profile name).[2] - ASU needs unique "dts compatible" to identify the devices profile. - and an ASU's profile identification limitation when several devices from a common target share SUPPORTED_DEVICES entries.[3] There is a proposal for these issues but not yet implemented [4][3]. Until these issues are fixed we won't allow "easy" sysupgrades between these two device variants. Commit [5] avoided the ASU profile identification limitation but missed the required two unique dts compatibles in order to make the two variants fully work, although not allowing easy sysupgrade between them. [1]: 8d30e07 [2]: sysupgrade stricter check #20566 (comment) [3]: ASU proposal openwrt/asu#1533 [4]: allow easy sysupgrade proposal #20947 [5]: b71f466 Fixes: b71f466 ("mediatek: filogic: fix supported_devices list for gl-mt2500") Fixes: 8d30e07 ("mediatek: filogic: fix for new GL.iNet GL-MT2500/GL-MT2500A hardware revision") Fixes: #20566 Fixes: openwrt/asu#1525 Signed-off-by: Mario Andrés Pérez <mapb_@outlook.com> Link: #21842 Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de> (cherry picked from commit 7aa1f7e)
| if profile in supported_devices: | ||
| return True | ||
|
|
||
| if len(profiles) == 1 and "generic" in profiles: |
There was a problem hiding this comment.
if (len(profiles) == 1 or "geos" in profiles) and "generic" in profiles:
To remark that "geos" case is an exception in the "generic" targets/platforms? Adding a comment too?
@efahl I can add a separate commit for that issue since this PR is "touching" this code
These devices share the same "compatible" in device tree causing some incompatibilities (sysupgrades, ASU profile identification), assign a unique "compatible" and "model" to each variant. Context: Commit [1] added each variant's dts compatible to the SUPPORTED_DEVICES field of the other variant to make easy sysupgrades between these physically indistinguishable devices variants possible. But there were found three issues which does not allow this: - the sysupgrade's stricter check still used in some sysupgrade paths(this check is being replaced(and redundant) with the newer fwtool's SUPPORTED_DEVICES check using the info in images METADATA), this check will fail when sysupgrading from a different board_name(compatible dts) that the image was created for (image profile name).[2] - ASU needs unique "dts compatible" to identify the devices profile. - and an ASU's profile identification limitation when several devices from a common target share SUPPORTED_DEVICES entries.[3] There is a proposal for these issues but not yet implemented [4][3]. Until these issues are fixed we won't allow "easy" sysupgrades between these two device variants. Commit [5] avoided the ASU profile identification limitation but missed the required two unique dts compatibles in order to make the two variants fully work, although not allowing easy sysupgrade between them. [1]: openwrt@8d30e07 [2]: sysupgrade stricter check openwrt#20566 (comment) [3]: ASU proposal openwrt/asu#1533 [4]: allow easy sysupgrade proposal openwrt#20947 [5]: openwrt@2c19228 Fixes: 2c19228 ("mediatek: filogic: fix supported_devices list for gl-mt2500") Fixes: 8d30e07 ("mediatek: filogic: fix for new GL.iNet GL-MT2500/GL-MT2500A hardware revision") Fixes: openwrt#20566 Fixes: openwrt/asu#1525 Signed-off-by: Mario Andrés Pérez <mapb_@outlook.com> Link: openwrt#21842 Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
|
I have been checking for any possible side effect and I do not find any, it is the normal and well stablished use of SUPPORTED_DEVICES mechanism to allow "force-less" sysupgrades and everything works as intended except the ASU profile identification. List of issues that are pending of this little fix, basically any "advanced" use of SUPPORTED_DEVICES:
*"owut" which is the only ASU client doing a parallel translation in order to known the specific profile needed for its nice default packages analysis, for example. It must do the same translation as the ASU server. I am willing to propose the needed same little change there to mimic the ASU translation if efahl wish (TODO: this should be an api call to ASU server for doing the translation, centralizing and avoiding inconsistencies?) @aparcar @Ansuel, I tried too much with efahl and that path only got worse and worse, sorry for appealling and pinging to you directly but this seems to be an important missing bit to fully integrate the fwtool's SUPPORTED_DEVICES with the ASU subsystem, and avoid the actual proposed workarounds(remove shared entries) which is limititing the SUPPORTED_DEVICES mechanism too much. TLDR: The proposal is just: look at all the SUPPORTED_DEVICES lists in the target/platform (profiles.json) and select the profile which has the board_name(dts compatible) as first entry in the SUPPORTED_DEVICES list if the entry is shared by several profiles. |
So, what should I understand from the 5 months of completely silent, no single comment? That you completely agree with efahl's unrespectfully feedback without giving a truly reasonable reason? And you just wait hopefully for me disappearing or...idk. What would you think in my shoes? Can you point me to my own errors so I can learn from them, at least? If you both do not beat me definitively and I try to contribute in the future? The best would be you help efahl and me to find a common page... All my comments are public on GitHub and a few in the forum if you want to check. @aparcar |
|
Hey @map-b thanks for your patience. There are so many things to take care of, sometimes it takes a while. I used an AI to tackle this upstream, from my understanding this should improve the situation? I think the "supported devices" string is misleading when it secretly switches your underlying storage and thereby resets your device. This should generally be avoided... @dangowrt not sure how much time you have, but maybe you can also comment on this. We could also attach a single profile into each firmware image, by making devices "self aware", we'd avoid this entire shenanigans... |
|
This exactly what I am trying to avoid... 🙃. Upstream handles this correctly (manual sysupgrading) and its one of the usefulness of the fwtool's supported_devices mechanism. It's ASU who is silently swapping the images. Can't we generalize and consolidate the mapping/translation process in the ASU side? There are Limiting the ability to allow these cross-sysupgrades from "downstream"(ASU) does not seem correct to me. And we will be losing valuable information. But I'm repeating myself now. *@hauke also thought in the "selecting the first entry" idea. |
|
My 2 cent on the topic... but can't we fix this upstream? Upgrade tool shouldn't implement workaround if the project is active and things can be fixed upstream... |
|
I'm not sure if It's a workaround, the actual profile selection in the upgrade tool is a bit, too much simple. The upstream fix is to remove the ability...?
|
The actual handling does not allow to several devices share a DTS compatible string to allow image compatibility between them.
Take the lists as lists and select the profile after checking all possible candidates.
Throw a new validation exception when it is not possible to identify a profile which will help to identify inconsistencies.
This also can help to make a little bit clearer the difference between "profile" and "DTS compatible string".
*There is not need to add [profile] to SUPPORTED_DEVICES list anymore.
asu/asu/util.py
Line 566 in 0328704
Reasoning about the reverts: #1511 (comment) I would like to add that those commits would make harder to understand how we are handling the translation from Device Tree compatible strings to actual profiles names.
Before: 1
After:
With this structure, select the profile where the board_name appear first in the list if the entry is shared.
Fixes: #1525
Fixes: openwrt/openwrt#20566
Footnotes
https://github.com/openwrt/asu/issues/1525#issuecomment-3497242570 ↩