Changed the Info type so that the config file can be displayed withou… #106228

sanchayanghosh · 2021-11-08T13:47:10Z

…t escape characters showing

What type of PR is this?

/kind bug

What this PR does / why we need it:

This fixes the readability of configuration log file for kube-scheduler, without escape sequences.

Which issue(s) this PR fixes:

Fixes #104868

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Fixed the issue where logging output of kube-scheduler configuration files included line breaks and escape characters. The output also attempted to output the configuration file in one section without showing the user a more readable format.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-11-08T13:47:13Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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.

k8s-ci-robot · 2021-11-08T13:47:17Z

@sanchayanghosh: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-ci-robot · 2021-11-08T13:47:17Z

Welcome @sanchayanghosh!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-11-08T13:47:18Z

Hi @sanchayanghosh. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

sanchayanghosh · 2021-11-08T14:50:58Z

/assign @damemi

damemi

/ok-to-test

damemi · 2021-11-08T14:53:22Z

cmd/kube-scheduler/app/options/configfile.go

@@ -93,7 +93,7 @@ func LogOrWriteConfig(fileName string, cfg *config.KubeSchedulerConfiguration, c
 	}

 	if klog.V(2).Enabled() {
-		klog.InfoS("Using component config", "config", buf.String())
+		klog.Info("Using component config", "config", buf.String())


We are intentionally trying to migrate logs to structured logging (see #105841), so I don't know if just switching this back to Info() is the right fix. Interesting that this addresses it though, so there may be something to dig into there

damemi · 2021-11-08T14:54:22Z

@sanchayanghosh thanks for the PR, I didn't realize the changes were in such a small spot

Could you paste the old-vs-current output here in code blocks so we can see what it looks like now?

sanchayanghosh · 2021-11-08T14:57:58Z

Hi @damemi

I tried to paste console output from the terminal in code blocks. It just doesn't work for me.

I'll try again.

sanchayanghosh · 2021-11-08T14:58:54Z


I1108 06:18:59.976038   23484 configfile.go:96] Using component config
-------------------------Configuration File Contents Start Here---------------------- 
apiVersion: kubescheduler.config.k8s.io/v1beta2
clientConnection:
  acceptContentTypes: ""
  burst: 100
  contentType: application/vnd.kubernetes.protobuf
  kubeconfig: /etc/kubernetes/scheduler.conf
  qps: 50
enableContentionProfiling: true
enableProfiling: true
healthzBindAddress: ""
kind: KubeSchedulerConfiguration
leaderElection:
  leaderElect: true
  leaseDuration: 15s
  renewDeadline: 10s
  resourceLock: leases
  resourceName: kube-scheduler
  resourceNamespace: kube-system
  retryPeriod: 2s
metricsBindAddress: ""
parallelism: 16
percentageOfNodesToScore: 0
podInitialBackoffSeconds: 1
podMaxBackoffSeconds: 10
profiles:
- pluginConfig:
  - args:
      apiVersion: kubescheduler.config.k8s.io/v1beta2
      kind: DefaultPreemptionArgs
      minCandidateNodesAbsolute: 100
      minCandidateNodesPercentage: 10
    name: DefaultPreemption
  - args:
      apiVersion: kubescheduler.config.k8s.io/v1beta2
      hardPodAffinityWeight: 1
      kind: InterPodAffinityArgs
    name: InterPodAffinity
  - args:
      apiVersion: kubescheduler.config.k8s.io/v1beta2
      kind: NodeAffinityArgs
    name: NodeAffinity
  - args:
      apiVersion: kubescheduler.config.k8s.io/v1beta2
      kind: NodeResourcesBalancedAllocationArgs
      resources:
      - name: cpu
        weight: 1
      - name: memory
        weight: 1
    name: NodeResourcesBalancedAllocation
  - args:
      apiVersion: kubescheduler.config.k8s.io/v1beta2
      kind: NodeResourcesFitArgs
      scoringStrategy:
        resources:
        - name: cpu
          weight: 1
        - name: memory
          weight: 1
        type: LeastAllocated
    name: NodeResourcesFit
  - args:
      apiVersion: kubescheduler.config.k8s.io/v1beta2
      defaultingType: System
      kind: PodTopologySpreadArgs
    name: PodTopologySpread
  - args:
      apiVersion: kubescheduler.config.k8s.io/v1beta2
      bindTimeoutSeconds: 600
      kind: VolumeBindingArgs
    name: VolumeBinding
  plugins:
    bind:
      enabled:
      - name: DefaultBinder
        weight: 0
    filter:
      enabled:
      - name: NodeUnschedulable
        weight: 0
      - name: NodeName
        weight: 0
      - name: TaintToleration
        weight: 0
      - name: NodeAffinity
        weight: 0
      - name: NodePorts
        weight: 0
      - name: NodeResourcesFit
        weight: 0
      - name: VolumeRestrictions
        weight: 0
      - name: EBSLimits
        weight: 0
      - name: GCEPDLimits
        weight: 0
      - name: NodeVolumeLimits
        weight: 0
      - name: AzureDiskLimits
        weight: 0
      - name: VolumeBinding
        weight: 0
      - name: VolumeZone
        weight: 0
      - name: PodTopologySpread
        weight: 0
      - name: InterPodAffinity
        weight: 0
    permit: {}
    postBind: {}
    postFilter:
      enabled:
      - name: DefaultPreemption
        weight: 0
    preBind:
      enabled:
      - name: VolumeBinding
        weight: 0
    preFilter:
      enabled:
      - name: NodeResourcesFit
        weight: 0
      - name: NodePorts
        weight: 0
      - name: VolumeRestrictions
        weight: 0
      - name: PodTopologySpread
        weight: 0
      - name: InterPodAffinity
        weight: 0
      - name: VolumeBinding
        weight: 0
      - name: NodeAffinity
        weight: 0
    preScore:
      enabled:
      - name: InterPodAffinity
        weight: 0
      - name: PodTopologySpread
        weight: 0
      - name: TaintToleration
        weight: 0
      - name: NodeAffinity
        weight: 0
    queueSort:
      enabled:
      - name: PrioritySort
        weight: 0
    reserve:
      enabled:
      - name: VolumeBinding
        weight: 0
    score:
      enabled:
      - name: NodeResourcesBalancedAllocation
        weight: 1
      - name: ImageLocality
        weight: 1
      - name: InterPodAffinity
        weight: 1
      - name: NodeResourcesFit
        weight: 1
      - name: NodeAffinity
        weight: 1
      - name: PodTopologySpread
        weight: 2
      - name: TaintToleration
        weight: 1
  schedulerName: default-scheduler

------------------------------------Configuration File Contents End Here---------------------------------

damemi · 2021-11-08T15:00:10Z

@sanchayanghosh for multiline code blocks use three backticks instead of 1: ```

…t escape characters showing

sanchayanghosh · 2021-11-08T15:08:01Z

We are intentionally trying to migrate logs to structured logging (see #105841), so I don't know if just switching this back to Info() is the right fix. Interesting that this addresses it though, so there may be something to dig into there

I too wanted to localize the fixes around structured logging itself. That would require a change in klog itself to support structured logging with formatting. Do you want to create an feature request in klog? We could then link that request to #105841

alculquicondor · 2021-11-08T15:10:02Z

cmd/kube-scheduler/app/options/configfile.go

@@ -93,7 +93,7 @@ func LogOrWriteConfig(fileName string, cfg *config.KubeSchedulerConfiguration, c
 	}

 	if klog.V(2).Enabled() {
-		klog.InfoS("Using component config", "config", buf.String())
+		klog.Info("Using component config", "\n-------------------------Configuration File Contents Start Here---------------------- \n", buf.String(), "\n------------------------------------Configuration File Contents End Here---------------------------------\n")


We would be giving up structured logging in this case. But I think it's better in this case.

@logicalhan WDYT?

@serathius probably has a more informed opinion

I think we should be able to do it using structured logging, however we might need a special formatter struct for this.

cc @pohly for recommendation

A formatter doesn't help at the moment. Strings as values in a key/value pair will always be quoted in text output, that cannot be suppressed. It's worthwhile to add that, though, therefore I have filed #106262.

In the meantime this PR can and should be merged as-is.

alculquicondor · 2021-11-09T15:05:17Z

/lgtm
/approve

k8s-ci-robot · 2021-11-09T15:06:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alculquicondor, sanchayanghosh

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:

~~cmd/kube-scheduler/OWNERS~~ [alculquicondor]

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

pohly · 2021-11-09T16:39:50Z

/retest

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 8, 2021

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 8, 2021

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 8, 2021

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Nov 8, 2021

k8s-ci-robot requested review from ahg-g and alculquicondor November 8, 2021 13:48

damemi reviewed Nov 8, 2021

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 8, 2021

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Nov 8, 2021

Changed the Info type so that the config file can be displayed withou…

2131fdb

…t escape characters showing

sanchayanghosh force-pushed the 104868-scheduler-logging-config-format-incorrectly branch from 496086f to 2131fdb Compare November 8, 2021 15:04

alculquicondor reviewed Nov 8, 2021

View reviewed changes

k8s-ci-robot assigned alculquicondor Nov 9, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2021

k8s-ci-robot merged commit 398e028 into kubernetes:master Nov 9, 2021

k8s-ci-robot added this to the v1.23 milestone Nov 9, 2021

sanchayanghosh deleted the 104868-scheduler-logging-config-format-incorrectly branch November 15, 2021 04:57

pohly mentioned this pull request Nov 15, 2021

structured logging: support log messages with line breaks kubernetes/klog#268

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changed the Info type so that the config file can be displayed withou… #106228

Changed the Info type so that the config file can be displayed withou… #106228

sanchayanghosh commented Nov 8, 2021 •

edited

k8s-ci-robot commented Nov 8, 2021

k8s-ci-robot commented Nov 8, 2021

k8s-ci-robot commented Nov 8, 2021

k8s-ci-robot commented Nov 8, 2021

sanchayanghosh commented Nov 8, 2021

damemi left a comment

damemi Nov 8, 2021

damemi commented Nov 8, 2021

sanchayanghosh commented Nov 8, 2021

sanchayanghosh commented Nov 8, 2021 •

edited

damemi commented Nov 8, 2021

sanchayanghosh commented Nov 8, 2021 •

edited

alculquicondor Nov 8, 2021

logicalhan Nov 8, 2021

serathius Nov 8, 2021

pohly Nov 9, 2021

alculquicondor commented Nov 9, 2021

k8s-ci-robot commented Nov 9, 2021

pohly commented Nov 9, 2021

Changed the Info type so that the config file can be displayed withou… #106228

Changed the Info type so that the config file can be displayed withou… #106228

Conversation

sanchayanghosh commented Nov 8, 2021 • edited

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Nov 8, 2021

k8s-ci-robot commented Nov 8, 2021

k8s-ci-robot commented Nov 8, 2021

k8s-ci-robot commented Nov 8, 2021

sanchayanghosh commented Nov 8, 2021

damemi left a comment

Choose a reason for hiding this comment

damemi Nov 8, 2021

Choose a reason for hiding this comment

damemi commented Nov 8, 2021

sanchayanghosh commented Nov 8, 2021

sanchayanghosh commented Nov 8, 2021 • edited

damemi commented Nov 8, 2021

sanchayanghosh commented Nov 8, 2021 • edited

alculquicondor Nov 8, 2021

Choose a reason for hiding this comment

logicalhan Nov 8, 2021

Choose a reason for hiding this comment

serathius Nov 8, 2021

Choose a reason for hiding this comment

pohly Nov 9, 2021

Choose a reason for hiding this comment

alculquicondor commented Nov 9, 2021

k8s-ci-robot commented Nov 9, 2021

pohly commented Nov 9, 2021

sanchayanghosh commented Nov 8, 2021 •

edited

sanchayanghosh commented Nov 8, 2021 •

edited

sanchayanghosh commented Nov 8, 2021 •

edited