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.

Sign up for GitHub

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

Jump to bottom

Kata installation using RHCOS image layer #416

Merged
merged 1 commit into from
Jul 8, 2024
Merged

Kata installation using RHCOS image layer #416

merged 1 commit into from
Jul 8, 2024

Conversation

bpradipt
Copy link
Contributor

@bpradipt bpradipt commented Jun 4, 2024

No description provided.

@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 Jun 4, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Jun 4, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@bpradipt bpradipt force-pushed the layered-image-new branch from 73cde2a to 0a52c6d Compare June 4, 2024 05:19
@bpradipt bpradipt force-pushed the layered-image-new branch 2 times, most recently from 87f3d52 to 1b5c4e1 Compare June 7, 2024 13:05
@bpradipt bpradipt force-pushed the layered-image-new branch 4 times, most recently from 70eca7b to 4b6ed27 Compare June 26, 2024 06:16
@bpradipt bpradipt marked this pull request as ready for review June 26, 2024 06:17
@openshift-ci 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 Jun 26, 2024
@bpradipt bpradipt requested a review from pmores June 26, 2024 06:17
@openshift-ci openshift-ci bot requested review from cpmeadors and gkurz June 26, 2024 06:18
@bpradipt bpradipt changed the title Draft: Kata installation using RHCOS image layer Kata installation using RHCOS image layer Jun 26, 2024
@bpradipt bpradipt requested review from snir911 and spotlesstofu June 26, 2024 11:40
@spotlesstofu
Copy link
Contributor

Avoid growing the file openshift_controller.go. The code that you added to openshift_controller.go can be moved to layered_image_handler.go as one or more functions. Then you call those functions from openshift_controller.go.

With this approach, you could also avoid the new variables ImgMc and ExistingMc.

For example, let's suppose you have a getImageMc function that returns the image MachineConfig when the feature is enabled. You could call getImageMc from newMCForCR. If that returns nil, you continue the usual flow, extension-based. If getImageMc returns a MachineConfig, you apply that.

Can this work?

@bpradipt
Copy link
Contributor Author

bpradipt commented Jun 27, 2024 •
edited
Loading

Avoid growing the file openshift_controller.go. The code that you added to openshift_controller.go can be moved to layered_image_handler.go as one or more functions. Then you call those functions from openshift_controller.go.

With this approach, you could also avoid the new variables ImgMc and ExistingMc.

For example, let's suppose you have a getImageMc function that returns the image MachineConfig when the feature is enabled. You could call getImageMc from newMCForCR. If that returns nil, you continue the usual flow, extension-based. If getImageMc returns a MachineConfig, you apply that.

Can this work?

Thanks for your suggestion @spotlesstofu . I'll spend some time to figure out how to incorporate the changes and get back.

@bpradipt
Copy link
Contributor Author

@spotlesstofu I have incorporated your suggestion and added it as a separate commit.

If the approach looks good, I'll squash the new commit with the first one.

spotlesstofu
spotlesstofu reviewed Jun 27, 2024
View reviewed changes
@bpradipt bpradipt force-pushed the layered-image-new branch 3 times, most recently from 2ea138a to 6ab6873 Compare July 4, 2024 10:47
@bpradipt
Support Kata installation using RHCOS layered image
e339996 
The functionality is exposed via feature gate.

The feature gate is named as `layeredImageDeployment` and is managed via
the `osc-feature-gates` ConfigMap. The actual MachineConfig specifying
the image details is stored in `layered-image-deploy-cm` ConfigMap.

Layered Image Deployment workflow:

- Admin creates a ConfigMap named `layered-image-deploy-cm` with `osImageURL` and `kernelArguments` keys.
  Note that `osImageURL` is mandatory and `kernelArguments` is optional

  Example:

	```sh
	apiVersion: v1
	data:
	  osImageURL: "quay.io/openshift_sandboxed_containers/kata-ocp415:tdx"
	  kernelArguments: "kvm_intel.tdx=1"

	kind: ConfigMap
	metadata:
	  name: layered-image-deploy-cm
	  namespace: openshift-sandboxed-containers-operator

	```

- The feature gate handling code in the OSC operator reads this ConfigMap, extracts the keys, and
  creates the MachineConfig object to kickstart the installation.

- This feature is tied to KataConfig creation/deletion. So any changes
  to this feature gate config post KataConfig creation will be a no-op.
  IOW, the installation method - whether using extension (default) or layered
  image cannot be changed post KataConfig creation.
  For changing the install method, the KataConfig needs to be deleted
  and recreated.
  Since installation and uninstallation are time consuming operation,
  this restriction is in place.

- The code limits the feature gate processing into a dedicated code
  separate from the general controller flow and publishes the results of
  the processing by storing them in the KataConfig reconciler struct,
  for the general controller code to use.

Also rename createExtensionMc to createMc since the method
is now handling both extension and image based MC.

Signed-off-by: Pradipta Banerjee <[email protected]>
@bpradipt bpradipt force-pushed the layered-image-new branch from 6ab6873 to e339996 Compare July 5, 2024 05:16
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Jul 5, 2024

@bpradipt: The following tests 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/check e339996 link false /test check
ci/prow/sandboxed-containers-operator-e2e e339996 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-sigs/prow repository. I understand the commands that are listed here.

@bpradipt bpradipt requested review from snir911 and spotlesstofu July 5, 2024 16:42
snir911
snir911 approved these changes Jul 7, 2024
View reviewed changes
Copy link
Contributor

@snir911 snir911 left a comment

Choose a reason for hiding this comment

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

LGTM overall, thanks.
( I'd still prefer to have fixed image value also if it's FG, however, no a deal breaker according to what you mentioned :) )

@bpradipt bpradipt merged commit a77c2e9 into openshift:devel Jul 8, 2024
2 of 4 checks passed
@bpradipt bpradipt deleted the layered-image-new branch July 8, 2024 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snir911 snir911 snir911 approved these changes

@spotlesstofu spotlesstofu spotlesstofu approved these changes

@pmores pmores Awaiting requested review from pmores

@cpmeadors cpmeadors Awaiting requested review from cpmeadors

@gkurz gkurz Awaiting requested review from gkurz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bpradipt @spotlesstofu @snir911 @pmores