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

Use complete Ubuntu release + update base images #34

Merged
merged 5 commits into from
Dec 17, 2020

Conversation

cevich
Copy link
Member

@cevich cevich commented Oct 23, 2020

As an alternative to #31, support multiple Ubuntu v20.X versions. This change will require ALL automation, in ALL repositories using these Ubuntu images to mirror the definition change:

diff --git a/lib.sh b/lib.sh
index 58f28fe..cf9ada3 100644
--- a/lib.sh
+++ b/lib.sh
@@ -11,7 +11,7 @@ SCRIPT_DIRPATH=$(dirname "$SCRIPT_FILEPATH")
 # By default, assume we're not running inside a container
 CONTAINER="${CONTAINER:-0}"

-OS_RELEASE_VER="$(source /etc/os-release; echo $VERSION_ID | cut -d '.' -f 1)"
+OS_RELEASE_VER="$(source /etc/os-release; echo $VERSION_ID | tr -d '.')"
 OS_RELEASE_ID="$(source /etc/os-release; echo $ID)"
 OS_REL_VER="$OS_RELEASE_ID-$OS_RELEASE_VER"

Also, ANY/ALL relevant references to $OS_RELEASE_VER values must be updated, for example 19 -> 1910, 20 -> 2004 or 2010.

Copy link
Member

@lsm5 lsm5 left a comment

Choose a reason for hiding this comment

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

LGTM

@cevich cevich force-pushed the new_ubuntu_versioning branch 2 times, most recently from bef0f02 to a9007c0 Compare October 26, 2020 19:14
@cevich cevich changed the title Use complete Ubuntu release + update base images WIP: Use complete Ubuntu release + update base images Oct 26, 2020
@cevich cevich force-pushed the new_ubuntu_versioning branch from a9007c0 to b4e768b Compare October 27, 2020 19:20
@cevich
Copy link
Member Author

cevich commented Nov 3, 2020

Note to me: OBS currently missing Ubuntu 20.10 packages

@lsm5
Copy link
Member

lsm5 commented Nov 3, 2020

Note to me: OBS currently missing Ubuntu 20.10 packages

intentional for now, unless we get more contributors/volunteers.

@cevich cevich force-pushed the new_ubuntu_versioning branch 2 times, most recently from cf5e84e to 567aa39 Compare November 3, 2020 16:37
@cevich
Copy link
Member Author

cevich commented Nov 3, 2020

Note to me: Need criu packages for Ubuntu 20.10

@lsm5
Copy link
Member

lsm5 commented Nov 3, 2020

Note to me: Need criu packages for Ubuntu 20.10

I never packaged criu for OBS. I think it would be in ubuntu repos by default, no?

@cevich
Copy link
Member Author

cevich commented Nov 3, 2020

It might be for 20.10, previously it came from a Launchpad repo.

@cevich cevich force-pushed the new_ubuntu_versioning branch from 567aa39 to 8948976 Compare November 3, 2020 19:49
@cevich
Copy link
Member Author

cevich commented Nov 3, 2020

@lsm5 confirmed, criu is now available in 20.10, no need to use launchpad.

@cevich cevich force-pushed the new_ubuntu_versioning branch from 8948976 to 6cf1395 Compare November 3, 2020 20:24
@cevich cevich force-pushed the new_ubuntu_versioning branch 2 times, most recently from 4795caf to cc398d3 Compare November 4, 2020 20:00
@cevich cevich force-pushed the new_ubuntu_versioning branch 4 times, most recently from 83d0e33 to ce03363 Compare November 12, 2020 14:28
@cevich cevich changed the title WIP: Use complete Ubuntu release + update base images Use complete Ubuntu release + update base images Nov 12, 2020
@cevich cevich force-pushed the new_ubuntu_versioning branch from ce03363 to e437459 Compare November 12, 2020 15:11
@cevich cevich force-pushed the new_ubuntu_versioning branch 5 times, most recently from a1ee3a9 to d6f6744 Compare December 9, 2020 15:54
@cevich
Copy link
Member Author

cevich commented Dec 11, 2020

c6233039174893568

@cevich cevich marked this pull request as ready for review December 14, 2020 20:30
@cevich
Copy link
Member Author

cevich commented Dec 14, 2020

Note: These images are passing tests in podman now. Giving this a positive github review will cause this PR to automatically merge.

Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

Some suggestions for making this a little easier to understand and maintain

cache_images/fedora_setup.sh Outdated Show resolved Hide resolved
cache_images/fedora_setup.sh Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
cache_images/ubuntu_packaging.sh Show resolved Hide resolved
cache_images/ubuntu_packaging.sh Outdated Show resolved Hide resolved
cache_images/ubuntu_setup.sh Outdated Show resolved Hide resolved
lib.sh Outdated Show resolved Hide resolved
@cevich cevich force-pushed the new_ubuntu_versioning branch from d6f6744 to 2d594a6 Compare December 16, 2020 22:00
@cevich cevich force-pushed the new_ubuntu_versioning branch from 2d594a6 to 384be09 Compare December 16, 2020 22:05
@cevich cevich marked this pull request as draft December 17, 2020 13:24
@cevich
Copy link
Member Author

cevich commented Dec 17, 2020

Note: Marking as a draft to block automatic merging. I'd like to confirm (at least in a test PR) that the new changes don't break podman testing.

@@ -220,6 +219,7 @@ $(_TEMPDIR)/%_podman.tar: podman/Containerfile podman/setup.sh $(wildcard base_i
podman build -t $*_podman:$(call err_if_empty,IMG_SFX) \
--build-arg=BASE_NAME=$(subst prior-,,$*) \
--build-arg=BASE_TAG=$(call err_if_empty,BASE_TAG) \
--build-arg=PACKER_BUILD_NAME=$(subst _podman,,$*) \
Copy link
Member Author

Choose a reason for hiding this comment

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

Re: Scripts using conditionals based on presence/absence of 'prior' appearing in the VM/Container image name.

Can you get it from $VM_IMAGE_NAME?

That var. doesn't exist here (completely different Cirrus-CI setup). In any case, it's value is derived from $PACKER_BUILD_NAME when producing VM images. The problem area is with the container build workflow, as it shares the same setup/packaging scripts as the VM setup but does not use the packer tool.

Ordinarily the scripts could ignore this, as all conditions on $PACKER_BUILD_NAME only applied to VM's (i.e. containers don't care about setting kernel options)...except now this isn't the case, since I've un-commented the (previously commented in this PR) $ENABLE_UPDATES_TESTING conditional (on 'prior' flavors) for fedora.

I don't like this fix, it's convoluted and non-obvious 😲 (esp. in the container image build case). However, right now I cannot think of a better way to solve this in a more uniform yet readable manner, and am loathe to add a giant comment to the Makefile (which s already very dense and complex) 😞

@edsantiago I'd really like to get this PR merged, since the previously built images were already rolled out all across container-org. CI-land. Is this something we should just live with, fix later, ignore, or maybe I could file an issue on it (here) for fixing next year?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooof...and it doesn't appear to work: F32 container build log sigh

Copy link
Member Author

Choose a reason for hiding this comment

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

@edsantiago nvm my question, this is broken anyway so I'll clean it up and comment the heck out of it.

Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

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

I thought it would be easy. Do what you need to do; revert back to the hardcoded 20-etc, that's fine.

@cevich cevich force-pushed the new_ubuntu_versioning branch from 384be09 to b3fd469 Compare December 17, 2020 14:20
This is intended to save time/resources since if container image
building fails, chances are good VM images will also fail.

Signed-off-by: Chris Evich <[email protected]>
@cevich cevich force-pushed the new_ubuntu_versioning branch from b3fd469 to 58e093d Compare December 17, 2020 14:23
@cevich
Copy link
Member Author

cevich commented Dec 17, 2020

I thought it would be easy. Do what you need to do; revert back to the hardcoded 20-etc, that's fine.

Except now that I'm aware of the problem and wrote a good comment...it would keep me up a night worrying someone might uncomment/un-hard-code this later and not realize the implications 😞

I need to re-roll and re-test the images here for the other fixes anyway, so, it's just one more thing (and I confirmed by local testing that the $PACKER_BUILD_NAME check works).

It's been completely removed from all upstream podman testing.  Any
failures with new images now and going forward, may be considered
podman bugs.

Signed-off-by: Chris Evich <[email protected]>
@cevich cevich force-pushed the new_ubuntu_versioning branch from 58e093d to 23c9930 Compare December 17, 2020 14:46
@cevich
Copy link
Member Author

cevich commented Dec 17, 2020

c5026195240648704

@cevich
Copy link
Member Author

cevich commented Dec 17, 2020

@cevich cevich marked this pull request as ready for review December 17, 2020 17:05
@cevich cevich dismissed lsm5’s stale review December 17, 2020 17:06

Lokesh is on PTO

@mergify mergify bot merged commit 18f9e2d into containers:master Dec 17, 2020
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