Bug 2033751: Library go bump by eggfoobar · Pull Request #2880 · openshift/machine-config-operator

eggfoobar · 2021-12-16T04:44:37Z

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.

- What I did

The latest library-go exposes methods for leader election related to SNO topology, this change will fetch cluster topology on start up and if the operator is running in an SNO cluster, a different set of leader election configs will be used, otherwise the normal conventions will be used for typical clusters.

- How to verify it

Leader election leaseDuration should be 137s for HA clusters as defined in the ha conventions based off of these calculations and 270s for SNO clusters and these calculations

- Description for the changelog

library-go bump to latest
controller-runtime update for k8s v1.23
changed leader election config to pull from library-go
updated struct Handler name change to ProbeHandler for k8s 1.23

sinnykumari · 2021-12-16T15:37:28Z

Some of the required tests are failing, need fixing

sinnykumari · 2021-12-16T15:43:18Z

Can we update k8s.io/kubernetes as well to 1.23 in replace section of go.mod?

sinnykumari

Few comments, other than that LGTM,

eggfoobar · 2021-12-16T17:48:00Z

Absolutely, digging into some of those test errors, and I'll update k8s, thanks!

sinnykumari · 2021-12-17T16:02:20Z

You will need to run make go-deps && make update as well and commit the updated vendored changes.

sinnykumari · 2021-12-17T17:18:16Z

With Feature Freeze in place, this PR will require a valid bug.

sinnykumari · 2021-12-20T13:59:58Z

/retest

sinnykumari · 2021-12-20T21:04:16Z

After discussion with MCO team, we think that this is not really a bug and we are post Feature Freeze that means only bug should go in. Putting hold and this can go once master is open for 4.11
/hold

damemi · 2021-12-21T12:31:13Z

Hi, just wondering what is the current status of this? We are seeing some failures in CI for the main 1.23 rebase (openshift/kubernetes#1087) around MCO and wondering if bumping it to 1.23 will resolve it. Updating to 1.23 should be done for 4.10

damemi · 2021-12-21T13:51:14Z

/retitle Bug 2033751: Library go bump
this is the 1.23 rebase BZ you can use

openshift-ci · 2021-12-21T13:51:21Z

@eggfoobar: This pull request references Bugzilla bug 2033751, 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)

Requesting review from QA contact:
/cc @wangke19

Details

In response to this:

Bug 2033751: 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.

kikisdeliveryservice · 2021-12-21T17:40:08Z

/retest-required

kikisdeliveryservice · 2021-12-21T20:39:46Z

/test e2e-gcp-op-single-node
/test e2e-aws-upgrade-single-node
/test e2e-vsphere-upgrade

kikisdeliveryservice · 2021-12-21T20:41:43Z

Required tests are passing just taking a look at a few others before approving.

Getting confirmation elsewhere that this should go in.

kikisdeliveryservice · 2021-12-21T22:43:02Z

e2e-agnostic-upgrade seems to be hitting unrelated errors:

 Container test exited with code 1, reason Error
---
operator cloud-controller-manager CloudConfigControllerDegraded is False with AsExpected: Cloud Config Controller works as expected
level=info msg=Cluster operator etcd RecentBackup is Unknown with ControllerStarted: 
level=info msg=Cluster operator insights Disabled is False with AsExpected: 
level=info msg=Cluster operator monitoring Progressing is True with RollOutInProgress: Rolling out the stack.
level=error msg=Cluster operator monitoring Degraded is True with UpdatingPrometheusK8SFailed: Failed to rollout the stack. Error: updating prometheus-k8s: waiting for Prometheus object changes failed: waiting for Prometheus openshift-monitoring/k8s: expected 2 replicas, got 1 updated replicas
level=info msg=Cluster operator monitoring Available is False with MultipleTasksFailed: Rollout of the monitoring stack failed and is degraded. Please investigate the degraded status error.
level=info msg=Cluster operator network ManagementStateDegraded is False with : 
level=error msg=Cluster initialization failed because one or more operators are not functioning properly.
level=error msg=The cluster should be accessible for troubleshooting as detailed in the documentation linked below,
level=error msg=https://docs.openshift.com/container-platform/latest/support/troubleshooting/troubleshooting-installations.html
level=error msg=The 'wait-for install-complete' subcommand can then be used to continue the installation
level=fatal msg=failed to initialize the cluster: Cluster operator monitoring is not available
Setup phase finished, prepare env for next steps

https://prow.ci.openshift.org/view/gs/origin-ci-test/pr-logs/pull/openshift_machine-config-operator/2880/pull-ci-openshift-machine-config-operator-master-e2e-agnostic-upgrade/1473397169183526912

/test e2e-agnostic-upgrade

kikisdeliveryservice

Overall, PR is good, but would like a few changes for consistency and a couple questions.

Thanks!

kikisdeliveryservice · 2021-12-22T03:11:50Z

Any background on this change? This was added a few months ago from @Prashanth684 who said it would only be external once there was a CCM for PowerVS, did that happen (I'm unsure about the status)

See: #2801 (comment)

correct. this should not be external yet and the comment referenced above still holds. could this change be reverted please.

I don't have the full context to make a recommendation here, the unit test will break since that information seems to be coming from library-go added in this commit

hmm..yeah..i'm checking with the IBM folks but looks like it has been marked as an external provider and i do see changes in CCM too, so this should be good. i'll confirm once i hear back from them.

alright yeah so CCM now includes PowerVS: openshift/cluster-cloud-controller-manager-operator#129 which is supported through the IBM cloud provider, so this change is good! thanks @kikisdeliveryservice for notifying and thanks @eggfoobar for this change!

kikisdeliveryservice · 2021-12-23T01:17:50Z

This is going to need a rebase otherise looks good thanks for the updates!

openshift-bot · 2022-01-05T05:58:54Z

/retest-required