Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

V2.10.0 - Docker Compose and Container Upgrades #374

Merged
merged 9 commits into from
Oct 15, 2023
Merged

V2.10.0 - Docker Compose and Container Upgrades #374

merged 9 commits into from
Oct 15, 2023

Conversation

jasonacox
Copy link
Owner

@jasonacox jasonacox commented Oct 15, 2023

v2.10.0 - Updates

Docker Compose Config Improvements

Upgrade Containers

  • This PR will update the pinning to newer versions of Telegraf and pyPowerwall in powerwall.yml: Telegraf (v1.28.2), pyPowerwall (v0.6.2t28)
  • setup.sh - Adds PW_STYLE to pypowerwall.env settings to support newer Grafana versions.
  • upgrade.sh - Some cleanup and handling for new PW_STYLE for existing installations.

Related:

@mcbirse
Copy link
Collaborator

mcbirse commented Oct 15, 2023

Hi Jason,

I have added commits with the proposed changes for powerwall.yml (similar to #366) which include:

  • Update powerwall.yml to use variables for "user" and "ports" in containers
  • Update compose.env.sample with explanation of latest supported options
  • Update upgrade.sh script to replace GRAFANAUSER with PWD_USER in existing compose.env files, since this is now used for all containers including Grafana
  • Change to powerwall.yml to use "unless-stopped" as the default restart policy for containers going forward (testing/research shows this may be a better choice, happy to discuss/explain my reasoning)

Also:

  • Update other Docker Hub upload scripts to include your verification of specific version
  • Add optional 3rd argument to compose-dash.sh script, which is useful sometimes to pass additional arguments to docker compose (e.g. being able run "compose-dash.sh up -d --remove-orphans" due to docker compose container changes could be handy)

Please review, and happy for you to modify as you see fit, or let me know if you see any issues with the changes.

@jasonacox jasonacox changed the title V2.10.0 - Update Versions of Grafana and Telegraf #373 V2.10.0 - Docker Compose and Container Upgrades Oct 15, 2023
sed -i "s@GRAFANAUSER@PWD_USER@g" "${COMPOSE_ENV_FILE}"
if grep -q "^PWD_USER=\"1000:1000\"" "${COMPOSE_ENV_FILE}"; then
sed -i "s@^PWD_USER=\"1000:1000\"@#PWD_USER=\"1000:1000\"@g" "${COMPOSE_ENV_FILE}"
fi
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Here is what this does:

  • It replaces all occurrences of "GRAFANAUSER" with "PWD_USER" in the compose.env file.
  • It checks if a line starting with "PWD_USER=" followed by "1000:1000" exists in the file. If it does, it comments out that line by adding a "#" at the beginning. This is because the powerwall.env will default to that.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep - all defaults are set in the powerwall.yml docker compose file now when a variable is undefined.

compose-dash.sh Outdated
if docker-compose version > /dev/null 2>&1; then
# Build Docker (v1)
docker-compose -f powerwall.yml $pwextend $1 $2
docker-compose -f powerwall.yml $pwextend $1 $2 $3
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mcbirse - This makes sense. In fact, had me wonder if we should just include all arguments. Should we just use $@ which would be all arguments?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Jason, I agree, it would probably make sense to simply use $@ so there is no limit on the number of additional arguments that could be passed. Please feel free to update it.

@jasonacox
Copy link
Owner Author

jasonacox commented Oct 15, 2023

GREAT additions @mcbirse! Thank you! 🙏 I'm going to start testing this before we merge...

First pass of verify.sh after upgrade.sh:

image

@jasonacox
Copy link
Owner Author

I'm seeing some issues with the Power Flow animation where a light gray border appears around the animation. I need to investigate...

image

I'm seeing JavaScript errors as the resize for the animation happens.

image

I'm going to try different Grafana versions to see if this is a bug or a conflict with our code. If it is a conflict, I'll likely revert the Grafana upgrade with the PW_STYLE=clear option in pypowerwall.env for anyone who wants to experiment with newer Grafana versions to get it to work.

@jasonacox
Copy link
Owner Author

I was finally able to get a clean run (no JavaScript errors) with Grafana v9.2.20. The border artifacts go away and the animation looks good with PW_STYLE=clear and PW_STYLE=grafana-dark. However, pushing it to recompute based on date ranges and other standard tasks I would do, did produce some JavaScript errors but not related to the animation.

From my testing (one version at a time) I discovered that something changed with v9.3+ that causes problems with our iFrame animation code and auto-resizing, specifically the border artifacts and JS errors. At v9.5 a different panel title formatting is introduced that seems to break the look as well. I think it will require a lot more investigation.

I'm reverting the upgrade of Grafana and keeping it at v9.1.2 as it seems the most stable in my testing. It also works well with both clear and grafana-dark modes. I'm going to continue to investigate how we get to a newer version. The other updates make sense to push forward and having grafana-dark as the default will help any others exploring newer Grafana version, including the ones raised in the Issues and Discussions.

@jasonacox jasonacox merged commit 6e26eee into main Oct 15, 2023
@jasonacox jasonacox deleted the v2.10.0 branch October 15, 2023 23:30
@jasonacox
Copy link
Owner Author

v2.10.0 has been released....

  • Tested Upgrade - no issues
  • Tested Clean Install - no issues
image

@jgleigh
Copy link

jgleigh commented Oct 16, 2023

@jasonacox Getting this error while trying to upgrade:
sed: 1: "compose.env": command c expects \ followed by text

@mcbirse
Copy link
Collaborator

mcbirse commented Oct 16, 2023

Hey @jgleigh - I have pushed a quick fix to the upgrade script which should hopefully fix this problem. 🤞

I looked into this error, and it appears to be a compatibility issue between GNU sed (Linux based) and BSD sed (used in Mac OS). Are you using Mac?

It seems sed -i (update in-place with no backup file) does not work on Mac. Instead it expects a backup file, whereas on Linux this is accepted and will skip creating the backup file.

sed -i'' would be acceptable on both platforms to skip creating the backup file. However I decided to change the command to sed -i.bak to create the backup file anyway, as this is consistent with use of sed throughout the other scripts so must work across all platforms (hopefully).

Let us know if the upgrade works for you now.

@jgleigh
Copy link

jgleigh commented Oct 16, 2023

@mcbirse That fixed it! Yes this was on a Mac. Thanks again.

@jasonacox
Copy link
Owner Author

Thanks @jgleigh for spotting the issue and thank @mcbirse for the fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants