search

facebookarchive / CommunityCellularManager

Tool for deploying, managing and controlling your Community Cellular Networks
Other
87 stars 36 forks source link

osmocom packaging, take 2 #3

Closed earwin closed 8 years ago

earwin commented 8 years ago

Fixed osmo-bts dependency on Debian. Postgres pre-depends stanza is now compatible with 'wheezy', 'jessie', 'trusty', 'xenial'

shaddi commented 8 years ago

Thanks for your PR!

As a meta-comment for the future -- please group your pull requests into logically distinct units. Your individual commits look well-structured, but it makes reviewing (and accepting) PRs easier and faster if we can break them into smaller chunks.

I'm going through and making a few comments in the commits themselves. One higher level comment (and consistent theme of my comments) -- I'd like to see Fairwaves-specific configuration factored out, similar to what we're doing now with Osmocom and OpenBTS support. That way we can allow vendors like yourselves to maintain configuration independently for your specific platform upstream, without requiring all users of a particular stack to assume your defaults.

earwin commented 8 years ago

@shaddi This was commissioned from me as a singular body of work with two requirements — "do the packaging for endaga osmocom flavor", and more important "make endaga osmocom work on fairwaves BTSs". Thus a single pull request with detailed commits. In addition to that PR there's also specially crafted set of osmocom packages that blends latest master and fairwaves not-yet-upstreamed patched versions + ubuntu-trusty-compatible build of freeswitch.

Had a pretty long chat with Kurtis and Omar a few days ago, detailing my changes and the reasons for them. You could ask them for the transcript I think. I'll go through your comments and answer them tomorrow.

Regarding factoring out.. I support the idea. But there are various opinions. E.g. I'd like to see osmo-trx configuration and runscripts completely out of endaga, as they are highly hardware-specific. Not even SDR-platform-specific, but down to whatever you have in your radio path after the SDR. Kurtis wants to have a turn-key solution. Again, please go through the chat logs.

kheimerl commented 8 years ago

I think intellectually we wanted the two requirements to be separate PRs and potentially separate vagrant targets: there's just going to be a lot of overlap. Step 1 was get osmo deployment packaged well and step 2 was get fairwaves deployment. It's likely that the smart path is to have a "fairwaves" deploy target (vm, maybe package?) that has the configs as fairwaves thinks they are best and have that run in parallel to the default osmo installation.

We do not want the default osmo installation to be the fairwaves specific version. That breaks requirement 1 on any other hardware. That's no beuno and I think the current state of this PR.

Does that make sense?

shaddi commented 8 years ago

Understood, the changes in this PR are fine, just in the future it will be easier and faster to review if it comes in multiple PRs. For example, the refactor of the build system could have been separated from the changes to the configs.

Exactly because of the platform specific nature of dependencies, we'd like to either figure out whether the defaults make sense globally (for all Osmocom-based platforms), and if not, let's factor them out into their own target. Perhaps you could add a new build target (e.g., "osmocom-fairwaves") that creates an "endaga-osmocom-fairwaves" package, which would let you all have your own set of dependencies. How does that sound to you?

earwin commented 8 years ago

osmo-sip-connector, osmo-pcu, osmocom-sgsn were pointing to non-existent versions. let's take osmocom-sgsn as an example:

rsyslog was downgraded to ubuntu trusty version. but as I mentioned in a chat, situation with rsyslog is a bit of a mess. if we accept that ubuntu users have to pull it from rsyslog's PPA, this downgrade might as well be dropped.

earwin commented 8 years ago

@kheimerl For osmo-trx, osmo-bts, (maybe also osmo-pcu) I don't think you could possibly have a default config that works for everyone. E.g. in this PR I change the band to GSM900. That's a definite lock-in to a certain subset of hardware. But you had it at GSM850, that's a lock-in as well!

just to be clear where it comes from — osmocom configuration I supplied is a copy of our working and tested in the field setup + endaga-specific mods mentioned by Omar.

Maybe you should consider config templating to marry whatever endaga requires with user settings.

earwin commented 8 years ago

So, currently, I'm not quite sure what I could pull into hypothetic endaga-osmocom-fairwaves. relaxed osmo-bts dependency, and that's it?

9muir commented 8 years ago

A couple of high-level comments, but I have nothing specific to add to the Osmo/Fairwaves discussion since I'm not familiar with Osmo (yet).

I'll go one step further than saying this should be two separate PRs (if you were starting from scratch - I'm not asking you to go and do this) and instead suggest a bunch of minimally sized PRs, to the extent that many of the commits are independent of each other.

I like the split of the Ansible deploy scripts, but it looks like you're starting to reinvent Ansible's roles, so why not just refactor these as roles?

kheimerl commented 8 years ago

@earwin Maybe that is it? I think you also point at a few repos that the mainline osmo release wouldn't as well?

earwin commented 8 years ago

@kheimerl If you're speaking of Fairwaves private repo, then you're wrong. This private repo is used on our BTSes to provide them with

But it is not mentioned in this pullrequest, and it is in no way required for it to operate. The repos you used before + debian, work as well.

(if you meant something else, please rephrase)

earwin commented 8 years ago

(Oh! A pullrequest tracks the original branch? I wasn't aware of it. So, what you see above is two rebases.)