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

Human-readable date-based image suffixes #247

Merged
merged 1 commit into from
Jan 27, 2023

Conversation

cevich
Copy link
Member

@cevich cevich commented Jan 17, 2023

Historically, the ID used to identify a set of images was taken from the
Cirrus-CI build ID. This was done to make auditing easier, since one
can easily retrieve the build logs using the ID. However, there are
many disadvantages to using the build id:

  • It's not human-readable, making it difficult to ascertain exactly when
    the images were built.
  • It's not guaranteed to be incremental, and therefore cannot be
    utilized as a "version".
  • It doesn't convey helpful information like when it was produced, or
    which release of Fedora is included in the set.

For these and other reasons, switch to a simple date-based image suffix
encoded in a repository file. Include a UTC-baesd timestamp to avoid
value collisions, and a string identifying what OS's releases were built.

@cevich cevich changed the title Human-readable incremental image suffixes [WIP] Human-readable incremental image suffixes Jan 17, 2023
@cevich cevich force-pushed the versionable_suffixes branch 3 times, most recently from fbafa3e to a1e2aa6 Compare January 17, 2023 18:16
@cevich
Copy link
Member Author

cevich commented Jan 17, 2023

Right....cirrus.yml has references to $IMG_SFX all over the place, and there's no way to pull the value from the Makefile 😢

@cevich cevich closed this Jan 17, 2023
@cevich cevich reopened this Jan 18, 2023
@cevich cevich force-pushed the versionable_suffixes branch from a1e2aa6 to c4e501a Compare January 18, 2023 18:52
@cevich cevich changed the title [WIP] Human-readable incremental image suffixes [WIP] Human-readable date-based image suffixes Jan 18, 2023
@cevich cevich force-pushed the versionable_suffixes branch from c4e501a to cff5b39 Compare January 18, 2023 19:11
@cevich
Copy link
Member Author

cevich commented Jan 18, 2023

@edsantiago This is not urgent, but I could use a second pair of eyes early on.

Big-picture: My eventual goal is to have renovate manage opening vm-image update PRs for us. For that to work, we need image-suffixes resembling version-numbers that sort properly, and can be git tag into this repo. Before you ask - "no" the current values are not guaranteed to be incremental.

So this PR is a first-step in that direction...but as usual, it's complicated 😞

The major change here so far, is that Cirrus will read a $IMG_SFX "version" value in from a file in the repo. The file can be generated (and updated) by make IMG_SFX. However, since it's date-based (for human readability), there can be clashes[0]. I added a v0 field that can be incremented to help avoid them, but this is less than ideal[1].

In short, with this PR, the new "build a new set of images" workflow would go like:

  1. Run make IMG_SFX.
  2. Manually verify there are/were no other open image-update PRs that used the same suffix value - this could be an automated check eventually, but that's for later.
  3. If there is a clash, edit IMG_SFX file and bump the v0 number to prevent it.
  4. Push PR and wait.
  5. Assuming success, github-action prints table of built images.
  6. Update downstream .cirrus.yml to use the new suffix,
    e.g. IMAGE_SUFFIX = c230118v0-f37p36u2204

Does this sound mostly sane so far?

Do you think it's helpful to include the OS revisions in the ID?


[0]Note: A IMG_SFX clash is potentially really very bad. At best it will clobber existing images from somebody else's PR. At worst it will break somebody's active (still building) images or images already in use by downstream CI (eek!).

[1]Note: There are character and size limitations to the IMG_SFX value. I'd prefer to keep it human readable if possible, but an option could be to swap the OS-versions part for a short git-commit hash to prevent clashes.

Makefile Outdated Show resolved Hide resolved
@cevich cevich force-pushed the versionable_suffixes branch from cff5b39 to 2bd0e5e Compare January 19, 2023 18:25
@cevich
Copy link
Member Author

cevich commented Jan 19, 2023

force-push change summary:

  • Clarified README.md changes further
  • Added '-' separator between distro versions in IMG_SFX format
  • Added hours/minutes/seconds value to IMG_SFX format
  • Moved validation commands into validate.sh w/ helpful failure messages.

@github-actions
Copy link

Cirrus CI build successful. Found built image names and IDs:

Stage Image Name IMAGE_SUFFIX
base fedora b230119-131309-f37-f36-u2204
base fedora-aws b230119-131309-f37-f36-u2204
base fedora-aws-arm64 b230119-131309-f37-f36-u2204
base image-builder b230119-131309-f37-f36-u2204
base prior-fedora b230119-131309-f37-f36-u2204
base ubuntu b230119-131309-f37-f36-u2204
cache build-push c230119-131309-f37-f36-u2204
cache fedora c230119-131309-f37-f36-u2204
cache fedora-aws c230119-131309-f37-f36-u2204
cache fedora-netavark c230119-131309-f37-f36-u2204
cache fedora-netavark-aws-arm64 c230119-131309-f37-f36-u2204
cache fedora-podman-aws-arm64 c230119-131309-f37-f36-u2204
cache fedora-podman-py c230119-131309-f37-f36-u2204
cache prior-fedora c230119-131309-f37-f36-u2204
cache ubuntu c230119-131309-f37-f36-u2204
cache win-server-wsl c230119-131309-f37-f36-u2204

@cevich
Copy link
Member Author

cevich commented Jan 19, 2023

w00t! 🎉

@Luap99
Copy link
Member

Luap99 commented Jan 20, 2023

I find it very hard to read this custom date format. Any reason to not just use the iso 8601 format? Also looks like the time is in the local timezone? I would prefer UTC.

Lastly would it make sense to encode the automation_images PR number in that ID? In case something broke it will be easy to find the correct PR here.

@cevich
Copy link
Member Author

cevich commented Jan 20, 2023

@Luap99 Using UTC is a good suggestion. Thanks.

I'm fairly limited in the character set and length for the image names. It's about 45 lower-case letters, numbers, and '-'. That's it. So I'm afraid a delimited date/time wouldn't look much better:

23-01-19-13-13-09-f37-f36-u2204

what about if I stuck a full year, and t in there in they style of 8601, so:

2023-01-19t13-13-09-f37-f36-u2204

any better?

@cevich
Copy link
Member Author

cevich commented Jan 20, 2023

Also keep in mind, it's not expected for humans to be reading/decoding this ID very much. Maybe once a month or so on average, if that. So I think some amount of ugliness is probably okay.

@cevich
Copy link
Member Author

cevich commented Jan 20, 2023

Hmmm. I think all the - actually make it harder to read. What about:

20230120-151738-f37f36u2204 (eventually will be d11 on the end)

@Luap99
Copy link
Member

Luap99 commented Jan 20, 2023

@Luap99 Using UTC is a good suggestion. Thanks.

I'm fairly limited in the character set and length for the image names. It's about 45 lower-case letters, numbers, and '-'. That's it. So I'm afraid a delimited date/time wouldn't look much better:

23-01-19-13-13-09-f37-f36-u2204

what about if I stuck a full year, and t in there in they style of 8601, so:

2023-01-19t13-13-09-f37-f36-u2204

any better?

20230120T141842Z would be also work for me and iso 8061

I don't really need the dashes, the full year alone is enough for my brain to recognize this as date.

@cevich cevich force-pushed the versionable_suffixes branch from 9546cd8 to e06eebd Compare January 20, 2023 15:25
@cevich
Copy link
Member Author

cevich commented Jan 20, 2023

SGTM

@cevich cevich force-pushed the versionable_suffixes branch from e06eebd to ee02f37 Compare January 20, 2023 15:26
Historically, the ID used to identify a set of images was taken from the
Cirrus-CI build ID.  This was done to make auditing easier, since one
can easily retrieve the build logs using the ID.  However, there are
many disadvantages to using the build id:

* It's not human-readable, making it difficult to ascertain exactly when
  the images were built.
* It's not guaranteed to be incremental, and therefore cannot be
  utilized as a "version".
* It doesn't convey helpful information like when it was produced, or
  which release of Fedora is included in the set.

For these and other reasons, switch to a simple date-based image suffix
encoded in a repository file.  Include a UTC-baesd timestamp to avoid
value collisions, and a string identifying what OS's releases were built.

Signed-off-by: Chris Evich <[email protected]>
@cevich cevich force-pushed the versionable_suffixes branch from ee02f37 to baa870c Compare January 20, 2023 15:27
@cevich
Copy link
Member Author

cevich commented Jan 20, 2023

Upper-case characters aren't supported, but I still think this works okay:

20230120t152650z-f37f36u2204

The real trick will be seeing if I can teach renovate how to interpret that as a version number.

@Luap99
Copy link
Member

Luap99 commented Jan 20, 2023

yeah that works for me as well, I really like the idea behind this.

@cevich
Copy link
Member Author

cevich commented Jan 20, 2023

I really like the idea behind this.

Ya, me too. I think it was actually @edsantiago that suggested it a few years ago ☺️ The thing that's motivating me is: Renovate will be able to post image-update PRs, just based on tagging one of these "versions" in this repo. I'm working on a POC in containers/automation_sandbox right now, as I write this.

@cevich
Copy link
Member Author

cevich commented Jan 20, 2023

W00t, it's working: containers/automation_sandbox#97

@github-actions
Copy link

Cirrus CI build successful. Found built image names and IDs:

Stage Image Name IMAGE_SUFFIX
base fedora b20230120t152650z-f37f36u2204
base fedora-aws b20230120t152650z-f37f36u2204
base fedora-aws-arm64 b20230120t152650z-f37f36u2204
base image-builder b20230120t152650z-f37f36u2204
base prior-fedora b20230120t152650z-f37f36u2204
base ubuntu b20230120t152650z-f37f36u2204
cache build-push c20230120t152650z-f37f36u2204
cache fedora c20230120t152650z-f37f36u2204
cache fedora-aws c20230120t152650z-f37f36u2204
cache fedora-netavark c20230120t152650z-f37f36u2204
cache fedora-netavark-aws-arm64 c20230120t152650z-f37f36u2204
cache fedora-podman-aws-arm64 c20230120t152650z-f37f36u2204
cache fedora-podman-py c20230120t152650z-f37f36u2204
cache prior-fedora c20230120t152650z-f37f36u2204
cache ubuntu c20230120t152650z-f37f36u2204
cache win-server-wsl c20230120t152650z-f37f36u2204

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.

High-level: wow, fantastic work, thank you. This is beautiful and will be much appreciated.

Low level: whewwwwww, thanks for your patience. A lot to learn and understand. I have one (possibly invalid) concern, and one question.

.cirrus.star Show resolved Hide resolved
ci/validate.sh Show resolved Hide resolved
ci/validate.sh Show resolved Hide resolved
cevich added a commit to cevich/automation_images that referenced this pull request Jan 26, 2023
This conditional was required temporarily so containers#247 could merge.  Remove
it and while we're there, backup/restore the original value in case it
saves somebody a headache or two.

Signed-off-by: Chris Evich <[email protected]>
@cevich
Copy link
Member Author

cevich commented Jan 26, 2023

whewwwwww, thanks for your patience

No worries, as always I appreciate the oversight MUCH more than I dislike waiting 😄

@cevich cevich changed the title [WIP] Human-readable date-based image suffixes Human-readable date-based image suffixes Jan 26, 2023
cevich added a commit to cevich/automation that referenced this pull request Jan 27, 2023
This compliments the date/time based versioning implemented
in containers/automation_images#247

Once merged, these changes will result in renovate opening PRs
for tagged CI VM image commits in containers/automation_images.
Renovate will ignore any existing configurations using the old
`$CIRRUS_BUILD_ID` based *IMAGE_SUFFIX* values.

Signed-off-by: Chris Evich <[email protected]>
@cevich cevich marked this pull request as ready for review January 27, 2023 16:04
@cevich cevich merged commit 3c24076 into containers:main Jan 27, 2023
cevich added a commit to cevich/automation_images that referenced this pull request Jan 27, 2023
This conditional was required temporarily so containers#247 could merge.  Remove
and replace it with a version that avoids modifying any files and only
checks if `IMG_SFX` changes were made.

Signed-off-by: Chris Evich <[email protected]>
cevich added a commit to cevich/automation_images that referenced this pull request Jan 27, 2023
This conditional was required temporarily so containers#247 could merge.  Remove
and replace it with a version that avoids modifying any files and only
checks if `IMG_SFX` changes were made.

Signed-off-by: Chris Evich <[email protected]>
cevich added a commit to cevich/automation_images that referenced this pull request Jan 27, 2023
This conditional was required temporarily so containers#247 could merge.  Remove
and replace it with a version that avoids modifying any files and only
checks if `IMG_SFX` changes were made.

Signed-off-by: Chris Evich <[email protected]>
cevich added a commit to cevich/automation_images that referenced this pull request Jan 27, 2023
This conditional was required temporarily so containers#247 could merge.  Remove
and replace it with a version that avoids modifying any files and only
checks if `IMG_SFX` changes were made.

Signed-off-by: Chris Evich <[email protected]>
cevich added a commit to cevich/automation_images that referenced this pull request Jan 27, 2023
This conditional was required temporarily so PR containers#247 could merge.  Remove
and replace it with a version that avoids modifying any files and only
checks if `IMG_SFX` changes were made.

Signed-off-by: Chris Evich <[email protected]>
cevich added a commit to cevich/automation_images that referenced this pull request Jan 27, 2023
This conditional was required temporarily so PR containers#247 could merge.  Remove
and replace it with a version that avoids modifying any files and only
checks if `IMG_SFX` changes were made.

Signed-off-by: Chris Evich <[email protected]>
cevich added a commit to cevich/automation_images that referenced this pull request Jan 27, 2023
This conditional was required temporarily so PR containers#247 could merge.  Remove
and replace it with a version that avoids modifying any files and only
checks if `IMG_SFX` changes were made.

Signed-off-by: Chris Evich <[email protected]>
cevich added a commit to cevich/automation_images that referenced this pull request Jan 30, 2023
This conditional was required temporarily so PR containers#247 could merge.  Remove
and replace it with a version that avoids modifying any files and only
checks if `IMG_SFX` changes were made.

Signed-off-by: Chris Evich <[email protected]>
cevich added a commit to cevich/podman that referenced this pull request Feb 1, 2023
Image content hasn't changed much, the biggest thing here is the
$IMAGE_SUFFIX value. This new schema is also fully manageable by
renovate. Allowing a tag-push to c/automation_images to create image
update PRs in all repos automatically.

ref: containers/automation_images#247

Also, cleanup a few comments and remove a disused testing task.

Signed-off-by: Chris Evich <[email protected]>
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