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

Move deployments to using versioned Kube informers #14728

Merged

Conversation

smarterclayton
Copy link
Contributor

Part of #8229

Switches to the versioned caches and propogates using versioned code throughout deployment for pods and RCs. Leaves a few helpers pointing to internal to reduce test refactoring.

[test]

@mfojtik
Copy link
Contributor

mfojtik commented Jun 19, 2017

LGTM

}
}
if pod.Spec.Containers[i].Resources.Requests == nil {
pod.Spec.Containers[i].Resources.Requests = kapi.ResourceList{}
pod.Spec.Containers[i].Resources.Requests = v1.ResourceList{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are you getting your pod from. There is defaulting on resource.Requests. Internal would always have run it. Does this path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This the whole "this isn't actually being used in admission" weirdness. This is called by a controller that has internal build and external pod.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually creating a new pod.


// ServiceServerCertificateExtension returns a CertificateExtensionFunc that will add the
// service UID as an x509 v3 extension to the server certificate.
func ServiceServerCertificateExtensionV1(svc *kapiv1.Service) crypto.CertificateExtensionFunc {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we still have the old one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can drop.

}
glog.V(4).Infof("Updated rollout status for %q from %s to %s (scale: %d)", deployutil.LabelForDeployment(deployment), currentStatus, nextStatus, deployment.Spec.Replicas)
glog.V(4).Infof("Updated rollout status for %q from %s to %s (scale: %d)", deployutil.LabelForDeploymentV1(deployment), currentStatus, nextStatus, deployment.Spec.Replicas)
Copy link
Contributor

Choose a reason for hiding this comment

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

deployment.Spec.Replicas needs to be dereferenced now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'll get printed correctly by fmt.

func IsOwnedByConfig(deployment *api.ReplicationController) bool {
_, ok := deployment.Annotations[deployapi.DeploymentConfigAnnotation]
func IsOwnedByConfig(obj metav1.Object) bool {
_, ok := obj.GetAnnotations()[deployapi.DeploymentConfigAnnotation]
Copy link
Contributor

Choose a reason for hiding this comment

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

@tnozicka @mfojtik we need to deprecate the old way of determining ownership in favor of owner references.

Goal of eliminating duplicate internal and external caches when running
in the controller by itself.
We are gradually moving to external clients for controllers, and this
will remove the need to have separate caches for them.
Removes the need for an additional cache for these. Also all controllers
are moving in this direction anyway.
@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 7d2ac22

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin/2413/) (Base Commit: b0c0635) (PR Branch Commit: 7d2ac22)

@smarterclayton smarterclayton merged commit 1d4dd0d into openshift:master Jun 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants