Explain the cause of the TopologyAffinityError #96890

pablitoergosum · 2020-11-26T13:59:55Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespace from that line:

/kind api-change
/kind bug
/kind cleanup
/kind deprecation
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
It provides some information explaining why TopologyAffinityError occured.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

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

k8s-ci-robot · 2020-11-26T14:00:03Z

Hi @pablitoergosum. 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.

k8s-ci-robot · 2020-11-26T14:00:11Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pablitoergosum
To complete the pull request process, please assign connordoyle after the PR has been reviewed.
You can assign the PR to them by writing /assign @connordoyle in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

pkg/kubelet/cm/topologymanager/OWNERS

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 · 2020-11-26T15:18:06Z

@pablitoergosum: Adding label do-not-merge/contains-merge-commits because PR contains merge commits, which are not allowed in this repository.
Use git rebase to reapply your commits on top of the target branch. Detailed instructions for doing so can be found 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.

ffromani · 2020-11-27T15:41:10Z

I like this idea in general (= make TM's behaviour easier to understand), but it seems this PR needs to be rebased? Also, not sure why we have a merge commit here.
Will review shortly

ffromani · 2020-11-27T15:47:59Z

pkg/kubelet/cm/topologymanager/policy.go

@@ -83,13 +89,15 @@ func filterProvidersHints(providersHints []map[string][]TopologyHint) [][]Topolo
 			if len(hints[resource]) == 0 {
 				klog.Infof("[topologymanager] Hint Provider has no possible NUMA affinities for resource '%s'", resource)
 				allProviderHints = append(allProviderHints, []TopologyHint{{nil, false}})
+				lackingResources = append(lackingResources, resource)
+				err = fmt.Errorf("not enough '%s' to allocate a pod", strings.Join(lackingResources, ", "))


I'm not sure in general about this specific log, but this can be done outside the loop, right before we return

Thanks for diving into the PR! The idea behind it was to create slice of all missing resources and return it entirely with TopologyAffinityError, which could be truly helpful for the ones not familiar with Scope and the TopologyManager itself.

ffromani

Hey, sorry for the long delay, had other things to take care.
Please rebase this PR, because the merge commits makes a bit too hard to review; for example it was nonobvious you want to bubble up the Topology Manager error up into the Topology Affinity Error.

I think the idea of making the topology manager behaviour more transparent is very nice and should totally be explored.
I was wondering if we want to make the actual error easier to be consumed, for example leveraging the pod status conditions (https://kubernetes.io/docs/concepts/workloads/pods/pod-lifecycle/) this is just a proposal, I think we can discuss it a bit more.

kikisdeliveryservice · 2021-01-22T01:27:01Z

Along with rebasing the PR @pablitoergosum could you also add a release note as this is a user facing change.

/kind cleanup
/triage accepted

k8s-ci-robot · 2021-04-06T03:04:59Z

@pablitoergosum: PR needs rebase.

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.

fejta-bot · 2021-07-05T03:58:54Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

k8s-triage-robot · 2021-08-04T04:49:16Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

k8s-triage-robot · 2021-09-03T04:57:12Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen
Mark this issue or PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-ci-robot · 2021-09-03T04:57:24Z

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied

After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Reopen this issue or PR with /reopen

Mark this issue or PR as fresh with /remove-lifecycle rotten

Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.

Add more logging messages

6d0e5cc

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. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Nov 26, 2020

k8s-ci-robot requested review from nolancon and vishh November 26, 2020 14:00

pablitoergosum changed the title ~~Add more logging messages~~ Explain the cause of the TopologyAffinityError Nov 26, 2020

Merge branch 'master' into logging

5598e4a

ffromani reviewed Nov 27, 2020

View reviewed changes

ffromani reviewed Dec 18, 2020

View reviewed changes

ehashman added this to Needs Reviewer in SIG Node PR Triage Jan 6, 2021

ehashman moved this from Needs Reviewer to Triage in SIG Node PR Triage Jan 6, 2021

kikisdeliveryservice moved this from Triage to Waiting on Author in SIG Node PR Triage Jan 22, 2021

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

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 5, 2021

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 4, 2021

k8s-ci-robot closed this Sep 3, 2021

SIG Node PR Triage automation moved this from Waiting on Author to Done Sep 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explain the cause of the TopologyAffinityError #96890

Explain the cause of the TopologyAffinityError #96890

pablitoergosum commented Nov 26, 2020 •

edited

k8s-ci-robot commented Nov 26, 2020

k8s-ci-robot commented Nov 26, 2020

k8s-ci-robot commented Nov 26, 2020

ffromani commented Nov 27, 2020 •

edited

ffromani Nov 27, 2020

pablitoergosum Nov 30, 2020

ffromani left a comment

kikisdeliveryservice commented Jan 22, 2021

k8s-ci-robot commented Apr 6, 2021

fejta-bot commented Jul 5, 2021

k8s-triage-robot commented Aug 4, 2021

k8s-triage-robot commented Sep 3, 2021

k8s-ci-robot commented Sep 3, 2021

Explain the cause of the TopologyAffinityError #96890

Explain the cause of the TopologyAffinityError #96890

Conversation

pablitoergosum commented Nov 26, 2020 • edited

k8s-ci-robot commented Nov 26, 2020

k8s-ci-robot commented Nov 26, 2020

k8s-ci-robot commented Nov 26, 2020

ffromani commented Nov 27, 2020 • edited

ffromani Nov 27, 2020

Choose a reason for hiding this comment

pablitoergosum Nov 30, 2020

Choose a reason for hiding this comment

ffromani left a comment

Choose a reason for hiding this comment

kikisdeliveryservice commented Jan 22, 2021

k8s-ci-robot commented Apr 6, 2021

fejta-bot commented Jul 5, 2021

k8s-triage-robot commented Aug 4, 2021

k8s-triage-robot commented Sep 3, 2021

k8s-ci-robot commented Sep 3, 2021

pablitoergosum commented Nov 26, 2020 •

edited

ffromani commented Nov 27, 2020 •

edited