-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
hooks: deprecate v1alpha1 & v1alpha2 #10842
Conversation
I'm all in! However, please send a note on kubevirt mailing list for more visibility |
For the record, just sent an email for awareness: https://groups.google.com/g/kubevirt-dev/c/HUdW66Elfs4 |
/assign |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AFAIK we do not have a deprecation policy in place, hence this will need some discussions. But I think deprecating and removing very old stuff is unavoidable if we want to keep the code maintainable.
rpc OnDefineDomain (OnDefineDomainParams) returns (OnDefineDomainResult) { | ||
option deprecated = true; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand correctly, this will produce a build-time warning if someone uses the call, right? I think it would be nice to have something at runtime as well. Perhaps the VM admission webhook can generate a warning if hooks.kubevirt.io/hookSidecars
annotation is used with --version v1alpha1
.
What do you think about going one step further and deprecating |
No. unfortunately we don't have it. @iholder101 was working on some guidelines and now there is @EdDev's proposal kubevirt/community#251 |
As I see it, there is basically no need to deprecate anything. This API does not fall under the "feature" category, therefore it falls between the cracks a bit. Except marking the version deprecated and anouncing it, what would be the benefit to drop it now? |
@EdDev is a design proposal a bit excessive for graduating an API? I mean, this isn't a new feature, what will you describe there? |
If the feature had a prev proposal, it could have been updated to reflect new suggestion changes and progress. The idea would be to allow discussion at the design and plan level. For example, keeping the feature and API in Alpha stage is problematic. There is no commitment for support and future extensions are considered only through the api version, which is odd.
Fair. |
Right.
Considering all the recent work around sidecars (passt/slirp, debugability), I'm sure we don't want to drop it.
@EdDev what would you suggest for the graduation process? Based on Feature Stages, we should move it to Beta. Should we open a design proposal to discuss it? With regards to the hooks alpha API, it might be alpha but we are using it like it is not. (e.g: it would hurt if remove it) So, in my opinion we can take @0xFelix suggestion, mark both |
If there was a design proposal in the past, we could re-open it and change it to express the planning and new ideas. I am also in favor of targeting towards Beta in the next release.
I agree. A good reason to proceed with Beta and hopefully GA.
I would start with announcing the intention on the googlegroup and the community meeting, add this to the design plan and eventually post a PR for adding warnings. If we move to Beta in the same version, it will send a message that we are serious to stabilize this API. The webhooks are handling the K8S API, not internal API/s. |
Sadly, the community meeting conflicts with another one that I have. I'll try to attend this week. Note that I did send an email to google groups about this deprecation, it is:
I started to draft the proposal, an early version is here: Let's make the graduation of the hooks API happen.
Yes, you are right. The |
45743ef
to
69d1396
Compare
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Updated with the runtime check for deprecation. It is just a warning on virt-launcher log, @vasiliy-ul do you think that would suffice? Just to clarify, my suggestion is to:
|
/cc |
A warning in the log is for sure useful. Though, I was rather thinking about smth similar to how it's done for kubevirt/pkg/virt-api/webhooks/mutating-webhook/mutators/vmi-mutator.go Lines 161 to 171 in 4ede357
If I understand correctly, this raises a warning when you try to run a VM with |
nit: the "Introduction" section of the current README could also be updated as a part of this PR to reflect that only one version is supported. |
pkg/hooks/manager.go
Outdated
hooksV1alpha2.Version, | ||
hooksV1alpha1.Version, | ||
supportedVersions := map[string]bool{ | ||
hooksV1alpha3.Version: hooksV1alpha3.Deprecated, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this mark v1alpha3 as deprecated too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@0xFelix Deprecated is a boolean, for v1alpha3 it is false
, the other two is true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah! My mistake, this pattern caught me off guard. What about a list of deprecated versions that you only maintain in manager.go
? You need to touch this file anyways when adding a new API version. This way you would get a direct overview of deprecated versions when looking at the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I missed your comment.
What about a list of deprecated versions that you only maintain in manager.go?
I can add it, I think it would be beneficial to avoid doing:
// Check for deprecated APIs being used.
hookVersions := map[string]bool{
hooksv1alpha3.Version: hooksv1alpha3.Deprecated,
hooksv1alpha2.Version: hooksv1alpha2.Deprecated,
hooksv1alpha1.Version: hooksv1alpha1.Deprecated,
}
but I think the deprecation value should still be part of each version module, like the Version string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/retest |
1 similar comment
/retest |
hi @victortoso the code looks good to me. However, we should probably merge the design proposal before this. I'll have a look to it |
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you.
I think the promotion to Beta should be discussed in parallel to this change and the design finalized.
While working on a sidecar lately, I realized we do have a problem with its implementation which makes it hard to extend in a reasonable way.
But this fits a design or a meeting discussion, not here.
pkg/hooks/manager.go
Outdated
var DeprecatedVersions = map[string]bool{ | ||
hooksV1alpha3.Version: hooksV1alpha3.Deprecated, | ||
hooksV1alpha2.Version: hooksV1alpha2.Deprecated, | ||
hooksV1alpha1.Version: hooksV1alpha1.Deprecated, | ||
} | ||
|
||
// The order matters. Newest versions first. | ||
var SupportedVersions = []string{ | ||
hooksV1alpha3.Version, | ||
hooksV1alpha2.Version, | ||
hooksV1alpha1.Version, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be nicer to avoid changing these slices in the future here.
The versions could be registered by the individual package/file through an init()
, leaving here only the infrastructure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I did not understand your suggestion. While we are alpha/beta, what is the problem with the slice of versions and map of deprecation? What is the change you would like to see here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant adding this to the specific version/s:
func init() {
RegisterSupportedVersion(version)
RegisterDeprecatedVersion(version)
}
And implement this at the manager:
func RegisterSupportedVersion(ver string) {
supportedVersions = append(supportedVersions, ver)
}
etc...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see what you mean now.
One problem would be the order. We can't guarantee the order using init(), can we?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EdDev ping on above question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, seems I have missed the answer.
One problem would be the order. We can't guarantee the order using init(), can we?
The order of the init()
calls cannot be controlled, but we can make sure items are added to the slice in an ordered manner (version names are sort-able).
I'm not that clear why we need them ordered in the first place.
Is it for picking the default/latest one when answering the client? (if that is the case, we can mark which is the "default" when registering).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The order of the init() calls cannot be controlled, but we can make sure items are added to the slice in an ordered manner (version names are sort-able).
Okay, will do that.
I'm not that clear why we need them ordered in the first place.
Is it for picking the default/latest one when answering the client?
Yes. We receive a list of implemented versions and our heuristic is to get latest one. So, some order is needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It will be nicer to avoid changing these slices in the future here. The versions could be registered by the individual package/file through an
init()
, leaving here only the infrastructure.
I actually worked on this Today. We can't do it how you proposed, by implementing init()
that call manager's Register
function would cause a dependency cycle between each version and hook package.
As we dropped adding the warning on webhook admitter, I'm not exporting both variables anymore. I'll add this to #11215 as well.
} | ||
} | ||
|
||
func warnDeprecatedSidecars(vmi *v1.VirtualMachineInstance) []string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think we should examine annotations, it is a step too far IMO.
Warning about it in the runtime that uses it makes sense, but not at this level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, it can be useful and we have precedence with warning over deprecation with featureGates in the code.
The real downside to me is that it isn't 100% reliable because the args are not mandatory, nor the version argument. So, once could be using passt or slirp would trigger no warning.
The runtime on log works reliably because it happens when the hook sidecar is registering itself.
@vasiliy-ul, wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think it's okay to raise awareness of the users. This annotation is part of the deprecated API. Why not raising a warning if it is used? Having only a log message is a good thing, but usually nobody looks at the logs if everything works fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another reason is that a user may just change/fix the annotation, and then all this is reversable.
I guess when the deprecation warning will become a strong rejection, you will not want to block changes on such settings at the webhook. As mentioned earlier, it can be fixed.
The fact that these values are not mandatory and defaults kick in, makes this check problematic IMO. I mean, even if the validation here passes, what really matters is the version of the sidecar server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. do I understand correctly that with this change the creation and/or change is actually rejected?
Compare to just logging, this is a breaking change, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could expose the server sidecar hook version in KubeVirt CR. At least in this way, users can read the expected version. Since #11001, all authenticated users should be able to read that information.
The sidecar server runs as part of the sidecar container/image. This is not a cluster global thing.
We can even have multiple versions running in the same virt-launcher pod.
So I am not clear how this suggestion fits exactly.
I do not think webhooks should validate versions, I also do not think it is correct to parse unstructured data from annotations for such validations, especially when the value checked is not required.
I am not even convinced it is relevant to tell the server side with which version it should operate, that should be negotiated between the client and server like most protocol do when a handshake is needed.
There is an opportunity here, once all these issues have been surfaced, to improve the "protocol" and provide a more sustainable solution. I would advice to work on the improvements and prioritize that over patching hacks on the problems seen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could expose the server sidecar hook version in KubeVirt CR. At least in this way, users can read the expected version. Since #11001, all authenticated users should be able to read that information.
The sidecar server runs as part of the sidecar container/image. This is not a cluster global thing. We can even have multiple versions running in the same virt-launcher pod. So I am not clear how this suggestion fits exactly.
True, but we also support a single version of the virt-launcher image which is the server and it could expose which version of the protocol its support. It could be a way to expose this information in a more friendly way. Right now, the sidecar server API is embedded in virt-launcher. Or is there a way to understand the supported API versions?
The alternative is that users will realize that they need to upgrade the sidecar API version when they fail running the virt-launcher pod.
Do we need to guarantee backward compatibility when the API graduate from alpha to beta?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Folks, this is awesome discussion, truly. I'll open a new issue with a tracker of proposed changes for sidecar promotion from alpha and beta and we can probably dig deeper there. I'll just give my opinion on what was raised so far:
Right now, the sidecar server API is embedded in virt-launcher. Or is there a way to understand the supported API versions?
@alicefr the gRPC server side is actually running in the sidecar. There is a predefined path that they should run their socket and virt-launcher will iterate over all available sockets, doing capabilities query (e.g: what version it is running in that socket and what hooks are handled/exposed).
If we remove, let's say, v1alpha1, the removal will be on virt-launcher, which means that sidecar exposing that version will not be recognized and will fail to run.
We could expose the server sidecar hook version in KubeVirt CR. At least in this way, users can read the expected version. Since #11001, all authenticated users should be able to read that information.
I'm not sure what the best way forward is, we need to brainstorm, but we do need a way to query version and more.
Do we need to guarantee backward compatibility when the API graduate from alpha to beta?
In my opinion, no. From KubeVirt point of view, I'm mostly worried about not breaking the network pluggins and their 'supported' scenarios. I don't know that, but knowing we can make a plan of how many releases we need to keep a given version deprecated. That's hopefully enough for folks using custom sidecars outside KubeVirt repository and a possible incentive to move minor use cases to use sidecar-shim as, I believe, the plan is that people would only need to rebuild their sidecar with a newer sidecar-shim image as that's focusing on gRPC/api versioning.
There is an opportunity here, once all these issues have been surfaced, to improve the "protocol" and provide a more sustainable solution. I would advice to work on the improvements and prioritize that over patching hacks on the problems seen.
@EdDev, I agree. I'll update this PR Today and open the tracker issue I mentioned, and we can start discussing ideas for the Beta version.
Thank you all so much for the discussion!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @victortoso for the explication! Sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As promised, I opened the following issue to track improvements we want to do for Beta:
#11215
Right. Please do file issues for us to discuss it, or we can perhaps discuss it in kubevirt/community#252 if you feel it is appropriate. |
This patch marks the whole hooks/v1alpha1 and hooks/v1alpha2 as deprecated. Users should use: * v1alpha3: introduced in v1.2.0 (to be released) with 613f0fe "hooks: Add v1alpha3 with Shutdown method" in 2023-11-27 This patch also includes the result of `make generate`. Signed-off-by: Victor Toso <[email protected]>
We add a new boolean to easily check if a given version is deprecated or not. Signed-off-by: Victor Toso <[email protected]>
Other versions are now deprecated. Signed-off-by: Victor Toso <[email protected]>
58e151c
to
b2619b9
Compare
New changes are detected. LGTM label has been removed. |
Sorry, I've been in and out the last couple of weeks and could not work on this. We are now in the Feature Freeze so I think we can postpone this for after the release. I'll get back to kubevirt/community#252 Tomorrow, which should be merged prior to this anyway. |
@victortoso: The following test failed, say
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. |
PR needs rebase. 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. |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with /lifecycle stale |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with /lifecycle rotten |
Rotten issues close after 30d of inactivity. /close |
@kubevirt-bot: Closed this PR. In response to this:
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. |
This patch marks the whole
hooks/v1alpha1
andhooks/v1alpha2
as deprecated. Users should prefer use:This patch also includes the result of
make generate
.What this PR does / why we need it:
On v1.2.0 release, the hooks will have three versions:
We should avoid supporting too many versions as the code gets more complex to maintain. There are minimal changes to move from
v1alpha1
tov1alpha2
and the same amount of work to move fromv1alpha2
tov1alpha3
.I propose we deprecate
v1alpha1
andv1alpha2
on KubeVirt v1.2.0 and remove it by KubeVirt v1.3.0.A formal proposal to promote to Beta and GA is being added with kubevirt/community#252
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:
This is just a formal deprecation.
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: