Decomission the upnp container and bring it into hm-config

[vpetersson]

Background

We currently have a container running on the miners with the sole purpose of enabling uPNP. We can quickly just sunset this in favor of a single command in this repo.

It is however worth noting that this will be unnecessary once we move to light miners, but the timeline for this keeps slipping.

Acceptance Criteria

• Remove the hm-upnp container from docker-compose.yml
• Add the upnp package to the Dockerfile
• Create a new task that runs every 12 hours that invokes the command

[shawaj]

@vpetersson IMO this should be in hm-config not hm-diag.

Because UPnP requires host networking and so does config. Whereas diag doesn't

[shawaj]

Also @vpetersson I don't think we need to run UPnP every 12 hours. We can just run it once at container start up

[vpetersson]

Yeah, you're right. I was thinking this would solve the IP announcement with Helium, but those are two separate issues altogether. Just mixed them up in my head.

[MuratUrsavas]

So, are we moving this task into hm-config?

[MuratUrsavas]

What about this:

while(regionID is None):
    # While no region set
    try:
        with open("/var/pktfwd/region", 'r') as regionOut:
            regionFile = regionOut.read()

            if(len(regionFile) > 3):
                print("Frequency: " + str(regionFile))
                regionID = str(regionFile).rstrip('\n')
                break
        print("Invalid Contents")
        sleep(60)
        print("Try loop again")
    except FileNotFoundError:
        print("File Not Detected, Sleeping")
        sleep(60)

Should we really wait for it before enabling UPnP?

[shawaj]

So, are we moving this task into hm-config?

Yes we should. We don't want host networking in diag container IMO

[shawaj]

Moved it now @MuratUrsavas

[MuratUrsavas]

Also, @vpetersson I'm running this task only once at the container startup like @shawaj said. Is that cool?

[vpetersson]

Yeah that's fine. I confused the uPNP container with the IP logic that resides in the miner.

[MuratUrsavas]

Right now I'm testing the changes and can see that hm-config container is listening port 44158 for uPnP. But HOST OS does not report the same (Checking from Balena Dashboard terminals). Is this the expected behavior?

[vpetersson]

uPNP shouldn't require any open/listening ports. All it does is to send a packet to the router where it asks to do a port forwarding to port 44158 on the device sending it (/usr/bin/upnpc -e Nebra Helium -r 44158 TCP).

Port 44158 is the port of the miner and not hm-config.

[MuratUrsavas]

This is interesting. The command you've mentioned is there right now and I'm seeing 44158 is listened in hm-config. Since gateway-config container is in Host network mode, it could be showing hm-miner's listening port? Could it be the case?

[vpetersson]

Yes, because of host networking, so you're picking up the one from hm-miner. hm-config doesn't have any ports exposed explicitly.

[MuratUrsavas]

Got it! Looks like this PR could make it into current sprint.

[MuratUrsavas]

@vpetersson I have a question about FIRMWARE_VERSION parameter. I've changed it in docker-compose.yml from hm-config repo. Normally this parameter is broadcasted by helium-miner-software repo. Why do we need this also in here? Wouldn't that create a confusion, if the environment variable wouldn't get overridden?

If we should keep it, then what's the rule?

[shawaj]

It's just for building and testing locally I think

[vpetersson]

@MuratUrsavas yeah so this is a bit confusing and I think we should overhaul this. At this point, we only care about exposing the Helium Miner Software git has to hm-diag (i.e. FIRMWARE_VERSION). This will allow us to manually traverse back and find what exact hash of each corresponding sub-component (since the docker tag is the same as the git hash).

In the future, we will be introducing the semantic versioning for Helium Miner Software (NebraLtd/helium-miner-software#351), which will make this a bit more human readable.

[MuratUrsavas]

@shawaj if that's the case then maybe we should consider removing it from all sub-repo's. IMHO it's a source of conflict. Maybe we could create MICRO_SERVICE_VERSION parameter or a similar one for that purpose later.

[shawaj]

@MuratUrsavas it needs to be FIRMWARE_VERSION because the codebase looks for that specific environment variable.

The actual content of that variable is irrelevant. It could just be TEST.0.1 or whatever

We don't ever use the local repo docker compose for anything other than testing.

[shawaj]

@MuratUrsavas yeah so this is a bit confusing and I think we should overhaul this. At this point, we only care about exposing the Helium Miner Software git has to hm-diag (i.e. FIRMWARE_VERSION). This will allow us to manually traverse back and find what exact hash of each corresponding sub-component (since the docker tag is the same as the git hash).

In the future, we will be introducing the semantic versioning for Helium Miner Software (NebraLtd/helium-miner-software#351), which will make this a bit more human readable.

@vpetersson this is not correct. Firmware version is the miner version. That is needed in both config and diag and possibly some of the other containers. But we put the firmware version into every container so that the containers have an environment variable update every time we push an update so the containers are all restarted.

[MuratUrsavas]

@vpetersson this is not correct. Firmware version is the miner version. That is needed in both config and diag and possibly some of the other containers. But we put the firmware version into every container so that the containers have an environment variable update every time we push an update so the containers are all restarted.

But the same environment variable also comes from helium-miner-software and the current one is overridden (AFAIK) by it. So we'll always have a FIRMWARE_VERSION parameter coming from main repo, unless the sub container is solely working just by itself.

[vpetersson]

@shawaj good point about pushing this to all containers. That's true.

That said, we want to move away from using the Helium Miner Version as our version tracker in favor of a Nebra flavored Semantic Version. This is the first step towards this.

(This Issue is the wrong issue to discuss this in fwiw.)

[shawaj]

The docker-compose.yml in this repo is never used when building for the actual fleet (testnet or mainnet). It's only ever used for local building and/or just as a reference on how the container needs to be run.

Maybe good to rename it to docker-compose-example.yml or something

[MuratUrsavas]

OK, this short discussion opened the way for me and I'm creating PR's right now.

[vpetersson]

The docker-compose.yml in this repo is never used when building for the actual fleet (testnet or mainnet). It's only ever used for local building and/or just as a reference on how the container needs to be run.

Maybe good to rename it to docker-compose-example.yml or something

Ah I see what you mean. Yeah this docker-compose.yml file should be renamed to something like docker-compose_dev.yml or similar.

[MuratUrsavas]

Done and merged to master.

[MuratUrsavas]

Merged into production, closing the issue.