Skip to content

feat: library-go bump#301

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
eggfoobar:library_go_bump
Dec 14, 2021
Merged

feat: library-go bump#301
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
eggfoobar:library_go_bump

Conversation

@eggfoobar
Copy link
Copy Markdown
Contributor

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
  • moved leader election config to util pkg
  • updated to use leader election configs from library-go

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

@openshift-ci 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 14, 2021
@jmencak
Copy link
Copy Markdown
Contributor

jmencak commented Dec 14, 2021

/test e2e-upgrade

@jmencak
Copy link
Copy Markdown
Contributor

jmencak commented Dec 14, 2021

/cc @yanirq something to be aware of that needs to be supported by the future NTO/PAO rewrite/merge.

@yanirq
Copy link
Copy Markdown
Contributor

yanirq commented Dec 14, 2021

/cc @yanirq something to be aware of that needs to be supported by the future NTO/PAO rewrite/merge.

rewrite will need to be rebase. Movement of PAO will contain this update openshift-kni/performance-addon-operators#795
Once we switch to controller runtime libs we will need to update the newManager builder to have:

	LeaseDuration:      &le.LeaseDuration.Duration,
	RetryPeriod:        &le.RetryPeriod.Duration,
	RenewDeadline:      &le.RenewDeadline.Duration,

@jmencak
Copy link
Copy Markdown
Contributor

jmencak commented Dec 14, 2021

/test e2e-upgrade

@jmencak
Copy link
Copy Markdown
Contributor

jmencak commented Dec 14, 2021

Thank you for the PR, @eggfoobar , however this doesn't seem to be working as expected at the moment. Tested on a HA cluster and SNO cluster and seeing HA cluster leader election values in both cases.

W1214 12:41:11.705444       1 leaderelection.go:30] unable to get cluster infrastructure status, using HA cluster values for leader election: infrastructures.config.openshift.io "cluster" is forbidden: User "system:serviceaccount:openshift-cluster-node-tuning-operator:cluster-node-tuning-operator" cannot get resource "infrastructures" in API group "config.openshift.io" at the cluster scope

Sounds like some perms are missing.

Copy link
Copy Markdown
Contributor

@jmencak jmencak left a comment

Choose a reason for hiding this comment

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

Thank you for the PR, this seems to work for me after the following change:

--- a/manifests/40-rbac.yaml
+++ b/manifests/40-rbac.yaml
@@ -48,9 +48,10 @@ rules:
 - apiGroups: [""]
   resources: ["nodes/metrics","nodes/specs"]
   verbs: ["get"]
-# Needed by every CVO-managed operator.
+# clusteroperators are needed by every CVO-managed operator,
+# infrastructures by the leader election code.
 - apiGroups: ["config.openshift.io"]
-  resources: ["clusteroperators"]
+  resources: ["clusteroperators", "infrastructures"]
   verbs: ["create","get","list","watch"]
 # Needed by every CVO-managed operator.
 - apiGroups: ["config.openshift.io"]

@eggfoobar
Copy link
Copy Markdown
Contributor Author

Thanks so much @jmencak ! Removing the WIP

@eggfoobar eggfoobar changed the title [WIP] feat: library-go bump feat: library-go bump Dec 14, 2021
@openshift-ci 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 14, 2021
@jmencak
Copy link
Copy Markdown
Contributor

jmencak commented Dec 14, 2021

Thanks so much @jmencak ! Removing the WIP

No worries, will you push the fix and squash so that I can approve this? Thank you!

updated library go to latest
moved leader election config to util pkg
updated to use leader election configs from library-go

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

feat: added rbac permission for infra resource

added infra resource rbac permission for getting cluster topology used by leader election code

Signed-off-by: ehila <ehila@redhat.com>
@eggfoobar
Copy link
Copy Markdown
Contributor Author

Absolutely, updated :)

@jmencak
Copy link
Copy Markdown
Contributor

jmencak commented Dec 14, 2021

Retested.

/lgtm
/approve

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Dec 14, 2021

[APPROVALNOTIFIER] This PR is APPROVED

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

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:

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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 14, 2021
@openshift-bot
Copy link
Copy Markdown
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

1 similar comment
@openshift-bot
Copy link
Copy Markdown
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Dec 14, 2021

@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-merge-robot openshift-merge-robot merged commit 62a385c into openshift:master Dec 14, 2021
@eggfoobar eggfoobar deleted the library_go_bump branch April 7, 2022 16:37
IlyaTyomkin pushed a commit to IlyaTyomkin/cluster-node-tuning-operator that referenced this pull request May 23, 2023
updated library go to latest
moved leader election config to util pkg
updated to use leader election configs from library-go

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

feat: added rbac permission for infra resource

added infra resource rbac permission for getting cluster topology used by leader election code

Signed-off-by: ehila <ehila@redhat.com>
IlyaTyomkin pushed a commit to IlyaTyomkin/cluster-node-tuning-operator that referenced this pull request Jun 13, 2023
updated library go to latest
moved leader election config to util pkg
updated to use leader election configs from library-go

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

feat: added rbac permission for infra resource

added infra resource rbac permission for getting cluster topology used by leader election code

Signed-off-by: ehila <ehila@redhat.com>
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. 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