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

tests: update CI images #10451

Merged
merged 4 commits into from
Jun 16, 2021
Merged

Conversation

giuseppe
Copy link
Member

@giuseppe giuseppe commented May 25, 2021

preparation work for: containers/common#573

Signed-off-by: Giuseppe Scrivano [email protected]

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 25, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 25, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 25, 2021
@giuseppe
Copy link
Member Author

we need an updated runc. v1.0-rc95 has the feature we need

@rhatdan
Copy link
Member

rhatdan commented May 25, 2021

@cevich PTAL

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 26, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 31, 2021
@cevich
Copy link
Member

cevich commented Jun 2, 2021

we need an updated runc. v1.0-rc95 has the feature we need

Sorry only just seeing this now after being on PTO last week. Okay, I'll build new VM images...okay, opened a PR.

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 2, 2021
@cevich
Copy link
Member

cevich commented Jun 2, 2021

@giuseppe okay, give image suffix c6704734020042752 a try.

$ podman run -it --rm quay.io/libpod/fedora_podman:c6704734020042752 rpm -q runc
Trying to pull quay.io/libpod/fedora_podman:c6704734020042752...
Getting image source signatures
Copying blob 1ab2777b591f done
Copying blob 76d9e8b34382 done
Copying config 3584509b8e done
Writing manifest to image destination
Storing signatures
runc-1.0.0-378.rc95.fc34.x86_64

@rhatdan
Copy link
Member

rhatdan commented Jun 2, 2021

@giuseppe needs a rebase. @cevich I think we need new VMs to get latest runc.

@giuseppe giuseppe force-pushed the test-ENOSYS branch 2 times, most recently from bb13885 to 1400ece Compare June 2, 2021 19:34
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 2, 2021
@giuseppe giuseppe force-pushed the test-ENOSYS branch 3 times, most recently from 4ce1c77 to 3d157ac Compare June 3, 2021 19:17
@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 3, 2021
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 3, 2021
@giuseppe giuseppe force-pushed the test-ENOSYS branch 6 times, most recently from f61ca4a to 8638024 Compare June 4, 2021 09:44
@giuseppe
Copy link
Member Author

addressed the comments ⬆️

@rhatdan
Copy link
Member

rhatdan commented Jun 16, 2021

So we revert this PR or at least the separation of the VMs once selinux-policy gets updated.

@cevich
Copy link
Member

cevich commented Jun 16, 2021

LGTM

@cevich
Copy link
Member

cevich commented Jun 16, 2021

So we revert this PR or at least the separation of the VMs once selinux-policy gets updated.

Just the one .cirrus.yml: use c5521575421149184 for Ubuntu commit.

But if this takes longer than 30-days, there's a very not-obvious problem WRT image pruning. I'll open a followup PR so the tests don't all need to be re-run for it (here).

@cevich
Copy link
Member

cevich commented Jun 16, 2021

...in case you want to include it in the one commit (would be convenient for reverting), this is what I'm thinking:

diff --git a/.cirrus.yml b/.cirrus.yml
index a403889c6..026843503 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -678,6 +678,8 @@ meta_task:
             ${PRIOR_FEDORA_CACHE_IMAGE_NAME}
             ${UBUNTU_CACHE_IMAGE_NAME}
             ${PRIOR_UBUNTU_CACHE_IMAGE_NAME}
+            "ubuntu-${IMAGE_SUFFIX}"
+            "prior-ubuntu-${IMAGE_SUFFIX}"
         BUILDID: "${CIRRUS_BUILD_ID}"
         REPOREF: "${CIRRUS_REPO_NAME}"
         GCPJSON: ENCRYPTED[3a198350077849c8df14b723c0f4c9fece9ebe6408d35982e7adf2105a33f8e0e166ed3ed614875a0887e1af2b8775f4]

@giuseppe
Copy link
Member Author

thanks for the suggestion. I've amended the change in the last version

cevich added a commit to cevich/podman that referenced this pull request Jun 16, 2021
A hidden non-obvious corner-case of temporary changes introduced by
containers#10451 could be unintended
pruning of some ubuntu images.  This could be impactful if for some
reason the `.cirrus.yml: use c5521575421149184 for Ubuntu` commit is
reverted beyond 30-days (the disused image-prune interval) and the _old_
images are needed (for an unforeseen reason).

Mitigate this by temporarily including the old images in the timestamp
updating task. This commit may be reverted (and the problem ignored)
if new VM images are built and deployed for all OS's (i.e. replacing
the Fedora/Ubuntu tag split workaround needed for the BZ).

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

cevich commented Jun 16, 2021

oof, my bad...

diff --git a/.cirrus.yml b/.cirrus.yml
index a403889c6..026843503 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -678,6 +678,8 @@ meta_task:
             ${PRIOR_FEDORA_CACHE_IMAGE_NAME}
             ${UBUNTU_CACHE_IMAGE_NAME}
             ${PRIOR_UBUNTU_CACHE_IMAGE_NAME}
+            ubuntu-${IMAGE_SUFFIX}
+            prior-ubuntu-${IMAGE_SUFFIX}
         BUILDID: "${CIRRUS_BUILD_ID}"
         REPOREF: "${CIRRUS_REPO_NAME}"
         GCPJSON: ENCRYPTED[3a198350077849c8df14b723c0f4c9fece9ebe6408d35982e7adf2105a33f8e0e166ed3ed614875a0887e1af2b8775f4]

@cevich
Copy link
Member

cevich commented Jun 16, 2021

Okay, welp, since we're re-spinning tests I can close my PR then. Sorry for the noise/churn, I was trying to help avoid needing to re-run all the tests while there's the annoying system-test flake (assuming it's still a thing).

@giuseppe
Copy link
Member Author

looks like the test doesn't like the last diff:


ERROR: (gcloud.compute.images.update) HTTPError 400: Invalid value for field 'image': '"ubuntu-c5348179051806720"'. Must be a match of regex '[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?|[1-9][0-9]{0,19}'
�[1;33mDetected update error for '"ubuntu-c5348179051806720"'�[0m
ERROR: (gcloud.compute.images.update) HTTPError 400: Invalid value for field 'image': '"prior-ubuntu-c5348179051806720"'. Must be a match of regex '[a-z](?:[-a-z0-9]{0,61}[a-z0-9])?|[1-9][0-9]{0,19}'
�[1;33mDetected update error for '"prior-ubuntu-c5348179051806720"'�[0m
�[1;31mERROR: ERROR: Failed to update one or more image timestamps:  "ubuntu-c5348179051806720" "prior-ubuntu-c5348179051806720"�[0m

@giuseppe
Copy link
Member Author

reverted it back, and the CI is smart to turn immediately green :-)

@rhatdan
Copy link
Member

rhatdan commented Jun 16, 2021

LGTM

@rhatdan
Copy link
Member

rhatdan commented Jun 16, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2021
@openshift-merge-robot openshift-merge-robot merged commit b3f61ec into containers:master Jun 16, 2021
cevich added a commit to cevich/podman that referenced this pull request Jun 16, 2021
A hidden non-obvious corner-case of temporary changes introduced by
containers#10451 could be unintended
pruning of some ubuntu images.  This could be impactful if for some
reason the `.cirrus.yml: use c5521575421149184 for Ubuntu` commit is
reverted beyond 30-days (the disused image-prune interval) and the _old_
images are needed (for an unforeseen reason).

Mitigate this by temporarily including the old images in the timestamp
updating task. This commit may be reverted (and the problem ignored)
if new VM images are built and deployed for all OS's (i.e. replacing
the Fedora/Ubuntu tag split workaround needed for the BZ).

Signed-off-by: Chris Evich <[email protected]>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 23, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants