Dockerise semonitor

[oliv3r]

This MR creates a container environment (Docker, podman etc) to launch semonitor within. This makes life a bit easier in managing solaredge (setting up python and the needed deps).

One thing missing, is a way to monitor semonitor's health. I know its listening on port 80, but is there an easy command we can check this against? (wget for example). I don't see anything 'LISTENING' on my ports (80, 22221/2), but during startup it does print ports: 22222, 22221, 80.

[oliv3r]

While not a hard need, I'd required to merge #179 first, to avoid warnings and errors.

Also, in theory, the container building stage of the pipeline can remove the 'test' as it runs the same tests. Currently, tests are only run for amd64 (or rather, the 'host' OS). But could even test all architectures.

[rgstephens]

I know its listening on port 80, but is there an easy command we can check this against?

You might want to look at uptime robot and upptime.

[jbuehl]

One thing missing, is a way to monitor semonitor's health. I know its listening on port 80, but is there an easy command we can check this against? (wget for example). I don't see anything 'LISTENING' on my ports (80, 22221/2), but during startup it does print ports: 22222, 22221, 80.

I'm not sure what this would accomplish. On startup semonitor listens on those 3 ports only when it is in network mode for a connection from an inverter to one of them. Once the connection is made it is maintained indefinitely and semonitor doesn't listen on ports any more as long as the connection stays up. If you are monitoring the output of semonitor you can quickly tell if something has stopped working.

[ericbuehl]

yay! lets bring this project into the 21st century with containers!

[ericbuehl]

I'm not a fan of alpine -- can you swap this for something like slim?

[oliv3r]

I'm a HUGE fan of alpine :p I don't mind switching it, its your project of course, but on what grounds? Because -slim will probably bring the container from the current 75M (which is quite large imo) to whatever slim will do; i'll try though of course.

btw, for the first time (though I don't watch the log often) got this:

| /dev/ttyUSB0 <-- message: 2484 length: 22
| Exception in thread master thread:
| Traceback (most recent call last):
|   File "/usr/local/lib/python3.11/threading.py", line 1038, in _bootstrap_inner
|     self.run()
|   File "/usr/local/lib/python3.11/threading.py", line 975, in run
|     self._target(*self._args, **self._kwargs)
|   File "/usr/local/src/semonitor/semonitor.py", line 128, in masterCommands
|     masterGrant(state, dataFile, recFile, slaveAddr)
|   File "/usr/local/src/semonitor/semonitor.py", line 150, in masterGrant
|     masterTimer.start()
|   File "/usr/local/lib/python3.11/threading.py", line 957, in start
|     _start_new_thread(self._bootstrap, ())
| RuntimeError: can't start new thread
| /dev/ttyUSB0 --> message: 2484 length: 22

I wonder if that's related to alpine; as I haven't seen this before ... (and been running semonitor for 5 years now)

[ericbuehl]

I don't mind switching it

Thank you.

there's plenty of blog posts out there detailing experiences souring on alpine. I've had my own as well. tl;dr: alpine is often too stripped down but the missing pieces are not always evident until performance or security issues pop up. if one wants to build a truly bare-bones image they should probably use distroless or scratch. the extra 75MB in this case is more than worth it

[oliv3r]

I don't mind switching it

Thank you.

there's plenty of blog posts out there detailing experiences souring on alpine. I've had my own as well. tl;dr: alpine is often too stripped down but the missing pieces are not always evident until performance or security issues pop up. if one wants to build a truly bare-bones image they should probably use distroless or scratch. the extra 75MB in this case is more than worth it

Hmm, I can only strongly dissagree here (and using alpine professionally as well. I do know however, that alpine and python aren't huge friends, mostly due to 'wheels'.

As for the 'too barebones', I guess that's to be expected when you only have busybox and musl. Actually, distroless/scratch mess up the docker-guidelines by not offering a shell. Yes, it'll save you a few k, but it makes life very difficult (even in CI). Though I ought to try klibc for that at some point, it offers an even smaller shell :)

Only thing that's not so easy with debian is virtual add, e.g. add packages, pip install, remove virtual packages (gcc and it's dependencies). So there will be some nastyness there.

Anyway, this all just gets philosophical, even though your change request is I suppose that :D anyway, I'm changing it.

Btw, the error I posted has nothing to do with alpine, but I reduced my number of processes to 64, because, what would semonitor need more then a handful of processes for. Guess this limit somehow affects thread spawning; so lets see what it does with my new limit of 4096, if semonitor needs 'infinite' due to some leak...

[oliv3r]

So the debian image is a bit ugly, and clocks in at 175MiB, so about a 100M more for the same thing. More then double the price to please you ;)

[ericbuehl]

what is this wrapper intended to do? i'd prefer to keep the stock entrypoint logic

[oliv3r]

What is stock? This is to follow the docker-guidelines, which says to make the container behave as the user would expect.

E.g. if no arguements are passed, or if a binary from path is launched etc etc, we don't execute semonitor. So launching it with /bin/sh; we get a shell to enter the container.

[ericbuehl]

ah! in that case can you put the expected invocation of semonitor as a COMMAND or ENTRYPOINT in the Containerfile?

[oliv3r]

That's not what the docker guidelines recommend, because it would become a really long confluent entrypoint. e.g. take the entrypoint script, and turn it into a huge array.

The CMD being empty, means right now it will run semonitorwithout args, e.g. --help.

What is your concern here? I've done probably 20 of these upstream containers (from tvheadend :p, to clamav) and they all follow the same pattern.

[ericbuehl]

If I replace ENTRYPOINT in your containerfile with [ "semonitor.sh" ] I get the same effect. eg:

✗ docker run -it 281fd1590d41 -h   
usage: semonitor.py [-h] [-a] [-b BAUDRATE] [-c COMMANDS] [-d LOGFILE] [-f] [-k KEYFILE] [-m] [-n INTERFACE] [-o OUTFILE] [-p PORTS] [-r RECORD] [-s SLAVES] [-t {2,4,n}] [-u UPDATEFILE] [-v] [-x] [datasource]

Parse Solaredge data to extract inverter and optimizer telemetry

positional arguments:
  datasource     Input filename or serial port (default: stdin)
...

Perhaps an example of what your wrapper enables would be helpful.

That's not what the docker guidelines recommend

This guide says that The best use for ENTRYPOINT is to set the image’s main command, allowing that image to be run as though it was that command, and then use CMD as the default flags.. If starting required more than one step then the helper script makes sense but in this case it's only ever invoking semonitor.sh -- which itself is another wrapper.

[oliv3r]

You might want to look at uptime robot and upptime.

And what will that do? An external service, that'll do what? The healthcheck is intended to validate that semonitor.py is still functioning properly. E.g. if you send a ping on port 80, it will reply with pong; and it would only do that, if semonitor is happily answering on its interfaces for example. So not sure how uptimerobot would help?

[oliv3r]

One thing missing, is a way to monitor semonitor's health. I know its listening on port 80, but is there an easy command we can check this against? (wget for example). I don't see anything 'LISTENING' on my ports (80, 22221/2), but during startup it does print ports: 22222, 22221, 80.

I'm not sure what this would accomplish. On startup semonitor listens on those 3 ports only when it is in network mode for a connection from an inverter to one of them. Once the connection is made it is maintained indefinitely and semonitor doesn't listen on ports any more as long as the connection stays up. If you are monitoring the output of semonitor you can quickly tell if something has stopped working.

Ah right, it's just I noticed that semonitor was printing it, that's all. Your reply makes total sense of course.

So semonitor's output goes to stdout, but the heatlcheck script, generally runs within the container, so we'd have to tee the output somewhere, or have something pickup the output. But whatever we use to check, it'll have to be scriptable and reliable of course.

Personally, I run semonitor with -vv which tells me that data was sent/received, with just a single -v it already becomes very silent... food for later ;)

[oliv3r]

I'll rebase and address the open points later; which should fix the pipeline

[ericbuehl]

If I replace ENTRYPOINT in your containerfile with [ "semonitor.sh" ] I get the same effect. eg:

✗ docker run -it 281fd1590d41 -h   
usage: semonitor.py [-h] [-a] [-b BAUDRATE] [-c COMMANDS] [-d LOGFILE] [-f] [-k KEYFILE] [-m] [-n INTERFACE] [-o OUTFILE] [-p PORTS] [-r RECORD] [-s SLAVES] [-t {2,4,n}] [-u UPDATEFILE] [-v] [-x] [datasource]

Parse Solaredge data to extract inverter and optimizer telemetry

positional arguments:
  datasource     Input filename or serial port (default: stdin)
...

Perhaps an example of what your wrapper enables would be helpful.

That's not what the docker guidelines recommend

This guide says that The best use for ENTRYPOINT is to set the image’s main command, allowing that image to be run as though it was that command, and then use CMD as the default flags.. If starting required more than one step then the helper script makes sense but in this case it's only ever invoking semonitor.sh -- which itself is another wrapper.

[ericbuehl]

TIL about "Containerfile". That said, my local runtime (docker mac) does not seem to find this file name as a default. Can we keep with the ad-hoc standard name of "Dockerfile" for now?

[oliv3r]

'Dockerfile' is slowly being faced out, because its not inclusive :) But i'll fix it.

btw, I can't reply to your previous comment about the guidelines, so I'll reply here.

If you look at https://github.com/docker-library/official-images#consistency

All official images should provide a consistent interface. A beginning user should be able to docker run official-image bash (or sh) without needing to learn about --entrypoint.

e.g. if you do
docker run -it 281fd1590d41 /bin/sh
it shouldn't work, which the guidelines say, the user is expected to do.

[ericbuehl]

If you look at https://github.com/docker-library/official-images#consistency

Gotcha -- It seems as though we're in limbo between conditions #1 and #2

1. If the startup process does not need arguments, just use CMD
2. If there is initialization that needs to be done on start, like creating the initial database, use an ENTRYPOINT along with CMD

Amending my previous suggestion: ENTRYPOINT CMD ["semonitor.sh"] may be more appropriate.

Ironically, this doc on consistency is inconsistent itself: how would one support both of these scenarios without a wrapper script and thus how could suggestion #1 ever be complete?

docker run official-image bash
docker run official-image --arg1 --arg2

The several "good" examples references in the next section all use CMD ["binary'] without a wrapper script.
https://github.com/docker-library/php/blob/b4aeb948e2e240c732d78890ff03285b16e8edda/5.6/Dockerfile#L65
https://github.com/docker-library/python/blob/3e5826ad0c6e29f07f6dc7ff8f30b4c54385d1bb/3.4/Dockerfile#L45

Do you think our situation is significantly different than these examples?

[oliv3r]

I think you can't :) what's worse, dumb-init/tini even states you should avoid having single process containers (where you run daemons/servers).

putting semonitor.sh in command would basically do the same thing as the wrapper, but you still need the wrapper for the entrypoint :) so then you just are duplicating stuff.

But I agree, the docs aren't perfect, some texts are almost 10 years old without update.

I think most of those scripts however, are all short-lived processes. e.g. jq to give a nice example. We have a 'server' that runs forever. That's the major difference I suppose ...

[oliv3r]

Finally the CI builds :) took some effort; but there it is.

Lets test it a bit before merging. I can/will test it on my RS482 SE5000HD setup; Can you test it with your setups? I think you use an Ethernet based setup (is that even supported? I thought the new crypto made it that this is now obsolete)?

[oliv3r]

@ericbuehl So I've made it that hopefully everybody is happy :) (which is also a very common way 'of the hub' :p).

One "issue" remains, is that for whatever reason, @jbuehl 's limits are linked to LFS and we have run out of LFS quota. I have no idea why this is or how this works. Even on my fork, which you'd expect has its own quota, it didn't work.

I had to create a dummy account, fork the repo (which even then, didn't work) and after requesting to remove the forking relationship (which took a few hours), did it work as you can see here: https://github.com/oliver-githubdummy1/solaredge/actions

The moment I push to this repo however, it still fails with LFS errors.

I therefore suggest to create a new org on github, and transfer (and rename it to semonitor maybe) to there. This would then break all restrictions linked to @jbuehl's account. For link-integrety sake, @jbuehl can then 'fork' the organisations repo to his own space (remove issues and PR's etc) so that it can be found in the same place.

Unless this is all just a bug in github :p but I can't find anybody else complaining ...

[jbuehl]

The LFS bandwidth quota for my account was exceeded due to the large number of times this PR and its associated test data was uploaded in the past month. The quota will automatically be reset in 13 days, so I would suggest waiting until after July 20 and trying again.

[oliv3r]

The LFS bandwidth quota for my account was exceeded due to the large number of times this PR and its associated test data was uploaded in the past month. The quota will automatically be reset in 13 days, so I would suggest waiting until after July 20 and trying again.

Oh wow, 1G isn't all that much. I wonder if turning the repo into an org would even remotly help in that case. Maybe opensource projects get more allocation?

It's really weird though that this got yanked out of your account, cause I did most of the work on my fork; with just some pushes (which also count, I know), but the LFS data was hardly touched/modified. I think they are doing something wacco where even a fork's bandwith for LFS accounts for the repo's original owner. I couldn't get my 'dummy' thing working until I disconnected the forking relation ship. Super weird for sure.

I make my case for turning this repo into an organization even more now though;

You must manage billing settings and paid features for each of your accounts separately. You can switch between settings for your personal account, organization accounts, and enterprise accounts using the context switcher on each settings page. For more information, see "About billing on GitHub

So limits are at the least tied to an org; meaning it won't affect your personal account. I've created an org, and made @jbuehl and @ericbuehl owners; feel free to kick me out ;) but this will at least not affect your personal account any longer.

[oliv3r]

So due to other priorities I hadn't touched this since last time I responded, but it's still complaining about being over quota (or again?).

Can't imagine how it relates to #181 from @ericbuehl ... and don't see any other MR's in this repo ...

[jbuehl]

The LFS quota is displaying exactly the same message saying it will reset in 13 days, but it obviously didn't. I don't know what is wrong but I don't have any time to spend on this.

[oliv3r]

@jbuehl understood; the option to transfer this to the org is still an option. I'll try again in 14 days if I remember/have time :)

Meanwhile, @ericbuehl how do you feel about transferring this project to the org? This would take minimal effort of @jbuehl by just hitting the 'transfer' button of this repo, after which you/I can figure out and fix what's going on, without burdering @jbuehl any more then needed.

After the transfer, having a (read-only) fork here (the original location) is a good idea for continuity, but that's secondary/optional.

Thanks!