openshift / machine-api-provider-ibmcloud

Apache License 2.0
0 stars 12 forks source link

OCPBUGS-28965: update textlogger usage #35

Closed elmiko closed 9 months ago

elmiko commented 9 months ago

This change adds the configuration and command line flags setup for the textlogger configuration. It is being added to address an error when botting with the current code, that was changed to an updated klogr during the kubernetes 1.29 update. The configuration code is inspired by the machine-api-provider-vsphere main function.

openshift-ci-robot commented 9 months ago

@elmiko: This pull request references Jira Issue OCPBUGS-28965, which is invalid:

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

The bug has been updated to refer to the pull request using the external bug tracker.

In response to [this](https://github.com/openshift/machine-api-provider-ibmcloud/pull/35): >This change adds the configuration and command line flags setup for the textlogger configuration. It is being added to address an error when botting with the current code, that was changed to an updated klogr during the kubernetes 1.29 update. The configuration code is inspired by the machine-api-provider-vsphere main function. Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fmachine-api-provider-ibmcloud). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
elmiko commented 9 months ago

/jira refresh

openshift-ci-robot commented 9 months ago

@elmiko: This pull request references Jira Issue OCPBUGS-28965, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug * bug is open, matching expected state (open) * bug target version (4.16.0) matches configured target version for branch (4.16.0) * bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact: /cc @sunzhaohua2

In response to [this](https://github.com/openshift/machine-api-provider-ibmcloud/pull/35#issuecomment-1932956060): >/jira refresh Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fmachine-api-provider-ibmcloud). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
elmiko commented 9 months ago

updated with Joel's suggestion

MayXuQQ commented 9 months ago

pre-merge test with e8aed223

$ oc get po -n openshift-machine-api
NAME READY STATUS RESTARTS AGE cluster-autoscaler-operator-6c4f8f7cd6-szgx2 2/2 Running 0 72m cluster-baremetal-operator-544f778588-wb5jt 2/2 Running 0 72m control-plane-machine-set-operator-89cc757b4-4qjx2 1/1 Running 0 72m machine-api-controllers-5c7b86795d-xjr4g 6/7 CrashLoopBackOff 17 (70s ago) 64m machine-api-operator-6c548cfd5f-vtvdg 2/2 Running 0 71m

$ oc logs -n openshift-machine-api -c machine-controller machine-api-controllers-5c7b86795d-xjr4g flag provided but not defined: -logtostderr Usage of /machine-controller-manager: -health-addr string ...

elmiko commented 9 months ago

@cjschaef happy for any help, and i thought i did remove that line in the latest version but seems like we still have a problem.

elmiko commented 9 months ago

updated to set the logtostderr in a similar manner as the vsphere code

cjschaef commented 9 months ago

Do we need to add the flag to the binary in order to support that klog option? Since it appears to be the same error yet. https://github.com/openshift/machine-api-operator/blob/8d0f4a1ea47f92c2e78273c24f549202f1ae8178/cmd/vsphere/main.go#L80-L84

I can try testing that out today, and some other tweaks if necessary to see what I find out.

cjschaef commented 9 months ago

I did some local testing and used a custom release and this appears to work properly

diff --git a/cmd/manager/main.go b/cmd/manager/main.go
index a5317135..34934a8c 100644
--- a/cmd/manager/main.go
+++ b/cmd/manager/main.go
@@ -90,11 +90,20 @@ func main() {
                "Address for hosting metrics",
        )

+       logToStderr := flag.Bool(
+               "logtostderr",
+               true,
+               "Log to Stderr instead of files",
+       )
+
        textLoggerConfig := textlogger.NewConfig()
        textLoggerConfig.AddFlags(flag.CommandLine)
        ctrl.SetLogger(textlogger.NewLogger(textLoggerConfig))
-       klog.LogToStderr(true)
+
        flag.Parse()
+       if logToStderr != nil {
+               klog.LogToStderr(*logToStderr)
+       }

        if *printVersion {
                fmt.Println(version.String)

Here's the commit I added on top of this PR (if that is easier to see diffs): https://github.com/cjschaef/machine-api-provider-ibmcloud/commit/28bdf2551e0d242748f8e473d1bd5ba7161c79f8

machine-api-controllers-5749bdff54-5r5pp              7/7     Running   0          5m19s
machine-api                                4.16.0-0.nightly-2024-02-08-073857   True        False         False      3m42s
elmiko commented 9 months ago

added @cjschaef 's commit into mine, i squashed for brevity but added co-author reference =)

openshift-ci[bot] commented 9 months ago

@elmiko: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-ibmcloud 3f0bdc5321c08b3df9b05ee714641d66a1455d33 link false /test e2e-ibmcloud

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
JoelSpeed commented 9 months ago

E2E looking better now /lgtm /approve

openshift-ci[bot] commented 9 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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: - ~~[OWNERS](https://github.com/openshift/machine-api-provider-ibmcloud/blob/main/OWNERS)~~ [JoelSpeed] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
openshift-ci-robot commented 9 months ago

@elmiko: Jira Issue OCPBUGS-28965: All pull requests linked via external trackers have merged:

Jira Issue OCPBUGS-28965 has been moved to the MODIFIED state.

In response to [this](https://github.com/openshift/machine-api-provider-ibmcloud/pull/35): >This change adds the configuration and command line flags setup for the textlogger configuration. It is being added to address an error when botting with the current code, that was changed to an updated klogr during the kubernetes 1.29 update. The configuration code is inspired by the machine-api-provider-vsphere main function. Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fmachine-api-provider-ibmcloud). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-bot commented 9 months ago

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-ibmcloud-machine-controllers-container-v4.16.0-202402121210.p0.gb9b0681.assembly.stream.el9 for distgit ose-ibmcloud-machine-controllers. All builds following this will include this PR.

openshift-merge-robot commented 9 months ago

Fix included in accepted release 4.16.0-0.nightly-2024-02-17-013806