design-proposal: instancetype.kubevirt.io - Expand during submission

[lyarwood]

What this PR does / why we need it:

The initial design of instance types and preferences captured a point in time copy of the original resource for future use by the VirtualMachine to ensure we always generate the same VirtualMachineInstance at runtime.

This additionally allows users to easily switch between classes, sizes and generations of these resources at some point in the future. However this flexibility comes at the cost of complexity within KubeVirt itself especially handling more advanced VirtualMachine lifecycle flows.

This design proposal aims to set out a new cluster configurable to control how instance types and preferences are referenced from a VirtualMachine.

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.

• Design: A design document was considered and is present (link) or not required
• PR: The PR description is expressive enough and will help future contributors
• Code: Write code that humans can understand and Keep it simple
• Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
• Upgrade: Impact of this change on upgrade flows was considered and addressed if required
• Testing: New code requires new unit tests. New features and bug fixes require at least on e2e test
• Documentation: A user-guide update was considered and is present (link) or not required. You want a user-guide update if it's a user facing feature / API change.
• Community: Announcement to kubevirt-dev was considered

Release note:

NONE

[kubevirt-bot]

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

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign davidvossel for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[0xFelix]

It is also no longer possible to change the resource sizing of the VM easily by selecting a different instance type.

[lyarwood]

That's basically suggested here but I'll explicitly call that out.

[vladikr]

/cc

[dankenigsberg]

Not sure I follow: a user interaction took place if the configurable was set to expand.

[lyarwood]

I'll reword to explicitly state that this can happen without the VM owner interacting with the VM thanks.

[dankenigsberg]

I find it recommended for KubeVirt to move the validation logic from the webhook into the controller. Validating VM definition against cluster-level objects such is instance types or preferences is raceful by design and breaks eventual consistency.

But I do not see how this is related to the design proposal. Can you elaborate?

[lyarwood]

I understand but while there are still validations taking place in the webhooks we will need to expand any provided instance type and preference to retain the same behaviour as a flat VirtualMachine. Once these validations are moved into the control loop then yes the need for this is also removed.

I wanted to mention this in passing as it's logic we would still need to carry in the project that isn't removed by eventually expanding later.

[dankenigsberg]

does the following represents what you are saying?

Suggested change

	We would also still need to retain webhook validation logic to ensure that a
	given `VirtualMachine` using an instance type or preference is valid during
	submission.
	Note that this proposal does not affect the validation logic to ensure that a
	given `VirtualMachine` using an instance type or preference is valid during
	submission.

[dankenigsberg]

Isn't adding policy:expand explicit enough? Why removing revisionName is required?

[lyarwood]

The current proposal stops short of expanding all VirtualMachines and would use revisionName to determine if the VirtualMachine had already been seen by the controller and revisions stored. If we went with the alternative of expanding all regardless of a revision already being captured then policy being set to expand would be enough to trigger the behaviour.

[dankenigsberg]

Why not? It is still OK to define instance-typed VM specs, why would it not be ok to modify an existing VM to be instance typed? In properly eventual-consistent systems, the intention of the user is what matters most, not the history of the resource.

[lyarwood]

At present instance types conflict if the VirtualMachine already provides an amount of vCPU or memory.

We have talked about changing this behaviour in #318 and in doing so could allow a user to resize flat VirtualMachines by selecting an instance type but that might require a follow up design proposal and discussion to work out the details.

[dankenigsberg]

I do not follow... Assume that I created a flat VM with 7 CPUs. I stopped my VM and then changed my mind. I would like it to be much larger and more standard and edit the VM to use cx1.8xlarge. Would KubeVirt attempt to block this? Why?

[lyarwood]

At present yes it would unless you also removed the 7 vCPUs you originally requested within the VirtualMachine.

We just don't allow you to request vCPUs and memory through the VirtualMachineInstanceSpec and instance type at the same time.

[dankenigsberg]

At present yes it would unless you also removed the 7 vCPUs you originally requested within the VirtualMachine.

We just don't allow you to request vCPUs and memory through the VirtualMachineInstanceSpec and instance type at the same time.

this is clear and expected. I just don't understand it from "Users will also be unable to resize their VirtualMachines by making a singular choice of instance type in the future". Would you agree to rephrase the text?

[lyarwood]

Ah apologies I see your point, I'll rephrase now.

[dankenigsberg]

Can we consider alternatives where this is not available? My question stems form me not being aware of the second user story (a VM owner wanting to control this). I heard only of cluster admins.

[lyarwood]

I can capture that in alternatives but personally I think that VM Owners should have the ability to expand or not regardless of the admins choice.

[dankenigsberg]

Thanks. But if I understand correctly, the biggest issue today is the complexity for third parties. If each VM owner can choose whatever they want, the complexity remains.

I would prefer have an opinionated solution, with less knobs and options; if we see a use case for the VM owners having this freedom to work against their cluster admin, we can add per-VM knob later. Removing knobs is practically impossible.

[lyarwood]

That complexity will remain still even if this configurable is limited to cluster admins surely, especially given that we are not changing the default here.

Making this a cluster admin only configurable will mean that the end user experience of using a VirtualMachine initially defined with instance types and preferences can change drastically between environments and there's nothing a user can do to avoid this.

By providing an additional VM owner configurable they can be explicit and know that the resulting VirtualMachine will behave as expected.

[dankenigsberg]

Users will also be unable to resize their VirtualMachines by making a singular
choice of instance type in the future

Why? A cluster admin that would like to interact with a third party can configure expand on her cluster, and that's that. If we complicate the solution and let the VM owner choose his own semantic, the cluster admin is left with uncertainty.

Making this a cluster admin only configurable will mean that the end user experience of using a VirtualMachine initially defined with instance types and preferences can change drastically between environments and there's nothing a user can do to avoid this.

Correct. And that's exactly what I, as a cluster admin, want. I need to interact with a third party that is unaware of instance types, so I want my cluster to comply.

By providing an additional VM owner configurable they can be explicit and know that the resulting VirtualMachine will behave as expected.

Right, but this adds complexity to the code, which I am not aware was requested by any user, increases development time, expands test matrix and add possibility of bugs, and cannot be dropped out of the API in the future.

[lyarwood]

Okay these are fair points, I'll remove the user configurables for now and we can always follow up in the future if requested.

[dankenigsberg]

Thanks. But if I understand correctly, the biggest issue today is the complexity for third parties. If each VM owner can choose whatever they want, the complexity remains.

I would prefer have an opinionated solution, with less knobs and options; if we see a use case for the VM owners having this freedom to work against their cluster admin, we can add per-VM knob later. Removing knobs is practically impossible.

[dankenigsberg]

does the following represents what you are saying?

Suggested change

	We would also still need to retain webhook validation logic to ensure that a
	given `VirtualMachine` using an instance type or preference is valid during
	submission.
	Note that this proposal does not affect the validation logic to ensure that a
	given `VirtualMachine` using an instance type or preference is valid during
	submission.

[dankenigsberg]

I do not follow... Assume that I created a flat VM with 7 CPUs. I stopped my VM and then changed my mind. I would like it to be much larger and more standard and edit the VM to use cx1.8xlarge. Would KubeVirt attempt to block this? Why?

[lyarwood]

It does help if I push to the correct branch when attempting to update a PR, apologies!

[dankenigsberg]

I find this text too vague. It is not clear here what the cluster admin would like to control and how. Can we return to something more similar to

• As a cluster owner I want to configure the expansion of all
  VirtualMachines referencing instance types and preferences

?

[lyarwood]

The suggested configurable still allows a VirtualMachine to reference an instance type and preference so to me it doesn't make sense to focus on expansion here.

[dankenigsberg]

I am not sure why we need the new expand option. The only use case I am familiar with is cluster-admins saying "instance types are too complex for me and the third parties I'd like to interact with my cluster; please expand all VMs in my cluster".

Adding more options typically means more development time, more tests, more docs, more bugs, more complexity. It would be quite possible to add this option any time in the future when it is needed. It is practically impossible to remove the option if we add it now. Hence I'd prefer not to have this functionality now.

Now I may well be wrong; Is there a class of cluster admins that need the differentiation between expand and expandAll? Can you elaborate on the goal they would like to achieve by the expand semantics?

[lyarwood]

Apologies for not coming back to this ahead of the feature landing in main but we were up against FF and have ended up placing this behind a FG thus allowing us to add and remove policies in the future.

I am not sure why we need the new expand option. The only use case I am familiar with is cluster-admins saying "instance types are too complex for me and the third parties I'd like to interact with my cluster; please expand all VMs in my cluster".

That use case being put forward by stakeholders rather than actual users FWIW. I also believe end users will want the ability to leave existing VMs alone but expand new VMs thus the additional policy here.

Adding more options typically means more development time, more tests, more docs, more bugs, more complexity. It would be quite possible to add this option any time in the future when it is needed. It is practically impossible to remove the option if we add it now. Hence I'd prefer not to have this functionality now.

Now that this entire feature is behind a FG we have some room to deprecate and remove policies moving forward. Given this policy also shares a common implementation with expandAll with only the trigger being different I wanted to include it in the initial PR.

Now I may well be wrong; Is there a class of cluster admins that need the differentiation between expand and expandAll? Can you elaborate on the goal they would like to achieve by the expand semantics?

I believe so but having not heard from any end users about any of this functionality thus far I'm obviously guessing. As I've said above the goal of the policy is to leave existing VMs alone.