Enable petsets in origin #9972

smarterclayton · 2016-07-21T02:03:14Z

[test]

smarterclayton · 2016-07-21T14:42:25Z

Has a mostly green run, would like to get this in @liggitt

deads2k · 2016-07-21T14:44:53Z

That integration flake is killing us.

deads2k · 2016-07-21T14:46:34Z

examples/pets/mysql/galera/mysql-galera.yaml

+        app: mysql
+      annotations:
+        pod.alpha.kubernetes.io/initialized: "true"
+        pod.alpha.kubernetes.io/init-containers: '[


SCC checks these, right?

https://github.com/openshift/origin/pull/9973/files#diff-eb43ba779f411e3ee6b772cceee9810d

deads2k · 2016-07-21T14:52:29Z

Update our hack script for tests to pull in this example each time so we don't lose it

deads2k · 2016-07-21T15:01:42Z

pkg/cmd/server/bootstrappolicy/infra_sa_policy.go

+				Name: PetSetControllerRoleName,
+			},
+			Rules: []authorizationapi.PolicyRule{
+				// PetSetController.podCache.ListWatch


This means its not using a shared informer. Either you wired it wrong or we need an upstream fix.

It didn't require this to work, I'm again just following the pattern in this file which is agnostic of shared informer.

smarterclayton · 2016-07-21T15:37:07Z

Update our hack script for tests to pull in this example each time so we don't lose it

I think I'm going to curate the examples list and focus on one or two that work, and point people to upstream.

smarterclayton · 2016-07-21T15:41:05Z

I'm going to enable these for read write for admin/edit now, rather than later, because there may not be a later and I don't want to hold QE up. Admins can always deny them if they have a problem.

deads2k · 2016-07-21T18:37:35Z

I'm going to enable these for read write for admin/edit now, rather than later, because there may not be a later and I don't want to hold QE up. Admins can always deny them if they have a problem.

You know we're only getting to do this because he's on vacation. I think you should now merge something crazy of mine :)

liggitt · 2016-07-21T18:38:09Z

I hate you both

smarterclayton · 2016-07-21T20:52:21Z

I don't anticipate pet set problems, and if we get the "insecure role" I'll move them there.

smarterclayton · 2016-07-22T02:01:38Z

Trying the examples out is mostly a DNS problem at this point (the upstream DNS code is opaquely tested, so I'm not even sure what format they are expecting, or whether they don't even work on centos for some reason).

smarterclayton · 2016-07-22T04:23:02Z

DNS is broken in Kubernetes (ish), sorting out in 29420

smarterclayton · 2016-07-26T18:34:43Z

Good news! DNS was broken on our side. Fixing. And adding mor tests.

deads2k · 2016-07-26T19:01:44Z

@smarterclayton What are the precise rules on hostname annotations? If you enable them here, it may have an impact on the service serving cert signer.

@liggitt There were issues with the first upstream implementations of this hostname thing. Is it good now?

smarterclayton · 2016-07-26T19:07:15Z

Rules are they have to be set on the pod. So create/update on pods allows you to add public DNS names under the controlling service.

smarterclayton · 2016-07-26T19:07:58Z

I'm not aware of any security issues w.r.t. hostname annotation upstream in the current design.

smarterclayton · 2016-07-27T03:25:30Z

Hells yeah! going to make status work real quick.

On Tue, Jul 26, 2016 at 11:17 PM, OpenShift Bot notifications@github.com
wrote:

continuous-integration/openshift-jenkins/test SUCCESS (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/6957/)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9972 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_py2Wi9XqtOUZEQ4BceQPCu8lhgbpks5qZs3NgaJpZM4JRYA_
.

smarterclayton · 2016-07-27T04:23:04Z

Ok, petset is now no longer in an embarrassing state in Origin. DNS is now conformant with Kube (tim's tests do not test what he thinks they test), the galera example worked for me with no modification except adding host path volumes to my system, and oc status is not terrible:

oc status
In project default on server https://10.1.2.2:8443

svc/galera (headless):3306
  petset/mysql manages erkules/galera:basic created 5 days ago - 3 pods

svc/kubernetes - 172.30.0.1 ports 443, 53->8053, 53->8053

I made a structural change to the graph to prepare for more than one controller (replaced ManagedByRC with ManagedByControllers, cleaned up some abstractions slightly) although I did not add petset vs RC fighting. I'll open an issue to replace our complicated "this manages this" with "this is covered by this controller" as the idle PR covers.

smarterclayton · 2016-07-30T21:06:59Z

[test]

deads2k · 2016-08-04T18:41:00Z

pkg/api/kubegraph/edges.go

-	ManagedByRCEdgeKind = "ManagedByRC"
+	// ManagedByControllerEdgeKind goes from Pod to ReplicationController when the Pod satisfies the ReplicationController's label selector
+	// TODO: rename to ManagedByController, make generic to controller ref
+	ManagedByControllerEdgeKind = "ManagedByRC"


Let's change the constant now.

deads2k · 2016-08-04T18:42:33Z

This pull got a lot bigger while my back was turned.

deads2k · 2016-08-04T18:46:31Z

pkg/cmd/server/bootstrappolicy/infra_sa_policy.go

+				},
+				// PetSetController.podClient
+				{
+					Verbs:     sets.NewString("get", "create", "delete", "update"),


deads2k · 2016-08-04T18:49:58Z

minor comments, didn't review the dns bits, lgtm otherwise.

Make a minor change to prepare for generic controller references (since PetSets and RCs could conflict over pods).

openshift-bot · 2016-08-04T19:47:28Z

Evaluated for origin test up to cc58509

openshift-bot · 2016-08-04T21:07:34Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7527/)

DirectXMan12 · 2016-08-04T21:17:13Z

pkg/dns/serviceresolver.go

+			}
+		}
+
+		matchHostname := len(segments) > 3 && !hasAllPrefixedSegments(segments[3:4], "_")


The godoc at the top of this function should be updated to clarify that we now either return a hash of the IP or a hostname value. Additionally, it would be very useful to people looking at/reviewing this code in the future if there were comments added before each "branch point"/conditional indicating which subquery they were matching (for instance, AFAICT, this one might read "matchHostname indicates we are matching ...endpoints, and not _endpoints...svc")

Will do a follow up PR with comments - saw this after I hit the big red. Keep commenting please.

Also: nit: I think this would be clearer as !strings.HasPrefix(segments[3], "_") (the use of the single-element slice here seems ad bit odd to me...)

smarterclayton · 2016-08-04T21:41:17Z

[merge] talked through DNS stuff with Jordan and will get more review later.

openshift-bot · 2016-08-04T21:42:21Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7527/) (Image: devenv-rhel7_4758)

openshift-bot · 2016-08-04T21:42:22Z

Evaluated for origin merge up to cc58509

DirectXMan12 · 2016-08-05T03:02:04Z

test/integration/dns_test.go

@@ -187,20 +187,29 @@ func TestDNS(t *testing.T) {
 			expect:          []*net.IP{&headlessIP},
 		},
 		{ // specific port of a headless service
-			dnsQuestionName: "unknown-port-2345.e1.headless.default.svc.cluster.local.",
+			dnsQuestionName: "_http._tcp.headless.default.svc.cluster.local.",


It looks like none of these exercise the _<protocol>.<svc>.<ns>.svc case (wildcard port match), so there should probably be such a test here. Furthermore, it looks like none of these test the .endpoints.cluster.local queries, and that seems like it would be valuable to test here as well.

Those are tested in the extended DNS test and the test-cmd DNS test.

On Thu, Aug 4, 2016 at 11:02 PM, Solly Ross notifications@github.com
wrote:

In test/integration/dns_test.go
#9972 (comment):

@@ -187,20 +187,29 @@ func TestDNS(t _testing.T) {
expect: []_net.IP{&headlessIP},
},
{ // specific port of a headless service

dnsQuestionName: "unknown-port-2345.e1.headless.default.svc.cluster.local.",

dnsQuestionName: "_http._tcp.headless.default.svc.cluster.local.",

It looks like none of these exercise the _...svc case
(wildcard port match), so there should probably be such a test here.
Furthermore, it looks like none of these test the .endpoints queries, and
that seems like it would be valuable to test here as well.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/openshift/origin/pull/9972/files/cc585097d4706f1d49fb9a451760f38e9863a794#r73636324,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABG_pymEzpOhGu0rsHQnD2TGkhSFO4LAks5qcqeugaJpZM4JRYA_
.

DirectXMan12 · 2016-08-05T03:08:02Z

Overall, I think the DNS stuff in general looks ok, but it seems like there are a couple of paths missing from the integration tests, and the DNS code really needs better commenting. The flow of the DNS code is somewhat convoluted (not particularly because of this PR, just in general), so I think comments to the effect of // now, we can only have x.y.z and a.b.z queries, so try those would be fairly useful in making this easier to read.

smarterclayton force-pushed the petset branch from 607a2a9 to 85a7c5d Compare July 21, 2016 04:06

deads2k self-assigned this Jul 21, 2016

deads2k reviewed Jul 21, 2016
View reviewed changes

smarterclayton force-pushed the petset branch 3 times, most recently from 0e46f68 to 2e1f4d0 Compare July 27, 2016 01:56

smarterclayton force-pushed the petset branch from 2e1f4d0 to 61014ae Compare July 27, 2016 04:20

smarterclayton force-pushed the petset branch from 61014ae to fb7983f Compare July 27, 2016 13:24

deads2k reviewed Aug 4, 2016
View reviewed changes

smarterclayton force-pushed the petset branch from fdbb46f to 69a7249 Compare August 4, 2016 19:11

smarterclayton added 8 commits August 4, 2016 15:46

Enable petsets

2e31d6a

PetSet examples

b9e2091

Debug should be able to skip init containers

140c6c9

generate: debug command

9d296e4

DNS should support hostname annotations

aa0cae0

Show PetSets in oc status

df50db2

Make a minor change to prepare for generic controller references (since PetSets and RCs could conflict over pods).

UPSTREAM: 29655: No PetSet client

eb0dc79

Remove unsupported DNS calls from tests

cc58509

smarterclayton force-pushed the petset branch from 69a7249 to cc58509 Compare August 4, 2016 19:46

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 4, 2016

DirectXMan12 reviewed Aug 4, 2016
View reviewed changes

openshift-bot merged commit e52a190 into openshift:master Aug 4, 2016

DirectXMan12 reviewed Aug 5, 2016
View reviewed changes

smarterclayton mentioned this pull request Aug 5, 2016

1.3 upstream picks #9790

Closed

ncdc mentioned this pull request Aug 18, 2016

No PetSet client in client/unversioned openshift/kubernetes#23

Closed

juanvallejo mentioned this pull request Oct 18, 2016

emit warning on no liveness probe defined for pods #10363

Merged

Enable petsets in origin #9972

Enable petsets in origin #9972

Conversation

smarterclayton commented Jul 21, 2016

smarterclayton commented Jul 21, 2016

deads2k commented Jul 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Jul 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Jul 21, 2016

smarterclayton commented Jul 21, 2016

deads2k commented Jul 21, 2016

liggitt commented Jul 21, 2016

smarterclayton commented Jul 21, 2016

smarterclayton commented Jul 22, 2016

smarterclayton commented Jul 22, 2016

smarterclayton commented Jul 26, 2016

deads2k commented Jul 26, 2016

smarterclayton commented Jul 26, 2016

smarterclayton commented Jul 26, 2016

smarterclayton commented Jul 27, 2016

smarterclayton commented Jul 27, 2016

smarterclayton commented Jul 30, 2016

Choose a reason for hiding this comment

deads2k commented Aug 4, 2016

Choose a reason for hiding this comment

deads2k commented Aug 4, 2016

openshift-bot commented Aug 4, 2016

openshift-bot commented Aug 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Aug 4, 2016

openshift-bot commented Aug 4, 2016 • edited Loading

openshift-bot commented Aug 4, 2016

DirectXMan12 Aug 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DirectXMan12 commented Aug 5, 2016

openshift-bot commented Aug 4, 2016 •

edited

Loading

DirectXMan12 Aug 5, 2016 •

edited

Loading