image-generator: fix image creation edge cases #369

snir911 · 2023-12-13T14:35:54Z

at creation: name images with unique naming to aviod name reuse failures
at deletion: ignore if image is already deleted

to test:

quay.io/snir/osc-operator:v9.9.2618
quay.io/snir/osc-operator-catalog:v9.9.2618
quay.io/snir/osc-operator-bundle:v9.9.2618

in OCP make sure to set also:
oc set env deployment.apps/controller-manager SANDBOXED_CONTAINERS_EXTENSION=sandboxed-containers -n openshift-sandboxed-containers-operator

* at creation: name images with unique naming to aviod name reuse failures * at deletion: ignore if image is already deleted Signed-off-by: Snir Sheriber <ssheribe@redhat.com>

abhbaner · 2023-12-14T07:37:51Z

Successfully Pre-Merge tested it on AWS

Edge case 1 : If there is an existing podvm Image with the name 'peer-pod-XXXX' (on the cloud provider) the kataconfig install does not begin as the IMAGE ID creation job fails (due to the same name of the existing podvm image) and that causes the kataconfig install to be stuck in and there is no progress

Edge case 2: If by mistake the user deletes the podvm IMAGE (from the console of the cloud provider ) the kataconfig deletion will not progress as the process waits for the image deletion job to complete which it wont in this case as the user has deleted the podvm image already and the job fails , unable to find the podvm image id.

Pre-Merge test Steps followed

Applied the catalog source (with the catalog quay.io/snir/osc-operator-catalog:v9.9.2618 in it)
Installed the OSC Operator
Post install of the operator I issued the cmd
oc set env deployment.apps/controller-manager SANDBOXED_CONTAINERS_EXTENSION=sandboxed-containers -n openshift-sandboxed-containers-operator
Created the the peer pod cm and secret
Created the kataconfig with enablepeerpods set to 'true'

Edge case 1 tested

The AWS console had an existing AMI with name (and ID) as peer-pod-ami (ami-0cc7239b859af5641). Despite that the image id creation job was able to create a new AMI ID and named it peer-pod-ami-3c37 (ami-0cd66ff9c8c21755c)
The kataconfig creation went through fine as well

Edge case 2 tested

Deregistered all AMIs from the AWS console with peer-pod-ami in their name
Successfully deleted kataconfig

cpmeadors · 2024-01-05T15:19:38Z

/override ci/prow/check

openshift-ci · 2024-01-05T15:20:48Z

@cpmeadors: Overrode contexts on behalf of cpmeadors: ci/prow/check

In response to this:

/override ci/prow/check

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

cpmeadors · 2024-01-05T16:38:22Z

@abhbaner can you approve since you did the pre-merge testing?

cpmeadors

Just some questions for docs (who should probably be reviewing as well) and a question about the output and user visibility.

cpmeadors · 2024-01-05T16:45:37Z

config/peerpods/podvm/aws-VM-image-create-job.yaml

@@ -59,6 +57,7 @@ spec:
        - -c
        - |
          set -e
+          [[ ! "${IMAGE_NAME}" ]] && UUID=$(uuidgen) && export IMAGE_NAME="peer-pod-ami-${UUID::6}" && echo "IMAGE_NAME:${IMAGE_NAME}"


If we are not requiring IMAGE_NAME to be explicitly unset we probably need to add that to the docs.

User shouldn't even know it's possible to set it, this variable is not mentioned in the docs

cpmeadors · 2024-01-05T16:47:20Z

config/peerpods/podvm/aws-VM-image-delete-job.yaml

+          [[ ! "${PODVM_AMI_ID}" ]] && echo "PODVM_AMI_ID is missing, it's unknown which image to delete" && exit 1
+          [[ "${IMAGE_NAME}" ]] && echo "IMAGE_NAME:${IMAGE_NAME} is set, it implies image was not automatically created, delete it manually" && exit 0


I think we need to make sure it is clear in docs as to what happens with manually created images and that user is responsible for deleting them as the message states.

Is custom images supported ATM? AFAIK the reason it's supported is for tech/dev-preview features only

cpmeadors · 2024-01-05T16:49:40Z

config/peerpods/podvm/aws-VM-image-delete-job.yaml

+          RES=$(aws ec2 deregister-image --image-id "${PODVM_AMI_ID}" --region "${AWS_REGION}" 2>&1) || ERR=true
+          echo ${RES}
+          [[ ${ERR} ]] && [[ "$RES" =~ InvalidAMIID\.(Unavailable|NotFound) ]] # if deregister returned error and image is already deleted, continue
+          echo "Deleted AMI: ${PODVM_AMI_ID} - DONE"


Are users supposed to see the error message in the output of the job? Is that captured in a log anywhere? Also a little weird that we say the AMI is deleted when it didn't exist. I feel like there is an "if" statement missing.

It's possible to see the error in the job logs, however, in this case the error means image is already deleted and there's nothing else to do which is fine, would it make more sense to change the wording to something like: "image deletion job completed"?

Are users supposed to see the error message in the output of the job?

Users can see these logs with oc logs but they aren't supposed to look at them. This is an internal job.

Is that captured in a log anywhere?

This should probably be captured by the OSC must-gather if it isn't the case already.

It's possible to see the error in the job logs, however, in this case the error means image is already deleted and there's nothing else to do which is fine, would it make more sense to change the wording to something like: "image deletion job completed"?

IIUC the only case where we get the InvalidAMIID\.(Unavailable|NotFound) error is when someone deleted the image automatically created by us in our back, right ? Assuming this is fine, then the job just needs to be idempotent and report something like "PODVM_AMI_ID: \"$PODVM_AMI_ID\" is deleted".

gkurz · 2024-01-08T16:49:37Z

config/peerpods/podvm/aws-VM-image-create-job.yaml

@@ -59,6 +57,7 @@ spec:
        - -c
        - |
          set -e
+          [[ ! "${IMAGE_NAME}" ]] && UUID=$(uuidgen) && export IMAGE_NAME="peer-pod-ami-${UUID::6}" && echo "IMAGE_NAME:${IMAGE_NAME}"


@snir911 I don't understand the motivation behind the [[ ! "${IMAGE_NAME}" ]] check... what is it for ? Don't we want to enforce the name of the image to be something like peer-pod-ami-XXXXXX ?

To preserve previous behavior mostly I think, for example if users wants to specify a name that would be easy for him to track later using AWS cli.
TBH i don't recall if i had better reason, but it make sense to me that if user explicitly set it we should comply

Ok, so IMAGE_NAME is a kind of hidden API that we don't want to document for users but we still want to be usable, correct ? It would be very nice to document this in some document for developers at least, or even comments in the yaml files, because the questions in this PR clearly show that it isn't obvious.

gkurz

I would have appreciated a comment to be added in the deletion scripts so that one doesn't need to git blame to fully understand what's going on. Not critical enough to hold this PR. Thanks @snir911 !

gkurz

Thanks @snir911 !

gkurz · 2024-01-15T16:14:48Z

/override ci/prow/check

openshift-ci · 2024-01-15T16:27:48Z

@gkurz: Overrode contexts on behalf of gkurz: ci/prow/check

In response to this:

/override ci/prow/check

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci · 2024-01-15T16:28:21Z

@snir911: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/sandboxed-containers-operator-e2e	`96cb6bc`	link	false	`/test sandboxed-containers-operator-e2e`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

image-generator: fix image creation edge cases

bf50128

* at creation: name images with unique naming to aviod name reuse failures * at deletion: ignore if image is already deleted Signed-off-by: Snir Sheriber <ssheribe@redhat.com>

snir911 self-assigned this Dec 13, 2023

openshift-ci bot requested review from cpmeadors and littlejawa December 13, 2023 14:42

cpmeadors reviewed Jan 5, 2024

View reviewed changes

gkurz reviewed Jan 8, 2024

View reviewed changes

gkurz approved these changes Jan 15, 2024

View reviewed changes

add comments to explain IMAGE_NAME behavior

96cb6bc

gkurz approved these changes Jan 15, 2024

View reviewed changes

gkurz merged commit cd3716d into openshift:devel Jan 15, 2024
3 of 4 checks passed

gkurz mentioned this pull request Feb 2, 2024

Merge to main for 1.5.2 #372

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

image-generator: fix image creation edge cases #369

image-generator: fix image creation edge cases #369

snir911 commented Dec 13, 2023 •

edited by openshift-ci bot

abhbaner commented Dec 14, 2023 •

edited

cpmeadors commented Jan 5, 2024

openshift-ci bot commented Jan 5, 2024

cpmeadors commented Jan 5, 2024

cpmeadors left a comment

cpmeadors Jan 5, 2024

snir911 Jan 10, 2024

cpmeadors Jan 5, 2024

snir911 Jan 7, 2024

cpmeadors Jan 5, 2024

snir911 Jan 7, 2024

gkurz Jan 10, 2024

gkurz Jan 8, 2024

snir911 Jan 10, 2024

gkurz Jan 10, 2024

gkurz left a comment

gkurz left a comment

gkurz commented Jan 15, 2024

openshift-ci bot commented Jan 15, 2024

openshift-ci bot commented Jan 15, 2024

		[[ ! "${PODVM_AMI_ID}" ]] && echo "PODVM_AMI_ID is missing, it's unknown which image to delete" && exit 1
		[[ "${IMAGE_NAME}" ]] && echo "IMAGE_NAME:${IMAGE_NAME} is set, it implies image was not automatically created, delete it manually" && exit 0

image-generator: fix image creation edge cases #369

image-generator: fix image creation edge cases #369

Conversation

snir911 commented Dec 13, 2023 • edited by openshift-ci bot

abhbaner commented Dec 14, 2023 • edited

cpmeadors commented Jan 5, 2024

openshift-ci bot commented Jan 5, 2024

cpmeadors commented Jan 5, 2024

cpmeadors left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkurz left a comment

Choose a reason for hiding this comment

gkurz left a comment

Choose a reason for hiding this comment

gkurz commented Jan 15, 2024

openshift-ci bot commented Jan 15, 2024

openshift-ci bot commented Jan 15, 2024

snir911 commented Dec 13, 2023 •

edited by openshift-ci bot

abhbaner commented Dec 14, 2023 •

edited