Bug 1997811: Bump k8s and controller-runtime dependencies to v0.21.x and v0.9.x #419
Conversation
|
/retest |
timflannagan
force-pushed
the
bump-k8s-1.21
branch
3 times, most recently
from
14463d6
to
d385af1
Compare
Update the controller-runtime dependency to a release version tag that has contains the requisite updates for k8s 1.21 dependencies.
Update the cmd/manager/main.go executable package and remove the direct operator-sdk package imports. Directly vendor the function responsible for determining the operator watch namespace in pkg/apis/operator/shared/shared.go and update the import to reference that package instead. Remove the operator-sdk version printing when the --version flag has been specified.
Remove the unused pkg/proxy/proxy.go test as it's no longer needed with the deprecated and removal of the OperatorSource API. Remove the ProxyTests test handler package. Previously, this test handler was being registered before the OperatorSource API was removed. During the removal of the OperatorSource API [1], the removal of this package was missed. [1] operator-framework/operator-marketplace@82b5ef3
…mports Remove the manual vendoring of the OLM CatalogSource API type introduced in operator-framework#329. Update relevant package imports to use the o-f/api dependency.
Update cmd/manager/main.go and avoid registering the v1beta1 CRD schema in the main executable package. Update the vendor tree and mark the `k8s.io/apiextensions-apiserver` as an indirect dependency.
timflannagan
force-pushed
the
bump-k8s-1.21
branch
from
d385af1
to
1355e3a
Compare
Remove the various operator-sdk-related bash scripts in scripts/. Remove any unneeded Makefile targets. Update the `e2e-job`, which is what o/release is configured to run, and prefer to inline the `go test ...` command instead of using a bash script.
timflannagan
force-pushed
the
bump-k8s-1.21
branch
from
1355e3a
to
1a9c7b3
Compare
|
/retest |
timflannagan
changed the title
openshift-ci
bot
added
bugzilla/severity-high
|
@timflannagan: This pull request references Bugzilla bug 1997811, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: 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/test-infra repository. |
|
@openshift-ci[bot]: GitHub didn't allow me to request PR reviews from the following users: jianzhangbjz. Note that only operator-framework members and repo collaborators can review this PR, and authors cannot review their own PRs. 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/test-infra repository. |
|
/retest |
|
/retest |
1 similar comment
|
/retest |
| @@ -87,12 +89,12 @@ type ReconcileCatalogSource struct { | |||
| client client.Client | |||
| } | |||
|
|
|||
| func (r *ReconcileCatalogSource) Reconcile(request reconcile.Request) (reconcile.Result, error) { | |||
| func (r *ReconcileCatalogSource) Reconcile(ctx context.Context, request reconcile.Request) (reconcile.Result, error) { | |||
There was a problem hiding this comment.
Is it necessary/helpful/reasonable to pass this context down to anything called from this function (e.g. GetSingleton().Get() or Ensure())?
There was a problem hiding this comment.
I did a cursory look through the repository and removed as many usages of context.TODO() outside of testing code that were easily replaceable. The status reported package, which is responsible for setting up a go routine to monitor the marketplace ClusterOperator resource (and as aside, I think we'd want to move that handlement to its own controller down the line) but the changeset for updating all the call sites to properly handle signals seemed riskier in the full context of these changes, so I held off there.
One other quick note, and something we can tackle outside of this PR, is we still embed OperatorSource logic in the various top-level functions/methods/etc. I wonder if it's worth the easier maintenance for 4.9+ to make the requisite changes. It likely would be a source for cherrypick failures when backporting bug fixes, but the surface area would be low. I'm not super worried about breaking other clients that depend on marketplace at this point either.
Update relevant functions and methods in the marketplace operator packages to avoid the usage of context.TODO() as much as possible. Instead, update the signatures to include a context.Context parameter that can be propagated at call sites.
| func createNewCatsrcInstance(client client.Client, catsrc olm.CatalogSource) error { | ||
| err := client.Create(context.TODO(), &catsrc) | ||
| if err != nil { | ||
| log.Warnf("Could not recreate default CatalogSource %s. Error: %s", catsrc.GetName(), err.Error()) | ||
| return err | ||
| } | ||
| return nil |
There was a problem hiding this comment.
I realize the diff for this is somewhat misleading when quickly reviewing:
- The
Reconcilemethod was updated to take a context.Context parameter - The
Ensure(...)andEnsureAll(...)now take context.Context parameters - The
createNewCatsrcInstancefunction was removed as it was dead code
| // Requested object was not found, could have been deleted after reconcile request. | ||
| // Owned objects are automatically garbage collected. For additional cleanup logic use finalizers. | ||
| // Return and don't requeue | ||
| return reconcile.Result{}, client.IgnoreNotFound(err) |
|
/retest |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: joelanford, kevinrizza, timflannagan 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 |
openshift-ci
bot
added
the
approved
|
/retest |
1 similar comment
|
/retest |
|
@timflannagan: The following test failed, say
Full PR test history. Your PR dashboard. 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. |
|
@timflannagan: All pull requests linked via external trackers have merged: Bugzilla bug 1997811 has been moved to the MODIFIED state. 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/test-infra repository. |
Overview
The marketplace operator is currently building and testing with an older Kubernetes version (v0.20.6) of clients and APIs. This can pose a maintenance burden going forward as we'll be deploying the marketplace operator using an unmaintained Kubernetes minor version as we progress future in the 4.x OCP lifecycle, and upgrading to OCP 4.9 will be needed for upgrading from EUS to EUS. Simply upgrading the controller runtime and k8s.io dependencies to a more recent (v0.21.x, v0.22.x) versions and making the requisite code changes is not possible as we're parking on a stale v0.19.x operator-sdk version. Currently, the marketplace operator uses the operator-sdk testing harness, which was deprecated in v0.19.x and removed entirely in v1.x in favor of controller runtime's envtest implementation. The operator-sdk testing harness is not compatible with client function signatures changes in controller-runtimes v0.7.x release, and bumping the operator-sdk dependency to a supported v1.x version involves several breaking changes. This PR is centered around the removal of the operator-sdk dependency entirely, replacing any direct package imports, and removing the testing harness entirely.
Minor Changes
GO111MODULE=off) when building the marketplace operator binary in the build/build.sh bash script.Notable Changes
Avoid manually vendoring the OLM API types
#329 introduced these manually vendored APIs as the operator-sdk version at the time didn't support Go modules.
This removes those vendored packages in favor of the operator-framework/api dependency, which currently houses the CatalogSource type definitions the marketplace operator needs.
Any existing package imports in the code base are replaced to use the v0.10.5 api dependency instead of the (now) deleted manually vendored API packages.
Handle controller runtime v0.7.x breaking changes
The controller-runtime v0.7.x release introduced a couple of breaking changes:
mgr.Start(...)now takes a context.Context as a parameter instead of a stop channel.Reconcilemethod now takes a context.Context parameter.runtime.Objectneeds to be updated to instead handle aclient.Objectandclient.ObjectListinstead.Removing the operator-sdk dependency
Any code paths that directly import the github.com/operator-framework/operator-sdk package import have been removed.
The
k8sutil.GetWatchNamespace()has been directly vendored in the pkg/apis/operators/shared package.Avoid printing the operator-sdk version when starting up the marketplace operator executable.
Migrate the e2e testing packages to use Ginkgo instead of the operator-sdk testing harness. The majority of the OLM-related repositories (and the Kubernetes ecosystem as a whole) use Ginkgo for e2e testing. Any relevant marketplace e2e test has been migrated to a testing package in the test/e2e package.
Update the scripts/ directory and remove any operator-sdk-related bash scripts that are no longer needed. The root Makefile has been also updated to remove targets that are no longer needed. The
e2e-jobMakefile target, which is what the o/release ci-operator configuration is responsible for running, has been updated to simply inline thego test ... ./test/e2e/...command instead of utilizing a wrapper bash script to setup and run e2e tests.