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

Enables using scripts to modify VMI definition #10479

Merged
merged 7 commits into from Oct 19, 2023

Conversation

dharmit
Copy link
Contributor

@dharmit dharmit commented Sep 26, 2023

What this PR does / why we need it:
Presently, users need to write a Go program if they want to use the hook sidecar functionality to modify VMI's XML definition before its creation. This PR aims to let the users write a shell/python script for the same.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Release note:

Ability to run scripts through hook sidecar

@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L labels Sep 26, 2023
@kubevirt-bot
Copy link
Contributor

Hi @dharmit. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@tiraboschi
Copy link
Member

/ok-to-test

@kubevirt-bot kubevirt-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 26, 2023
@dharmit dharmit force-pushed the fix-cnv-31062 branch 2 times, most recently from c48a58a to 2a6d6f2 Compare September 29, 2023 12:33
@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 29, 2023
@kubevirt-bot kubevirt-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 1, 2023
@dharmit dharmit marked this pull request as ready for review October 2, 2023 12:15
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 2, 2023
@dharmit
Copy link
Contributor Author

dharmit commented Oct 2, 2023

@victortoso since you have worked extensively on sidecar, can you please provide your feedback on this PR? Perhaps the README.md at cmd/example-hook-generic/README.md would help understand what I'm trying to do here.

@victortoso
Copy link
Member

Hi @dharmit, I'll be happy to but I can't Today. Tomorrow or the day after at latest ;)
/assign

@dharmit
Copy link
Contributor Author

dharmit commented Oct 3, 2023

The job "pull-kubevirt-generate" fails with following log:

./hack/verify-generate.sh
ERROR: git tree state is not clean!
You probably need to run 'make generate' or 'make' and commit the changes
On branch main
Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
	deleted:    examples/vmi-with-generic-hook.yaml
no changes added to commit (use "git add" and/or "git commit -a") 

But we need the examples/vmi-with-generic-hook.yaml file. If I delete it, I think, the tests will pass, but I need it to show a working example of what's done in this PR.

How do I ensure it's not a problem for make generate?

EDIT: Fixed this in 89e2c20.

@victortoso
Copy link
Member

Hi, I just started looking at this, sorry the delay.

Presently, users need to write a Go program if they want to use the hook sidecar functionality to modify VMI's XML definition before its creation. This PR aims to let the users write a shell/python script for the same.

Have you tried the sidecar-shim image. The sidecar-shim binary looks for an executable in $PATH for the Hook you want to implement (e.g: onDefineDomain), check the README. I honestly haven't tested a shell/python script but I think it can be done.

I'll keep looking into the patch series to follow the ConfigMap introduction/usage

Copy link
Member

@victortoso victortoso left a comment

Choose a reason for hiding this comment

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

I like the idea. I consider myself new to kubevirt/k8s design so it would be great to have second opinion on using ConfigMap to store scripts and creating the volumes, etc.

I'd test with sidecar-shim container image and if possible use it here. There are a few reasons for this suggestion:

  1. Reduce boilerplate with gRPC
  2. We can add new APIs in the hooks, implement it on the sidecar-shim and should benefit this or other sidecars that use sidecar-shim. For example, this attempt to fix second migration on VMI with sidecar.
  3. It already does similar logic, like executing command on $PATH. The difference is that the command has to be the hook name.

ConfigMap: &k8sv1.ConfigMapVolumeSource{
LocalObjectReference: k8sv1.LocalObjectReference{Name: cm.Name},
DefaultMode: pointer.Int32(0777),
},
Copy link
Member

Choose a reason for hiding this comment

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

This is a very interesting approach. It means that with a single container-image we could deploy different scripts.

BUILD.bazel Outdated
container_push(
name = "push-example-hook-generic",
format = "Docker",
image = "//cmd/example-hook-generic:example-hook-generic-image",
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest moving it under the cmd/sidecars folder. I'm on the process of doing that, just waiting it to be unhold, see: #10320

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2023
creates a new type ConfigMap in hooks.go and adds necessary logic in
template.go to populate volume

Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>
Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>
Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>
Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>
Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>
Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>
@kubevirt-bot kubevirt-bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 19, 2023
@dharmit dharmit force-pushed the fix-cnv-31062 branch 2 times, most recently from eed1566 to 86a47df Compare October 19, 2023 06:52
@alicefr
Copy link
Member

alicefr commented Oct 19, 2023

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2023
Signed-off-by: Dharmit Shah <shahdharmit@gmail.com>
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2023
@alicefr
Copy link
Member

alicefr commented Oct 19, 2023

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2023
@kubevirt-bot
Copy link
Contributor

kubevirt-bot commented Oct 19, 2023

@dharmit: 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
pull-kubevirt-verify-rpms 8acea18 link false /test pull-kubevirt-verify-rpms
pull-kubevirt-e2e-k8s-1.26-sig-compute cb461a2 link unknown /test pull-kubevirt-e2e-k8s-1.26-sig-compute
pull-kubevirt-e2e-k8s-1.27-sig-compute cb461a2 link unknown /test pull-kubevirt-e2e-k8s-1.27-sig-compute

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.

@kubevirt-bot kubevirt-bot merged commit 03568c2 into kubevirt:main Oct 19, 2023
35 of 37 checks passed
@victortoso victortoso mentioned this pull request Oct 20, 2023
8 tasks
@dharmit dharmit deleted the fix-cnv-31062 branch October 23, 2023 10:46
dharmit pushed a commit to dharmit/kubevirt that referenced this pull request Oct 23, 2023
Enables using scripts to modify VMI definition
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants