Add domain specific configs to gluon #1216
Conversation
|
I think, i need to change the api of The following example is not easy to solve with this api in my oppinion: site = {
arr_value = {
somekey = {
special_number = 42
}
}
}
domain = {
arr_value = {
somekey = {
another_setting = 'magic'
}
}
}We could now do the following to call the subcheck('somekey', { special_number=42, another_setting='magic'})In other words, we merge the tables. But then inside My idea would be to drop the pass of local prefix = string.format('autoupdater.branches[%q].', key)
need_string(prefix .. 'name')This should work for all purposes and already covers proper error messages. The only thing would be:
|
allowed in both site and domain config
allowed in site only
allowed in domain only
helpful commands:grep -r need_ package/*/check_site.lua | cut -d: -f 2 | sed 's/need_/\nneed_/g' | grep need_ | grep need_site
grep -r need_ package/*/check_site.lua | cut -d: -f 2 | sed 's/need_/\nneed_/g' | grep need_ | grep need_domain
grep -r need_ package/*/check_site.lua | cut -d: -f 2 | sed 's/need_/\nneed_/g' | grep need_ | grep -v need_site | grep -v need_domain |
|
Yeay, it works: trying to put trying to put no domain config: Yeay :) |
|
@NeoRaider I think, I implemented your improvements. What do you say for now? Especially in regard to the chosen (allowed) places for the values? |
|
Maybe the "hostname_prefix" should be in both, site and domain config, because we will start with the zip code and separate by the first two digits. 60xxx and 61xxx will be different meshes. |
|
@skorpy2009 Hm. I think this will introduce another level of complexity. I think the hostname_prefix is only used during config mode and maybe the domain will be set in config mode (or maybe first in the post upgrade process, aka the scripts in /lib/gluon/upgrade). So the changed hostname_prefix only read, during the second time in config mode. Normally there is already a hostname set yet, so the hostname_prefix won't be used neither. Besides: I don't get the advantages, why one wants to store such information in the node name? Wouldn't be storing the domain in another uci value be enough? Maybe add a postal uci value, which is mandatory? |
|
i agree with @lemoer , bad idea to try to store additional data in the hostname |
|
I can first go on with my work, if I know, when the domain config should be changeable. We need a decision here!
@NeoRaider I think this is your decision. I would be happy if you could state your position, so I could go on. |
|
I think 2. is fine for now. Allowing online config changes doesn't really have anthing to do with separating the config into site and domain sections, so I'd like to postpone that to a later PR. |
|
|
|
|
|
The binary for gluon-print-site is about 4K when the debug infos are stripped. |
lemoer
force-pushed
the
pr_check_site_domain
branch
from
767be0c
to
edde0b5
Compare
|
@NeoRaider I think, this is ready to review. |
lemoer
force-pushed
the
pr_check_site_domain
branch
from
edde0b5
to
5541b25
Compare
lemoer
changed the title
|
Some more work to allow aliases:
|
package/gluon.mk
Outdated
| end | ||
|
|
||
| local dir = os.getenv('IPKG_INSTROOT') .. '/lib/gluon/domains/' | ||
| local pfile = io.popen('find '..dir..' -iname \\*.json') -- TODO: not sure about the doublebackslash? why do we need it? |
There was a problem hiding this comment.
leaving a comment as a marker for the TODO
|
please check your code indention again. the current lua code seems to use 3 spaces while yours sometimes has 2 or even 5 spaces |
|
|
||
| domain=$(uci get gluon.@system[0].domain) | ||
|
|
||
| ls /lib/gluon/domains/${domain}.json > /dev/null |
There was a problem hiding this comment.
this is a subtle way of printing an error message. How about
[[ -f /lib/gluon/domains/${domain}.json ]] || echo "file not found: /lib/gluon/domains/${domain}.json" >&2
| */ | ||
| static struct json_object * merge_json(struct json_object *a, struct json_object *b) { | ||
| if (!json_object_is_type(a, json_type_object) || !json_object_is_type(b, json_type_object)) { | ||
| json_object_put(b); |
There was a problem hiding this comment.
why is the reference count for b decremented here? I do not think this should be inside this function.
also returning a if a is not a json_type_object is possible from l46-48
There was a problem hiding this comment.
I'm not sure, how this should be possible outside the function? merge_json() is called recursively.
|
Honestly swapping between site and domain is confusing at times. Can we call the shared configuration |
lemoer
force-pushed
the
pr_check_site_domain
branch
2 times, most recently
from
09b5bdd
to
8cc67ea
Compare
| @@ -0,0 +1,10 @@ | |||
| #!/bin/sh | |||
|
|
|||
| domain=$(uci get gluon.@system[0].domain) | |||
There was a problem hiding this comment.
The variable is not populated by default, also gluon would be an entirely new section, while gluon-core already exists.
There was a problem hiding this comment.
Yes, neoraider said, that he want to merge all the gluon-* sections in one gluon section in near future. And I already use this new section. But I'm currently not sure how to introduce sections. I'll add a todo for this.
But you pointed me to an other error. It should be uci get gluon.system.domain. Thanks.
|
Found a small bug: |
| GLUON_SITEDIR='$(call qstrip,$(CONFIG_GLUON_SITEDIR))' lua -e 'print(require("cjson").encode(assert(dofile("../../scripts/domain_config.lua")("'$$$${dc}'"))))' > $(PKG_BUILD_DIR)/domains/$$$${dc}.json; \ | ||
| aliases=$$$$(GLUON_SITEDIR='$(call qstrip,$(CONFIG_GLUON_SITEDIR))' lua -e 'for alias_name, _ in pairs(dofile("../../scripts/domain_config.lua")("'$$$${dc}'")["domain_aliases"] or {}) do print(alias_name.." ") end'); \ | ||
| for alias in $$$${aliases}; do \ | ||
| ln -s $$$${dc}.json $(PKG_BUILD_DIR)/domains/$$$${alias}.json; \ |
There was a problem hiding this comment.
replace ln -s with ln -sf here.
There was a problem hiding this comment.
I decided to use rm -r to clean the whole directory domains/ before entering the for loop, so the whole process should be a little more idempotent. Deleting a domain in site repo hat no effect before this additionional rm.
lemoer
force-pushed
the
pr_check_site_domain
branch
from
2f40bbe
to
2619c85
Compare
|
Pushed a fix. |
|
Why should we continue executing upgrade scripts if one fails? I think this increases the chance to overlook the errors. E.g. the openWRT build scripts also fail if there is an error instead of trying to continue. |
|
|
As domain_changed.sh is only to be called directly by the user, when they manually changed the |
|
I think it makes sense to unify the logic handling initial configuration and changing domains (eventually), so this logic should not behave differently. |
|
Do you only mean it should behave equally or do you also mean it should be the same piece of code executing the scripts? |
|
In the long term, it should be the same piece of code, but this is not a requirement for getting this PR merged. |
lemoer
force-pushed
the
pr_check_site_domain
branch
from
2619c85
to
33765c7
Compare
|
Updated. |
|
I've just pushed 0b80f1b, which allows us to avoid some code duplication. |
|
I have applied parts of this PR that also work without actual multi-domain support. In 50812b1, I have made the following changes:
|
rotanid
added
the
2. status: merge conflict
|
I've pulled the rest of the changes into a local branch and will take it from here. The most important piece missing now is an updated site documentation. |
|
@NeoRaider isnt #1261 the documentation update? if that is not complete, please comment over there |
|
@NeoRaider Thanks for reworking. I like the idea to merge stub no-ops as a prepare step. (I think we can leave this PR/issue open, until the feature is merged into the master branch) |
|
Multidomain support is merged now. It is disabled by default and can be enabled by setting GLUON_MULTIDOMAIN=1 in site.mk. |
intro
As freifunk networks become larger the ground noise in the batman layer 2 network increases. At about ~1000 nodes, we reach a point, where we want to split the network in different domains (layer 2 segments). So need to serve different configs. With current implementation in gluon, we need to build one firmware per domain, as there is a possibility for one
site.confper image. The idea is to introduce multiple domain configs per image, which could be changed without reflashing the router.about this pr
The gluon framework ensures, that the
site.confis valid before the image is build (at least in some limits). This is done per module (package/gluon-pkg-name/check_site.lua) with help of thecheck_site_lib.lua. This PR tries to extend the functionalities of the check_site_lib to verify domain specific configs also. This PR does not implement a functionality to read the domain values inside the firmware yet.ideas
site/domains/domainname.confsite/site.confstate
/lib/gluon/domains/domainname.json)forbid_in_domain('config_mode.geo_location.show_altitude'))need_table(...)function should be handled correctlyfail at compiletime, since they could use values, which should be forbidden in domain specific config?site/domains/directory is not existing?forbid_in_*toneed_*,need_domain_*,need_site_*domain.confbecomes necessary.if domain ~= nilcheckscheck_domain(site, nil)implement the query in each packageif domain.opkg or if site.opkg ...site