Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

logs: remove deprecated klog flags #112120

Merged
merged 2 commits into from Sep 21, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion hack/jenkins/benchmark-dockerized.sh
Expand Up @@ -66,7 +66,8 @@ cd "${GOPATH}/src/k8s.io/kubernetes"
./hack/install-etcd.sh

# Run the benchmark tests and pretty-print the results into a separate file.
make test-integration WHAT="$*" KUBE_TEST_ARGS="-run='XXX' -bench=${TEST_PREFIX:-.} -benchtime=${BENCHTIME:-1s} -benchmem -alsologtostderr=false -logtostderr=false -data-items-dir=${ARTIFACTS}" \
# Log output of the tests go to stderr.
make test-integration WHAT="$*" KUBE_TEST_ARGS="-run='XXX' -bench=${TEST_PREFIX:-.} -benchtime=${BENCHTIME:-1s} -benchmem -data-items-dir=${ARTIFACTS}" \
| (go run test/integration/benchmark/extractlog/main.go) \
| tee \
>(prettybench -no-passthrough > "${ARTIFACTS}/BenchmarkResults.txt") \
Expand Down
6 changes: 3 additions & 3 deletions hack/make-rules/test-e2e-node.sh
Expand Up @@ -162,7 +162,7 @@ if [ "${remote}" = true ] && [ "${remote_mode}" = gce ] ; then
echo "Kubelet Config File: ${kubelet_config_file}"

# Invoke the runner
go run test/e2e_node/runner/remote/run_remote.go --logtostderr --vmodule=*=4 --ssh-env="gce" \
go run test/e2e_node/runner/remote/run_remote.go --vmodule=*=4 --ssh-env="gce" \
--zone="${zone}" --project="${project}" --gubernator="${gubernator}" \
--hosts="${hosts}" --images="${images}" --cleanup="${cleanup}" \
--results-dir="${artifacts}" --ginkgo-flags="${ginkgoflags}" --runtime-config="${runtime_config}" \
Expand All @@ -189,7 +189,7 @@ elif [ "${remote}" = true ] && [ "${remote_mode}" = ssh ] ; then
test_args='--kubelet-flags="--cluster-domain='${KUBE_DNS_DOMAIN:-cluster.local}'" '${test_args}

# Invoke the runner
go run test/e2e_node/runner/remote/run_remote.go --mode="ssh" --logtostderr --vmodule=*=4 \
go run test/e2e_node/runner/remote/run_remote.go --mode="ssh" --vmodule=*=4 \
--hosts="${hosts}" --results-dir="${artifacts}" --ginkgo-flags="${ginkgoflags}" \
--test_args="${test_args}" --system-spec-name="${system_spec_name}" \
--runtime-config="${runtime_config}" \
Expand Down Expand Up @@ -222,7 +222,7 @@ else
go run test/e2e_node/runner/local/run_local.go \
--system-spec-name="${system_spec_name}" --extra-envs="${extra_envs}" \
--ginkgo-flags="${ginkgoflags}" \
--test-flags="--alsologtostderr --v 4 --report-dir=${artifacts} --node-name $(hostname) ${test_args}" \
--test-flags="--v 4 --report-dir=${artifacts} --node-name $(hostname) ${test_args}" \
--runtime-config="${runtime_config}" \
--kubelet-config-file="${kubelet_config_file}" \
--build-dependencies=true 2>&1 | tee -i "${artifacts}/build-log.txt"
Expand Down
2 changes: 1 addition & 1 deletion hack/make-rules/test-integration.sh
Expand Up @@ -78,7 +78,7 @@ runTests() {
make -C "${KUBE_ROOT}" test \
WHAT="${WHAT:-$(kube::test::find_integration_test_dirs | paste -sd' ' -)}" \
GOFLAGS="${GOFLAGS:-}" \
KUBE_TEST_ARGS="--alsologtostderr=true ${SHORT:--short=true} --vmodule=${KUBE_TEST_VMODULE} ${KUBE_TEST_ARGS:-}" \
KUBE_TEST_ARGS="${SHORT:--short=true} --vmodule=${KUBE_TEST_VMODULE} ${KUBE_TEST_ARGS:-}" \
KUBE_TIMEOUT="${KUBE_TIMEOUT}" \
KUBE_RACE=""

Expand Down
1 change: 0 additions & 1 deletion hack/update-openapi-spec.sh
Expand Up @@ -84,7 +84,6 @@ kube::log::status "Starting kube-apiserver"
--service-account-lookup="${SERVICE_ACCOUNT_LOOKUP}" \
--service-account-issuer="https://kubernetes.default.svc" \
--service-account-signing-key-file="${SERVICE_ACCOUNT_KEY}" \
--logtostderr \
--v=2 \
--service-cluster-ip-range="10.0.0.0/24" >"${API_LOGFILE}" 2>&1 &
APISERVER_PID=$!
Expand Down
Expand Up @@ -62,7 +62,7 @@ func TestAddGlobalFlags(t *testing.T) {
}{
{
// Happy case
expectedFlag: []string{"add-dir-header", "alsologtostderr", "help", "log-backtrace-at", "log-dir", "log-file", "log-file-max-size", "log-flush-frequency", "logtostderr", "one-output", "skip-headers", "skip-log-headers", "stderrthreshold", "v", "vmodule"},
expectedFlag: []string{"help", "log-flush-frequency", "v", "vmodule"},
matchExpected: false,
},
{
Expand Down
23 changes: 3 additions & 20 deletions staging/src/k8s.io/component-base/logs/api/v1/options.go
Expand Up @@ -20,7 +20,6 @@ import (
"flag"
"fmt"
"math"
"sort"
"strings"
"time"

Expand All @@ -32,6 +31,7 @@ import (
"k8s.io/apimachinery/pkg/util/validation/field"
cliflag "k8s.io/component-base/cli/flag"
"k8s.io/component-base/featuregate"
"k8s.io/component-base/logs/klogflags"
)

const (
Expand Down Expand Up @@ -183,12 +183,8 @@ func apply(c *LoggingConfiguration, featureGate featuregate.FeatureGate) error {

// AddFlags adds command line flags for the configuration.
func AddFlags(c *LoggingConfiguration, fs *pflag.FlagSet) {
// The help text is generated assuming that flags will eventually use
// hyphens, even if currently no normalization function is set for the
// flag set yet.
unsupportedFlags := strings.Join(unsupportedLoggingFlagNames(cliflag.WordSepNormalizeFunc), ", ")
formats := logRegistry.list()
fs.StringVar(&c.Format, "logging-format", c.Format, fmt.Sprintf("Sets the log format. Permitted formats: %s.\nNon-default formats don't honor these flags: %s.\nNon-default choices are currently alpha and subject to change without warning.", formats, unsupportedFlags))
fs.StringVar(&c.Format, "logging-format", c.Format, fmt.Sprintf("Sets the log format. Permitted formats: %s.", formats))
// No new log formats should be added after generation is of flag options
logRegistry.freeze()

Expand Down Expand Up @@ -236,14 +232,13 @@ var loggingFlags pflag.FlagSet

func init() {
var fs flag.FlagSet
klog.InitFlags(&fs)
klogflags.Init(&fs)
loggingFlags.AddGoFlagSet(&fs)
}

// List of logs (k8s.io/klog + k8s.io/component-base/logs) flags supported by all logging formats
var supportedLogsFlags = map[string]struct{}{
"v": {},
// TODO: support vmodule after 1.19 Alpha
}

// unsupportedLoggingFlags lists unsupported logging flags. The normalize
Expand All @@ -268,15 +263,3 @@ func unsupportedLoggingFlags(normalizeFunc func(f *pflag.FlagSet, name string) p
})
return allFlags
}

// unsupportedLoggingFlagNames lists unsupported logging flags by name, with
// optional normalization and sorted.
func unsupportedLoggingFlagNames(normalizeFunc func(f *pflag.FlagSet, name string) pflag.NormalizedName) []string {
unsupportedFlags := unsupportedLoggingFlags(normalizeFunc)
names := make([]string, 0, len(unsupportedFlags))
for _, f := range unsupportedFlags {
names = append(names, "--"+f.Name)
}
sort.Strings(names)
return names
}
Expand Up @@ -39,9 +39,7 @@ func TestFlags(t *testing.T) {
fs.SetOutput(&output)
fs.PrintDefaults()
want := ` --log-flush-frequency duration Maximum number of seconds between log flushes (default 5s)
--logging-format string Sets the log format. Permitted formats: "text".
Non-default formats don't honor these flags: --add-dir-header, --alsologtostderr, --log-backtrace-at, --log-dir, --log-file, --log-file-max-size, --logtostderr, --one-output, --skip-headers, --skip-log-headers, --stderrthreshold, --vmodule.
Non-default choices are currently alpha and subject to change without warning. (default "text")
--logging-format string Sets the log format. Permitted formats: "text". (default "text")
-v, --v Level number for the log level verbosity
--vmodule pattern=N,... comma-separated list of pattern=N settings for file-filtered logging (only works for text log format)
`
Expand Down
41 changes: 41 additions & 0 deletions staging/src/k8s.io/component-base/logs/klogflags/klogflags.go
@@ -0,0 +1,41 @@
/*
Copyright 2022 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package klogflags

import (
"flag"

"k8s.io/klog/v2"
)

// Init is a replacement for klog.InitFlags which only adds those flags
// that are still supported for Kubernetes components (i.e. -v and -vmodule).
// See
// https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/2845-deprecate-klog-specific-flags-in-k8s-components.
func Init(fs *flag.FlagSet) {
var allFlags flag.FlagSet
klog.InitFlags(&allFlags)
if fs == nil {
fs = flag.CommandLine
}
allFlags.VisitAll(func(f *flag.Flag) {
switch f.Name {
case "v", "vmodule":
fs.Var(f.Value, f.Name, f.Usage)
}
})
}
48 changes: 11 additions & 37 deletions staging/src/k8s.io/component-base/logs/logs.go
Expand Up @@ -27,16 +27,11 @@ import (

"github.com/spf13/pflag"
logsapi "k8s.io/component-base/logs/api/v1"
"k8s.io/component-base/logs/klogflags"
"k8s.io/klog/v2"
)

const 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"

// TODO (https://github.com/kubernetes/kubernetes/issues/105310): once klog
// flags are removed, stop warning about "Non-default formats don't honor these
// flags" in config.go and instead add this remark here.
//
// const vmoduleUsage = " (only works for the default text log format)"
const vmoduleUsage = " (only works for the default text log format)"

var (
packageFlags = flag.NewFlagSet("logging", flag.ContinueOnError)
Expand All @@ -47,7 +42,7 @@ var (
)

func init() {
klog.InitFlags(packageFlags)
klogflags.Init(packageFlags)
Copy link
Member

Choose a reason for hiding this comment

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

I'm unclear why this moved to a different package, personally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's used in k8s.io/component-base/logs/api/v1 to access the klog settings and determine which logging flags exist, which in turn is used to warn about those that only have an effect in the text format (used to more, now it's only -vmodule which isn't supported by zapr).

If it was not moved into a separate leaf package, there would be a circular dependency. Before, api/v1 directly called klog.InitFlags, but that's not correct anymore because only a subset of those flags are active.

Copy link
Contributor Author

@pohly pohly Sep 20, 2022

Choose a reason for hiding this comment

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

Perhaps I also need to explain why klogflags is needed and why it is under component-base:

  • We cannot simply remove the flags from klog.InitFlags, that would break non-Kubernetes applications that use it.
  • We could add a klog.InitKubernetesFlags which registers only the flags supported in Kubernetes, but IMHO that is too specific to Kubernetes and belongs more into component-base than klog.

packageFlags.DurationVar(&logFlushFreq, logsapi.LogFlushFreqFlagName, logsapi.LogFlushFreqDefault, "Maximum number of seconds between log flushes")
}

Expand Down Expand Up @@ -81,42 +76,29 @@ var NewOptions = logsapi.NewLoggingConfiguration
//
// May be called more than once.
func AddFlags(fs *pflag.FlagSet, opts ...Option) {
// Determine whether the flags are already present by looking up one
// which always should exist.
if fs.Lookup("logtostderr") != nil {
return
}

o := addFlagsOptions{}
for _, opt := range opts {
opt(&o)
}

// Add flags with pflag deprecation remark for some klog flags.
// Add all supported flags.
packageFlags.VisitAll(func(f *flag.Flag) {
pf := pflag.PFlagFromGoFlag(f)
switch f.Name {
case "v":
// unchanged, potentially skip it
if o.skipLoggingConfigurationFlags {
return
}
case logsapi.LogFlushFreqFlagName:
case "v", logsapi.LogFlushFreqFlagName:
// unchanged, potentially skip it
if o.skipLoggingConfigurationFlags {
return
}
case "vmodule":
// TODO: see above
// pf.Usage += vmoduleUsage
if o.skipLoggingConfigurationFlags {
return
}
default:
// deprecated, but not hidden
pf.Deprecated = deprecated
pf.Usage += vmoduleUsage
}
if fs.Lookup(pf.Name) == nil {
fs.AddFlag(pf)
}
fs.AddFlag(pf)
})
}

Expand All @@ -137,24 +119,16 @@ func AddGoFlags(fs *flag.FlagSet, opts ...Option) {
packageFlags.VisitAll(func(f *flag.Flag) {
usage := f.Usage
switch f.Name {
case "v":
// unchanged
if o.skipLoggingConfigurationFlags {
return
}
case logsapi.LogFlushFreqFlagName:
case "v", logsapi.LogFlushFreqFlagName:
// unchanged
if o.skipLoggingConfigurationFlags {
return
}
case "vmodule":
// TODO: see above
// usage += vmoduleUsage
if o.skipLoggingConfigurationFlags {
return
}
default:
usage += " (DEPRECATED: " + deprecated + ")"
usage += vmoduleUsage
}
fs.Var(f.Value, f.Name, usage)
})
Expand Down
1 change: 0 additions & 1 deletion test/e2e_node/conformance/run_test.sh
Expand Up @@ -201,7 +201,6 @@ start_kubelet --kubeconfig "${KUBELET_KUBECONFIG}" \
--system-cgroups=/system \
--cgroup-root=/ \
--v=$log_level \
--logtostderr

wait_kubelet

Expand Down
2 changes: 1 addition & 1 deletion test/e2e_node/jenkins/conformance/conformance-jenkins.sh
Expand Up @@ -33,7 +33,7 @@ TIMEOUT=${TIMEOUT:-"45m"}
mkdir -p "${ARTIFACTS}"

go run test/e2e_node/runner/remote/run_remote.go --test-suite=conformance \
--logtostderr --vmodule=*=4 --ssh-env="gce" --ssh-user="$GCE_USER" \
--vmodule=*=4 --ssh-env="gce" --ssh-user="$GCE_USER" \
Copy link
Contributor

Choose a reason for hiding this comment

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

So the change in the e2e_node tests is just to remove the --logtostderr flag across the board?
I don't object in principle, but what are the implications of removing this exactly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

None. Logging to stderr has been the default for several years now, so using this parameter didn't change anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good. Thanks for clarifying.

--zone="$GCE_ZONE" --project="$GCE_PROJECT" --hosts="$GCE_HOSTS" \
--images="$GCE_IMAGES" --image-project="$GCE_IMAGE_PROJECT" \
--image-config-file="$GCE_IMAGE_CONFIG_PATH" --cleanup="$CLEANUP" \
Expand Down
2 changes: 1 addition & 1 deletion test/e2e_node/remote/node_conformance.go
Expand Up @@ -181,7 +181,7 @@ func launchKubelet(host, workspace, results, testArgs, bearerToken string) error
return fmt.Errorf("failed to create kubelet pod manifest path %q: error - %v output - %q",
podManifestPath, err, output)
}
startKubeletCmd := fmt.Sprintf("./%s --run-kubelet-mode --logtostderr --node-name=%s"+
startKubeletCmd := fmt.Sprintf("./%s --run-kubelet-mode --node-name=%s"+
" --bearer-token=%s"+
" --report-dir=%s %s --kubelet-flags=--pod-manifest-path=%s > %s 2>&1",
conformanceTestBinary, host, bearerToken, results, testArgs, podManifestPath, filepath.Join(results, kubeletLauncherLog))
Expand Down
2 changes: 1 addition & 1 deletion test/e2e_node/remote/node_e2e.go
Expand Up @@ -200,7 +200,7 @@ func (n *NodeE2ERemote) RunTest(host, workspace, results, imageDesc, junitFilePr
klog.V(2).Infof("Starting tests on %q", host)
cmd := getSSHCommand(" && ",
fmt.Sprintf("cd %s", workspace),
fmt.Sprintf("timeout -k 30s %fs ./ginkgo %s ./e2e_node.test -- --system-spec-name=%s --system-spec-file=%s --extra-envs=%s --runtime-config=%s --logtostderr --v 4 --node-name=%s --report-dir=%s --report-prefix=%s --image-description=\"%s\" %s",
fmt.Sprintf("timeout -k 30s %fs ./ginkgo %s ./e2e_node.test -- --system-spec-name=%s --system-spec-file=%s --extra-envs=%s --runtime-config=%s --v 4 --node-name=%s --report-dir=%s --report-prefix=%s --image-description=\"%s\" %s",
timeout.Seconds(), ginkgoArgs, systemSpecName, systemSpecFile, extraEnvs, runtimeConfig, host, results, junitFilePrefix, imageDesc, testArgs),
)
return SSH(host, "sh", "-c", cmd)
Expand Down
4 changes: 2 additions & 2 deletions test/e2e_node/runner/remote/run_remote.go
Expand Up @@ -15,9 +15,9 @@ limitations under the License.
*/

// To run the node e2e tests remotely against one or more hosts on gce:
// $ go run run_remote.go --logtostderr --v 2 --ssh-env gce --hosts <comma separated hosts>
// $ go run run_remote.go --v 2 --ssh-env gce --hosts <comma separated hosts>
// To run the node e2e tests remotely against one or more images on gce and provision them:
// $ go run run_remote.go --logtostderr --v 2 --project <project> --zone <zone> --ssh-env gce --images <comma separated images>
// $ go run run_remote.go --v 2 --project <project> --zone <zone> --ssh-env gce --images <comma separated images>
package main

import (
Expand Down
2 changes: 1 addition & 1 deletion test/e2e_node/services/kubelet.go
Expand Up @@ -252,7 +252,7 @@ func (e *E2EServices) startKubelet(featureGates map[string]bool) (*server, error
cmdArgs = append(cmdArgs,
"--kubeconfig", kubeconfigPath,
"--root-dir", KubeletRootDirectory,
"--v", LogVerbosityLevel, "--logtostderr",
"--v", LogVerbosityLevel,
)

// Apply test framework feature gates by default. This could also be overridden
Expand Down
6 changes: 3 additions & 3 deletions test/integration/scheduler_perf/README.md
Expand Up @@ -32,7 +32,7 @@ How To Run

```shell
# In Kubernetes root path
make test-integration WHAT=./test/integration/scheduler_perf ETCD_LOGLEVEL=warn KUBE_TEST_VMODULE="''" KUBE_TEST_ARGS="-alsologtostderr=false -logtostderr=false -run=^$$ -benchtime=1ns -bench=BenchmarkPerfScheduling"
make test-integration WHAT=./test/integration/scheduler_perf ETCD_LOGLEVEL=warn KUBE_TEST_VMODULE="''" KUBE_TEST_ARGS="-run=^$$ -benchtime=1ns -bench=BenchmarkPerfScheduling"
```

The benchmark suite runs all the tests specified under config/performance-config.yaml.
Expand All @@ -47,14 +47,14 @@ Otherwise, the golang benchmark framework will try to run a test more than once

```shell
# In Kubernetes root path
make test-integration WHAT=./test/integration/scheduler_perf ETCD_LOGLEVEL=warn KUBE_TEST_VMODULE="''" KUBE_TEST_ARGS="-alsologtostderr=false -logtostderr=false -run=^$$ -benchtime=1ns -bench=BenchmarkPerfScheduling/SchedulingBasic/5000Nodes/5000InitPods/1000PodsToSchedule"
make test-integration WHAT=./test/integration/scheduler_perf ETCD_LOGLEVEL=warn KUBE_TEST_VMODULE="''" KUBE_TEST_ARGS="-run=^$$ -benchtime=1ns -bench=BenchmarkPerfScheduling/SchedulingBasic/5000Nodes/5000InitPods/1000PodsToSchedule"
```

To produce a cpu profile:

```shell
# In Kubernetes root path
make test-integration WHAT=./test/integration/scheduler_perf KUBE_TIMEOUT="-timeout=3600s" ETCD_LOGLEVEL=warn KUBE_TEST_VMODULE="''" KUBE_TEST_ARGS="-alsologtostderr=false -logtostderr=false -run=^$$ -benchtime=1ns -bench=BenchmarkPerfScheduling -cpuprofile ~/cpu-profile.out"
make test-integration WHAT=./test/integration/scheduler_perf KUBE_TIMEOUT="-timeout=3600s" ETCD_LOGLEVEL=warn KUBE_TEST_VMODULE="''" KUBE_TEST_ARGS="-run=^$$ -benchtime=1ns -bench=BenchmarkPerfScheduling -cpuprofile ~/cpu-profile.out"
```

### How to configure benchmark tests
Expand Down
1 change: 1 addition & 0 deletions vendor/modules.txt
Expand Up @@ -2029,6 +2029,7 @@ k8s.io/component-base/logs
k8s.io/component-base/logs/api/v1
k8s.io/component-base/logs/json
k8s.io/component-base/logs/json/register
k8s.io/component-base/logs/klogflags
k8s.io/component-base/logs/logreduction
k8s.io/component-base/logs/testinit
k8s.io/component-base/metrics
Expand Down