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

[WIP] Change hashing algorithm in Deployments #38714

Closed
wants to merge 2 commits into from
Closed

[WIP] Change hashing algorithm in Deployments #38714

wants to merge 2 commits into from

Conversation

0xmichalis
Copy link
Contributor

@0xmichalis 0xmichalis commented Dec 13, 2016

Superseded by #44774

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 13, 2016
@k8s-reviewable
Copy link

This change is Reviewable

@0xmichalis 0xmichalis added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. retest-not-required labels Dec 13, 2016
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Dec 13, 2016
@0xmichalis
Copy link
Contributor Author

@wojtek-t I want to write a test that spins up a cluster, creates X namespaces, creates Y deployments in each namespace, runs Z rollouts for each deployment, restart the controller manager with --deployment-hashing-algorithm=migrates-to-fnv, kick new rolllouts for all the existing deployments, and verify in the end that all replica sets have migrated. Any ideas where such a test should live?

X (average # of namespaces per cluster)
Y (average # of deployments per namespace)
Z (can be 2, can be 200)

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 14, 2016
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 6, 2017
k8s-github-robot pushed a commit that referenced this pull request Jan 6, 2017
…r-and-fnv

Automatic merge from submit-queue (batch tested with PRs 39466, 39490, 39527)

Test and benchmark adler and fnv

Split out of #38714

@kubernetes/sig-apps-misc
@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 6, 2017
@0xmichalis
Copy link
Contributor Author

@smarterclayton where do we expose metrics for the controller manager? I can't seem to find those under the master /metrics endpoint.

@bgrant0607
Copy link
Member

cc @janetkuo

@smarterclayton
Copy link
Contributor

No idea - scheduler has it, and I thought it was enabled for controller manager because someone was looking at queue depths a while back.

@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 10, 2017
@janetkuo
Copy link
Member

where do we expose metrics for the controller manager?

Is this what you're looking for?
https://github.com/kubernetes/kubernetes/blob/master/pkg/metrics/metrics_grabber.go#L122

func (g *MetricsGrabber) GrabFromControllerManager() (ControllerManagerMetrics, error) {
	if !g.registeredMaster {
		return ControllerManagerMetrics{}, fmt.Errorf("Master's Kubelet is not registered. Skipping ControllerManager's metrics gathering.")
	}
	output, err := g.getMetricsFromPod(fmt.Sprintf("%v-%v", "kube-controller-manager", g.masterName), api.NamespaceSystem, ports.ControllerManagerPort)
	if err != nil {
		return ControllerManagerMetrics{}, err
	}
	return parseControllerManagerMetrics(output)
}

@0xmichalis
Copy link
Contributor Author

@janetkuo the library you are linking is assuming we are running a self-hosted cluster (components run inside pods). I can't see us using it anywhere in the Kubernetes repository apart from some e2e tests.

k8s-github-robot pushed a commit that referenced this pull request Jan 13, 2017
Automatic merge from submit-queue (batch tested with PRs 39807, 37505, 39844, 39525, 39109)

Update deployment equality helper

@mfojtik @janetkuo this is split out of #38714 to reduce the size of that PR, ptal
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 13, 2017
@janetkuo
Copy link
Member

@Kargakis FYI the controller manager metrics handler is here

@0xmichalis
Copy link
Contributor Author

@janetkuo thanks for the pointer.

Note that if the update on the Deployment proposal that I have up for review is accepted (kubernetes/community#384), then we may not need to migrate old replica sets.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 23, 2017
@k8s-github-robot
Copy link

@Kargakis PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2017
@0xmichalis
Copy link
Contributor Author

A migration won't be needed. Closing this one

@0xmichalis 0xmichalis closed this Apr 21, 2017
@0xmichalis 0xmichalis deleted the change-hashing-algo branch April 21, 2017 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants