Skip to content

Rework Agent containers build #2607

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

Merged
merged 4 commits into from
Feb 9, 2022

Conversation

webbnh
Copy link
Member

@webbnh webbnh commented Jan 20, 2022

The current build procedure for the Agent containers no longer works. The first problem is that it insists upon cross-checking all of its default build targets, including Fedora 32 (even if the requested build doesn't include F32...), which doesn't work now that we've removed our support for Fedora 32 from COPR and COPR itself has deprecated Fedora 33 support. The second problem is that its build targets are specific to the respective target platforms, and so adding a new platform requires cookie-cutter copying the existing targets to create targets for the new platform.

This PR makes the following changes:

  • Replace the old cookie-cutter approach with generic templates and pattern rules. With this change, the Makefile no longer refers to specific platforms or targets except in the default list of targets to build.
  • Switch from docker.io to quay.io as the source for base images. Since the repositories have different names, and since the tags have different formats, this required some changes to the plumbing.
  • Update the default list of targets to Fedora 34, Fedora 35, and CentOS 8 -- dropping Fedora 32, Fedora 33, and CentOS 7.

Note that there is nothing intrinsic to these changes which would prevent Fedora 32 or Fedora 36 from working, but we don't currently provide the required stuff on COPR for these targets. Likewise, there is nothing in these changes which prevents CentOS 7 or CentOS 9 from working -- the CentOS 7 build had stopped working prior to these changes (and its build now fails in exactly the same way with these changes), and packagecloud.io, from which we source the Prometheus RPMs, does not yet support CentOS 9.

[I've tagged @portante as a reviewer for, oh, so many reasons; @ndokos, I would appreciate it if you could be my second reviewer, and anyone else is welcome to step up.]

@webbnh webbnh added Agent Code Infrastructure packaging Issues related to software packaging Installation Containerization Of and relating to the process of setting up and maintaining container images Tool Meister Of and relating to the Tool Meister sub-system labels Jan 20, 2022
@webbnh webbnh added this to the v0.71 milestone Jan 20, 2022
@webbnh webbnh self-assigned this Jan 20, 2022
@webbnh webbnh force-pushed the agent-make branch 5 times, most recently from cee822b to 0d43bf6 Compare January 21, 2022 01:27
Copy link
Member

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

I'm impressed. It's still "Draft" so I won't bother to approve, but it seems to be looking rather comprehensive assuming testing proves it all out in practice.

@webbnh webbnh force-pushed the agent-make branch 7 times, most recently from e448d46 to 0261cba Compare January 25, 2022 21:48
@webbnh webbnh requested a review from portante January 25, 2022 21:52
@webbnh webbnh marked this pull request as ready for review January 25, 2022 21:53
@webbnh webbnh force-pushed the agent-make branch 2 times, most recently from 6253f4f to 2ace48d Compare January 26, 2022 01:00
Replace cookie-cutter approach with templates and pattern rules
Switch from docker.io to quay.io for base images
Update default list to:  Fedora 34, Fedora 35, and CentOS 8
@webbnh webbnh requested review from dbutenhof and removed request for portante February 7, 2022 16:15
@webbnh webbnh requested review from portante and ndokos February 7, 2022 16:16
dbutenhof
dbutenhof previously approved these changes Feb 8, 2022
Copy link
Member

@dbutenhof dbutenhof left a comment

Choose a reason for hiding this comment

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

The make magic is kinda blinding, but it looks good.

Copy link
Member

@ndokos ndokos left a comment

Choose a reason for hiding this comment

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

A tour-de-force of makefileology! Opaque but impressive.

A couple of questions:

  • Is it possible to produce the Makefile after all the automatic expansions have happened? That might be a good thing to have if one needs to debug it.
  • I wonder if some of the opaqueness could be alleviated by using a more expressive language, e.g. by having a program that would write a makefile for some distro (e.g centos-8) that the main Makefile could then run.

In general, I like programs that write programs, but I'm not sure how I feel about Makefiles that create Makefiles (particularly if they don't write them out) :-)

Nevertheless, once the CentOS problems are resolved, I'm ready to approve.

@ndokos ndokos self-requested a review February 9, 2022 01:00
ndokos
ndokos previously approved these changes Feb 9, 2022
Copy link
Member

@ndokos ndokos left a comment

Choose a reason for hiding this comment

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

LGTM

dbutenhof
dbutenhof previously approved these changes Feb 9, 2022
@webbnh
Copy link
Member Author

webbnh commented Feb 9, 2022

Is it possible to produce the Makefile after all the automatic expansions have happened? That might be a good thing to have if one needs to debug it.

You can have Make dump its database (which is pretty voluminous) by using the -p flag, which yields all the rules, dependencies, environment variable values, and the normal variable assignments after building whatever target you specify or default to.

Also, using make --trace is nice, as it tells you why Make is performing each step (i.e., it lists the targets as it's building them; --no-silent also prints the recipe commands as they are executed, but it omits the target.

If you're really interested, you can use make -d (or make --debug=<flag-values>) to have Make share its thought processes as it proceeds, but the output is quite voluminous (even compared to the -p output), and usually --trace will suffice.

I wonder if some of the opaqueness could be alleviated by using a more expressive language, e.g. by having a program that would write a makefile for some distro (e.g centos-8) that the main Makefile could then run.

This is certainly a valid approach, but it's not one that I'm partial to. It might be easier to debug, and it might be easier for someone with limited facility with Make to deal with, but, when things are working, it's a more cumbersome solution which often requires more dependencies and ends up being more fragile. Make is intended to be able to handle things like this without resorting to external super- or sub-structures (although, Make is quite capable of make'ing Makefiles (when they are missing or out of date) and then invoking them all from a single make command line...), so, if we can stomach it, I would prefer to go with a single tool with a single input (ignoring all the helper scripts that we've chosen to employ...).

@webbnh webbnh dismissed stale reviews from dbutenhof and ndokos via 147f38f February 9, 2022 17:28
@ndokos ndokos self-requested a review February 9, 2022 20:25
@webbnh webbnh requested a review from dbutenhof February 9, 2022 21:18
@webbnh webbnh merged commit ec1c586 into distributed-system-analysis:main Feb 9, 2022
@webbnh webbnh deleted the agent-make branch February 9, 2022 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Agent Code Infrastructure Containerization Of and relating to the process of setting up and maintaining container images Installation packaging Issues related to software packaging Tool Meister Of and relating to the Tool Meister sub-system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants