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

Events in federated namespace controller #31640

Merged
merged 1 commit into from Aug 30, 2016

Conversation

Projects
None yet
6 participants
@mwielgus
Copy link
Contributor

mwielgus commented Aug 29, 2016

@quinton-hoole @nikhiljindal @wojtek-t @kubernetes/sig-cluster-federation


This change is Reviewable

@@ -290,6 +308,9 @@ func (nc *NamespaceController) reconcileNamespace(namespace string) {
}
err = nc.federatedUpdater.Update(operations, nc.updateTimeout)
if err != nil {
nc.eventRecorder.Event(baseNamespace, api.EventTypeWarning, "FailedUpdate",
"Failed to execute updates")

This comment has been minimized.

Copy link
@nikhiljindal

nikhiljindal Aug 29, 2016

Member

We should say which update failed and why

This comment has been minimized.

Copy link
@mwielgus

mwielgus Aug 30, 2016

Author Contributor

Done. Needed #31641 to do this.

@@ -290,6 +308,9 @@ func (nc *NamespaceController) reconcileNamespace(namespace string) {
}
err = nc.federatedUpdater.Update(operations, nc.updateTimeout)
if err != nil {
nc.eventRecorder.Event(baseNamespace, api.EventTypeWarning, "FailedUpdate",

This comment has been minimized.

Copy link
@nikhiljindal

nikhiljindal Aug 29, 2016

Member

FailedClusterUpdate?

This comment has been minimized.

Copy link
@mwielgus

mwielgus Aug 30, 2016

Author Contributor

Done.

@@ -264,6 +276,9 @@ func (nc *NamespaceController) reconcileNamespace(namespace string) {
}

if !found {
nc.eventRecorder.Eventf(baseNamespace, api.EventTypeNormal, "UpdateCluster",
"Create namespace in cluster %s", cluster.Name)

This comment has been minimized.

Copy link
@nikhiljindal

nikhiljindal Aug 29, 2016

Member

Its better to be explicit about is this Creat_ed_ or Creat_ing_. I think it should be Creating here.

This comment has been minimized.

Copy link
@mwielgus

mwielgus Aug 30, 2016

Author Contributor

Done.


broadcaster := record.NewBroadcaster()
broadcaster.StartRecordingToSink(eventsink.NewFederatedEventSink(client))
recorder := broadcaster.NewRecorder(api.EventSource{Component: "namespace-controller"})

This comment has been minimized.

Copy link
@nikhiljindal

nikhiljindal Aug 29, 2016

Member

Not sure where this is used and should this be unique across all controllers, but Component name "federation-namespace-controller" should be better?

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Aug 30, 2016

Member

+1 to include something about federation

This comment has been minimized.

Copy link
@mwielgus

mwielgus Aug 30, 2016

Author Contributor

Done.

@nikhiljindal

This comment has been minimized.

Copy link
Member

nikhiljindal commented Aug 29, 2016

Not sure whats the rule you are using around when to generate an event. Should we generate an event each time there is an error?
Also should we generate an event when a namespace is deleted? (We are generating an event when it is marked terminating)

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Aug 29, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

@@ -74,13 +80,19 @@ type NamespaceController struct {

// NewNamespaceController returns a new namespace controller
func NewNamespaceController(client federation_release_1_4.Interface) *NamespaceController {

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Aug 30, 2016

Member

nit: please remove empty line

This comment has been minimized.

Copy link
@mwielgus

mwielgus Aug 30, 2016

Author Contributor

Done.

@@ -264,6 +276,9 @@ func (nc *NamespaceController) reconcileNamespace(namespace string) {
}

if !found {
nc.eventRecorder.Eventf(baseNamespace, api.EventTypeNormal, "UpdateCluster",

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Aug 30, 2016

Member

Actually, why is "reason" set to "UpdateCluster"?

This comment has been minimized.

Copy link
@mwielgus

mwielgus Aug 30, 2016

Author Contributor

Done.

@@ -75,7 +75,8 @@ func TestNamespaceController(t *testing.T) {

ns1 := api_v1.Namespace{
ObjectMeta: api_v1.ObjectMeta{
Name: "test-namespace",
Name: "test-namespace",
SelfLink: "/api/v1/namespaces/test-namespace",

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Aug 30, 2016

Member

Hmm - why do you need this?

This comment has been minimized.

Copy link
@mwielgus

mwielgus Aug 30, 2016

Author Contributor

Events need selflink for proper attaching.

@@ -309,6 +330,7 @@ func (nc *NamespaceController) delete(namespace *api_v1.Namespace) {
},
}
if namespace.Status.Phase != api_v1.NamespaceTerminating {
nc.eventRecorder.Event(namespace, api.EventTypeNormal, "Delete", fmt.Sprintf("Marking for deletion"))

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Aug 30, 2016

Member

Delete"Something" (Delete itself is not very informative...)

This comment has been minimized.

Copy link
@mwielgus

mwielgus Aug 30, 2016

Author Contributor

Done.

@mwielgus mwielgus force-pushed the mwielgus:events-for-ns branch from c3d86d9 to a276ff4 Aug 30, 2016

@mwielgus

This comment has been minimized.

Copy link
Contributor Author

mwielgus commented Aug 30, 2016

@nikhiljindal We should generate an event when there is an explicit problem with a namespace, not with controller, syncing, etc.

@wojtek-t wojtek-t added lgtm and removed lgtm labels Aug 30, 2016

@wojtek-t

This comment has been minimized.

Copy link
Member

wojtek-t commented Aug 30, 2016

LGTM - but please fix tests :-)

@mwielgus mwielgus force-pushed the mwielgus:events-for-ns branch from a276ff4 to 044fab9 Aug 30, 2016

@mwielgus

This comment has been minimized.

Copy link
Contributor Author

mwielgus commented Aug 30, 2016

Yeah, I the ran tests in a wrong directory :-/. Fixed.

@mwielgus mwielgus added the lgtm label Aug 30, 2016

@k8s-bot

This comment has been minimized.

Copy link

k8s-bot commented Aug 30, 2016

GCE e2e build/test passed for commit 044fab9.

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Aug 30, 2016

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 1b05640 into kubernetes:master Aug 30, 2016

7 checks passed

Jenkins GCE Node e2e Build finished. 651 tests run, 160 skipped, 0 failed.
Details
Jenkins GCE e2e Build finished. 385 tests run, 176 skipped, 0 failed.
Details
Jenkins GKE smoke e2e Build finished. 385 tests run, 382 skipped, 0 failed.
Details
Jenkins unit/integration Build finished. 3945 tests run, 24 skipped, 0 failed.
Details
Jenkins verification Build finished.
Details
Submit Queue Queued to run github e2e tests a second time.
Details
cla/google All necessary CLAs are signed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.