Skip to content
This repository has been archived by the owner on Jun 24, 2020. It is now read-only.

Discussion: whether we should merge serving and eventing operators into one repository #275

Open
houshengbo opened this issue Jan 29, 2020 · 16 comments

Comments

@houshengbo
Copy link

houshengbo commented Jan 29, 2020

As it has been brought during the Serving API meeting today on Jan 29, https://docs.google.com/document/d/1NC4klOdNaU-N-PsKLyXBqDKgNSHtxCDep29Ta2b5FK0, we need a broader discussion of this topic.

Please share your comments down below, regarding which option you would like to pick and why.

  1. Keep serving and eventing operators separated as they are.
  2. Merge serving and eventing operators into one operator, living in one repository.
  3. Abstain.

Remember! Say why from any perspective. Thanks.

@houshengbo houshengbo changed the title Discussion: pros and cons of merging serving and eventing operators into one repository Discussion: whether we should merge serving and eventing operators into one repository Jan 29, 2020
@duglin
Copy link

duglin commented Jan 29, 2020

2 - merge. Easier UX and management. Customers will want (in essence) a snapshot of "Knative". A consistent set of components that have been tested by us and packaged together.

@trshafer
Copy link
Contributor

2 - merge.

Agree with @duglin. I think eventing will become more expected when "installing knative" as eventing grows. The goal of a kubernetes operator is to automate what a human operator would do. If a human operator wants to update knative components, the human should only need to go to one place.

The dependency of serving from eventing, imo, means that the operator should be responsible for ensuring the two systems are compatible.

Sort of an implementation detail but I could still see two separate CRDs that the operator listens to depending on configuring and operating serving & eventing.

@salaboy
Copy link
Member

salaboy commented Jan 29, 2020

I agree with @trshafer and @duglin I believe that a unified operator will simplify the user experience.

@trshafer
Copy link
Contributor

I should provide some thoughts on why we would not want to merge the eventing and serving operator.

Eventing and serving could have different release cadences. If the operator is the way to install knative (everything), then there is a possibility that serving changes block a release of eventing. If eventing has their own operator then eventing has fewer dependencies on releasing.

@houshengbo
Copy link
Author

houshengbo commented Jan 29, 2020

Based on the nature that one version of operator installs one version of knative software, I do not know how to do the PATCH release for operator, when only serving/eventing does a PATCH release.

If serving release 0.13.1, but eventing does not have 0.13.1, does operator release the version 0.13.1 for 0.13.1 serving and 0.13.0 eventing?

Personally, I would like to see operator catch up all the releases of serving and eventing.

@anniefu
Copy link
Contributor

anniefu commented Jan 29, 2020

2 - merge. In addition to above previously mentioned reasons, because eventing is dependent on serving version, the eventing operator would always have to have built-in logic regarding serving CR and versions anyway, and that could get very hard to maintain quickly

For user's clarity, it would be best if the CRDs were merged too since it's important to be aware of the serving version when making changes to the eventing CR. It also minimizes the potential race conditions in the reconcile loops if serving and eventing are both changed.

@houshengbo Would the operator release tied to specific versions of serving and eventing? I would expect an operator release to be compatible with multiple versions of serving & eventing to support upgrades.

@duglin
Copy link

duglin commented Jan 29, 2020

w.r.t. eventing and serving have different patch releases... they should be the same. If eventing changes and needs 0.13.1 but serving doesn't, then serving's 0.13.1 release will look the same as 0.13.0.

vX.Y.Z should be a snapshot of the entire system at that point in time, even if a particular component didn't change from the previous version. That makes it very easy for a user to know what they're getting for any particular Knative-wide snapshot/version.

@evankanderson
Copy link
Member

2 - merge.

I'd prefer to see the operator handle the difficulties of potentially needing to combine e.g. a 0.13.1 serving and a 0.13.2 eventing release to get an "up to date" 0.13 release, rather than asking every human running a Kubernetes cluster to do that.

@houshengbo
Copy link
Author

@anniefu It is going to be a very complicated matrix, we have to maintain for each version of operator, able to install multiple(how many) versions of knative, in terms of single version testing and upgrade testing. It is a great feature support, but it is a lot of overhead as knative community has to maintain.

@matzew
Copy link
Member

matzew commented Jan 30, 2020

If serving release 0.13.1, but eventing does not have 0.13.1, does operator release the version 0.13.1 for 0.13.1 serving and 0.13.0 eventing?

wouldn't it be also worth a thought, that the operator might have it's own release cycle ? That way wwould not artificially need to make a serving 0.13.0 to be a 0.13.1, when the code is exactly the same

At some point ... IMO, it will not scale to release all the things in the same version schema...?

@aliok
Copy link
Member

aliok commented Jan 30, 2020

From a code maintenance perspective, it makes sense. I have pretty much the same doubts above because of the versioning though.

For user's clarity, it would be best if the CRDs were merged too since it's important to be aware of the serving version when making changes to the eventing CR. It also minimizes the potential race conditions in the reconcile loops if serving and eventing are both changed.

If the CRDs is a single, 2 controllers will be touching a CR. I think this would increase the potential race conditions. ...unless the controllers are also merged.

I don't have a very strong opinion on single vs multiple CRDs though. We can make both approaches work. Having separate CRDs perhaps makes more sense because it is simpler compared to a big CRD.

@markusthoemmes
Copy link
Contributor

markusthoemmes commented Jan 30, 2020

I feel strongly against merging the CRDs. We could in theory provide one top-level CRD that includes both the KnativeServing and the KnativeEventing CRD spec but I don't see a ton of value in that. I'd imagine it to look like this:

struct Knative {
  Serving KnativeServingSpec ...
  Eventing KnativeEventingSpec ...
}

The only thing it's controller would do is crank out the respective children and copy up the status of those children. Best of both worlds, but as I said: Not sure about the value of that.

The PATCH releases don't worry me a lot as (at least until now), they have only been manifest upgrades and didn't need any changes to the operator's code itself. The operator's versioning though, might need to be adjusted as was pointed out. It's worth noting that up until now, we have not cut a noop patch release if only one part of the system needed it. If we really want to unify our releases, we should probably do that. That'd also make the operator releasable on the same version always, which is preferable I suppose.

@pmorie
Copy link
Member

pmorie commented Feb 4, 2020

During the WG meeting today, I took an AI to write up a feature track for this so we can understand exactly what is being proposed before moving forward; hope to have that completed in the next couple of days.

@matzew
Copy link
Member

matzew commented Feb 5, 2020

While I support the merge of the operators - I'd not want the CRs for serving/eventing to merge ...

@k4leung4
Copy link
Contributor

@pmorie Any update on the feature track?

thanks

@aliok
Copy link
Member

aliok commented Feb 18, 2020

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests