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.

Sign up for GitHub

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

Jump to bottom

Migrate scheduler files interpodaffinity/filtering.go,podtopologyspread/filtering.go, volume_zone.go to structured logging #105931

Merged
merged 1 commit into from Oct 29, 2021
Merged

Migrate scheduler files interpodaffinity/filtering.go,podtopologyspread/filtering.go, volume_zone.go to structured logging #105931

merged 1 commit into from Oct 29, 2021

Conversation

mengjiao-liu
Copy link
Member

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Migrate scheduler files to structured logging

  • pkg/scheduler/framework/plugins/interpodaffinity/filtering.go
  • pkg/scheduler/framework/plugins/podtopologyspread/filtering.go
  • pkg/scheduler/framework/plugins/volumezone/volume_zone.go

Which issue(s) this PR fixes:

Ref #105841

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Migrate `pkg/scheduler/framework/plugins/interpodaffinity/filtering.go`,`pkg/scheduler/framework/plugins/podtopologyspread/filtering.go`, `pkg/scheduler/framework/plugins/volumezone/volume_zone.go` to structured logging

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

/wg structured-logging
/area logging
/priority important-soon
/kind cleanup
/triage accepted
/sig instrumentation
/cc @kubernetes/wg-structured-logging-reviews

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. wg/structured-logging Categorizes an issue or PR as relevant to WG Structured Logging. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. area/logging priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. triage/accepted Indicates an issue or PR is ready to be actively worked on. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. labels Oct 27, 2021
@shiva1333 shiva1333 moved this from Inbox to Waiting on Reviewer in WG Structured Logging - 1.23 Migration Oct 27, 2021
@jyz0309 jyz0309 mentioned this pull request Oct 27, 2021
Closed
@mengjiao-liu mengjiao-liu force-pushed the structured_logging_scheduler_part2 branch from 442c306 to 818693c Compare October 28, 2021 10:32
@shiva1333
Copy link
Contributor

/lgtm
/assign @serathius

@shiva1333 shiva1333 moved this from Waiting on Reviewer to Needs Approval in WG Structured Logging - 1.23 Migration Oct 28, 2021
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 28, 2021
serathius
serathius reviewed Oct 28, 2021
View reviewed changes
pkg/scheduler/framework/plugins/podtopologyspread/filtering.go Outdated
@@ -296,7 +296,7 @@ func (pl *PodTopologySpread) Filter(ctx context.Context, cycleState *framework.C
tpKey := c.TopologyKey
tpVal, ok := node.Labels[c.TopologyKey]
if !ok {
klog.V(5).Infof("node '%s' doesn't have required label '%s'", node.Name, tpKey)
klog.V(5).InfoS("Node doesn't have required label", "nodeName", node.Name, "label", tpKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
klog.V(5).InfoS("Node doesn't have required label", "nodeName", node.Name, "label", tpKey)
klog.V(5).InfoS("Node doesn't have required label", "node", klog.KObj(node), "label", tpKey)

Copy link
Contributor

Choose a reason for hiding this comment

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

hard to guess based on code what is the type of node. If it's not *v1.Node than please replace it code with klog.KRef("", node.Name)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, that has been changed.

serathius
serathius reviewed Oct 28, 2021
View reviewed changes
pkg/scheduler/framework/plugins/podtopologyspread/filtering.go Outdated
@@ -321,7 +321,7 @@ func (pl *PodTopologySpread) Filter(ctx context.Context, cycleState *framework.C
}
skew := matchNum + selfMatchNum - minMatchNum
if skew > c.MaxSkew {
klog.V(5).Infof("node '%s' failed spreadConstraint[%s]: MatchNum(%d) + selfMatchNum(%d) - minMatchNum(%d) > maxSkew(%d)", node.Name, tpKey, matchNum, selfMatchNum, minMatchNum, c.MaxSkew)
klog.V(5).InfoS("Node failed spreadConstraint: matchNum + selfMatchNum - minMatchNum > maxSkew", "nodeName", node.Name, "topologyKey", tpKey, "matchNum", matchNum, "selfMatchNum", selfMatchNum, "minMatchNum", minMatchNum, "maxSkew", c.MaxSkew)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
klog.V(5).InfoS("Node failed spreadConstraint: matchNum + selfMatchNum - minMatchNum > maxSkew", "nodeName", node.Name, "topologyKey", tpKey, "matchNum", matchNum, "selfMatchNum", selfMatchNum, "minMatchNum", minMatchNum, "maxSkew", c.MaxSkew)
klog.V(5).InfoS("Node failed spreadConstraint: matchNum + selfMatchNum - minMatchNum > maxSkew", "node", klog.KObj(node), "topologyKey", tpKey, "matchNum", matchNum, "selfMatchNum", selfMatchNum, "minMatchNum", minMatchNum, "maxSkew", c.MaxSkew)

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

serathius
serathius reviewed Oct 28, 2021
View reviewed changes
pkg/scheduler/framework/plugins/volumezone/volume_zone.go Outdated
@@ -157,7 +157,7 @@ func (pl *VolumeZone) Filter(ctx context.Context, _ *framework.CycleState, pod *
}

if !volumeVSet.Has(nodeV) {
klog.V(10).Infof("Won't schedule pod %q onto node %q due to volume %q (mismatch on %q)", pod.Name, node.Name, pvName, k)
klog.V(10).InfoS("Won't schedule pod onto node due to volume (mismatch on label key)", "podName", pod.Name, "nodeName", node.Name, "PVName", pvName, "PVLabelKey", k)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
klog.V(10).InfoS("Won't schedule pod onto node due to volume (mismatch on label key)", "podName", pod.Name, "nodeName", node.Name, "PVName", pvName, "PVLabelKey", k)
klog.V(10).InfoS("Won't schedule pod onto node due to volume (mismatch on label key)", "pod", klog.KObj(pod), "node", klog.KObj(node), "PVName", pvName, "PVLabelKey", k)

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

@mengjiao-liu mengjiao-liu force-pushed the structured_logging_scheduler_part2 branch from 818693c to cd1ca1e Compare October 28, 2021 12:28
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 28, 2021
@shiva1333 shiva1333 moved this from Needs Approval to Waiting on Reviewer in WG Structured Logging - 1.23 Migration Oct 28, 2021
@shiva1333
Copy link
Contributor

/retest

serathius
serathius reviewed Oct 28, 2021
View reviewed changes
pkg/scheduler/framework/plugins/volumezone/volume_zone.go Outdated
@@ -157,7 +157,7 @@ func (pl *VolumeZone) Filter(ctx context.Context, _ *framework.CycleState, pod *
}

if !volumeVSet.Has(nodeV) {
klog.V(10).Infof("Won't schedule pod %q onto node %q due to volume %q (mismatch on %q)", pod.Name, node.Name, pvName, k)
klog.V(10).InfoS("Won't schedule pod onto node due to volume (mismatch on label key)", "pod", klog.KObj(pod), "node", klog.KObj(node), "PVName", pvName, "PVLabelKey", k)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
klog.V(10).InfoS("Won't schedule pod onto node due to volume (mismatch on label key)", "pod", klog.KObj(pod), "node", klog.KObj(node), "PVName", pvName, "PVLabelKey", k)
klog.V(10).InfoS("Won't schedule pod onto node due to volume (mismatch on label key)", "pod", klog.KObj(pod), "node", klog.KObj(node), "PV", klog.KRef("", pvName), "PVLabelKey", k)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, this place needs to be changed.

For non-namespaced object we can pass empty string to namespace argument.

ref https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md

Copy link
Member Author

Choose a reason for hiding this comment

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

PR has been updated.

@mengjiao-liu
Migrate scheduler files interpodaffinity/filtering.go,`podtopologys…
2783ddc 
…pread/filtering.go`, `volume_zone.go` to structured logging
@mengjiao-liu mengjiao-liu force-pushed the structured_logging_scheduler_part2 branch from cd1ca1e to 2783ddc Compare October 29, 2021 03:41
@Huang-Wei
Copy link
Member

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 29, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Huang-Wei, mengjiao-liu

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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 29, 2021
@k8s-ci-robot k8s-ci-robot merged commit adff4a7 into kubernetes:master Oct 29, 2021
WG Structured Logging - 1.23 Migration automation moved this from Waiting on Reviewer to Done Oct 29, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Oct 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Huang-Wei Huang-Wei Huang-Wei left review comments

@yuzhiquan yuzhiquan yuzhiquan left review comments

@serathius serathius serathius left review comments

@shiva1333 shiva1333 shiva1333 left review comments

Assignees

@Huang-Wei Huang-Wei

@serathius serathius

@shiva1333 shiva1333

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/logging cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on. wg/structured-logging Categorizes an issue or PR as relevant to WG Structured Logging.
Projects
WG Structured Logging - 1.23 Migration
Done
Milestone
v1.23
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mengjiao-liu @shiva1333 @Huang-Wei @k8s-ci-robot @yuzhiquan @serathius