Bug 2034484: Library go bump by eggfoobar · Pull Request #716 · openshift/cluster-version-operator

eggfoobar · 2021-12-16T06:09:17Z

Bumping the library-go dependency to latest, this should take advantage of leader election changes for SNO clusters proposed in this library-go PR and performance improvements in this PR.

Changes:

updated library-go to latest
added logic to make use of leader election configs from library-go

Generated with: $ go get github.com/openshift/library-go $ go mod tidy $ go mod vendor $ git add -A go.* vendor using: $ go version go version go1.17.3 linux/amd64

Catching up with [1]. [1]: kubernetes/kubernetes#105979

jottofar · 2021-12-16T16:39:04Z

This appears to be a dup (at least partially) of #710.
/hold
/cc @wking

ggiguash · 2021-12-22T16:22:52Z

/retest-required

ggiguash · 2021-12-23T20:27:21Z

/retest-required

ggiguash · 2021-12-27T09:20:18Z

/retest-required

qJkee · 2021-12-28T08:53:56Z

@jottofar
This PR brings the latest changes of library-go with OCPVE team features, while #710 is the old one

vrutkovs · 2022-01-03T09:05:19Z

/approve
/lgtm
/retest

Trevor, please pull the hold if this looks good to you as well

wking · 2022-01-04T00:33:25Z

f81233d touches go.mod and go.sum, but doesn't seem to change any vendored code. Can we drop that commit or squash it into 577e121 or something?

wking · 2022-01-04T00:35:10Z

edc3628 looks like it could get squashed into b92dee3 as well.

wking · 2022-01-04T00:56:32Z

Ok, I think I've wrapped my head around this, and the changes break down like:

Bump library-go, as in vendor: Bump library-go #710.
Fix Handler -> ProbeHandler, as in vendor: Bump library-go #710.
Move from ObjectReflectDiff to github.com/google/go-cmp/cmp.Diff, which was the bit I was missing in vendor: Bump library-go #710. I've just updated vendor: Bump library-go #710 with b859c46 to bring over this part.
The pkg/start changes, which is the point of the single-node effort.

I think those should be separate commits, and my just-updated #710 covers the first three points as a no-functional-change vendor bump. That would leave this pull request for the final pkg/start changes. Or feel free to generate your own commits for all of those steps in this PR, if you'd rather not use the ones from #710. But I'd rather not have vendor/ changes mixed into the same commit that lands the pkg/start changes, because then it's harder to find the "a human carefully crafted $CHANGES because $REASONS" portion inside the "and then the robots bumped all of these vendor/` files too" noise ;).

Because ObjectReflectDiff is deprecated and doesn't handle the internal 'id' property. We're using the recommended replacement [1]. [1]: https://github.com/kubernetes/apimachinery/blob/de7147de41c9331e4697399f48564f1d970cc01e/pkg/util/diff/diff.go#L70

Now that we're using these in the pkg/payload unit tests. Generated with: $ go mod tidy $ go mod vendor $ git add -A go.* vendor using: $ go version go version go1.17.3 linux/amd64

leader election configs are now delegated to library-go and are topology aware integration test was updated to set the new leader election configs ran: go mod tidy && go mod vendor && go mod verify Signed-off-by: ehila <ehila@redhat.com>

eggfoobar · 2022-01-04T15:44:33Z

Thanks for the break down @wking , I'm all for making things easier to reason about. So if we want to merge in #710 I can rebase off of that and we can have this be a much simpler PR. I did pull in your changes into this PR, just incase, if we want to take that route then this PR is ready to go.

wking · 2022-01-07T08:32:04Z

+
+	if infra, err := clusterstatus.GetClusterInfraStatus(ctx, restcfg); err == nil && infra != nil {
+		if infra.ControlPlaneTopology == configv1.SingleReplicaTopologyMode {
+			return libgoleaderelection.LeaderElectionSNOConfig(defaultLeaderElection)


We're not all that particular about our lease settings, and we do have reasonably solid graceful shutdown logic where we release the leases when termination gives us enough time to do that. So I'm personally fine simplifying all of this and just using libgoleaderelection.LeaderElectionSNOConfig all the time, regardless of the Infrastructure configuration. Thoughts?

vrutkovs · 2022-01-07T09:03:48Z

/retest

openshift-ci · 2022-01-07T14:15:39Z

@eggfoobar: This pull request references Bugzilla bug 2034484, which is valid. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla (jhou@redhat.com), skipping review request.

Details

In response to this:

Bug 2034484: Library go bump

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

/hold cancel
/lgtm

etcd leader election and PDB issues are unrelated:

/override ci/prow/e2e-agnostic-upgrade

KubePersistentVolumeErrors are unrelated:

/override ci/prow/e2e-agnostic

openshift-ci · 2022-01-07T17:33:05Z

@wking: Overrode contexts on behalf of wking: ci/prow/e2e-agnostic, ci/prow/e2e-agnostic-upgrade

Details

In response to this:

/hold cancel
/lgtm

etcd leader election and PDB issues are unrelated:

/override ci/prow/e2e-agnostic-upgrade

KubePersistentVolumeErrors are unrelated:

/override ci/prow/e2e-agnostic

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 · 2022-01-07T17:33:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eggfoobar, vrutkovs, wking

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [vrutkovs,wking]

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

openshift-ci · 2022-01-07T18:35:53Z

@eggfoobar: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

openshift-ci · 2022-01-07T18:37:04Z

@eggfoobar: Some pull requests linked via external trackers have merged:

The following pull requests linked via external trackers have not merged:

openshift/cloud-credential-operator#434 is open

These pull request must merge or be unlinked from the Bugzilla bug in order for it to move to the next state. Once unlinked, request a bug refresh with /bugzilla refresh.

Bugzilla bug 2034484 has not been moved to the MODIFIED state.

Details

In response to this:

Bug 2034484: Library go bump

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 added 2 commits December 10, 2021 15:29

vendor: Bump library-go

1e3eb2b

Generated with: $ go get github.com/openshift/library-go $ go mod tidy $ go mod vendor $ git add -A go.* vendor using: $ go version go version go1.17.3 linux/amd64

lib/resourcemerge/core: Replace Probe with ProbeHandler

5936796

Catching up with [1]. [1]: kubernetes/kubernetes#105979

openshift-ci Bot requested review from vrutkovs and wking December 16, 2021 06:09

eggfoobar force-pushed the library_go_bump branch from bd0ed99 to efabb58 Compare December 16, 2021 09:25

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 16, 2021

vrutkovs mentioned this pull request Jan 3, 2022

vendor: Bump library-go #710

Closed

openshift-ci Bot assigned vrutkovs Jan 3, 2022

openshift-ci Bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 3, 2022

vrutkovs mentioned this pull request Jan 3, 2022

[WIP] Library go bump test #720

Closed

eggfoobar and others added 2 commits January 4, 2022 09:06

vendor: Invlude github.com/google/go-cmp/cmp/cmpopts

f867479

Now that we're using these in the pkg/payload unit tests. Generated with: $ go mod tidy $ go mod vendor $ git add -A go.* vendor using: $ go version go version go1.17.3 linux/amd64

eggfoobar force-pushed the library_go_bump branch from edc3628 to 666d502 Compare January 4, 2022 14:21

openshift-ci Bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 4, 2022

eggfoobar force-pushed the library_go_bump branch from 666d502 to a6ec7eb Compare January 4, 2022 14:35

wking reviewed Jan 7, 2022

View reviewed changes

eggfoobar changed the title ~~Library go bump~~ Bug 2034484: Library go bump Jan 7, 2022

openshift-ci Bot added the bugzilla/severity-high Referenced Bugzilla bug's severity is high for the branch this PR is targeting. label Jan 7, 2022

openshift-ci Bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jan 7, 2022

wking approved these changes Jan 7, 2022

View reviewed changes

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 7, 2022

openshift-ci Bot assigned wking Jan 7, 2022

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jan 7, 2022

openshift-merge-robot merged commit addb685 into openshift:master Jan 7, 2022

Conversation

eggfoobar commented Dec 16, 2021

Uh oh!

jottofar commented Dec 16, 2021

Uh oh!

ggiguash commented Dec 22, 2021

Uh oh!

ggiguash commented Dec 23, 2021

Uh oh!

ggiguash commented Dec 27, 2021

Uh oh!

qJkee commented Dec 28, 2021

Uh oh!

vrutkovs commented Jan 3, 2022

Uh oh!

wking commented Jan 4, 2022

Uh oh!

wking commented Jan 4, 2022

Uh oh!

wking commented Jan 4, 2022

Uh oh!

eggfoobar commented Jan 4, 2022

Uh oh!

wking Jan 7, 2022

Choose a reason for hiding this comment

Uh oh!

vrutkovs commented Jan 7, 2022

Uh oh!

openshift-ci Bot commented Jan 7, 2022

Uh oh!

wking left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Jan 7, 2022

Uh oh!

openshift-ci Bot commented Jan 7, 2022

Uh oh!

openshift-ci Bot commented Jan 7, 2022

Uh oh!

openshift-ci Bot commented Jan 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants