Remove handling code for glog #2325

stephenfin · 2023-08-16T13:24:39Z

What this PR does / why we need it:

Everything in k8s has switched to klog for some time now. Lots more context in the individual commit messages.

Some minor issues introduced in #2324 are addressed. Again, there's lots more context in the commit messages.

Which issue this PR fixes(if applicable):
N/A

Special notes for reviewers:
N/A

Release note:

NONE

jichenjc · 2023-08-17T06:24:37Z

seems all test failed, is it related to gate issue or this PR?

stephenfin · 2023-08-17T09:27:00Z

seems all test failed, is it related to gate issue or this PR?

I think it's because I converted it to a draft and Prow doesn't run jobs on drafts so they got aborted. In any case, there's still work to do here.

jichenjc · 2023-08-28T02:37:06Z

@stephenfin you want to rebase and re-submit this PR?

stephenfin · 2023-08-28T16:21:06Z

Yup, I will pick this up again. Just side tracked on other stuff at the moment. Hopefully later this week

This is already handled by the earlier call to the (confusingly named) 'InitFlags' function provided by 'k8s.io/component-base/cli/flag' [1]. [1] https://github.com/kubernetes/component-base/blob/v0.28.1/cli/flag/flags.go#L51-L59 Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

This is already handled by the earlier call to the (confusingly named) 'InitFlags' function provided by 'k8s.io/component-base/cli/flag' [1]. We need to move the handling for the '--version' flag to after this to ensure we parse the flags first. [1] https://github.com/kubernetes/component-base/blob/v0.28.1/cli/flag/flags.go#L51-L59 Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

The comment here seems to have been left over when this module was extensively reworked in commit 7f1e9ed (kubernetes#2278). In any case, it's unnecessary: the call to 'klog.InitFlags' with a 'nil' argument results in the flags being registered against the global 'flag.CommandLine' flagset [1], but since cobra uses pflag rather than flag this doesn't do anything useful. You can validate this by simply building the binary without this change: you'll note that we only have '-v' and '-vmodule' arguments, which are actually added by 'cli.Run' [2][3][4]. As such, we can simply remove the calls. [1] https://github.com/kubernetes/klog/blob/v2.100.1/klog.go#L432-L434 [2] https://github.com/kubernetes/component-base/blob/v0.28.1/cli/run.go#L46 [3] https://github.com/kubernetes/component-base/blob/v0.28.1/cli/run.go#L120 [4] https://github.com/kubernetes/component-base/blob/v0.28.1/logs/logs.go#L73-L105 Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

The kubernetes ecosystem has migrated to klog now and the flags registered are for klog [1]. As such, there is no need to continue translating from glog to klog. [1] https://github.com/kubernetes/component-base/blob/v0.28.1/logs/logs.go#L73-L105 Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

Remove the code to handle translation of legacy glog options to klog options since glog is no longer a thing in kubernetes. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

As with our earlier change to cinder-csi-plugin, the handling code for glog is no longer necessary now that the kubernetes ecosystem has migrated to klog. However, unlike that change, client-keystone-auth is not using cobra but rather plain old pflag. As a result, it is still actually registering all the klog options. We preserve this behavior. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

This is quite similar but not identical to the earlier removal of glog handling in client-keystone-auth. As with that utility, we are using pflag rather than cobra here, but unlike that utility the klog options are not being registered. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

stephenfin · 2023-09-07T15:40:04Z

This is respun now. One thing that I noticed while reworking (and called out in many of the commit messages) is that we don't appear to be consistently registering the many options that klog provides, namely:

alsologtostderr
log_backtrace_at
log_dir
log_file
log_file_max_size
logtostderr
one_output
skip_headers
skip_log_headers
stderrthreshold
vmodule
add_dir_header

It seems that only the client-keystone-auth and magnum-auto-healer commands are registering these, while the rest are at most registering the -v and -vmodule options. My question is this: do we care? If so, do we want to (re-)add them?

(Verified with the rather dumb script below)

#!/usr/bin/env bash

for release in "1.20" "1.24" "1.28"; do
    echo "Building $release"

    # build package
    git checkout "release-${release}"
    make build

    # store '--help' strings to directory
    dir="/tmp/help-comparisons/${release}"

    mkdir -p "$dir"

    for binary in "barbican-kms-plugin" "cinder-csi-plugin" "client-keystone-auth" "k8s-keystone-auth" "magnum-auto-healer" "manila-csi-plugin" "octavia-ingress-controller" "openstack-cloud-controller-manager"; do
        "$binary" --help > "${dir}/${binary}"
    done
done

echo "Comparing help pages"

for binary in "barbican-kms-plugin" "cinder-csi-plugin" "client-keystone-auth" "k8s-keystone-auth" "magnum-auto-healer" "manila-csi-plugin" "octavia-ingress-controller" "openstack-cloud-controller-manager"; do
    if ! grep -q 'alsologtostderr' "/tmp/help-comparisons/1.20/${binary}"; then
        echo "${binary} was not registering klog options in 1.20"
        continue
    fi

    if ! grep -q 'alsologtostderr' "/tmp/help-comparisons/1.24/${binary}"; then
        echo "FAIL: ${binary} has dropped klog options in 1.24!"
    elif ! grep -q 'alsologtostderr' "/tmp/help-comparisons/1.28/${binary}"; then
        echo "FAIL: ${binary} has dropped klog options in 1.28!"
        fail=1
    else
        echo "OK: ${binary} still has klog options"
    fi
done

which yields:

FAIL: barbican-kms-plugin has dropped klog options in 1.28!                                                  
FAIL: cinder-csi-plugin has dropped klog options in 1.28!                                                    
OK: client-keystone-auth still has klog options                                                              
FAIL: k8s-keystone-auth has dropped klog options in 1.28!                                                    
OK: magnum-auto-healer still has klog options
FAIL: manila-csi-plugin has dropped klog options in 1.28!                                                    
FAIL: octavia-ingress-controller has dropped klog options in 1.28!                                           
FAIL: openstack-cloud-controller-manager has dropped klog options in 1.28!

stephenfin · 2023-09-07T15:42:49Z

Looking at the above further, I see the following in the 1.24 help strings:

$ cat /tmp/help-comparisons/1.24/client-keystone-auth 
Usage of ./client-keystone-auth:
      --add-dir-header                         If true, adds the file directory to the header of the log messages (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --alsologtostderr                        log to standard error as well as files (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      ...

Based on the KEP, I'm guessing the two users in client-keystone-auth and magnum-auto-healer should be actually be removed now. Do we want to do that in this series? Do we need a release note?

mdbooth · 2023-09-08T08:59:03Z

Looking at the above further, I see the following in the 1.24 help strings:

$ cat /tmp/help-comparisons/1.24/client-keystone-auth 
Usage of ./client-keystone-auth:
      --add-dir-header                         If true, adds the file directory to the header of the log messages (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --alsologtostderr                        log to standard error as well as files (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      ...

Based on the KEP, I'm guessing the two users in client-keystone-auth and magnum-auto-healer should be actually be removed now. Do we want to do that in this series? Do we need a release note?

Looks like it. I would personally be in favour of getting ahead of that change. It is doubtful that we have implemented these features thoughtfully, they're most likely just cribbed from elsewhere.

Probably best to do that in a separate PR, though. Also worth posting about it in slack in case anybody knows of existing users.

stephenfin · 2023-09-08T09:50:36Z

Looking at the above further, I see the following in the 1.24 help strings:
$ cat /tmp/help-comparisons/1.24/client-keystone-auth 
Usage of ./client-keystone-auth:
      --add-dir-header                         If true, adds the file directory to the header of the log messages (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --alsologtostderr                        log to standard error as well as files (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      ...
Based on the KEP, I'm guessing the two users in client-keystone-auth and magnum-auto-healer should be actually be removed now. Do we want to do that in this series? Do we need a release note?
Looks like it. I would personally be in favour of getting ahead of that change. It is doubtful that we have implemented these features thoughtfully, they're most likely just cribbed from elsewhere.

Yes, to be clear, we removed them for the other executables at some point between 1.24 and 1.28 (or rather, they were removed for us via a change to the utility functions in k8s.io/component-base). I would guess this is just a remnant caused by us doing the option registration ourselves rather than relying on one of those utility functions.

Probably best to do that in a separate PR, though. Also worth posting about it in slack in case anybody knows of existing users.

Can do.

mdbooth

IIUC we're removing handling here for a bunch of command line options which are no longer used, which is good.

My concern, which is mostly based on ignorance of the multiple different types of flag handling we have here, is that we might be dropping flags in a way which would cause the binary not to start if given those flags. Is that what your multiple version test script was doing?

If not, I think this is good to go.

If so, I think we need to come up with a deprecation process which gives users notice that they need to update their deployments.

mdbooth · 2023-09-08T09:01:49Z

cmd/client-keystone-auth/main.go


-	pflag.Parse()
+	kflag.InitFlags()


Confirmed: https://github.com/kubernetes/kubernetes/blob/9068bec08e7ff8a049b19ff329c6c677ebbc2aeb/staging/src/k8s.io/component-base/cli/flag/flags.go#L55

mdbooth · 2023-09-08T09:24:07Z

cmd/cinder-csi-plugin/main.go

-			// This is a temporary hack to enable proper logging until upstream dependencies
-			// are migrated to fully utilize klog instead of glog.
-			klogFlags := flag.NewFlagSet("klog", flag.ExitOnError)
-			klog.InitFlags(klogFlags)


Reviewer note: This copies all the options from the commandLine flagset defined here to klogFlags.

This includes --log_dir, --log_file, et al.

mdbooth · 2023-09-08T09:33:08Z

cmd/cinder-csi-plugin/main.go

-			cmd.Flags().VisitAll(func(f1 *pflag.Flag) {
-				f2 := klogFlags.Lookup(f1.Name)
-				if f2 != nil {
-					value := f1.Value.String()
-					_ = f2.Value.Set(value)
-				}
-			})


This iterates over the cobra flags. For each one it looks up a correspondingly named klog flag and sets its value to the value of the cobra flag.

IOW it has no effect on the cobra flags, which are the only flags now in use.

mdbooth · 2023-09-08T09:45:46Z

cmd/client-keystone-auth/main.go

-	// This is a temporary hack to enable proper logging until upstream dependencies
-	// are migrated to fully utilize klog instead of glog.
-	klogFlags := flag.NewFlagSet("klog", flag.ExitOnError)
-	klog.InitFlags(klogFlags)


Now done below, so we keep the same flags.

mdbooth · 2023-09-08T09:47:43Z

cmd/client-keystone-auth/main.go

-	flag.CommandLine.VisitAll(func(f1 *flag.Flag) {
-		f2 := klogFlags.Lookup(f1.Name)
-		if f2 != nil {
-			value := f1.Value.String()
-			_ = f2.Value.Set(value)
-		}
-	})


Sets flags in flag.CommandLine with a corresponding klogflag to the value of the klog flag.

Nothing is now using these flags, so this is redundant.

mdbooth · 2023-09-08T09:53:38Z

cmd/k8s-keystone-auth/main.go

-	// Sync the glog and klog flags.
-	flag.CommandLine.VisitAll(func(f1 *flag.Flag) {
-		f2 := klogFlags.Lookup(f1.Name)
-		if f2 != nil {
-			value := f1.Value.String()
-			_ = f2.Value.Set(value)
-		}
-	})


This appears to set values in flag.CommandLine, although nothing is using it?

Oh it's used alright: the kflag.InitFlags call below adds flag.CommandLine as a flagset. However, it's not doing anything useful since no upstream dependency should be using glog any more, which means there's nothing valuable to copy. You can test this by sticking some debug printf calls in here (which is what I did 😅)

mdbooth · 2023-09-08T10:02:02Z

Incidentally, I wonder if we could move the outliers to using cobra as part of the same process. The inconsistency is expensive.

stephenfin · 2023-09-08T11:15:22Z

Incidentally, I wonder if we could move the outliers to using cobra as part of the same process. The inconsistency is expensive.

Agreed, and I have a local series to do this. I just haven't pushed it yet cos sucky GitHub PR model presents everything together rather than as tidy atomic changes 🙃

stephenfin · 2023-09-08T11:29:23Z

IIUC we're removing handling here for a bunch of command line options which are no longer used, which is good.

My concern, which is mostly based on ignorance of the multiple different types of flag handling we have here, is that we might be dropping flags in a way which would cause the binary not to start if given those flags. Is that what your multiple version test script was doing?

No, it was simply testing for the presence of the various klog-specific options in --help. That's really tangential to this: as I noted in the comment, I was concerned that we had already regressed in a past release by removing these options. However, it turned out this was intentional and should have been done for all of our executables.

If not, I think this is good to go.

If so, I think we need to come up with a deprecation process which gives users notice that they need to update their deployments.

fwiw, the klog options were marked as deprecated for a release or two before they were removed. If you build and look at the help string for e.g. magnum-auto-healer in the v1.24 release, you'll see:

Auto healer is responsible for monitoring the nodes’ status periodically in the cloud environment, searching for unhealthy instances and triggering replacements when needed, maximizing the cluster’s efficiency and performance. OpenStack is supported by default.

Usage:
  magnum-auto-healer [flags]

Flags:
      --add-dir-header                   If true, adds the file directory to the header of the log messages (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --alsologtostderr                  log to standard error as well as files (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --config string                    config file (default is $HOME/.kube_autohealer_config.yaml)
  -h, --help                             help for magnum-auto-healer
      --log-backtrace-at traceLocation   when logging hits line file:N, emit a stack trace (default :0) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --log-dir string                   If non-empty, write log files in this directory (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --log-file string                  If non-empty, use this log file (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --log-file-max-size uint           Defines the maximum size a log file can grow to. Unit is megabytes. If the value is 0, the maximum file size is unlimited. (default 1800) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --log-flush-frequency duration     Maximum number of seconds between log flushes (default 5s)
      --logtostderr                      log to standard error instead of files (default true) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --one-output                       If true, only write logs to their native severity level (vs also writing to each lower severity level) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --skip-headers                     If true, avoid header prefixes in the log messages (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --skip-log-headers                 If true, avoid headers when opening log files (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
      --stderrthreshold severity         logs at or above this threshold go to stderr (default 2) (DEPRECATED: will be removed in a future release, see https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components)
  -v, --v Level                          number for the log level verbosity
      --vmodule moduleSpec               comma-separated list of pattern=N settings for file-filtered logging

Note all the DEPRECATED notes for the various options. We clearly did our due diligence there (or rather, someone did it for us and we got the behaviour from our libraries). However, for the two binaries that still have these options, we appear to have undeprecated them by mistake. In 1.28, we get the following for magnum-auto-healer:

Auto healer is responsible for monitoring the nodes’ status periodically in the cloud environment, searching for unhealthy instances and triggering replacements when needed, maximizing the cluster’s efficiency and performance. OpenStack is supported by default.

Usage:
  magnum-auto-healer [flags]

Flags:
      --add-dir-header                   If true, adds the file directory to the header of the log messages
      --alsologtostderr                  log to standard error as well as files (no effect when -logtostderr=true)
      --config string                    config file (default is $HOME/.kube_autohealer_config.yaml)
  -h, --help                             help for magnum-auto-healer
      --log-backtrace-at traceLocation   when logging hits line file:N, emit a stack trace (default :0)
      --log-dir string                   If non-empty, write log files in this directory (no effect when -logtostderr=true)
      --log-file string                  If non-empty, use this log file (no effect when -logtostderr=true)
      --log-file-max-size uint           Defines the maximum size a log file can grow to (no effect when -logtostderr=true). Unit is megabytes. If the value is 0, the maximum file size is unlimited. (default 1800)
      --log-flush-frequency duration     Maximum number of seconds between log flushes (default 5s)
      --logtostderr                      log to standard error instead of files (default true)
      --one-output                       If true, only write logs to their native severity level (vs also writing to each lower severity level; no effect when -logtostderr=true)
      --skip-headers                     If true, avoid header prefixes in the log messages
      --skip-log-headers                 If true, avoid headers when opening log files (no effect when -logtostderr=true)
      --stderrthreshold severity         logs at or above this threshold go to stderr when writing to files and stderr (no effect when -logtostderr=true or -alsologtostderr=false) (default 2)
  -v, --v Level                          number for the log level verbosity
      --version                          version for magnum-auto-healer
      --vmodule moduleSpec               comma-separated list of pattern=N settings for file-filtered logging (only works for the default text log format)

Personally, I think we can safely remove them outright (in a separate PR) but if someone wants to go through the deprecation dance again, I guess we could...

EDIT: Oh, and concerning the glog options: there were 6. 2 of them - -v and -vmodule - are still handled by all binaries. The other 4 - -log_backtrace_at, -logtostderr, -alsologtostderr, and -stderrthreshold - were handled by klog but obviously no longer are (with the exception of the two outstanding executables) after the deprecation and removal of the klog options. We didn't drop anything during the migration.

mdbooth · 2023-09-08T11:31:21Z

/lgtm

Thanks, Stephen. This is very thorough, and carefully removes some very confusing code.

stephenfin · 2023-10-03T10:18:20Z

@mdbooth Anything else needed to move this along?

jichenjc · 2023-10-03T11:22:25Z

/approve

thanks for the great update~

k8s-ci-robot · 2023-10-03T11:22:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc

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

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

* client-keystone-auth: Remove duplicate pflag.Parse call This is already handled by the earlier call to the (confusingly named) 'InitFlags' function provided by 'k8s.io/component-base/cli/flag' [1]. [1] https://github.com/kubernetes/component-base/blob/v0.28.1/cli/flag/flags.go#L51-L59 Signed-off-by: Stephen Finucane <stephenfin@redhat.com> * k8s-keystone-auth: Remove duplicate pflag.Parse call This is already handled by the earlier call to the (confusingly named) 'InitFlags' function provided by 'k8s.io/component-base/cli/flag' [1]. We need to move the handling for the '--version' flag to after this to ensure we parse the flags first. [1] https://github.com/kubernetes/component-base/blob/v0.28.1/cli/flag/flags.go#L51-L59 Signed-off-by: Stephen Finucane <stephenfin@redhat.com> * barbican-kms-plugin: Remove unnecessary klog flag calls The comment here seems to have been left over when this module was extensively reworked in commit 7f1e9ed (kubernetes#2278). In any case, it's unnecessary: the call to 'klog.InitFlags' with a 'nil' argument results in the flags being registered against the global 'flag.CommandLine' flagset [1], but since cobra uses pflag rather than flag this doesn't do anything useful. You can validate this by simply building the binary without this change: you'll note that we only have '-v' and '-vmodule' arguments, which are actually added by 'cli.Run' [2][3][4]. As such, we can simply remove the calls. [1] https://github.com/kubernetes/klog/blob/v2.100.1/klog.go#L432-L434 [2] https://github.com/kubernetes/component-base/blob/v0.28.1/cli/run.go#L46 [3] https://github.com/kubernetes/component-base/blob/v0.28.1/cli/run.go#L120 [4] https://github.com/kubernetes/component-base/blob/v0.28.1/logs/logs.go#L73-L105 Signed-off-by: Stephen Finucane <stephenfin@redhat.com> * cinder-csi-plugin: Remove handling for glog The kubernetes ecosystem has migrated to klog now and the flags registered are for klog [1]. As such, there is no need to continue translating from glog to klog. [1] https://github.com/kubernetes/component-base/blob/v0.28.1/logs/logs.go#L73-L105 Signed-off-by: Stephen Finucane <stephenfin@redhat.com> * manila-csi-plugin: Remove handling for glog Remove the code to handle translation of legacy glog options to klog options since glog is no longer a thing in kubernetes. Signed-off-by: Stephen Finucane <stephenfin@redhat.com> * client-keystone-auth: Remove handling for glog As with our earlier change to cinder-csi-plugin, the handling code for glog is no longer necessary now that the kubernetes ecosystem has migrated to klog. However, unlike that change, client-keystone-auth is not using cobra but rather plain old pflag. As a result, it is still actually registering all the klog options. We preserve this behavior. Signed-off-by: Stephen Finucane <stephenfin@redhat.com> * k8s-keystone-auth: Remove handling for glog This is quite similar but not identical to the earlier removal of glog handling in client-keystone-auth. As with that utility, we are using pflag rather than cobra here, but unlike that utility the klog options are not being registered. Signed-off-by: Stephen Finucane <stephenfin@redhat.com> --------- Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

jichenjc · 2024-02-07T08:39:26Z

/cherry-pick release-1.28

jichenjc · 2024-02-07T08:39:31Z

/cherry-pick release-1.27

jichenjc · 2024-02-07T08:39:35Z

/cherry-pick release-1.26

k8s-infra-cherrypick-robot · 2024-02-07T08:40:07Z

@jichenjc: #2325 failed to apply on top of branch "release-1.27":

Applying: client-keystone-auth: Remove duplicate pflag.Parse call
Using index info to reconstruct a base tree...
M	cmd/client-keystone-auth/main.go
Falling back to patching base and 3-way merge...
Auto-merging cmd/client-keystone-auth/main.go
CONFLICT (content): Merge conflict in cmd/client-keystone-auth/main.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 client-keystone-auth: Remove duplicate pflag.Parse call
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.27

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.

k8s-infra-cherrypick-robot · 2024-02-07T08:40:08Z

@jichenjc: new pull request created: #2534

In response to this:

/cherry-pick release-1.28

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.

k8s-infra-cherrypick-robot · 2024-02-07T08:40:09Z

@jichenjc: #2325 failed to apply on top of branch "release-1.26":

Applying: client-keystone-auth: Remove duplicate pflag.Parse call
Using index info to reconstruct a base tree...
M	cmd/client-keystone-auth/main.go
Falling back to patching base and 3-way merge...
Auto-merging cmd/client-keystone-auth/main.go
CONFLICT (content): Merge conflict in cmd/client-keystone-auth/main.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 client-keystone-auth: Remove duplicate pflag.Parse call
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.26

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.

* client-keystone-auth: Remove duplicate pflag.Parse call This is already handled by the earlier call to the (confusingly named) 'InitFlags' function provided by 'k8s.io/component-base/cli/flag' [1]. [1] https://github.com/kubernetes/component-base/blob/v0.28.1/cli/flag/flags.go#L51-L59 Signed-off-by: Stephen Finucane <stephenfin@redhat.com> * k8s-keystone-auth: Remove duplicate pflag.Parse call This is already handled by the earlier call to the (confusingly named) 'InitFlags' function provided by 'k8s.io/component-base/cli/flag' [1]. We need to move the handling for the '--version' flag to after this to ensure we parse the flags first. [1] https://github.com/kubernetes/component-base/blob/v0.28.1/cli/flag/flags.go#L51-L59 Signed-off-by: Stephen Finucane <stephenfin@redhat.com> * barbican-kms-plugin: Remove unnecessary klog flag calls The comment here seems to have been left over when this module was extensively reworked in commit 7f1e9ed (kubernetes#2278). In any case, it's unnecessary: the call to 'klog.InitFlags' with a 'nil' argument results in the flags being registered against the global 'flag.CommandLine' flagset [1], but since cobra uses pflag rather than flag this doesn't do anything useful. You can validate this by simply building the binary without this change: you'll note that we only have '-v' and '-vmodule' arguments, which are actually added by 'cli.Run' [2][3][4]. As such, we can simply remove the calls. [1] https://github.com/kubernetes/klog/blob/v2.100.1/klog.go#L432-L434 [2] https://github.com/kubernetes/component-base/blob/v0.28.1/cli/run.go#L46 [3] https://github.com/kubernetes/component-base/blob/v0.28.1/cli/run.go#L120 [4] https://github.com/kubernetes/component-base/blob/v0.28.1/logs/logs.go#L73-L105 Signed-off-by: Stephen Finucane <stephenfin@redhat.com> * cinder-csi-plugin: Remove handling for glog The kubernetes ecosystem has migrated to klog now and the flags registered are for klog [1]. As such, there is no need to continue translating from glog to klog. [1] https://github.com/kubernetes/component-base/blob/v0.28.1/logs/logs.go#L73-L105 Signed-off-by: Stephen Finucane <stephenfin@redhat.com> * manila-csi-plugin: Remove handling for glog Remove the code to handle translation of legacy glog options to klog options since glog is no longer a thing in kubernetes. Signed-off-by: Stephen Finucane <stephenfin@redhat.com> * client-keystone-auth: Remove handling for glog As with our earlier change to cinder-csi-plugin, the handling code for glog is no longer necessary now that the kubernetes ecosystem has migrated to klog. However, unlike that change, client-keystone-auth is not using cobra but rather plain old pflag. As a result, it is still actually registering all the klog options. We preserve this behavior. Signed-off-by: Stephen Finucane <stephenfin@redhat.com> * k8s-keystone-auth: Remove handling for glog This is quite similar but not identical to the earlier removal of glog handling in client-keystone-auth. As with that utility, we are using pflag rather than cobra here, but unlike that utility the klog options are not being registered. Signed-off-by: Stephen Finucane <stephenfin@redhat.com> --------- Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 16, 2023

k8s-ci-robot requested review from dulek and jichenjc August 16, 2023 13:24

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 16, 2023

stephenfin force-pushed the glog-cleanup branch from f0af5ee to aae1a87 Compare August 16, 2023 13:25

stephenfin marked this pull request as draft August 16, 2023 13:26

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 16, 2023

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 17, 2023

stephenfin added 7 commits September 7, 2023 15:17

manila-csi-plugin: Remove handling for glog

4b92df2

Remove the code to handle translation of legacy glog options to klog options since glog is no longer a thing in kubernetes. Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

stephenfin force-pushed the glog-cleanup branch from aae1a87 to c57e0ad Compare September 7, 2023 14:21

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 7, 2023

stephenfin marked this pull request as ready for review September 7, 2023 14:23

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 7, 2023

k8s-ci-robot requested a review from mdbooth September 7, 2023 14:23

mdbooth reviewed Sep 8, 2023

View reviewed changes

k8s-ci-robot assigned mdbooth Sep 8, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 8, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 2023

k8s-ci-robot merged commit 01ccded into kubernetes:master Oct 3, 2023
10 checks passed

stephenfin deleted the glog-cleanup branch October 3, 2023 16:41

stephenfin mentioned this pull request Oct 3, 2023

Convert outstanding commands to cobra #2384

Merged

This was referenced Nov 6, 2023

[k8s-keystone-auth] Invalid args in keystone auth examples #2464

Closed

[release-1.28] Remove duplicate pflag.Parse calls #2467

Merged

GlassOfWhiskey mentioned this pull request Feb 3, 2024

[barbican-kms-plugin] Container broken in v1.28.1 #2531

Closed

k8s-infra-cherrypick-robot mentioned this pull request Feb 7, 2024

[release-1.28] Remove handling code for glog #2534

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove handling code for glog #2325

Remove handling code for glog #2325

stephenfin commented Aug 16, 2023 •

edited

jichenjc commented Aug 17, 2023

stephenfin commented Aug 17, 2023

jichenjc commented Aug 28, 2023

stephenfin commented Aug 28, 2023

stephenfin commented Sep 7, 2023 •

edited

stephenfin commented Sep 7, 2023 •

edited

mdbooth commented Sep 8, 2023

stephenfin commented Sep 8, 2023

mdbooth left a comment

mdbooth Sep 8, 2023

mdbooth Sep 8, 2023

mdbooth Sep 8, 2023

mdbooth Sep 8, 2023

mdbooth Sep 8, 2023

mdbooth Sep 8, 2023

stephenfin Sep 8, 2023 •

edited

mdbooth commented Sep 8, 2023

stephenfin commented Sep 8, 2023

stephenfin commented Sep 8, 2023 •

edited

mdbooth commented Sep 8, 2023

stephenfin commented Oct 3, 2023

jichenjc commented Oct 3, 2023

k8s-ci-robot commented Oct 3, 2023

jichenjc commented Feb 7, 2024

jichenjc commented Feb 7, 2024

jichenjc commented Feb 7, 2024

k8s-infra-cherrypick-robot commented Feb 7, 2024

k8s-infra-cherrypick-robot commented Feb 7, 2024

k8s-infra-cherrypick-robot commented Feb 7, 2024

Remove handling code for glog #2325

Remove handling code for glog #2325

Conversation

stephenfin commented Aug 16, 2023 • edited

jichenjc commented Aug 17, 2023

stephenfin commented Aug 17, 2023

jichenjc commented Aug 28, 2023

stephenfin commented Aug 28, 2023

stephenfin commented Sep 7, 2023 • edited

stephenfin commented Sep 7, 2023 • edited

mdbooth commented Sep 8, 2023

stephenfin commented Sep 8, 2023

mdbooth left a comment

Choose a reason for hiding this comment

mdbooth Sep 8, 2023

Choose a reason for hiding this comment

mdbooth Sep 8, 2023

Choose a reason for hiding this comment

mdbooth Sep 8, 2023

Choose a reason for hiding this comment

mdbooth Sep 8, 2023

Choose a reason for hiding this comment

mdbooth Sep 8, 2023

Choose a reason for hiding this comment

mdbooth Sep 8, 2023

Choose a reason for hiding this comment

stephenfin Sep 8, 2023 • edited

Choose a reason for hiding this comment

mdbooth commented Sep 8, 2023

stephenfin commented Sep 8, 2023

stephenfin commented Sep 8, 2023 • edited

mdbooth commented Sep 8, 2023

stephenfin commented Oct 3, 2023

jichenjc commented Oct 3, 2023

k8s-ci-robot commented Oct 3, 2023

jichenjc commented Feb 7, 2024

jichenjc commented Feb 7, 2024

jichenjc commented Feb 7, 2024

k8s-infra-cherrypick-robot commented Feb 7, 2024

k8s-infra-cherrypick-robot commented Feb 7, 2024

k8s-infra-cherrypick-robot commented Feb 7, 2024

stephenfin commented Aug 16, 2023 •

edited

stephenfin commented Sep 7, 2023 •

edited

stephenfin commented Sep 7, 2023 •

edited

stephenfin Sep 8, 2023 •

edited

stephenfin commented Sep 8, 2023 •

edited