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

Version 4.1 #370

Merged
merged 23 commits into from
Dec 16, 2018
Merged

Version 4.1 #370

merged 23 commits into from
Dec 16, 2018

Conversation

diginc
Copy link
Collaborator

@diginc diginc commented Dec 11, 2018

Here are code changes for the new version's release. I'm running some more automated tests locally first.

I need to check the multi-architecture version out to see if it's still suffering from a problem that appeared after it's base image changed (see #361)

dhutchison and others added 16 commits September 29, 2018 22:20
Signed-off-by: David Hutchison <dhutchison86+github@googlemail.com>
Signed-off-by: Adam Hill <adam@diginc.us>
Signed-off-by: Peter Dave Hello <hsu@peterdavehello.org>
Implemented missing support for DNSMASQ_LISTENING environment variable
Implemented missing support for DNSMASQ_LISTENING environment variable

Signed-off-by: Adam Hill <adam@diginc.us>
…evelopment

Signed-off-by: Adam Hill <adam@diginc.us>
Refactor Dockerfile for smaller and less layer image
…evelopment

Signed-off-by: Adam Hill <adam@diginc.us>
Signed-off-by: Adam Hill <adam@diginc.us>
Signed-off-by: Adam Hill <adam@diginc.us>
Signed-off-by: Adam Hill <adam@diginc.us>
Synology sudo audit error fix, disable audit in pam modules
Signed-off-by: Adam Hill <adam@diginc.us>
Copy link
Collaborator Author

@diginc diginc left a comment

Choose a reason for hiding this comment

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

Reviewing / Commentating on my own stuff.

@@ -34,7 +29,7 @@ EXPOSE 443
ENV S6_LOGGING 0
ENV S6_KEEP_ENV 1
ENV S6_BEHAVIOUR_IF_STAGE2_FAILS 2
ENV FTL_CMD no-daemon
ENV FTL_CMD debug
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@pi-hole/api-approvers FYI no-daemon wasn't staying in the foreground for me in my hasty update last night, so I changed it to debug as a workaround to get things running (s6-service requires a foregrounded process)

Choose a reason for hiding this comment

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

Thanks, I found the issue: pi-hole/FTL#430

Copy link
Member

@DL6ER DL6ER Dec 12, 2018

Choose a reason for hiding this comment

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

I just tried to run it as pihole-FTL no-daemon and - as @Mcat12 commented in his PR - observed that Ctrl+C, etc. are not caught anymore, however, pihole-FTL indeed stays in foreground and works as expected. We should continue this here: pi-hole/FTL#432

@@ -1,22 +1,25 @@
#!/bin/bash -ex
#!/bin/bash -ex /
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

wait where did this / come from.......

echo y | bash -x pihole checkout core $CORE_TAG
echo y | bash -x pihole checkout web $CORE_TAG
echo y | bash -x pihole checkout ftl ${CORE_TAG/v/}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

New way to checkout dev copies of FTL added as I was pre-release testing. Noticed no 'v' was in this tag where as the others weren't so I'm search/replacing it in this bash variable.

@@ -10,6 +10,10 @@ fi
# used to start dnsmasq here for gravity to use...now that conflicts port 53

$bashCmd /start.sh
# Gotta go fast, no time for gravity
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe not required, added before I added --dns 1.1.1.1 which actually solved my gravity issues (known issue that never manifested in tests before)

if expected_stdout == 'IPv4':
assert 'IPv6' not in function.stdout
# weird slow write/sync problem; no sleep == old state of file, sleep == updated/setup state of file
time.sleep(1)
Copy link
Collaborator Author

@diginc diginc Dec 11, 2018

Choose a reason for hiding this comment

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

setup_ipv4_ipv6 does what it is supposed to do according to every manual test I perform but automation/docker seems to read the file from the old state unless you wait a small amount of time at which point it shows the result of setup_ipv4_ipv6's execution

@@ -4,10 +4,10 @@ envlist = py27
[testenv]
whitelist_externals = docker
deps = -rrequirements.txt
# auto parallel max b/c race condition with docker fixture (I think?)
# 2 parallel max b/c race condition with docker fixture (I think?)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

My local > 2 core machine started failing more when I set this to auto. I thought I had a parallel race condition fixed previously but there maybe more or perhaps it's related to the time.sleep() "fix"; slow to sync/write disks in docker's overlay2 FS.

@diginc
Copy link
Collaborator Author

diginc commented Dec 11, 2018

Hopefully my commentary helps other understand more of these changes where I didn't leave enough code comments.

I pushed the dev image tag with 4.1 which needs a little more testing before going to the prod tag.

Besides changing FTL no-daemon to debug the test's break/fixes had little to do with the updating of pi-hole it's self and more to do with existing/known flakiness in tests.

Signed-off-by: Adam Hill <adam@diginc.us>
Signed-off-by: Adam Hill <adam@diginc.us>
Signed-off-by: Adam Hill <adam@diginc.us>
…ng all local manifest cache

Signed-off-by: Adam Hill <adam@diginc.us>
@diginc
Copy link
Collaborator Author

diginc commented Dec 14, 2018

The dev tag was suffering from an issue I had encountered previously with 'development' - the fix was rm -r ~/.docker/manifests/<image> to properly clear out that manifest's old versions.

This should be good to go, I tried the new features on my copy and test are passing-ish (albeit flakily, I have a separate plan for that).

@diginc diginc requested a review from a team December 14, 2018 01:12
Signed-off-by: Adam Hill <adam@diginc.us>
dschaper
dschaper previously approved these changes Dec 14, 2018
@diginc
Copy link
Collaborator Author

diginc commented Dec 14, 2018

As I feared the arm images still give an error when ran on real arm hardware. The multiarch base images aren't good anymore :( I'll have to dig into this more and report back. What I tried last time is peeking at the history of our current v4.0 image to see if I could pull out the base layer of the old version of multiarch (they don't seem to have any old tags) but the history command just says "missing" instead of giving a hash to tag.

@nemchik
Copy link

nemchik commented Dec 14, 2018

Any chance you guys might be willing to have a chat with the team at linuxserver.io to discuss the build process for this image? I asked if any of them might have any insight on this PR and they are open to discussing it on their discord server. To any who don't know lsio is a team of pretty awesome people who build a lot of pretty popular docker images.

Signed-off-by: Adam Hill <adam@diginc.us>
@diginc
Copy link
Collaborator Author

diginc commented Dec 16, 2018

Found the root of the issue : 10c44c9

I had introduced a template variable to make updating s6 a single change rather than 3 and somehow it got reverted / hardcoded to amd64 again during the #358 work.

Travis is being very slow to run but tests are passing very consistently locally now so I think I'll force this through and release.

@diginc diginc merged commit 23e6501 into master Dec 16, 2018
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/4-1-docker-logging-to-stdout/15633/5

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.

8 participants