Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Unify operators into one binary. #21

Merged
merged 2 commits into from
Apr 28, 2020
Merged

Unify operators into one binary. #21

merged 2 commits into from
Apr 28, 2020

Conversation

markusthoemmes
Copy link
Contributor

Proposed Changes

At this point, there is very little reason to spawn both operators into their own pods. We can unify them into one binary and run them off of the same caches to reduce overhead by quite a bit.

Release Note

Unified the operator binaries into one.

@googlebot googlebot added the cla: yes Indicates the PR's author has signed the CLA. label Apr 17, 2020
@knative-prow-robot knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 17, 2020
knative-prow-robot
knative-prow-robot reviewed Apr 17, 2020
View reviewed changes
Copy link
Contributor

@knative-prow-robot knative-prow-robot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markusthoemmes: 0 warnings.

In response to this:

Proposed Changes

At this point, there is very little reason to spawn both operators into their own pods. We can unify them into one binary and run them off of the same caches to reduce overhead by quite a bit.

Release Note

Unified the operator binaries into one.

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.

@knative-prow-robot
Copy link
Contributor

Welcome @markusthoemmes! It looks like this is your first PR to knative-sandbox/operator 🎉

@knative-prow-robot knative-prow-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 17, 2020
@knative-prow-robot
Copy link
Contributor

Hi @markusthoemmes. Thanks for your PR.

I'm waiting for a knative-sandbox member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@jcrossley3
Copy link
Contributor

/ok-to-test

@knative-prow-robot knative-prow-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 17, 2020
Cynocracy
Cynocracy reviewed Apr 17, 2020
View reviewed changes
config/role.yaml Outdated
@@ -207,139 +208,4 @@ aggregationRule:
# automatically.
- matchExpressions:
- {key: eventing.knative.dev/release, operator: Exists}
rules: [] # Rules are automatically filled in by the controller manager.
---
kind: ClusterRole
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you'll still need this Role (or something like it) in order for the operator to begin bootstrapping.

Did you find it was unnecessary? It would be great if we only needed the aggregated role, but I'm suspicious that we need more than that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original knative-serving-operator role looked like a superset of this. I honestly haven't really checked that in depth though, I mostly eyeballed it.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, nice. that's entirely possible.

One change we had to make recently was to allow create/delete/list/watch/update for Jobs, I think you may need to readd that (as without it our upgrade tests were failing)

This was referenced Apr 21, 2020
Merged
Merged
@jcrossley3
Copy link
Contributor

/lgtm

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 23, 2020
houshengbo
houshengbo reviewed Apr 23, 2020
View reviewed changes
cmd/operator/main.go Outdated
if err != nil {
log.Fatal("Error building kubeconfig", err)
}
sharedmain.MainWithConfig(signals.NewContext(), "serving-operator", cfg, knativeserving.NewController)
sharedmain.MainWithConfig(signals.NewContext(), "serving-operator", cfg,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we change the name into operator or knative-operator

@houshengbo
Copy link
Contributor

@markusthoemmes Generally it looks good to me, except a naming issue.
Since we use one operator deployment instead of two operator deployments, I am wondering how we can smoothly upgrade it.
When users install 0.14.x, two operator deployments exists in their kube. When they install 0.15.0, what is the automation we can give them to remove the old deployments?
We need to make our prow job for upgrade able to check that.
This is what I care regarding this change.

@jcrossley3
Copy link
Contributor

When users install 0.14.x, two operator deployments exists in their kube. When they install 0.15.0, what is the automation we can give them to remove the old deployments?

Good point. We can easily have the operator delete the old deployments when it starts up, similar to how the deleteObsoleteResources works.

@knative-prow-robot knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Apr 27, 2020
@markusthoemmes markusthoemmes force-pushed the unify-entrypoints branch 2 times, most recently from 8465a2e to 0919ef4 Compare April 27, 2020 11:00
@markusthoemmes
Copy link
Contributor Author

Sorry, I had to heavily rebase against the version changes. Squashed in that process to ease reviewing again.

@markusthoemmes
Copy link
Contributor Author

Oh nice... a case of "works on my machine" 😂. Gonna look at getting log output from our tests.

@knative knative deleted a comment from markusthoemmes Apr 27, 2020
@houshengbo
Copy link
Contributor

@markusthoemmes go-coverage asks for unit test coverage of new added files. I am not sure how difficult/easy it can be added to upgrade.go to remove the deployments and service accounts.

@markusthoemmes
Copy link
Contributor Author

Should be straightforward, I got that on my todo list.

@knative-metrics-robot
Copy link

The following is the coverage report on the affected files.
Say /test pull-knative-sandbox-operator-go-coverage to re-run this coverage report

File Old Coverage New Coverage Delta
pkg/reconciler/upgrade.go Do not exist 60.0%

@markusthoemmes
Copy link
Contributor Author

/assign @houshengbo

jcrossley3
jcrossley3 approved these changes Apr 28, 2020
View reviewed changes
Copy link
Contributor

@jcrossley3 jcrossley3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!
/lgtm
/approve

@knative-prow-robot knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 28, 2020
@knative-prow-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcrossley3, markusthoemmes

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@knative-prow-robot knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 28, 2020
@knative-prow-robot knative-prow-robot merged commit 4d8c1c1 into knative:master Apr 28, 2020
matzew pushed a commit to matzew/knative-operator that referenced this pull request Aug 11, 2020
@jcrossley3
Reduce chances of name collision with serving (knative#21)
c07f825 
This is accounted for upstream in f8fdf32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knative-prow-robot knative-prow-robot knative-prow-robot left review comments

@houshengbo houshengbo houshengbo left review comments

@Cynocracy Cynocracy Cynocracy left review comments

@jcrossley3 jcrossley3 jcrossley3 approved these changes

@matzew matzew Awaiting requested review from matzew

@trshafer trshafer Awaiting requested review from trshafer

Assignees

@jcrossley3 jcrossley3

@houshengbo houshengbo

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cla: yes Indicates the PR's author has signed the CLA. lgtm Indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@markusthoemmes @knative-prow-robot @jcrossley3 @houshengbo @knative-metrics-robot @Cynocracy @googlebot