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

Jump to bottom

scheduler: inefficient cache operation slows down scheduler #18255

Closed
xiang90 opened this issue Dec 6, 2015 · 18 comments
Closed

scheduler: inefficient cache operation slows down scheduler #18255

xiang90 opened this issue Dec 6, 2015 · 18 comments
Labels
priority/backlog Higher priority than priority/awaiting-more-evidence.

Comments

@xiang90
Copy link
Contributor

xiang90 commented Dec 6, 2015

After #18126 fixed by #18170, I run density test again.

Now the hot spot on the critical path is an inefficient cache access(https://github.com/kubernetes/kubernetes/blob/master/pkg/client/cache/store.go#L75) when listing nodes that uses reflection. Reflection causes a lot of allocations.

A simple fix would be

func NodeKeyFunc(obj interface{}) (string, error) {
    key, ok := obj.(*api.Node)
    if !ok {
        return "", fmt.Errorf("not *api.Node type")
    }
    meta := key.ObjectMeta
    if len(meta.Namespace) > 0 {
        return meta.Namespace + "/" + meta.Name, nil
    }
    return meta.Name, nil
}

The simple fix improves the throughput of scheduler by 15%-20% on 1000 nodes case.

@lavalamp What do you think about this issue?

/cc @hongchaodeng @wojtek-t @gmarek @davidopp
@wojtek-t
Copy link
Member

wojtek-t commented Dec 6, 2015

The MetaKeyFunc is a generic method - what you're suggesting is obviously not.
However, I think that it definitely makes sense to use what you're suggesting, if we already now that we are tranforming a node.

@xiang90
Copy link
Contributor Author

xiang90 commented Dec 6, 2015

@wojtek-t

Yes. I suggest pass in a special keyfunc to nodeLister since you already know you are transforming a node struct.

@wojtek-t
Copy link
Member

wojtek-t commented Dec 6, 2015

SGTM - since this is NodeLister, I agree it makes sense to use a dedicated type-specific functions there.

@lavalamp
Copy link
Member

lavalamp commented Dec 7, 2015

So a more general fix would be to add a type switch in the existing key function, or write one in scheduler that uses a type switch. e.g.,

func KeyFunc(obj interface{}) (string, error) {
  switch t := obj.(type) {
  case *api.Node: return ObjectMetaToKey(&t.ObjectMeta)
  case *api.Pod: return ObjectMetaToKey(&t.ObjectMeta)
  case *api.Service: return ObjectMetaToKey(&t.ObjectMeta)
  ...
  }
  // fall through to reflection based method
}

@xiang90
Copy link
Contributor Author

xiang90 commented Dec 7, 2015

@lavalamp A cleaner and performant way is probably to let all relevant types (where performance is needed, like node struct) to satisfy a Key interface?

@lavalamp
Copy link
Member

lavalamp commented Dec 8, 2015

Yes-- actually @smarterclayton just proposed such a change in a large refactoring PR he's working on.

@xiang90
Copy link
Contributor Author

xiang90 commented Dec 8, 2015

@smarterclayton Can you please point me to the pr? So I can avoid duplicate work. Thanks!

@lavalamp
Copy link
Member

lavalamp commented Dec 8, 2015

It's #17922

@smarterclayton
Copy link
Contributor

I plan on making a separate PR for that shortly - we see this check show up in lots of large profiles so I want to make sure it goes away. Will update both issues once I do that.

@xiang90
Copy link
Contributor Author

xiang90 commented Dec 8, 2015

@smarterclayton Great. So I will wait for your update. Thanks!

@smarterclayton
Copy link
Contributor

As a side node, making this change has dramatically reduced the ugliness and incomprehensibility of a bunch of nasty code paths.

@davidopp davidopp added the priority/backlog Higher priority than priority/awaiting-more-evidence. label Dec 8, 2015
@davidopp
Copy link
Member

davidopp commented Dec 9, 2015

ref/ #18418

@kevin-wangzefeng
Copy link
Member

/cc @HardySimpson

@smarterclayton
Copy link
Contributor

Opened #18473

@smarterclayton
Copy link
Contributor

Actual fix will be in #18534

@hongchaodeng hongchaodeng mentioned this issue Dec 17, 2015
Closed
@smarterclayton
Copy link
Contributor

Issue was merged

@hongchaodeng
Copy link
Contributor

@smarterclayton
#18534 was way too huge. Can you help explain briefly how it avoids expensive reflect operations in MetaNamespaceKeyFunc?

@smarterclayton
Copy link
Contributor

smarterclayton commented Dec 18, 2015 via email

openshift-publish-robot pushed a commit to openshift/kubernetes that referenced this issue Feb 27, 2018
@k8s-publishing-bot
Merge pull request kubernetes#18255 from joelsmith/master
764d921 
Automatic merge from submit-queue (batch tested with PRs 18423, 18255, 18526, 18539, 18509).

UPSTREAM: 58720: Ensure that the runtime mounts RO volumes read-only

This is a backport of kubernetes#58720

This change makes it so that containers cannot write to secret, configMap, downwardAPI and projected volumes since the runtime will now mount them read-only. This change makes things less confusing for a user since any attempt to update a secret volume will result in an error rather than a successful change followed by a revert by the kubelet when the volume next syncs.

**Which issue(s) this PR fixes**
N/A

**Release note**:
```
Containers now mount secret, configMap, downwardAPI and projected volumes read-only.

```

Origin-commit: 6ce0af039208656c9b27d0d9a918794d9821f68c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
priority/backlog Higher priority than priority/awaiting-more-evidence.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
8 participants
@lavalamp @hongchaodeng @smarterclayton @mikedanese @xiang90 @kevin-wangzefeng @davidopp @wojtek-t