-
Notifications
You must be signed in to change notification settings - Fork 171
restructure image builds #1913
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
restructure image builds #1913
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @deitch - we have scripts that expect a rootfs-.squash to be there. Please restore that (but now as a symlink). I think the scripts will be happy with that (not sure if that ever gets extracted -- but we can deal with it later).
That's fine, we can do that. But which scripts? I hunted down dependencies before proposing this, and didn't find them. Obviously I missed them, but I would like to know which ones. I will add comments into the PR as well. |
Test scripts -- some of them in Eden -- some of them you don't get to see |
Also, no comments on the proposed changed installer structure? |
What exactly do they depend upon? Just having a file named |
I think we need to discuss it in a more 1-1 fashion |
They depend on the version being embedded in the filename -- hence the exact rootfs-.squash link needs to be restored. |
We can do that, but again, that's not a great pattern. Don't we have the version on the image tag, or at least the image metadata? |
Actually, we might need to discuss it now. See this:
It isn't quite that simple. The old one just copied everything over as: COPY installer /bits This lets the dockerfile not care what the actual filename is, which can change (it includes the branch name and the hash and hypervisor and arch, etc.). That also, unfortunately, is what caused it to install every build ever made that still lingers in the I cannot think of a sane way to pass the name of the targeted build file to the docker build. Theoretically, we could use docker build args, but linuxkit doesn't support them for the same reason that docker originally didn't: reproducible builds. If you can change the arg, you can change the build, but nothing on the filesystem is changed, so it isn't reproducible. We can debate the merits of that policy - I certainly have, and see both sides of it - but it isn't getting changed between now and tomorrow. Given the above, I think my proposal to have a per-build installer directory actually may be the easiest solution here. I would just modify it slightly to fit your requirements:
|
The above makes it really easy for the dockerfile, as it goes back to installing just the needed bits, but it installs them by blindly taking from a build-specific installer directory. Makefile, which already knows all of the correct filenames and such, can set it up fairly easily. What do you both say? Any reason not to make the change that way? |
Yup -- I like the latest proposal @deitch -- can you please push relevant commits to this PR? |
c1bda30
to
500cfa1
Compare
The installer directory has been completely restructured. I still am working on making sure that @rvs I think you know this the best, so can you review? Specifically, I want to make sure we didn't miss any interesting use cases, e.g. certain HV like kvm or adam combinations. |
500cfa1
to
0ffbf19
Compare
Particularly, this does simplify quite a bit in the |
0ffbf19
to
fd9533a
Compare
OK, I think I have those now too. Good for someone to take a look. |
To make the changes clear, here is a similar tree structure. Ran
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hey @deitch -- this looks much better now. Still, the following target seems to break:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @deitch -- this looks much better now, but while testing it I uncovered two problems:
- If you repository is considered dirty by git then the version will change every time you run build or run targets which means that a sequence of the following commands will fail
make live && make run
- It seems that a versioned rootfs file is gone from installer folder and I'd like to have it around for now
I think for both of these -- it is just a simple matter of an extra symlink (as we've agree I think). One to be put under dist/<ARCH>
(to always point at the latest version so that run commands can use that) and the other one under dist/<ARCH>/<VERSION/installer/rootfs-<VERSION>.squash
I do realize that the later is a bit redundant -- but lets keep it for compatibility reasons for now -- we can drop it early in 6.x cycle.
Once these two changes are in (plus a tiny nit I spotted above) we can merge this
fd9533a
to
94ca2f4
Compare
I saw that, but it should be an issue with the existing mainline as well (pre-this-PR), because it uses the |
Oh I see, |
94ca2f4
to
48792d3
Compare
Signed-off-by: Avi Deitcher <[email protected]>
48792d3
to
dfc20df
Compare
OK, this looks good for that first issue now @rvs . Every time you do a build - |
I don't know what you mean by this. I just ran
That symlink is there. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for taking care of this so quickly @deitch
Definitely! Happy to see this moving ahead. |
Does two things:
LABEL
s to the dockerfilelfedge/eve
indicating where the rootfs and initrd are, so they can be found laterinstaller/
in toto, copy each file/dir as desiredThe first part is important for allowing anything to look at the
lfedge/eve
container and, just by inspecting its config, discover where the artifacts are. It lays more groundwork for baseos in OCI images.The second part helps keep containers clean, and is a longstanding issue.
make
for various kinds of images dumps everything - with a filename that reflects the build hash - indist/<arch>/installer/
.The image we build includes the following components. Here is an example from looking at the tree on my test amd64 env:
This is a first step to cleaning it up, by explicitly adding to the image only those things we need:
In the future, we might want to think about having a per-build installer, so that it would be even cleaner. It would look something like this:
Is this duplicative? Sure it is. But it duplicates everything in the git-ignored
dist/
tree, and shows very clearly everything you build that gets installed. It also has the benefit of sending docker context only the necessary files, not every single image that is there, making the builds slower as time goes on.If people think this is better day one, I am happy to rework this PR to arrange the above.