FEAT: Disable APT-Cacher by default #249

micaelmalta · 2021-07-11T01:10:58Z

CONTEXT

apt-cacher is not working with all OS (ex: MacOS)

PROPOSAL

Option to enable apt-cacher in a docker container
Use docker network host to access it

OTHER FIX

Ubuntu ESM Infra is not available, delete it from sources_list

devrandom

Looks good overall - but I'm seeking feedback from others whether this should be feature gated in case people have a reason to use the system apt-cacher-ng

bin/make-base-vm

achow101 · 2021-07-11T17:43:04Z

I think it would be better to have the apt-cacher container run when it is explicitly asked for. I don't think it should be on enabled by default.

Additionally, the changes made here are in make-base-vm, but this script is typically run once. The apt-cacher is still used by gbuild when it installs the packages specified for the current build. So you would need to have it start it there too.

In general, I don't really like this approach. It is already possible to specify the host running apt-cacher using MIRROR_HOST. If it is not possible to run apt-cacher in a docker container and set MIRROR_HOST to point to that, then I would prefer changes be made to allow that to work rather than having gitian run another container.

micaelmalta · 2021-07-11T18:36:14Z

I think it would be better to have the apt-cacher container run when it is explicitly asked for. I don't think it should be on enabled by default.

Additionally, the changes made here are in make-base-vm, but this script is typically run once. The apt-cacher is still used by gbuild when it installs the packages specified for the current build. So you would need to have it start it there too.

In general, I don't really like this approach. It is already possible to specify the host running apt-cacher using MIRROR_HOST. If it is not possible to run apt-cacher in a docker container and set MIRROR_HOST to point to that, then I would prefer changes be made to allow that to work rather than having gitian run another container.

That's fair. I removed apt-cacher and just let the fix for ESM and also to plug with the network host

As we are now using docker network host, we dont need to change the MIRROR_DEFAULT in case of Docker

devrandom · 2021-07-11T19:52:10Z

That's fair. I removed apt-cacher and just let the fix for ESM and also to plug with the network host

@achow101 makes a good point. However, it would still be nice to have a Dockerfile / startup script for a apt-cacher-ng container, perhaps in a contrib/ directory. What do you think?

devrandom · 2021-07-11T19:53:22Z

Or if you don't feel like that should go into this PR, could you change the PR title / description to reflect the new scope?

AbcSxyZ

Usage not updated, open --disable-apt-cacher should be added.

micaelmalta · 2021-07-12T16:59:46Z

Usage not updated, open --disable-apt-cacher should be added.

Based on this comment, #249 (comment), I change to disable apt-cacher by default

micaelmalta · 2021-07-14T04:26:32Z

Hello!

So the whole purpose of this fix was to be able to easily launch a GITIAN build either locally or on a CI.
The original idea was to have as many DOGECOIN developers to use it easily

Here is my work: https://github.com/micaelmalta/gitian-builder-dogecoin

You can have here a DEMO of a working CI: https://cirrus-ci.com/build/4704793919750144

devrandom · 2021-07-19T10:28:39Z

Sorry for the delay reviewing.

I would prefer not to change the default, since I believe most existing users of Gitian do use apt-cacher. Let's have it continue to be on by default and the flag would be to disable.

AbcSxyZ · 2021-07-28T11:40:42Z

[I don't have important usage/knowledge of the soft, used with dogecoin only.]

At first, without thinking about existing users, what would be the best between enable or disable apt-cacher by default ? Did you get user feedbacks ?
Look like if you use it for a single crypto once, it may be overkill, but maybe when a user release regularly versions or different cryptos it has an interest.

In the scenario apt-cacher isn't really required by default, you would prefer to conserve backward compatibility over simpler interface ?
Because you're in Alpha version, you may be able to do those change between the 0.2 and 0.3 release. In the worst case, the problem would be to download source instead of using cache.

In case if apt-cacher is an advantage by default, life is easier :)

devrandom · 2021-07-31T19:45:36Z

The main use-case for Gitian has been developers that do regular releases.

Also, in the case of CI, I believe that caching should be on (with persistent storage), so as to not abuse the OS repositories.

cculianu · 2021-08-26T12:11:11Z

I'm a fan of this feature.

Whatever the default is (on or off), I'm ok with, so long as it can be turned on/off at will. The ability to turn it off is important for macOS. My modest PR here: #252, which is related, just turns it off unconditionally for the macOS case....

AbcSxyZ · 2021-09-16T14:03:41Z

Regarding #227 (comment), I suspect a communication issue where everyone was waiting on each other :)

I suggested some changes to his code to have apt cacher used by default, see micaelmalta#1.

devrandom · 2021-09-16T14:28:04Z

bin/make-base-vm

@@ -208,7 +219,7 @@ WORKDIR /home/$DISTRO
 CMD ["sleep", "infinity"]
 EOF

-  docker build --pull -f $OUT.Dockerfile -t $OUT .
+  docker build --network host --pull -f $OUT.Dockerfile -t $OUT .


unfortunately, this only works on Linux. if this is desired, it has to be behind a flag or a conditional based on the host OS.

(this also relates to the change above, which points to 127.0.0.1 unconditionally)

I will try to investigate the purpose of this, why he had to specify the host network and the relation with apt-cacher. Try to understand why he had to play with MIRROR_DEFAULT.

For now, if I'm right, it's related to the configuration of apt-cacher when you're using docker.

He's just using adding conditionally the following line in make-base-vm :

gitian-builder/bin/make-base-vm

Line 201 in 32b1145

RUN echo 'Acquire::http { Proxy "$MIRROR_BASE"; };' > /etc/apt/apt.conf.d/50cacher

So, I guess if mirrors configurations were working with apt-cacher before, not touching to those configurations when disabling apt-cacher shouldn't be a problem ?

Would keeping MIRROR_DEFAULT and docker network like it was be a solution ?

devrandom · 2021-09-16T14:28:34Z

libexec/start-target

@@ -37,6 +37,6 @@ case $VMSW in
    echo "Gitian-${2}" > var/target.vmname
    ;;
  DOCKER)
-    docker run -d --name gitian-target base-$SUFFIX:latest > /dev/null
+    docker run --network host -d --name gitian-target base-$SUFFIX:latest > /dev/null


see comment above regarding this being Linux only

AbcSxyZ · 2021-09-16T18:03:54Z

I guess lxc & kvm are using cacher ? In the current state, isn't it used for them even with --disable-apt-cacher ?

AbcSxyZ · 2021-09-16T20:32:48Z

Yes, lxc & kvm were using apt-cacher. Added this commit https://github.com/AbcSxyZ/gitian-builder/commit/f9a993ec2b28dc1a55382542e41c60e5c793b0b9 to disable apt cacher for them to my PR on @micaelmalta's repo.

I'm wondering if leaving MIRROR* variables like it was may do the job to enable this option, it may look good after this change.

AbcSxyZ · 2021-09-19T21:14:53Z

bin/make-base-vm

@@ -193,12 +197,19 @@ if [ $DOCKER = "1" ]; then
    base_image="$DISTRO:$SUITE"
  fi

+  apt_cacher=""
+  if [ "$APT_CACHER" = 1 ]; then
+    apt_cacher="RUN echo 'Acquire::http { Proxy "$MIRROR_BASE"; };' > /etc/apt/apt.conf.d/50cacher"


This line introduces a syntax error when using apt-cacher.

Within the 50cacher file of the Dockerfile, will render :

Acquire::http { Proxy http://172.17.0.1:3142; };

instead of

Acquire::http { Proxy "http://172.17.0.1:3142"; };

Need to escape quotes : { Proxy \"$MIRROR_BASE\"; }.

Fixed in 525e33a

micaelmalta requested a review from devrandom as a code owner July 11, 2021 01:10

micaelmalta force-pushed the docker branch 2 times, most recently from f9eaa91 to 4c31f58 Compare July 11, 2021 02:51

devrandom reviewed Jul 11, 2021

View reviewed changes

bin/make-base-vm Outdated Show resolved Hide resolved

bin/make-base-vm Outdated Show resolved Hide resolved

bin/make-base-vm Outdated Show resolved Hide resolved

bin/make-base-vm Outdated Show resolved Hide resolved

micaelmalta force-pushed the docker branch from a087175 to 801f213 Compare July 11, 2021 18:35

AbcSxyZ suggested changes Jul 12, 2021

View reviewed changes

micaelmalta force-pushed the docker branch 3 times, most recently from 21d345b to 54ccd5a Compare July 12, 2021 16:41

micaelmalta changed the title ~~FEAT: use docker cross OS~~ FEAT: Disable APT-Cacher by default Jul 12, 2021

Docker apt-cacher

f9aa177

micaelmalta force-pushed the docker branch from 54ccd5a to f9aa177 Compare July 14, 2021 03:42

micaelmalta mentioned this pull request Jul 15, 2021

Gitian builder dogecoin/dogecoin#2396

Closed

This was referenced Sep 6, 2021

Problems with proxy on docker on Fedora 30 #227

Open

Allow for using docker with macOS #252

Open

AbcSxyZ mentioned this pull request Sep 16, 2021

make-base-vm: switch apt cacher option, using cacher enabled by default micaelmalta/gitian-builder#1

Closed

devrandom reviewed Sep 16, 2021

View reviewed changes

AbcSxyZ mentioned this pull request Sep 16, 2021

Feature: add option --disable-apt-cacher (following #249) #253

Closed

AbcSxyZ reviewed Sep 19, 2021

View reviewed changes

AbcSxyZ mentioned this pull request Oct 23, 2021

Docs4Doge: update of gitian builder script + documentation (follow #2396) dogecoin/dogecoin#2579

Closed

micaelmalta closed this Dec 2, 2022

FEAT: Disable APT-Cacher by default #249

FEAT: Disable APT-Cacher by default #249

Uh oh!

Conversation

micaelmalta commented Jul 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CONTEXT

PROPOSAL

OTHER FIX

Uh oh!

devrandom left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

achow101 commented Jul 11, 2021

Uh oh!

micaelmalta commented Jul 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devrandom commented Jul 11, 2021

Uh oh!

devrandom commented Jul 11, 2021

Uh oh!

AbcSxyZ left a comment

Choose a reason for hiding this comment

Uh oh!

micaelmalta commented Jul 12, 2021

Uh oh!

micaelmalta commented Jul 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devrandom commented Jul 19, 2021

Uh oh!

AbcSxyZ commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devrandom commented Jul 31, 2021

Uh oh!

cculianu commented Aug 26, 2021

Uh oh!

AbcSxyZ commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devrandom Sep 16, 2021

Choose a reason for hiding this comment

Uh oh!

AbcSxyZ Sep 16, 2021

Choose a reason for hiding this comment

Uh oh!

AbcSxyZ Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devrandom Sep 16, 2021

Choose a reason for hiding this comment

Uh oh!

AbcSxyZ commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AbcSxyZ commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AbcSxyZ Sep 19, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

micaelmalta commented Jul 11, 2021 •

edited

Loading

micaelmalta commented Jul 11, 2021 •

edited

Loading

micaelmalta commented Jul 14, 2021 •

edited

Loading

AbcSxyZ commented Jul 28, 2021 •

edited

Loading

AbcSxyZ commented Sep 16, 2021 •

edited

Loading

AbcSxyZ Sep 16, 2021 •

edited

Loading

AbcSxyZ commented Sep 16, 2021 •

edited

Loading

AbcSxyZ commented Sep 16, 2021 •

edited

Loading