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.

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

Bug 1886939: vendor: Bump client-go to v0.18.9 #470

Merged
merged 3 commits into from
Oct 9, 2020

Conversation

wking
Copy link
Member

@wking wking commented Oct 9, 2020

Generation notes in the commit messages. This avoids panics:

panic: assignment to entry in nil map

goroutine 80 [running]:
k8s.io/apimachinery/pkg/util/runtime.HandleCrash(0x0, 0x0, 0x0)
  /go/src/github.com/openshift/cluster-version-operator/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:55 +0x105
panic(0x163d780, 0x1a61ab0)
  /usr/local/go/src/runtime/panic.go:679 +0x1b2
k8s.io/client-go/tools/leaderelection/resourcelock.(*ConfigMapLock).Update(0xc0000f6000, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
  /go/src/github.com/openshift/cluster-version-operator/vendor/k8s.io/client-go/tools/leaderelection/resourcelock/configmaplock.go:90 +0x129
k8s.io/client-go/tools/leaderelection.(*LeaderElector).release(0xc000f56b40, 0x6fc23ac00)
  /go/src/github.com/openshift/cluster-version-operator/vendor/k8s.io/client-go/tools/leaderelection/leaderelection.go:306 +0xc9
k8s.io/client-go/tools/leaderelection.(*LeaderElector).renew(0xc000f56b40, 0x1abc1a0, 0xc0004a4180)
  /go/src/github.com/openshift/cluster-version-operator/vendor/k8s.io/client-go/tools/leaderelection/leaderelection.go:294 +0x166
k8s.io/client-go/tools/leaderelection.(*LeaderElector).Run(0xc000f56b40, 0x1abc1a0, 0xc0004a4140)
  /go/src/github.com/openshift/cluster-version-operator/vendor/k8s.io/client-go/tools/leaderelection/leaderelection.go:208 +0x15d
k8s.io/client-go/tools/leaderelection.RunOrDie(0x1abc1a0, 0xc00030de00, 0x1acbcc0, 0xc0000f6000, 0x14f46b0400, 0xa7a358200, 0x6fc23ac00, 0xc001060f30, 0xc0010baa10, 0x0, ...)
  /go/src/github.com/openshift/cluster-version-operator/vendor/k8s.io/client-go/tools/leaderelection/leaderelection.go:221 +0x96
github.com/openshift/cluster-version-operator/pkg/start.(*Options).run.func4(0x1abc1a0, 0xc00030de00, 0xc0000f6000, 0xc000f3ab40, 0xc000099d80, 0x1abc1a0, 0xc00030dd80, 0xc00094b500, 0xc0010ba9d0)
  /go/src/github.com/openshift/cluster-version-operator/pkg/start/start.go:179 +0x1b2
created by github.com/openshift/cluster-version-operator/pkg/start.(*Options).run
  /go/src/github.com/openshift/cluster-version-operator/pkg/start/start.go:177 +0x3f9

by pulling in kubernetes/kubernetes#87821. And it catches us up with the expected 0.18 clients for OpenShift 4.5. The panic was exposed by the #446 backport. This PR also backports a2c3e27 from #406 to get Context handling for the more modern client-go, and it backports c0fe21b from #403 to get a2c3e27 to apply with fewer conflicts (although I still had some manual conflicts to resolve, notes in the commit messages).

Generated with:

  $ github.com/openshift/client-go@05eb9880269c5ff1696fe20f511f9efa8bbdba0e
  $ go get k8s.io/client-go@v0.18.9
  $ go get k8s.io/apiextensions-apiserver@v0.18.9
  $ go get k8s.io/kube-aggregator@v0.18.9
  $ go mod tidy
  $ go mod vendor
  $ git add -A go.* vendor

using:

  $ go version
  go1.15.2 linux/amd64

To pick up [1] and avoid [2]:

  panic: assignment to entry in nil map

  goroutine 80 [running]:
  k8s.io/apimachinery/pkg/util/runtime.HandleCrash(0x0, 0x0, 0x0)
    /go/src/github.com/openshift/cluster-version-operator/vendor/k8s.io/apimachinery/pkg/util/runtime/runtime.go:55 +0x105
  panic(0x163d780, 0x1a61ab0)
    /usr/local/go/src/runtime/panic.go:679 +0x1b2
  k8s.io/client-go/tools/leaderelection/resourcelock.(*ConfigMapLock).Update(0xc0000f6000, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
    /go/src/github.com/openshift/cluster-version-operator/vendor/k8s.io/client-go/tools/leaderelection/resourcelock/configmaplock.go:90 +0x129
  k8s.io/client-go/tools/leaderelection.(*LeaderElector).release(0xc000f56b40, 0x6fc23ac00)
    /go/src/github.com/openshift/cluster-version-operator/vendor/k8s.io/client-go/tools/leaderelection/leaderelection.go:306 +0xc9
  k8s.io/client-go/tools/leaderelection.(*LeaderElector).renew(0xc000f56b40, 0x1abc1a0, 0xc0004a4180)
    /go/src/github.com/openshift/cluster-version-operator/vendor/k8s.io/client-go/tools/leaderelection/leaderelection.go:294 +0x166
  k8s.io/client-go/tools/leaderelection.(*LeaderElector).Run(0xc000f56b40, 0x1abc1a0, 0xc0004a4140)
    /go/src/github.com/openshift/cluster-version-operator/vendor/k8s.io/client-go/tools/leaderelection/leaderelection.go:208 +0x15d
  k8s.io/client-go/tools/leaderelection.RunOrDie(0x1abc1a0, 0xc00030de00, 0x1acbcc0, 0xc0000f6000, 0x14f46b0400, 0xa7a358200, 0x6fc23ac00, 0xc001060f30, 0xc0010baa10, 0x0, ...)
    /go/src/github.com/openshift/cluster-version-operator/vendor/k8s.io/client-go/tools/leaderelection/leaderelection.go:221 +0x96
  github.com/openshift/cluster-version-operator/pkg/start.(*Options).run.func4(0x1abc1a0, 0xc00030de00, 0xc0000f6000, 0xc000f3ab40, 0xc000099d80, 0x1abc1a0, 0xc00030dd80, 0xc00094b500, 0xc0010ba9d0)
    /go/src/github.com/openshift/cluster-version-operator/pkg/start/start.go:179 +0x1b2
  created by github.com/openshift/cluster-version-operator/pkg/start.(*Options).run
    /go/src/github.com/openshift/cluster-version-operator/pkg/start/start.go:177 +0x3f9

The bump from 0.17 to 0.18 also aligns us with the API-server tooling
for OpenShift 4.5 [3].  The openshift/client-go commit is the current
branch tip [4], and avoids issues like:

  $ go test ./...
  ...
  ../../../../pkg/mod/github.com/openshift/client-go@v0.0.0-20191001081553-3b0e988f8cb0/security/clientset/versioned/typed/security/v1/rangeallocation.go:133:5: not enough arguments in call to c.client.Delete().Resource("rangeallocations").VersionedParams(&listOptions, scheme.ParameterCodec).Timeout(timeout).Body(options).Do
    have ()
    want (context.Context)
  ...

[1]: kubernetes/kubernetes#89909
[2]: https://bugzilla.redhat.com/show_bug.cgi?id=1886600
[3]: openshift/origin@d634a58
[4]: https://github.com/openshift/client-go/commits/release-4.5
Error for unrecognized CRD version, because that will be easier to
debug than trying to figure out why a CRD manifest is being silently
ignored by the CVO (which is what we've done since the version switch
landed in 4ee7b07, Add apiextensions.k8s.io/v1 support for CRDs,
2019-10-22, openshift#259).
Catching up with the vendored client library bump.

There are a handful of context.TODO() where I have to wrap a modern
function to fit into a legacy lister interface like:

  $ grep ^func vendor/github.com/openshift/client-go/config/listers/config/v1/clusterversion.go
  func NewClusterVersionLister(indexer cache.Indexer) ClusterVersionLister {
  func (s *clusterVersionLister) List(selector labels.Selector) (ret []*v1.ClusterVersion, err error) {
  func (s *clusterVersionLister) Get(name string) (*v1.ClusterVersion, error) {

I imagine we'll be able to drop the TODO and
dummyContextOperatorGetter and such in some future vendor bump.

Cherry-picked from a2c3e27 (openshift#406).  Conflicts:

* lib/resourcebuilder/apps.go
* pkg/autoupdate/autoupdate.go
* pkg/cvo/cvo.go
* pkg/start/start.go
* pkg/start/start_integration_test.go
@openshift-ci-robot openshift-ci-robot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Oct 9, 2020
@openshift-ci-robot
Copy link
Contributor

@wking: This pull request references Bugzilla bug 1886939, which is invalid:

  • expected dependent Bugzilla bug 1886935 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is MODIFIED instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1886939: vendor: Bump client-go to v0.18.9

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-robot openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Oct 9, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 9, 2020
@openshift-ci-robot
Copy link
Contributor

@wking: This pull request references Bugzilla bug 1886939, which is invalid:

  • expected dependent Bugzilla bug 1886935 to be in one of the following states: VERIFIED, RELEASE_PENDING, CLOSED (ERRATA), but it is MODIFIED instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

Bug 1886939: vendor: Bump client-go to v0.18.9

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.

@sdodson sdodson added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Oct 9, 2020
@sdodson
Copy link
Member

sdodson commented Oct 9, 2020

The 4.6 branch received a similar bump months ago and has been fine, a bug has been opened to verify none the less. Since this is fixing a recent regression in 4.5.z I will cherry-pick-approve this once it's been peer reviewed. If the associated bug fails QE we will revert this and #446 on Monday, but I don't think that will be necessary.

@jottofar
Copy link
Contributor

jottofar commented Oct 9, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 9, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jottofar, wking

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

@sdodson sdodson added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Oct 9, 2020
@openshift-merge-robot openshift-merge-robot merged commit 0a34ac3 into openshift:release-4.5 Oct 9, 2020
@openshift-ci-robot
Copy link
Contributor

@wking: All pull requests linked via external trackers have merged:

Bugzilla bug 1886939 has been moved to the MODIFIED state.

In response to this:

Bug 1886939: vendor: Bump client-go to v0.18.9

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.

@wking wking deleted the v0.18-go-clients branch October 9, 2020 21:33
wking added a commit to wking/cluster-version-operator that referenced this pull request Feb 23, 2021
Address a bug introduced by cc1921d (pkg/start: Release leader
lease on graceful shutdown, 2020-08-03, openshift#424), where canceling the
Operator.Run context would leave the operator with no time to attempt
the final sync [1]:

  E0119 22:24:15.924216       1 cvo.go:344] unable to perform final sync: context canceled

With this commit, I'm piping through shutdownContext, which gets a
two-minute grace period beyond runContext, to give the operator time
to push out that final status (which may include important information
like the fact that the incoming release image has completed
verification).

---

This commit picks c4ddf03 (pkg/cvo: Use shutdownContext for final
status synchronization, 2021-01-19, openshift#517) back to 4.5.  It's not a
clean pick, because we're missing changes like:

* b72e843 (Bug 1822844: Block z level upgrades if
  ClusterVersionOverridesSet set, 2020-04-30, openshift#364).
* 1d1de3b (Use context to add timeout to cincinnati HTTP request,
  2019-01-15, openshift#410).

which also touched these lines.  But we've gotten this far without
backporting rhbz#1822844, and openshift#410 was never associated with a bug in
the first place, so instead of pulling back more of 4.6 to get a clean
pick, I've just manually reconciled the pick conflicts.

Removing Start from pkg/start (again) fixes a buggy re-introduction in
the manually-backported 20421b6 (*: Add lots of Context and options
arguments, 2020-07-24, openshift#470).

[1]: https://bugzilla.redhat.com/show_bug.cgi?id=1916384#c10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants