VEP144: graduate reservedOverhead to beta

[bgartzi]

VEP Metadata

Tracking issue: #144
SIG label: /sig compute

What this PR does

• Require functional testing as a requirement to graduate ReservedOverheadMemlock to beta.
• Set target version for ReservedOverheadMemlock beta graduation to v1.9

Special notes for your reviewer

Might not be of good etiquette but moved documentation and functional testing requirements out from Alpha requirements in the VEP md, which were not met and moved them into other graduation targets.

[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 xpivarc for approval. For more information see the Code Review Process.

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

Details

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

[bgartzi]

Using this VEP graduation PR as the bait, I would like to take the chance to have a little discussion about the original API design for ReservedOverheadMemlock before we move on.

One of the use-cases that we talked about during the initial design phase was using this field as a target for mutating admission webhooks that would add overhead/memlock requirements to this field based on their needs.

We tested a mutating admission webhook implementation that would track and modify that field on VMI CREATE. In case reservedOverhead already has a value, it's assumed that the user added it so add "our value" to that. However, we're having some problems to figure out how it should behave in front of UPDATE actions. It could be a trivial action, such as the user hotplugging another interface, for example. We need to increase the limit. But, what if the user updates both the reservedOverhead value and other related fields at the same time? It's difficult to know from a webhook perspective whether the new VMI spec's reservedOverhead values were taken from the spec that was already modified by the mutating webhook, or from the original spec that was modified in the user's local environment and sent as an update request.

I'm flagging this here not to beg for hints (which I would appreciate in a separate discussion thread) but to flag it as a possible initial design flaw that I would like to discuss with you.

cc: @iholder101, @vladikr, @pprabhu3

[vladikr]

Using this VEP graduation PR as the bait, I would like to take the chance to have a little discussion about the original API design for ReservedOverheadMemlock before we move on.

One of the use-cases that we talked about during the initial design phase was using this field as a target for mutating admission webhooks that would add overhead/memlock requirements to this field based on their needs.

We tested a mutating admission webhook implementation that would track and modify that field on VMI CREATE. In case reservedOverhead already has a value, it's assumed that the user added it so add "our value" to that. However, we're having some problems to figure out how it should behave in front of UPDATE actions. It could be a trivial action, such as the user hotplugging another interface, for example. We need to increase the limit. But, what if the user updates both the reservedOverhead value and other related fields at the same time? It's difficult to know from a webhook perspective whether the new VMI spec's reservedOverhead values were taken from the spec that was already modified by the mutating webhook, or from the original spec that was modified in the user's local environment and sent as an update request.

I'm flagging this here not to beg for hints (which I would appreciate in a separate discussion thread) but to flag it as a possible initial design flaw that I would like to discuss with you.

cc: @iholder101, @vladikr, @pprabhu3

Thank you @bgartzi , but I'm curious, why is UPDATE needed in this case? What is the use case?

[bgartzi]

Thank you @bgartzi , but I'm curious, why is UPDATE needed in this case? What is the use case?

iface hot-(un)plug. If I understood correctly currently is not supported for network binding plugins, but could be interesting for other cases I guess?

[vladikr]

Thank you @bgartzi , but I'm curious, why is UPDATE needed in this case? What is the use case?

iface hot-(un)plug. If I understood correctly currently is not supported for network binding plugins, but could be interesting for other cases I guess?

@bgartzi I don't have a clear picture of how that would work. However, I think that a specific webhook or a Mutation Admission Policy would need to know that something it cares/responsible for has changed and would need to "add" the additional overhead. The main point is that the webhook wouldn't react to an arbitrary change but only track a specific resource.

[iholder101]

Thank you @bgartzi , but I'm curious, why is UPDATE needed in this case? What is the use case?

iface hot-(un)plug. If I understood correctly currently is not supported for network binding plugins, but could be interesting for other cases I guess?

@bgartzi I don't have a clear picture of how that would work. However, I think that a specific webhook or a Mutation Admission Policy would need to know that something it cares/responsible for has changed and would need to "add" the additional overhead. The main point is that the webhook wouldn't react to an arbitrary change but only track a specific resource.

I tend to agree to this line of thinking.

Mutating policies/webhooks run serially, and I think in this case they shouldn't care about what the others do. It is possible to create a mutating policy/webhook for each managed device, each one will only query the device it's responsible for. If there's a case where it's important to know which webhook updated the value and by how much, this can be done via helper annotations (although I'm not sure I see the use-case for that).

As a side note - if more logic is needed (to modify the domain XML / perform node-level operations) - it the mutating webhook/policy can be wrapped into a plugin.

@bgartzi what do you think?

[enp0s3]

Thank you @bgartzi , but I'm curious, why is UPDATE needed in this case? What is the use case?

iface hot-(un)plug. If I understood correctly currently is not supported for network binding plugins, but could be interesting for other cases I guess?

@bgartzi I don't have a clear picture of how that would work. However, I think that a specific webhook or a Mutation Admission Policy would need to know that something it cares/responsible for has changed and would need to "add" the additional overhead. The main point is that the webhook wouldn't react to an arbitrary change but only track a specific resource.

I tend to agree to this line of thinking.

Mutating policies/webhooks run serially, and I think in this case they shouldn't care about what the others do. It is possible to create a mutating policy/webhook for each managed device, each one will only query the device it's responsible for. If there's a case where it's important to know which webhook updated the value and by how much, this can be done via helper annotations (although I'm not sure I see the use-case for that).

As a side note - if more logic is needed (to modify the domain XML / perform node-level operations) - it the mutating webhook/policy can be wrapped into a plugin.

@bgartzi what do you think?

@iholder101 Hi, I think that the webhook approach is too risky here. I prefer to go with a plugin-based design so that the final calculation could be orchestrated by the handler. I think webhooks are risky in case we have few of them operating on the same field. There is no way to enforce proper implementation of the webhook.

[EdDev]

But, what if the user updates both the reservedOverhead value and other related fields at the same time? It's difficult to know from a webhook perspective whether the new VMI spec's reservedOverhead values were taken from the spec that was already modified by the mutating webhook, or from the original spec that was modified in the user's local environment and sent as an update request.

The webhook or any hook point in general should observe the state and operate in an idempotent manner. It should not care about changes and imperative operations.
This is how we expect the domain xml hook point that the network binding plugin is using to behave, i.e. the vendor should not assume it is called once or twice, it should be assumed to be called multiple times for different reasons.

If you depend on the change sequence, there is something wrong with the architecture.

[bgartzi]

Hi, I'm replying everything in a single message.

@vladikr

I think that a specific webhook or a Mutation Admission Policy would need to know that something it cares/responsible for has changed and would need to "add" the additional overhead. The main point is that the webhook wouldn't react to an arbitrary change but only track a specific resource.

Yes, when a new VMI is created is pretty simple. The webhook scans for things that it is interested in (e.g. devices) and adds to the additional overhead. 1 device? 1Gi. 2? 2Gi, whatever.

My concern is about later in the VMI lifecycle. What if the user removes one of those devices? We had 2, now we have 1 so we need 1Gi. But how do we know if the user used the original spec for k apply -f or if they did k get vm withReservedOverhead + k apply -f. In other words, how does the webhook know if in this case, an UPDATE action in terms of the mutating admission webhook, needs to still add or remove 1Gi from the incoming spec?

@iholder101

Mutating policies/webhooks run serially, and I think in this case they shouldn't care about what the others do. It is possible to create a mutating policy/webhook for each managed device, each one will only query the device it's responsible for.

Yes, when a new VMI is defined, there's no problem. Each mutating webhook assumes that there could be others and limits itself to creating the field if it wasn't there, or adding some value to it.

It is during a spec update (device hot unplug, for example) that we don't know if the value that the user sent was meant to be the result of the sum of all mutating admission webhooks, if it is some value they came up for any other use-case they needed, or if it is the original vmi spec in which the additionalOverhead value was possibly empty.

If there's a case where it's important to know which webhook updated the value and by how much, this can be done via helper annotations (although I'm not sure I see the use-case for that).

It would be helpful if all interactions to the field were "recorded", even user's, which makes it harder to use IMO as they would need to track "the same thing" twice.

@enp0s3

Hi, I think that the webhook approach is too risky here. I prefer to go with a plugin-based design so that the final calculation could be orchestrated by the handler.

That can be a way to make each webhook/plugin agnostic of the rest, I believe. Instead of operating on an overall value, in which webhooks do not know whether they need to add/remove, you have an array of plugins that declare what they need, which virt-handler limits to sum and apply in the end. This also makes it "hidden" so users can't retrieve the result and feed it back in a spec update, making the amounts double.

Another thing I had in mind was making the field somewhat like a dictionary, in which different use-cases could add their 'definition': value key-value pairs, which the virt-handler would sum and apply. However, these values are explicit to the rest, which could lead to bad webhook implementations that remove the existing ones or something like that.

@EdDev

The webhook or any hook point in general should observe the state and operate in an idempotent manner. It should not care about changes and imperative operations.

It is pretty much what we can do on VMI creation, as we know that the mutating webhook will be called just once for the default reinvocationPolicy value.

If it wasn't for that, we wouldn't know if the incoming value was set by the user, another webhook, or the webhook in a previous call to it.

If you depend on the change sequence, there is something wrong with the architecture.

Which it is pretty much what I was trying to call out in this discussion. Even if the use-case I had in mind for updates was interface hot-(un)plug, which is not supported for network binding plugins afaict, this seemed like something that needed to be pointed out "early" in alpha, rather than later, when this was available generally and thus harder to get rid of.

[iholder101]

@bgartzi so if I understand your line of thinking correctly, the main concern here is that the webhook doesn't know who set and current value of the overhead and why, i.e. if it was the user (assessing by himself how much overhead is needed) or the previous webhook call when the VMI was created. Is that correct?

My general opinion here is that KubeVirt should not enforce how exactly people will be using this mechanism, but will provide the necessary facility in order to achieve it, in multiple possible ways. It's fine if some users will delegate the overhead calculation to the VM owner, while others will decide that only webhooks will use this value to update it automatically. Some may even choose to prevent (via VAP) users from setting this value explicitly altogether. It's even possible, as said earlier, to "document" which webhooks did what via annotations, although I tend to agree with @EdDev that this might imply that there's something wrong with the architecture. Either way, this is a matter of how every user will decide the architecture for their environment.

WDYT?

[bgartzi]

@iholder101

so if I understand your line of thinking correctly, the main concern here is that the webhook doesn't know who set and current value of the overhead and why, i.e. if it was the user (assessing by himself how much overhead is needed) or the previous webhook call when the VMI was created. Is that correct?

Pretty much, because we'd need to know which part of the result is owned by "us" and ensure that total is indeed the sum of all pieces (so no component is left behind).

WDYT?

As long as everyone is aware of the implications of this, I'm fine. AFAICT, in-place reservedOverhead updates are not supported at the moment, as memlock limits are set on creation and VM migration. If some sort of migration-based hot (un)plug mechanism would rely on reservedOverhead we would need to tell users to be careful with how they set up the addedOverhead calculation mechanism.

This is a good hint of the limitations of the current API proposal in my opinion, which is worth it to at least document I believe.

Does this concern anyone else more than that?