Bug 2034484: feat: library-go bump and leader election conventions by eggfoobar · Pull Request #795 · openshift-kni/performance-addon-operators

eggfoobar · 2021-12-13T17:00:56Z

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

Changes:

updated library-go to latest
updated leader eleciton to follow convention and use SNO topology aware methods

Signed-off-by: ehila ehila@redhat.com

openshift-ci · 2021-12-13T17:01:06Z

Hi @eggfoobar. Thanks for your PR.

I'm waiting for a openshift-kni 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.

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.

cynepco3hahue · 2021-12-14T07:41:09Z

/ok-to-test

coveralls · 2021-12-14T16:28:17Z

Pull Request Test Coverage Report for Build 2194

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 60.598%

Totals
Change from base Build 2187:	0.0%
Covered Lines:	1581
Relevant Lines:	2609

💛 - Coveralls

yanirq · 2021-12-15T08:32:12Z

 			return reservedCPUSet.Result(), isolatedCPUSet, fmt.Errorf("can't allocate odd number of CPUs from a NUMA Node")
 		}
-		addCoresToCPUSet(reservedCPUSet, max, node.Cores)
+		addCoresToCPUSet(*reservedCPUSet, max, node.Cores)


I think a rebase is needed to make sure we are aligned with the latest and remove these diffs

Sounds good, I rebased but that change seems to be coming from the updated kubernetes pkg, the builder return type is a pointer now

this probably causes the current errors we get in the e2e tests. we will need to take a look track all the cpuset.CPUSet occurrences and handling and see if they are aligned with the change (also in the tests themselves)

I think you'll also need to examine the functions such as

performance-addon-operators/pkg/profilecreator/profilecreator.go

Line 356 in 4a65bd7

func (ghwHandler GHWHandler) getCPUsSplitAcrossNUMA(reservedCPUCount int, htEnabled bool, topologyInfoNodes []*topology.Node) (cpuset.CPUSet, cpuset.CPUSet, error) {

This should start by looking at the failing tests around cpusets under ci/prow/ci job

the rebase and the related fixes should be addressed on a separate PR. Do we need kube >= 1.23?

ffromani

vendor changes should be in their own commit to make the change manageable

ffromani · 2021-12-17T12:38:21Z

-	k8s.io/apiextensions-apiserver v0.22.3
-	k8s.io/apimachinery v0.22.3
-	k8s.io/client-go v0.22.3
+	k8s.io/api v0.23.0


what are the minimum dependencies of the library-go version we need for the new leader election mechanism?

The leader election methods are part of the latest library-go, but just before the 0.23.0 rebase. Given the amount of failures I'll go ahead and just bump to that commit.

ffromani · 2021-12-17T12:40:34Z

 			return reservedCPUSet.Result(), isolatedCPUSet, fmt.Errorf("can't allocate odd number of CPUs from a NUMA Node")
 		}
-		addCoresToCPUSet(reservedCPUSet, max, node.Cores)
+		addCoresToCPUSet(*reservedCPUSet, max, node.Cores)


the rebase and the related fixes should be addressed on a separate PR. Do we need kube >= 1.23?

ffromani · 2021-12-17T15:18:46Z

I think we should handle the rebase (and fix our code, because the rebase uncovered a bug) separately: #812

yanirq · 2021-12-20T08:33:51Z

I think we should handle the rebase (and fix our code, because the rebase uncovered a bug) separately: #812

#812 is merged. lets rebase.

ffromani · 2021-12-20T08:45:35Z

@eggfoobar please rebase, all of the failures should be solved now and your change should apply cleanly

ffromani · 2021-12-21T06:23:48Z

@@ -0,0 +1,36 @@
+package leaderelection


this can probably put in pkg/leaderelection (skip utils) but not a big deal, and we can move later

ffromani · 2021-12-21T06:24:06Z

 	github.com/openshift/api v3.9.1-0.20191111211345-a27ff30ebf09+incompatible
 	github.com/openshift/cluster-node-tuning-operator v0.0.0-20200914165052-a39511828cf0
 	github.com/openshift/custom-resource-status v0.0.0-20200602122900-c002fd1547ca
+	github.com/openshift/library-go v0.0.0-20211220195323-eca2c467c492


why another lib bump? can we squash in the previous?

Absolutely let me squash things up, the bump is to just make sure we're on the latest version that pulls in this PR 1273

ffromani · 2021-12-21T06:24:15Z

 	github.com/modern-go/concurrent v0.0.0-20180306012644-bacd9c7ef1dd // indirect
 	github.com/modern-go/reflect2 v1.0.2 // indirect
 	github.com/nxadm/tail v1.4.8 // indirect
+	github.com/openshift/client-go v0.0.0-20210916133943-9acee1a0fb83 // indirect


why yet another lib bump? can we squash in the previous?

Signed-off-by: ehila <ehila@redhat.com>

updated to use new library-go methods for leader election defaults leader election is now defaulting to library-go and uses different configs for SNO topology clusters Signed-off-by: ehila <ehila@redhat.com>

eggfoobar · 2021-12-21T16:02:47Z

/retitle Bug 2034484: feat: library-go bump and leader election conventions
This PR bumps to latest library-go to utilize changes for performance gains on SNO DU clusters

openshift-ci · 2021-12-21T16:02:55Z

@eggfoobar: Re-titling can only be requested by trusted users, like repository collaborators.

Details

In response to this:

/retitle Bug 2034484: feat: library-go bump and leader election conventions
This PR bumps to latest library-go to utilize changes for performance gains on SNO DU clusters

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.

yanirq · 2021-12-21T16:26:54Z

/retitle Bug 2034484: feat: library-go bump and leader election conventions

openshift-ci · 2021-12-21T16:27:01Z

@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: feat: library-go bump and leader election conventions

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.

yanirq · 2021-12-27T17:47:32Z

/approve

openshift-ci · 2021-12-27T17:47:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: eggfoobar, yanirq

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~~ [yanirq]

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

yanirq · 2021-12-27T17:50:39Z

/lgtm

openshift-ci · 2021-12-27T17:52:14Z

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

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

openshift/cluster-authentication-operator#525 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: feat: library-go bump and leader election conventions

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 added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 13, 2021

openshift-ci Bot requested review from ffromani and yanirq December 13, 2021 17:00

openshift-ci Bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Dec 13, 2021

openshift-ci Bot 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 Dec 14, 2021

yanirq mentioned this pull request Dec 14, 2021

feat: library-go bump openshift/cluster-node-tuning-operator#301

Merged

yanirq reviewed Dec 14, 2021

View reviewed changes

Comment thread pkg/utils/leaderelection/leaderelection.go

eggfoobar force-pushed the library_go_bump branch 2 times, most recently from 3cc3a48 to d7198be Compare December 14, 2021 16:25

eggfoobar force-pushed the library_go_bump branch from d7198be to 13aa05a Compare December 14, 2021 18:59

yanirq reviewed Dec 15, 2021

View reviewed changes

eggfoobar force-pushed the library_go_bump branch from 13aa05a to 4058a52 Compare December 15, 2021 15:09

eggfoobar changed the title ~~[WIP] feat: library-go bump and leader election conventons~~ feat: library-go bump and leader election conventions Dec 15, 2021

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 15, 2021

ffromani reviewed Dec 17, 2021

View reviewed changes

eggfoobar force-pushed the library_go_bump branch from 4058a52 to c99b586 Compare December 17, 2021 15:15

eggfoobar force-pushed the library_go_bump branch from c99b586 to 13e222e Compare December 20, 2021 20:24

ffromani reviewed Dec 21, 2021

View reviewed changes

eggfoobar added 2 commits December 21, 2021 10:58

feat: bumped library-go to use latest leader election changes

d7e7499

Signed-off-by: ehila <ehila@redhat.com>

feat: updated to leader election configs

1e45af4

updated to use new library-go methods for leader election defaults leader election is now defaulting to library-go and uses different configs for SNO topology clusters Signed-off-by: ehila <ehila@redhat.com>

eggfoobar force-pushed the library_go_bump branch from 283c03f to 1e45af4 Compare December 21, 2021 16:00

openshift-ci Bot changed the title ~~feat: library-go bump and leader election conventions~~ Bug 2034484: feat: library-go bump and leader election conventions Dec 21, 2021

openshift-ci Bot added 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. labels Dec 21, 2021

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 27, 2021

openshift-ci Bot assigned yanirq Dec 27, 2021

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Dec 27, 2021

openshift-merge-robot merged commit f1d790b into openshift-kni:master Dec 27, 2021

Conversation

eggfoobar commented Dec 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci Bot commented Dec 13, 2021

Uh oh!

cynepco3hahue commented Dec 14, 2021

Uh oh!

Uh oh!

coveralls commented Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 2194

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ffromani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ffromani commented Dec 17, 2021

Uh oh!

yanirq commented Dec 20, 2021

Uh oh!

ffromani commented Dec 20, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eggfoobar commented Dec 21, 2021

Uh oh!

openshift-ci Bot commented Dec 21, 2021

Uh oh!

yanirq commented Dec 21, 2021

Uh oh!

openshift-ci Bot commented Dec 21, 2021

Uh oh!

yanirq commented Dec 27, 2021

Uh oh!

openshift-ci Bot commented Dec 27, 2021

Uh oh!

yanirq commented Dec 27, 2021

Uh oh!

openshift-ci Bot commented Dec 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

eggfoobar commented Dec 13, 2021 •

edited

Loading

coveralls commented Dec 14, 2021 •

edited

Loading