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.

Sign up for GitHub

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

Federated Ingress Controller #30419

Merged
merged 1 commit into from Aug 22, 2016
Merged

Federated Ingress Controller #30419

merged 1 commit into from Aug 22, 2016

Conversation

ghost
Copy link

@ghost ghost commented Aug 11, 2016 •
edited by ghost
Loading

Based on new federated controller libraries.

cc @kubernetes/sig-cluster-federation @mfanjie @nikhiljindal @mwielgus @mml @madhusudancs FYI

This change is Reviewable

@ghost ghost added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. team/control-plane labels Aug 11, 2016
@ghost ghost added this to the v1.4 milestone Aug 11, 2016
@ghost ghost self-assigned this Aug 11, 2016
@googlebot
Copy link

CLAs look good, thanks!

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Aug 11, 2016
mfanjie
mfanjie reviewed Aug 11, 2016
View reviewed changes
federation/pkg/federation-controller/ingress/ingress-controller.go
ic.ingressInformerStore, ic.ingressInformerController = framework.NewInformer(
&cache.ListWatch{
ListFunc: func(options api.ListOptions) (pkg_runtime.Object, error) {
return client.Extensions().Ingresses("<all-namespaces>").List(options)
Copy link

Choose a reason for hiding this comment

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

should use api.NamespaceAll?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, agreed. I couldn't remember the correct incantation, so I added that placeholder instead :-)

@ghost ghost mentioned this pull request Aug 11, 2016
Closed
jianhuiz
jianhuiz reviewed Aug 16, 2016
View reviewed changes
federation/pkg/federation-controller/ingress/ingress-controller.go
},
&extensions.Ingress{},
controller.NoResyncPeriodFunc(),
util.NewTriggerOnAllChanges(func(obj pkg_runtime.Object) { ic.deliverIngressObj(obj, 0, 0) }))
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we still need to handle DeletedFinalStateUnknown case?

@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 Aug 17, 2016
@ghost ghost changed the title [WIP] Federated Ingress Controller Federated Ingress Controller Aug 18, 2016
@ghost ghost mentioned this pull request Aug 18, 2016
Closed
@ghost ghost assigned mwielgus and unassigned ghost Aug 18, 2016
@ghost
Copy link
Author

ghost commented Aug 18, 2016

This is ready for review now. Unit tests pass, although I'm sure we'll find some issues when tested against the new e2e tests in #29773.

I will add the following in small followup PR's:

  1. DNS record creation
  2. namespace handling
  3. sharing a public IP across shards

cc @mml @nikhiljindal @madhusudancs

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@ghost
Copy link
Author

ghost commented Aug 18, 2016

Whoops! It seems I messed up a branch merge. Until I fix that up, reviewers need only look at

  1. federation/pkg/federation-controller/ingress
  2. federation/cmd/federation-controller-manager/app/controllermanager.go

@k8s-github-robot k8s-github-robot added kind/design Categorizes issue or PR as related to design. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 18, 2016
@ghost
Copy link
Author

ghost commented Aug 18, 2016

OK, I fixed my merge mess-up.

@k8s-github-robot k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed kind/design Categorizes issue or PR as related to design. labels Aug 18, 2016
@ghost ghost added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Aug 19, 2016
mwielgus
mwielgus reviewed Aug 19, 2016
View reviewed changes
federation/pkg/federation-controller/ingress/ingress_controller.go
// error (like failure to create).
type ingressItem struct {
ingress string
prevAttempts int64
Copy link
Contributor

Choose a reason for hiding this comment

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

you probably don't need prev attempts (and this struct at all).

Copy link
Author

Choose a reason for hiding this comment

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

Done.

@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2016
@ghost
Copy link
Author

ghost commented Aug 22, 2016

Thanks for the review @mwielgus . I've:

  1. addressed all of your comments
  2. added support for namespaces (as we discussed)
  3. fixed a bug in the create and update logic around ObjectMeta (as discussed)
  4. added logic to share a single global IP across all shards as described here

PTAL. I think this is ready for LGTM now. All unit tests, and manual e2e tests passed. Automated e2e tests are lined up in #29773 which should merge in the next couple of days.

@ghost
Copy link
Author

ghost commented Aug 22, 2016

Review status: 0 of 6 files reviewed at latest revision, 8 unresolved discussions.

federation/pkg/federation-controller/ingress/ingress_controller.go, line 62 [r1] (raw file):

Previously, quinton-hoole (Quinton Hoole) wrote…

Agreed, done.

Done.

federation/pkg/federation-controller/ingress/ingress_controller.go, line 96 [r1] (raw file):

Previously, quinton-hoole (Quinton Hoole) wrote…

Yes, agreed. I couldn't remember the correct incantation, so I added that placeholder instead :-)

Done.

federation/pkg/federation-controller/ingress/ingress_controller.go, line 107 [r1] (raw file):

Previously, quinton-hoole (Quinton Hoole) wrote…

Agreed, but fix in a separate PR, to keep this PR clean.

Done.

federation/pkg/federation-controller/ingress/ingress_controller.go, line 176 [r1] (raw file):

Previously, quinton-hoole (Quinton Hoole) wrote…

Agreed. Will do. The controller that this is modelled on is for namespaces themselves, which are not scoped by namespace, hence this.

Done.

federation/pkg/federation-controller/ingress/ingress_controller.go, line 80 [r5] (raw file):

Previously, quinton-hoole (Quinton Hoole) wrote…

Done.

Done.

federation/pkg/federation-controller/ingress/ingress_controller.go, line 255 [r5] (raw file):

Previously, quinton-hoole (Quinton Hoole) wrote…

Done.

Done.

Comments from Reviewable

mwielgus
mwielgus reviewed Aug 22, 2016
View reviewed changes
federation/pkg/federation-controller/ingress/ingress_controller.go
"k8s.io/kubernetes/federation/pkg/federation-controller/util"
"k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/types"
// api_v1 "k8s.io/kubernetes/pkg/api/v1"
Copy link
Contributor

Choose a reason for hiding this comment

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

remove

Copy link
Author

Choose a reason for hiding this comment

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

Yup, done.

@mwielgus
Copy link
Contributor

minor comments, LGTM assuming a clean-up PR is submitted shortly.

@ghost
Copy link
Author

ghost commented Aug 22, 2016

Fixed the nit, squashed, and checked out the e2e test failure, which looks like a flake - the test cluster never came up. Applying LGTM as per review feedback above.

@ghost ghost added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 22, 2016
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2016
@ghost
Copy link
Author

ghost commented Aug 22, 2016

Rebased. Re-applying LGTM.

@ghost ghost added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 22, 2016
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2016
@k8s-bot
Copy link

k8s-bot commented Aug 22, 2016

GCE e2e build/test passed for commit 97d6494.

@k8s-github-robot k8s-github-robot mentioned this pull request Aug 22, 2016
Closed
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Aug 22, 2016

GCE e2e build/test passed for commit 97d6494.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 5f9f8cc into kubernetes:master Aug 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@mwielgus mwielgus

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
6 participants
@googlebot @mwielgus @k8s-bot @k8s-github-robot @jianhuiz @mfanjie