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

✨ Add vm.spec.cdrom API and initial reconcile logic #594

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

dilyar85
Copy link
Member

@dilyar85 dilyar85 commented Jun 24, 2024

What does this PR do, and why is it needed?

This PR introduces a new vm.spec.cdrom field and the initial reconciliation logic based on the VM power state change:

  • If VM is about to power on, it adds/removes/updates/ CD-ROM devices with the expected backing (from ISO-type Virtual Machine Image) and connection state.
  • If VM is already powered on, it only updates the existing CD-ROM device connection state if changed.

Which issue(s) is/are addressed by this PR?

Fixes N/A.

Are there any special notes for your reviewer:

This PR only focuses on the API and reconciliation logic regarding CD-ROM device changes.
Follow-up PRs will be submitted to address necessary VM webhook and documentation updates.

Please add a release note if necessary:

Add vm.spec.cdrom API and initial reconcile logic to support VM deployment from ISO.

Testing Done

  • Added cdrom_test.go file to test the new cdrom.go file with 100% coverage:
$ go test -coverprofile=/tmp/coverage.out -run ^TestVSphereVirtualMachine$ github.com/vmware-tanzu/vm-operator/pkg/util/vsphere/vm
ok  	github.com/vmware-tanzu/vm-operator/pkg/util/vsphere/vm	77.980s	coverage: 86.0% of statements

$ go tool cover -func=/tmp/coverage.out | grep pkg/util/vsphere/vm/cdrom.go
github.com/vmware-tanzu/vm-operator/pkg/util/vsphere/vm/cdrom.go:31:		UpdateCdromDeviceChanges		100.0%
github.com/vmware-tanzu/vm-operator/pkg/util/vsphere/vm/cdrom.go:105:		UpdateConfigSpecCdromDeviceConnection	100.0%
github.com/vmware-tanzu/vm-operator/pkg/util/vsphere/vm/cdrom.go:147:		getCdromAndBackingByImgRef		100.0%
github.com/vmware-tanzu/vm-operator/pkg/util/vsphere/vm/cdrom.go:176:		createNewCdrom				100.0%
github.com/vmware-tanzu/vm-operator/pkg/util/vsphere/vm/cdrom.go:202:		ensureAllCdromsHaveControllers		100.0%
github.com/vmware-tanzu/vm-operator/pkg/util/vsphere/vm/cdrom.go:232:		addNewAHCIController			100.0%
github.com/vmware-tanzu/vm-operator/pkg/util/vsphere/vm/cdrom.go:285:		getIsoFilenameFromImageRef		100.0%
github.com/vmware-tanzu/vm-operator/pkg/util/vsphere/vm/cdrom.go:349:		updateCurCdromsConnectionState		100.0%
  • Updated the existing session_vm_update_test.go when ISO FSS is enabled
  • Deployed the change to an internal testbed and verified adding/removing/updating CD-ROM devices against a VM Service VM

πŸ“š Documentation preview πŸ“š: https://vm-operator--594.org.readthedocs.build/en/594/

@github-actions github-actions bot added the size/L Denotes a PR that changes 100-499 lines. label Jun 24, 2024
@dilyar85 dilyar85 force-pushed the feature/add-cd-rom branch 3 times, most recently from 6c46cb5 to bf02bdf Compare June 24, 2024 23:41
@dilyar85 dilyar85 marked this pull request as draft June 25, 2024 02:14
Copy link
Collaborator

@akutz akutz left a comment

Choose a reason for hiding this comment

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

Great work @dilyar85! Please see the requested changes.

api/v1alpha3/virtualmachine_types.go Outdated Show resolved Hide resolved
api/v1alpha3/virtualmachine_types.go Outdated Show resolved Hide resolved
api/v1alpha3/virtualmachine_types.go Outdated Show resolved Hide resolved
pkg/util/vsphere/vm/cdrom.go Show resolved Hide resolved
pkg/util/vsphere/vm/cdrom.go Outdated Show resolved Hide resolved
pkg/util/vsphere/vm/cdrom.go Outdated Show resolved Hide resolved
@github-actions github-actions bot added size/XL Denotes a PR that changes 500-999 lines. and removed size/L Denotes a PR that changes 100-499 lines. labels Jun 27, 2024
@github-actions github-actions bot added size/XXL Denotes a PR that changes 1000+ lines. and removed size/XL Denotes a PR that changes 500-999 lines. labels Jul 2, 2024
@dilyar85 dilyar85 marked this pull request as ready for review July 2, 2024 18:00
@dilyar85 dilyar85 force-pushed the feature/add-cd-rom branch 3 times, most recently from d6bf977 to 924ee88 Compare July 2, 2024 18:28
Copy link
Contributor

@dougm dougm left a comment

Choose a reason for hiding this comment

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

lgtm!

@dilyar85
Copy link
Member Author

dilyar85 commented Jul 2, 2024

Submitted #611 to address the ci/vulncheck-go check failure.

@dilyar85 dilyar85 force-pushed the feature/add-cd-rom branch 2 times, most recently from c42de1c to 3b0e2d7 Compare July 3, 2024 14:20
@dilyar85 dilyar85 force-pushed the feature/add-cd-rom branch 4 times, most recently from fbc095c to 8547995 Compare July 8, 2024 15:12
@dilyar85 dilyar85 requested a review from akutz July 8, 2024 15:35
api/v1alpha3/virtualmachine_types.go Outdated Show resolved Hide resolved
api/v1alpha3/virtualmachine_types.go Outdated Show resolved Hide resolved
api/v1alpha3/virtualmachine_types.go Outdated Show resolved Hide resolved
api/v1alpha3/virtualmachine_types.go Outdated Show resolved Hide resolved
api/v1alpha3/virtualmachine_types.go Outdated Show resolved Hide resolved
pkg/util/vsphere/vm/cdrom.go Outdated Show resolved Hide resolved
pkg/util/vsphere/vm/cdrom.go Show resolved Hide resolved
pkg/util/vsphere/vm/cdrom.go Show resolved Hide resolved
pkg/util/vsphere/vm/cdrom.go Show resolved Hide resolved
pkg/util/vsphere/vm/cdrom.go Show resolved Hide resolved
Copy link
Collaborator

@akutz akutz left a comment

Choose a reason for hiding this comment

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

Thank you @dilyar85, this is really good, but I did have a few thoughts.

Copy link
Contributor

@aruneshpa aruneshpa left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the update.

api/v1alpha3/virtualmachine_types.go Outdated Show resolved Hide resolved
@dilyar85 dilyar85 force-pushed the feature/add-cd-rom branch 2 times, most recently from c53d88f to c9a1acb Compare July 9, 2024 21:42
@dilyar85 dilyar85 requested a review from akutz July 9, 2024 21:50
This patch introduces a new `vm.spec.cdrom` field and the reconciliation logic based on the VM power state change:

- If VM is about to power on, it adds/removes/updates/ CD-ROM devices with the expected backing (from ISO-type Virtual Machine Image) and connection state.
- If VM is already powered on, it only updates the existing CD-ROM device connection state if changed.
@dilyar85
Copy link
Member Author

dilyar85 commented Jul 9, 2024

To wrap up the above conversations about the "connection state" in the CD-ROM spec, the team has concluded to name that API field connected with a default true value. The PR has been updated to reflect this change.

Copy link

github-actions bot commented Jul 9, 2024

Code Coverage

Package Line Rate Health
github.com/vmware-tanzu/vm-operator/controllers/contentlibrary/clustercontentlibraryitem 81% βž–
github.com/vmware-tanzu/vm-operator/controllers/contentlibrary/contentlibraryitem 85% βž–
github.com/vmware-tanzu/vm-operator/controllers/contentlibrary/utils 97% βœ”
github.com/vmware-tanzu/vm-operator/controllers/infra/capability 86% βž–
github.com/vmware-tanzu/vm-operator/controllers/infra/configmap 73% ❌
github.com/vmware-tanzu/vm-operator/controllers/infra/node 77% ❌
github.com/vmware-tanzu/vm-operator/controllers/infra/secret 77% ❌
github.com/vmware-tanzu/vm-operator/controllers/util/encoding 73% ❌
github.com/vmware-tanzu/vm-operator/controllers/virtualmachine 87% βž–
github.com/vmware-tanzu/vm-operator/controllers/virtualmachineclass 75% ❌
github.com/vmware-tanzu/vm-operator/controllers/virtualmachinepublishrequest 81% βž–
github.com/vmware-tanzu/vm-operator/controllers/virtualmachinereplicaset 67% ❌
github.com/vmware-tanzu/vm-operator/controllers/virtualmachineservice 83% βž–
github.com/vmware-tanzu/vm-operator/controllers/virtualmachineservice/providers 92% βœ”
github.com/vmware-tanzu/vm-operator/controllers/virtualmachinesetresourcepolicy 80% βž–
github.com/vmware-tanzu/vm-operator/controllers/virtualmachinewebconsolerequest/v1alpha1 72% ❌
github.com/vmware-tanzu/vm-operator/controllers/virtualmachinewebconsolerequest/v1alpha1/conditions 88% βž–
github.com/vmware-tanzu/vm-operator/controllers/virtualmachinewebconsolerequest/v1alpha1/patch 78% ❌
github.com/vmware-tanzu/vm-operator/controllers/virtualmachinewebconsolerequest/v1alpha2 73% ❌
github.com/vmware-tanzu/vm-operator/controllers/volume 85% βž–
github.com/vmware-tanzu/vm-operator/pkg/builder 95% βœ”
github.com/vmware-tanzu/vm-operator/pkg/conditions 88% βž–
github.com/vmware-tanzu/vm-operator/pkg/config 100% βœ”
github.com/vmware-tanzu/vm-operator/pkg/config/env 100% βœ”
github.com/vmware-tanzu/vm-operator/pkg/patch 78% ❌
github.com/vmware-tanzu/vm-operator/pkg/prober 91% βœ”
github.com/vmware-tanzu/vm-operator/pkg/prober/probe 91% βœ”
github.com/vmware-tanzu/vm-operator/pkg/prober/worker 77% ❌
github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere 75% ❌
github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/client 80% βž–
github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/clustermodules 71% ❌
github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/config 72% ❌
github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/contentlibrary 72% ❌
github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/credentials 100% βœ”
github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/network 80% βž–
github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/placement 77% ❌
github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/session 74% ❌
github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/sysprep 100% βœ”
github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/vcenter 82% βž–
github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/virtualmachine 74% ❌
github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/vmlifecycle 67% ❌
github.com/vmware-tanzu/vm-operator/pkg/record 78% ❌
github.com/vmware-tanzu/vm-operator/pkg/topology 92% βœ”
github.com/vmware-tanzu/vm-operator/pkg/util 87% βž–
github.com/vmware-tanzu/vm-operator/pkg/util/cloudinit 89% βœ”
github.com/vmware-tanzu/vm-operator/pkg/util/cloudinit/validate 91% βœ”
github.com/vmware-tanzu/vm-operator/pkg/util/image 100% βœ”
github.com/vmware-tanzu/vm-operator/pkg/util/kube 81% βž–
github.com/vmware-tanzu/vm-operator/pkg/util/ptr 100% βœ”
github.com/vmware-tanzu/vm-operator/pkg/util/resize 99% βœ”
github.com/vmware-tanzu/vm-operator/pkg/util/vmopv1 90% βœ”
github.com/vmware-tanzu/vm-operator/pkg/util/vsphere/client 68% ❌
github.com/vmware-tanzu/vm-operator/pkg/util/vsphere/vm 86% βž–
github.com/vmware-tanzu/vm-operator/pkg/webconsolevalidation 100% βœ”
github.com/vmware-tanzu/vm-operator/webhooks/common 100% βœ”
github.com/vmware-tanzu/vm-operator/webhooks/persistentvolumeclaim/validation 95% βœ”
github.com/vmware-tanzu/vm-operator/webhooks/virtualmachine/mutation 85% βž–
github.com/vmware-tanzu/vm-operator/webhooks/virtualmachine/validation 95% βœ”
github.com/vmware-tanzu/vm-operator/webhooks/virtualmachineclass/mutation 62% ❌
github.com/vmware-tanzu/vm-operator/webhooks/virtualmachineclass/validation 89% βž–
github.com/vmware-tanzu/vm-operator/webhooks/virtualmachinepublishrequest/validation 92% βœ”
github.com/vmware-tanzu/vm-operator/webhooks/virtualmachinereplicaset/validation 90% βœ”
github.com/vmware-tanzu/vm-operator/webhooks/virtualmachineservice/mutation 67% ❌
github.com/vmware-tanzu/vm-operator/webhooks/virtualmachineservice/validation 92% βœ”
github.com/vmware-tanzu/vm-operator/webhooks/virtualmachinesetresourcepolicy/validation 89% βœ”
github.com/vmware-tanzu/vm-operator/webhooks/virtualmachinewebconsolerequest/v1alpha1/validation 92% βœ”
github.com/vmware-tanzu/vm-operator/webhooks/virtualmachinewebconsolerequest/v1alpha2/validation 92% βœ”
Summary 81% (8369 / 10283) βž–

Minimum allowed line rate is 79%

Copy link
Collaborator

@akutz akutz left a comment

Choose a reason for hiding this comment

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

Thank you @dilyar85 !!

@dilyar85 dilyar85 merged commit 290e215 into vmware-tanzu:main Jul 10, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-not-required size/XXL Denotes a PR that changes 1000+ lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants