search

kubevirt / community

Community content
https://kubevirt.io
46 stars 101 forks source link

sig-lifecycle: Introduce and populate sigs.yaml #299

Closed lyarwood closed 1 day ago

lyarwood commented 4 weeks ago

/cc @0xFelix /cc @vladikr /cc @fabiand /cc @jean-edouard /cc @dominikholler /cc @stu-gott

What this PR does / why we need it:

The scope of SIG Lifecycle is currently limited to the implementation and graduation of the instancetype.kubevirt.io API and CRDs to v1.

In the future it is hoped to increase the scope of the SIG to other aspects of a VirtualMachine lifecycle within KubeVirt as part of the process to break up the responsibilities of the current default sig-compute.

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:

NONE
kubevirt-bot commented 4 weeks ago

[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 alonakaplan 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](https://github.com/kubevirt/community/blob/main/OWNERS)** Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
fabiand commented 4 weeks ago

We have only chairs, no members. And only two chairs, no additional members. I'm not sure how a SIGN can ensure to have an appropriate coverage (time wise) for the areas it is suggesting to own.

life-cycle is also a pretty generic term. And hope does not seem to be a ... fact based methology in order to define the future scope of the SIG.

To me there is much to discuss in this proposal.

/hold

lyarwood commented 4 weeks ago

We have only chairs, no members. And only two chairs, no additional members.

I'm not sure why you had to make the same point twice here but in any case no other charter or sigs.yaml definition includes a members list?

Even if they did why would a small number of contributors during the initial founding of a SIG be a blocker?

I'm not sure how a SIG can ensure to have an appropriate coverage (time wise) for the areas it is suggesting to own.

Given the limited scope of this proposal I don't see the issue and even if it was larger I still don't see an issue with founding a SIG with a few members with the ambition to then grow?

life-cycle is also a pretty generic term.

Yes it is, happy to make it more specific to the lifecycle of a VM, something like sig-vm-lifecycle?

I also know our own version of sig-api-machinery has also been suggested elsewhere to handle the entire control plane. That could also work but IMHO specifically targeting the lifecycle of the VM, our most important resource after all, might be a better approach.

And hope does not seem to be a ... fact based methology in order to define the future scope of the SIG.

Fact based methodology? Do you mean fact-based decision-making?

Why would such a formal approach be required for an initial draft of a SIG charter?

Why does suggesting the future increased scope of the SIG in the initial draft draw such a response?

Why are we yet again in this community setting the bar so ridiculously high (even if this was suggested in jest) instead of attempting to make incremental progress forward?

fabiand commented 4 weeks ago

Why are we yet again in this community setting the bar so ridiculously high (even if this was suggested in jest) instead of attempting to make incremental progress forward?

My intention is to help to clarify what we need and to help shape patterns to support this.

In my opinion we need to have a clear understanding of who is responsible to take of certain parts of the codebase (ownership) in order to allow new code in, and then take care of it.

To me ownership is provided by SIGs. Thus SIGs must be a group of people, more than 2, in order to be reislient to mundane yet important events such as PTO, holidays, sick leave etc. At the same time, this group of people must have the knowledge to maintain and take care of the code it owns. Within this group, not all are expected to take care of everything, but as a group, it must be able to tackle anything which is coming up in their part of the codebase. Thus SIGs are mostly about ownership, but also about the people.

For clarity, there should be exactly one owning SIG for every piece of code. If SIG instancetypes is now owning instance types, and SIG instancetypes has only 2 members. Who will be taking care of this code once the two are not available?

A WG pattern to me is solving this easily:

sig-compute maintains the ownership in the worst case. However, it's delegating the responsibility for this area to these two people, and even giving them approval rights.

Thus from a project perspective this code is owned by sig-compute which can be approached in any situation.

IOW we ensure that sig-compute has always enough members to provide coverage for code an time. WG is internal struct to delegate responsibilities to SMEs.

To me https://github.com/kubevirt/kubevirt/pull/12058 is a pragamtic, good, and fast example of how this can be achieved.

lyarwood commented 4 weeks ago

Why are we yet again in this community setting the bar so ridiculously high (even if this was suggested in jest) instead of attempting to make incremental progress forward?

My intention is to help to clarify what we need and to help shape patterns to support this.

I appreciate that but it's starting to come across as whataboutism.

In my opinion we need to have a clear understanding of who is responsible to take of certain parts of the codebase (ownership) in order to allow new code in, and then take care of it.

This proposal is explicit in this regard.

To me ownership is provided by SIGs. Thus SIGs must be a group of people, more than 2, in order to be reislient to mundane yet important events such as PTO, holidays, sick leave etc. At the same time, this group of people must have the knowledge to maintain and take care of the code it owns. Within this group, not all are expected to take care of everything, but as a group, it must be able to tackle anything which is coming up in their part of the codebase. Thus SIGs are mostly about ownership, but also about the people.

Yes agreed, given the initial scope of this proposal I still don't see an issue with myself and Felix as chairs with additional reviewers then also listed elsewhere in the OWNERs files.

If you want that list written down as part of this proposal just say so.

For clarity, there should be exactly one owning SIG for every piece of code.

Eventually once code is decoupled enough sure but that is just fantasy with the current state of the code base.

Take the admission webhooks for example, where there's often SIG specific logic present. How could a single SIG own and maintain multiple SIGs worth of business logic until that's extracted (as Network have been aggressively doing to their credit)?

IMHO multi SIG ownership of code paths is fine in the short term but the goal should be single SIG ownership whenever possible.

If SIG instancetypes is now owning instance types, and SIG instancetypes has only 2 members. Who will be taking care of this code once the two are not available?

Again, the chairs list isn't a membership list and I was going to approach 2 or 3 additional folks who have already reviewed this area in the past for membership. If you want a membership list to be provided as part of the proposal please ask or better yet add it to the charter template.

Additionally I'm suggesting a broader SIG than just instance types here but leaving the expansion to follow up discussions.

A WG pattern to me is solving this easily:

As I've said elsewhere the k8s WG pattern explicitly doesn't include code ownership and is a multi SIG collaboration vehicle. If we want to define it as something else that's also fine but lets do that first instead of assuming folks have the same vision of WGs as you.

sig-compute maintains the ownership in the worst case. However, it's delegating the responsibility for this area to these two people, and even giving them approval rights.

That's still possible with a multi SIG ownership model where SIG compute retains approval rights alongside this SIG.

Thus from a project perspective this code is owned by sig-compute which can be approached in any situation.

IOW we ensure that sig-compute has always enough members to provide coverage for code an time. WG is internal struct to delegate responsibilities to SMEs.

Again my issue here is our divergence from k8s WGs and lack of any written down agreement within KubeVirt about what they mean here. I'm basically asking for the same thing here but using a child SIG to SIG compute instead of a WG.

To me kubevirt/kubevirt#12058 is a pragamtic, good, and fast example of how this can be achieved.

Yup this is basically what I had in mind but replacing the use of a WG with another SIG.

fabiand commented 4 weeks ago

I appreciate that but it's starting to come across as whataboutism.

Fair point. To me it's me failing to get across some core points.

If you want that list written down as part of this proposal just say so.

If it is a SIG; then yes, this list of people os required. but I still do not think that a SIG is applicable for this case.

Take the admission webhooks for example, …

Is this also an example of whataboutism ? :)

Additionally I'm suggesting a broader SIG than just instance types here but leaving the expansion to follow up discussions.

Maybe we can do it the other way round: Define the area that belongs together, and therefore create a dedicted SIG to own it?

That's still possible with a multi SIG ownership model where SIG compute retains approval rights alongside this SIG.

To me this is defeating hte principle of clear ownership. Today we have the "problem" that root approvers can approve anything in the codebase, thus they overlap with SIG boundaries. In future however, SIGs should be non overlapping in order to make ownership clear.

If I understand you correctly, then you are saying that sig instancetypes would own it, but sig compute would still be able to approve? I'm not seeing why this is good. To me this reality would rather reflect: sig instancetypes is a subgroup of si compute. and this would take me further to say because sig compute is the parent of sig instancetypes, sig instanctypes should be a wg.

I'm basically asking for the same thing here but using a child SIG to SIG compute instead of a WG. … Yup this is basically what I had in mind but replacing the use of a WG with another SIG.

To me there are two paths out:

  1. Define a more broadly scope SIG i.e. sig-api-machinery to include instancetypes but also other stuff
  2. Go ahead with wg do keep ownership clear, but delegate to people

To me 2 is quick, and 1 willr equire discussion with sig-compute.

lyarwood commented 4 weeks ago

Apologies I was going to reply to each point above but I think it's easier if I just skip to the suggested ways forward.

To me there are two paths out:

  1. Define a more broadly scope SIG i.e. sig-api-machinery to include instancetypes but also other stuff
  2. Go ahead with wg do keep ownership clear, but delegate to people

To me 2 is quick, and 1 willr equire discussion with sig-compute.

I've gone back over the k8s community docs and I think there's actually a third path that's a compromise here:

I'll comment on the WG PR now to highlight that I really don't think we should be redefining the k8s meaning of WG.

Anyway, hopefully this is a constructive suggestion and thanks for continuing to engage with this ;)

[1] https://github.com/kubernetes/community/blob/master/governance.md#working-groups [2] https://github.com/kubernetes/community/blob/master/governance.md#subprojects

lyarwood commented 4 weeks ago
  • Introduce a subproject under sig-compute for instancetypes [2]
  • Introduce sig-compute-instancetype-{approvers,reviewers} in OWNER_ALIASES
  • Populate OWNERS files for the code owned by this subproject. This should also list the existing sig-compute-{approvers,reviewers} as ownership still resides with the wider SIG.

Here's a draft attempt to help illustrate what I'm suggesting here:

https://github.com/kubevirt/kubevirt/pull/12062 https://github.com/kubevirt/community/pull/302

lyarwood commented 1 day ago

/close

kubevirt-bot commented 1 day ago

@lyarwood: Closed this PR.

In response to [this](https://github.com/kubevirt/community/pull/299#issuecomment-2203355447): >/close Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.