search

jasonacox / Powerwall-Dashboard

Grafana Monitoring Dashboard for Tesla Solar and Powerwall Systems
MIT License
302 stars 66 forks source link

Docker Compose version v2.25.0 reports version is obsolete #454

Closed BJReplay closed 8 months ago

BJReplay commented 8 months ago

Addresses https://github.com/jasonacox/Powerwall-Dashboard/issues/453

jasonacox commented 8 months ago

Thanks @BJReplay - I like the direction but see a problem with V1 docker-compose. Here is what I get on my V1 host:

Running Docker Compose...
ERROR: Invalid interpolation format for "weather411" option in service "services": "${PWD_USER:-1000:1000}"

It seems that version 3.5 is needed to support that. Unless you have another idea for us to try, we could look at removing V1 support but would want to provide warning before doing the upgrade. I don't know how widely V1 is used on older devices or NAS devices like Synology.

BJReplay commented 8 months ago

Here is what I get on my V1 host:

Fair enough. I'm guessing you have a V1 host for testing compatibility.

It is just a warning, so it is OK to ignore, so perhaps the right approach is to just ignore it.

That said, I'd hate to be running something like a Synology (or anything else) that is stuck running 2016 era software; I'm running a 2015 vintage QNAP, but I would scrap it if it didn't have more up to date software available as I would see 2016 vintage software as such a massive security issue. I imagine I could find a number of serious security issues with docker between 2016 and now, for example.

jasonacox commented 8 months ago

Yes, I keep the old V1 system around for testing, but I think this is enough to help justify removing V1 support:

https://docs.docker.com/compose/compose-file/compose-versioning/

image

We can add logic to upgrade.sh to test to see what version the users is running and stop the upgrade until they upgrade their Docker compose version.

BJReplay commented 8 months ago

Actually, hang on a minute; on your V1 host, what exactly are you running? As in version of docker and version of docker compose?

Because the docker file format that you're running is V2 (that's the version that introduced the services tag).

And V2 docker compose was introduced a long time ago; now it wasn't mandatory to upgrade to the new version of docker compose, but it does sound like you've got a version that has been unsupported.

I see (your reply popped in while writing this).

BJReplay commented 8 months ago

So, to expand for a second: you can't have a services tag without the file being a V2 format. And, in that format, the version tag is optional. But only very early (pre 2016) version of docker-compose don't handle that. There's a small set of docker-compose versions that are new enough to handle the service tag but old enough to not handle the missing version tag.

I suspect it's a very small set.

It could be just our luck that's the set baked into Synology (or pick any other NAS) firmware, but it's unsupported.

jasonacox commented 8 months ago

Yes, my test box has docker-compose version 1.17.1-2. Based on what you say, we probably already broke older V1's by introducing the version 3.5 format. If it is a small set, more reason to remove support for V1. It also means, I can remove one of my tests. :)

I would still like to provide the check and ERROR message to the user to upgrade Docker before running upgrade.sh. I would like to avoid breaking a working system even if it is depreciated.

BJReplay commented 8 months ago

I would still like to provide the check and ERROR message to the user to upgrade Docker before running upgrade.sh.

I'll have a look and see if I can work out what is required, and if I can, include that in this PR. If I think it's beyond my meagre bash skills, I'll let you know. I won't be able to test a V1 system, but I can test on my system to test the happy case (user already on V2) assuming I can get it working. Let me get back to you.

BJReplay commented 8 months ago

Added a check to upgrade.sh. Basically borrowed the check from compose-dash.sh and modified it.

jasonacox commented 8 months ago

Looks good. I want to run some tests. Also @mcbirse , I would love your thoughts on this as well. Any concerns?

Thanks @BJReplay

mcbirse commented 8 months ago

@jasonacox - I have not tested, but no concerns from my end.

I noted long ago the "version" tag was optional in the docker compose yaml V2 spec, although no warnings were being given at that stage...

I am definitely in favour of removing support for V1 and advising users to upgrade.

Apologies for not being active lately. Life/work has been hectic since trying to catch up from my covid knockdown/outage recently. 😠

BJReplay commented 8 months ago

@mcbirse I hope you're feeling better: my second covid infection set me back as it exacerbated heart conditions that were previously well managed; it took me a long time to get back to feeling myself, so I wish you well.

jasonacox commented 8 months ago

Test (PASS - correct behavior) on old V1 system:

$ ./upgrade.sh
Upgrade Powerwall-Dashboard from 4.1.2 to 4.2.0
---------------------------------------------------------------------
This script will attempt to upgrade you to the latest version without
removing existing data. A backup is still recommended.

Checking Docker Compose...
ERROR: Docker Compose V1 Found: Upgrade Required
See Migration Instructions at https://docs.docker.com/compose/migrate/
jasonacox commented 8 months ago

Thanks @BJReplay !

I set this as 4.2.0 (indicating possible breaking change for some folks due to the V1 support removal).

BuongiornoTexas commented 8 months ago

@jasonacox - does this allow reverting of #442? From the discussion, the main reason for rejecting was to maintain compatability with docker-compose 1.17.

jasonacox commented 8 months ago

Honestly, I'm not smart enough to know how to do that. What would you like to see? Happy to push forward.

BuongiornoTexas commented 8 months ago

I should rephrase - would you like to reinstitute the change? There wasn't much to it, and I can probably create a new PR with a bit of diff work on verify.sh.

jasonacox commented 8 months ago

OH! That was the logic in upgrade.sh that offered to upgrade older grafana.env, right? I made it more hard core (just nuked the old grafana.env with the new list you built, unless the user said otherwise). Was there another benefit that would help with customization setups like yours?

In any case, I'm happy to add it back in if you think it is helpful or leave it as is. And very happy with your tuned plugin list even brute-force mode. That was a good PR improvement. Start up time are significantly faster.

BJReplay commented 8 months ago

Thanks @BJReplay !

I set this as 4.2.0 (indicating possible breaking change for some folks due to the V1 support removal).

Always a pleasure to contribute, even in a tiny way... Speaking of tiny; I'm just getting into home assistant and Tuya (I have just two "smart" devices in my house - most of the house has MR12 LED downlights that were upgrades from halogen) and the two devices were not Tuya devices, but Arlec (an Australian brand), but anyway, tuya internals. And, in my travels today I discovered, of course, that you wrote tinytuya! I've just got my light globes hooked up to local control via localtuya in HA, and I used tinytuya to understand how to set them up; they have a switch, brightness, and a scene setting but it wasn't obvious how to map the DPs. So thanks for that, as well!

BuongiornoTexas commented 8 months ago

OH! That was the logic in upgrade.sh that offered to upgrade older grafana.env, right? I made it more hard core (just nuked the old grafana.env with the new list you built, unless the user said otherwise). Was there another benefit that would help with customization setups like yours?

In any case, I'm happy to add it back in if you think it is helpful or leave it as is.

My turn to discover that I should have checked what you did more carefully - very happy with the nuke and replace option. My version was around managing the few who have custom plugins (ironically, I'm not one of them).

I think in the interim, I'll just put the code in an archive and come back to it if it becomes relevant in the future. Less work for both of us and we are both happy with the current setup!

The one thing that might be worth considering is nuking all downloaded plugins with each upgrade. Grafana already downloads all plugins speciifed in the env file with each restart, but as far as I can tell it doesn't clean up old plugins - which was where some of my sluggishness in grafana was coming from.

jasonacox commented 8 months ago

you wrote tinytuya! ... So thanks for that, as well!

Thanks @BJReplay!! TinyTuya started off as a way for me to monitor power usage with Tuya smart plugs (you can see how it is related to this one). But I have to give credit to the incredible community of enthusiast and contributors there. It wouldn't be where it is without the brilliant folks there, especially @uzlonewolf who is an absolute wizard. All the recent protocol support came from his genius.

Always a pleasure to contribute, even in a tiny way...

Tiny is good! Thanks again for this contribution. Considering the age of Docker V1, I'm hopeful that we don't see a lot of impact (e.g. issues opened) but will watch for that. My fear on any of these updates is that we have forgotten to consider a part of the community that has limited ability to upgrade (thinking more of appliance style hosts like Synology). I know I should get one to add to my test suite, but life seems to find other plans for that money and time. 😂

Speaking of time... we are preparing to merge a fairly major rewrite of pypowerwall (see https://github.com/jasonacox/pypowerwall/pull/77) thanks to @emptywee. It will help us make it much easier to accommodate any future Telsa API changes and modes (e.g. tedapi and FleetAPI support). I will be dropping a new pypowerwall container to test soon. If you have time to help test and provide feedback, that would be awesome.