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

pre-install-payload: Make its creation more generic #253

Conversation

fidencio
Copy link
Member

This is a ground work that will make things way simpler for #251

@fidencio fidencio force-pushed the topic/make-pre-install-image-expandable-rebased branch 3 times, most recently from a617bf4 to 87b2252 Compare August 25, 2023 16:14
@fidencio
Copy link
Member Author

/test

@fidencio
Copy link
Member Author

This PR is rebased atop of #179

@fidencio fidencio requested review from wainersm and bpradipt August 25, 2023 16:26
@fidencio fidencio force-pushed the topic/make-pre-install-image-expandable-rebased branch from 87b2252 to ccf41f0 Compare August 25, 2023 16:34
@fidencio
Copy link
Member Author

/test

FROM ubuntu:22.04
#### Confidential Containers forked containerd

FROM alpine:3.18 as coco-containerd-binary-downloader
Copy link
Member

Choose a reason for hiding this comment

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

Just a nit: instead of repeat FROM alpine:3.18 for the three images, you can create a FROM alpine:3.18 as base and the subsequent images takes FROM base. On that base image you can even update the image like apt get update (don't know if Alpine has the same update mechanism)

Copy link
Member Author

Choose a reason for hiding this comment

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

Perfect, will do!

@wainersm
Copy link
Member

@fidencio I reviewed the last 5 commits, no more nits :)

Actually, maybe one more... as you are changing names... pre-install-post-uninstall is somewhat confusing. I understand the idea, it contain the code to pre-install and post-uninstall but what if we call it node-requirements or coco-requirements or coco-deps or setup-pre-coco... but I don't want to hold your progress on this work if it will take much efforts to change.

@fidencio
Copy link
Member Author

it contain the code to pre-install and post-uninstall but what if we call it node-requirements or coco-requirements or coco-deps or setup-pre-coco

I just renamed it to reqs-payload.

@fidencio fidencio force-pushed the topic/make-pre-install-image-expandable-rebased branch from ccf41f0 to 33626be Compare August 26, 2023 07:57
@fidencio
Copy link
Member Author

/test

@fidencio fidencio force-pushed the topic/make-pre-install-image-expandable-rebased branch from 33626be to cd15fae Compare August 26, 2023 08:19
@fidencio
Copy link
Member Author

/test

The script had in its name "container-engine-for-cc-", which made sense
when we were only using it to deploy the container engine needed for cc.

However, we'll be expanding the functionality of the script to also make
sure the needed snapshotters are deployed at the users will.

With this in mind, let's simply rename the script to something more
generic like "reqs-deploy.sh".

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
Having those under the "containerd" directory served us the purpose
while we were only using those scripts for building containerd.

Now that we we'll have to expand those, we better move them one dir up
and make them more "all purpose" than just "containerd specific".

As we're still only building containerd, this is still the only thing
for what it's been used.

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
Let's make it generic by just renaming the function to build_payload().

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
Let's start using a multi-step build Dockerfile in order to make things
a little bit more contained for each parts we'll build.

Together with this change, we're also moving to using alpine as the base
image, as it's way smaller than the Ubuntu one, and is also multi-arch.

I know, I know, it seems that having a multi-stage build here makes no
sense, as we'll end up using an alpine image in the end, but it'll pay
off by the moment we'll start building the artefacts, which for sure
will happen with the nydus snapshotter.

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
As we're changing the names, let's make sure that all the cnnfigs are
consuming the new payload image from the correct registry.

Signed-off-by: Fabiano Fidêncio <fabiano.fidencio@intel.com>
@fidencio fidencio force-pushed the topic/make-pre-install-image-expandable-rebased branch from cd15fae to e13884b Compare August 29, 2023 07:22
@fidencio
Copy link
Member Author

/test

Copy link
Contributor

@mythi mythi left a comment

Choose a reason for hiding this comment

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

@wainersm already provided good set of comments. Naming is hard but reqs-* works OK for me.

+1

@wainersm
Copy link
Member

ah, @fidencio , I just remembered... there is a automation to build the (old) pre-install image on PR merger (https://github.com/confidential-containers/operator/blob/main/.github/workflows/pre-install-image-publish-on-merge.yml) that will need changes. I can send an follow up PR with those things as I was the one who introduced the workflow on first place.

Copy link
Member

@wainersm wainersm left a comment

Choose a reason for hiding this comment

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

Thanks @fidencio ! But it is a pity we can't kill the pre-install image :(

@fidencio
Copy link
Member Author

ah, @fidencio , I just remembered... there is a automation to build the (old) pre-install image on PR merger (https://github.com/confidential-containers/operator/blob/main/.github/workflows/pre-install-image-publish-on-merge.yml) that will need changes. I can send an follow up PR with those things as I was the one who introduced the workflow on first place.

This is covered here already: https://github.com/confidential-containers/operator/pull/253/files#diff-0688aecd94d71cf248e44a407562596e3f8337eebf8d324e419403927a100089R44

@fidencio fidencio merged commit e45d4e8 into confidential-containers:main Aug 29, 2023
@wainersm
Copy link
Member

ah, @fidencio , I just remembered... there is a automation to build the (old) pre-install image on PR merger (https://github.com/confidential-containers/operator/blob/main/.github/workflows/pre-install-image-publish-on-merge.yml) that will need changes. I can send an follow up PR with those things as I was the one who introduced the workflow on first place.

This is covered here already: https://github.com/confidential-containers/operator/pull/253/files#diff-0688aecd94d71cf248e44a407562596e3f8337eebf8d324e419403927a100089R44

oh, true, I missed that.

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