This series is a re-rewrite of the image generator code and related manifests #375

bpradipt · 2024-02-14T13:15:50Z

Closes https://issues.redhat.com/browse/KATA-2633

Current flow : Create podvm image -> create mcp, kata rpm etc -> create kata runtimeclass -> create kata-remote runtimeclass
New flow: create mcp, kata rpm etc -> create kata runtimeclass -> create podvm image -> create kata-remote runtimeclass

The image generation status events are posted as Kubernetes events for monitoring

How to check for events during image generation when creating kataconfig?

oc get events -n openshift-sandboxed-containers-operator --field-selector involvedObject.name=osc-podvm-image-creation

How to check for events during image deletion when deleting kataconfig?

oc get events -n openshift-sandboxed-containers-operator --field-selector involvedObject.name=osc-podvm-image-deletion

bpradipt · 2024-02-14T13:34:52Z

Catalog for testing: quay.io/bpradipt/openshift-sandboxed-containers-operator-catalog:v1.5.2

bpradipt · 2024-02-14T13:35:04Z

@vvoronko @snir911

bpradipt · 2024-02-14T14:03:11Z

oc get events -n openshift-sandboxed-containers-operator --field-selector involvedObject.name=osc-podvm-image-creation 
LAST SEEN   TYPE     REASON                 OBJECT                         MESSAGE
153m        Normal   PodVMImageJobRunning   job/osc-podvm-image-creation   PodVM image job (osc-podvm-image-creation) is running
155m        Normal   PodVMImageJobRunning   job/osc-podvm-image-creation   PodVM image job (osc-podvm-image-creation) is running
153m        Normal   PodVMImageJobRunning   job/osc-podvm-image-creation   PodVM image job (osc-podvm-image-creation) is running
155m        Normal   PodVMImageJobRunning   job/osc-podvm-image-creation   PodVM image job (osc-podvm-image-creation) is running
155m        Normal   PodVMImageJobRunning   job/osc-podvm-image-creation   PodVM image job (osc-podvm-image-creation) is running
155m        Normal   PodVMImageJobRunning   job/osc-podvm-image-creation   PodVM image job (osc-podvm-image-creation) is running
155m        Normal   PodVMImageJobRunning   job/osc-podvm-image-creation   PodVM image job (osc-podvm-image-creation) is running
155m        Normal   PodVMImageJobRunning   job/osc-podvm-image-creation   PodVM image job (osc-podvm-image-creation) is running
155m        Normal   PodVMImageJobRunning   job/osc-podvm-image-creation   PodVM image job (osc-podvm-image-creation) is running
155m        Normal   PodVMImageJobRunning   job/osc-podvm-image-creation   PodVM image job (osc-podvm-image-creation) is running
148m        Normal   PodVMImageJobRunning   job/osc-podvm-image-creation   PodVM image job (osc-podvm-image-creation) is running
155m        Normal   PodVMImageJobRunning   job/osc-podvm-image-creation   PodVM image job (osc-podvm-image-creation) is running
155m        Normal   PodVMImageJobRunning   job/osc-podvm-image-creation   PodVM image job (osc-podvm-image-creation) is running
155m        Normal   PodVMImageJobRunning   job/osc-podvm-image-creation   PodVM image job (osc-podvm-image-creation) is running
155m        Normal   PodVMImageJobRunning   job/osc-podvm-image-creation   PodVM image job (osc-podvm-image-creation) is running
155m        Normal   PodVMImageJobRunning   job/osc-podvm-image-creation   PodVM image job (osc-podvm-image-creation) is running
155m        Normal   PodVMImageJobRunning   job/osc-podvm-image-creation   PodVM image job (osc-podvm-image-creation) is running
148m        Normal   PodVMImageJobRunning   job/osc-podvm-image-creation   PodVM image job (osc-podvm-image-creation) is running
155m        Normal   PodVMImageJobRunning   job/osc-podvm-image-creation   PodVM image job (osc-podvm-image-creation) is running
155m        Normal   SuccessfulCreate       job/osc-podvm-image-creation   Created pod: osc-podvm-image-creation-q9d76
141m        Normal   Completed              job/osc-podvm-image-creation   Job completed

snir911

Thanks @bpradipt ! added some comments, do you have a build for this?
If @abhbaner have some cycles for pre-merge test it, it would be perfect (as he found the corner cases in the previous implementation)

controllers/image_generator.go

config/peerpods/podvm/aws-podvm-image-handler.sh

controllers/image_generator.go

bpradipt · 2024-02-15T11:15:22Z

@snir911 catalog for testing - quay.io/bpradipt/openshift-sandboxed-containers-operator-catalog:v1.5.2

snir911

LGTM overall, assuming pre-merge testing was ok, thanks!

controllers/image_generator.go

vvoronko · 2024-02-19T16:16:27Z

new AMI generation flow was pre-merge tested

vvoronko · 2024-02-22T07:02:05Z

I appreciate Pradipta's effort to produce quality code, after few iterations we managed to:

keep AMI ID at single place
keep creation and deletion pods (I don't think really, that single pod demands much OCP resources and always could be deleted manually by admin)
AMI job status provided via kataconfig status is visible in Console
Overall IMHO this improves customer experience

status:
conditions:

lastTransitionTime: "2024-02-22T06:51:26Z"
message: Creating Pod VM Image
reason: PodVMImageJobRunning
status: "True"
type: InProgress
kataNodes:
installed:
- vvoron415azr22-5hft5-worker-eastus1-8szq5
  nodeCount: 1
  readyNodeCount: 1
  runtimeClasses:
kata
waitingForMcoToStart: false

1. Fix reading of required attributes from secret and configmap 2. Refactor code for better readibility and modularity 3. Add event notification to indicate the status of the job 4. Improve job status evaluation 5. Convert image generator instance to singleton Signed-off-by: Pradipta Banerjee <[email protected]>

The event "reason" code is used as the cache key. The default time for suppressing the event having the same reason code is 2 min. Signed-off-by: Pradipta Banerjee <[email protected]>

Move the podvm image creation post kata installation. Otherwise the kata installation remains stuck with no indication on what is happening Signed-off-by: Pradipta Banerjee <[email protected]>

Signed-off-by: Pradipta Banerjee <[email protected]>

bpradipt · 2024-02-23T09:34:59Z

@gkurz @pmores @littlejawa @jensfr if you have time, it will be great to have your comments on this PR, especially on the following:

New kataconfig status to indicate the pod vm image job creation/deletion
I took the liberty to fix some staticcheck warnings and include those as separate commits in the same PR.

The latest code is pre-merge tested by @vvoronko Thanks a lot for his help.
Thanks to Snir for initial round of reviews and feedback.

vvoronko · 2024-02-27T15:41:59Z

@jensfr @bpradipt @snir911 Does this PR affect the default AMI image in downstream?

There are cloud provider specific handler scripts for image creation and deletion. Also better error handling and documented scripts Signed-off-by: Pradipta Banerjee <[email protected]>

The controller-manager was gettign OOMkilled with latest changes. Hence bumping up the memory limit Signed-off-by: Pradipta Banerjee <[email protected]>

Consolidate all podvm image related constant in a single place Signed-off-by: Pradipta Banerjee <[email protected]>

This can be used in openshift controller for updating KataConfig CRD Status Signed-off-by: Pradipta Banerjee <[email protected]>

Add status update to KataConfig as well in addition to Kubernetes events Signed-off-by: Pradipta Banerjee <[email protected]>

Modify error strings to not use caps Signed-off-by: Pradipta Banerjee <[email protected]>

Adjust the return values to ensure error is the last return value. Also avoids go-staticcheck warnings Signed-off-by: Pradipta Banerjee <[email protected]>

getConditionReason and kataOcExists methods are no longer used. However let's keep it for now but ignore it as part of staticcheck warnings Signed-off-by: Pradipta Banerjee <[email protected]>

bpradipt · 2024-02-28T05:30:08Z

@jensfr @bpradipt @snir911 Does this PR affect the default AMI image in downstream?

If you meant whether this PR changes the AMI image that gets created using the existing code, then answer is yes. Since this will use latest pod vm components (like agent, protocol-forwarder etc)
Other than that there are no changes

littlejawa

I don't have the environment to test it with, but I trust @vvoronko on that.
LGTM from the code standpoint.

Thanks @bpradipt

bpradipt · 2024-02-28T14:25:26Z

Thanks @littlejawa @snir911 for acks
Thanks @vvoronko for pre-merge testing.

bpradipt · 2024-02-28T14:32:46Z

/override ci/prow/check

gkurz · 2024-02-28T14:34:22Z

/override ci/prow/check

openshift-ci · 2024-02-28T14:41:36Z

@bpradipt: Overrode contexts on behalf of bpradipt: 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-02-28T14:45:37Z

@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-02-28T14:46:18Z

@bpradipt: 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	`fe0fdc0`	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.

bpradipt marked this pull request as draft February 14, 2024 13:16

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 Feb 14, 2024

openshift-ci bot requested review from littlejawa and snir911 February 14, 2024 13:20

bpradipt force-pushed the podvm-builder branch from a62a0d7 to 474b288 Compare February 14, 2024 13:23

bpradipt marked this pull request as ready for review February 14, 2024 13:34

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 14, 2024

openshift-ci bot requested a review from pmores February 14, 2024 13:35

bpradipt force-pushed the podvm-builder branch 3 times, most recently from 6fd6c68 to da4a26e Compare February 14, 2024 18:26

snir911 reviewed Feb 15, 2024

View reviewed changes

bpradipt force-pushed the podvm-builder branch from da4a26e to eb2913b Compare February 15, 2024 11:11

bpradipt force-pushed the podvm-builder branch from eb2913b to 5c7e8eb Compare February 15, 2024 11:22

snir911 approved these changes Feb 15, 2024

View reviewed changes

bpradipt force-pushed the podvm-builder branch 4 times, most recently from 22672be to f71d64f Compare February 16, 2024 17:25

snir911 reviewed Feb 19, 2024

View reviewed changes

controllers/image_generator.go Outdated Show resolved Hide resolved

bpradipt force-pushed the podvm-builder branch 3 times, most recently from 29f36d8 to f95c5c1 Compare February 21, 2024 17:23

bpradipt force-pushed the podvm-builder branch from c89e239 to 51994da Compare February 22, 2024 10:20

bpradipt force-pushed the podvm-builder branch from 51994da to ce078f4 Compare February 22, 2024 13:46

bpradipt added 4 commits February 22, 2024 20:30

Create an event cache to suppress frequent event generation

f202851

The event "reason" code is used as the cache key. The default time for suppressing the event having the same reason code is 2 min. Signed-off-by: Pradipta Banerjee <[email protected]>

Rearrange imagecreate calling location

ae4b1ba

Move the podvm image creation post kata installation. Otherwise the kata installation remains stuck with no indication on what is happening Signed-off-by: Pradipta Banerjee <[email protected]>

Remove existing podvm image manifest files

ba88b03

Signed-off-by: Pradipta Banerjee <[email protected]>

bpradipt force-pushed the podvm-builder branch from ce078f4 to aad8918 Compare February 22, 2024 15:00

bpradipt added 8 commits February 28, 2024 10:17

Add podvm image manifest files and helpers

6aff778

There are cloud provider specific handler scripts for image creation and deletion. Also better error handling and documented scripts Signed-off-by: Pradipta Banerjee <[email protected]>

Increase memory limit to avoid OOMKilled

a4e4814

The controller-manager was gettign OOMkilled with latest changes. Hence bumping up the memory limit Signed-off-by: Pradipta Banerjee <[email protected]>

Move podvm image jobs file path to image_generator

869a193

Consolidate all podvm image related constant in a single place Signed-off-by: Pradipta Banerjee <[email protected]>

Convert the PodVM Image job events to string constants

cc452b1

This can be used in openshift controller for updating KataConfig CRD Status Signed-off-by: Pradipta Banerjee <[email protected]>

Update PodVM image job status in KataConfig

36f3d26

Add status update to KataConfig as well in addition to Kubernetes events Signed-off-by: Pradipta Banerjee <[email protected]>

Fix staticcheck warning of using caps in error string

de4c7b3

Modify error strings to not use caps Signed-off-by: Pradipta Banerjee <[email protected]>

Use acceptable golang pattern to return err

093a38b

Adjust the return values to ensure error is the last return value. Also avoids go-staticcheck warnings Signed-off-by: Pradipta Banerjee <[email protected]>

Keep the unused methods for now

fe0fdc0

getConditionReason and kataOcExists methods are no longer used. However let's keep it for now but ignore it as part of staticcheck warnings Signed-off-by: Pradipta Banerjee <[email protected]>

bpradipt force-pushed the podvm-builder branch from aad8918 to fe0fdc0 Compare February 28, 2024 04:47

littlejawa approved these changes Feb 28, 2024

View reviewed changes

snir911 mentioned this pull request Feb 28, 2024

credentials-controller: fetch credentials using cloud-credentials-operator #376

Merged

bpradipt merged commit a96f806 into openshift:devel Feb 28, 2024

bpradipt deleted the podvm-builder branch February 28, 2024 14:51

This series is a re-rewrite of the image generator code and related manifests #375

This series is a re-rewrite of the image generator code and related manifests #375

Uh oh!

Conversation

bpradipt commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bpradipt commented Feb 14, 2024

Uh oh!

bpradipt commented Feb 14, 2024

Uh oh!

bpradipt commented Feb 14, 2024

Uh oh!

snir911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bpradipt commented Feb 15, 2024

Uh oh!

snir911 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vvoronko commented Feb 19, 2024

Uh oh!

vvoronko commented Feb 22, 2024

Uh oh!

bpradipt commented Feb 23, 2024

Uh oh!

vvoronko commented Feb 27, 2024

Uh oh!

bpradipt commented Feb 28, 2024

Uh oh!

littlejawa left a comment

Choose a reason for hiding this comment

Uh oh!

bpradipt commented Feb 28, 2024

Uh oh!

bpradipt commented Feb 28, 2024

Uh oh!

gkurz commented Feb 28, 2024

Uh oh!

openshift-ci bot commented Feb 28, 2024

Uh oh!

openshift-ci bot commented Feb 28, 2024

Uh oh!

openshift-ci bot commented Feb 28, 2024

Uh oh!

Uh oh!

bpradipt commented Feb 14, 2024 •

edited

Loading