-
Notifications
You must be signed in to change notification settings - Fork 38.7k
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
Add Event Registry/REST types #1700
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
1c75d76
Shorten 'CodecForVersionOrDie' name, add 'ResourceVersioner' to testapi
lavalamp 9a9362e
Add generic registry object so we can stop rewriting this code
lavalamp 3e076e1
Add event registry and type
lavalamp 1568073
Add event endpoint to apiserver
lavalamp d3d9f7a
ObjectDiff moved after rebase
lavalamp b1a6b3e
Split generic; add test, address other review comments
lavalamp File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| /* | ||
| Copyright 2014 Google Inc. All rights reserved. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| // Package event provides Registry interface and it's REST | ||
| // implementation for storing Event api objects. | ||
| package event |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| /* | ||
| Copyright 2014 Google Inc. All rights reserved. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package event | ||
|
|
||
| import ( | ||
| "path" | ||
|
|
||
| "github.com/GoogleCloudPlatform/kubernetes/pkg/api" | ||
| etcderr "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors/etcd" | ||
| "github.com/GoogleCloudPlatform/kubernetes/pkg/registry/generic" | ||
| etcdgeneric "github.com/GoogleCloudPlatform/kubernetes/pkg/registry/generic/etcd" | ||
| "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" | ||
| "github.com/GoogleCloudPlatform/kubernetes/pkg/tools" | ||
| ) | ||
|
|
||
| // registry implements custom changes to generic.Etcd. | ||
| type registry struct { | ||
| *etcdgeneric.Etcd | ||
| ttl uint64 | ||
| } | ||
|
|
||
| // Create stores the object with a ttl, so that events don't stay in the system forever. | ||
| func (r registry) Create(ctx api.Context, id string, obj runtime.Object) error { | ||
| err := r.Etcd.Helper.CreateObj(r.Etcd.KeyFunc(id), obj, r.ttl) | ||
| return etcderr.InterpretCreateError(err, r.Etcd.EndpointName, id) | ||
| } | ||
|
|
||
| // NewEtcdRegistry returns a registry which will store Events in the given | ||
| // EtcdHelper. ttl is the time that Events will be retained by the system. | ||
| func NewEtcdRegistry(h tools.EtcdHelper, ttl uint64) generic.Registry { | ||
| return registry{ | ||
| Etcd: &etcdgeneric.Etcd{ | ||
| NewFunc: func() runtime.Object { return &api.Event{} }, | ||
| NewListFunc: func() runtime.Object { return &api.EventList{} }, | ||
| EndpointName: "events", | ||
| KeyRoot: "/registry/events", | ||
| KeyFunc: func(id string) string { | ||
| return path.Join("/registry/events", id) | ||
| }, | ||
| Helper: h, | ||
| }, | ||
| ttl: ttl, | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,105 @@ | ||
| /* | ||
| Copyright 2014 Google Inc. All rights reserved. | ||
|
|
||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
|
|
||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
|
|
||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package event | ||
|
|
||
| import ( | ||
| "reflect" | ||
| "testing" | ||
|
|
||
| "github.com/GoogleCloudPlatform/kubernetes/pkg/api" | ||
| "github.com/GoogleCloudPlatform/kubernetes/pkg/api/errors" | ||
| "github.com/GoogleCloudPlatform/kubernetes/pkg/api/testapi" | ||
| "github.com/GoogleCloudPlatform/kubernetes/pkg/registry/generic" | ||
| "github.com/GoogleCloudPlatform/kubernetes/pkg/runtime" | ||
| "github.com/GoogleCloudPlatform/kubernetes/pkg/tools" | ||
| "github.com/GoogleCloudPlatform/kubernetes/pkg/util" | ||
|
|
||
| "github.com/coreos/go-etcd/etcd" | ||
| ) | ||
|
|
||
| var testTTL uint64 = 60 | ||
|
|
||
| func NewTestEventEtcdRegistry(t *testing.T) (*tools.FakeEtcdClient, generic.Registry) { | ||
| f := tools.NewFakeEtcdClient(t) | ||
| f.TestIndex = true | ||
| h := tools.EtcdHelper{f, testapi.Codec(), tools.RuntimeVersionAdapter{testapi.ResourceVersioner()}} | ||
| return f, NewEtcdRegistry(h, testTTL) | ||
| } | ||
|
|
||
| func TestEventCreate(t *testing.T) { | ||
| eventA := &api.Event{ | ||
| TypeMeta: api.TypeMeta{ID: "foo"}, | ||
| Reason: "forTesting", | ||
| } | ||
| eventB := &api.Event{ | ||
| TypeMeta: api.TypeMeta{ID: "foo"}, | ||
| Reason: "forTesting", | ||
| } | ||
|
|
||
| nodeWithEventA := tools.EtcdResponseWithError{ | ||
| R: &etcd.Response{ | ||
| Node: &etcd.Node{ | ||
| Value: runtime.EncodeOrDie(testapi.Codec(), eventA), | ||
| ModifiedIndex: 1, | ||
| CreatedIndex: 1, | ||
| TTL: int64(testTTL), | ||
| }, | ||
| }, | ||
| E: nil, | ||
| } | ||
|
|
||
| emptyNode := tools.EtcdResponseWithError{ | ||
| R: &etcd.Response{}, | ||
| E: tools.EtcdErrorNotFound, | ||
| } | ||
|
|
||
| path := "/registry/events/foo" | ||
| key := "foo" | ||
|
|
||
| table := map[string]struct { | ||
| existing tools.EtcdResponseWithError | ||
| expect tools.EtcdResponseWithError | ||
| toCreate runtime.Object | ||
| errOK func(error) bool | ||
| }{ | ||
| "normal": { | ||
| existing: emptyNode, | ||
| expect: nodeWithEventA, | ||
| toCreate: eventA, | ||
| errOK: func(err error) bool { return err == nil }, | ||
| }, | ||
| "preExisting": { | ||
| existing: nodeWithEventA, | ||
| expect: nodeWithEventA, | ||
| toCreate: eventB, | ||
| errOK: errors.IsAlreadyExists, | ||
| }, | ||
| } | ||
|
|
||
| for name, item := range table { | ||
| fakeClient, registry := NewTestEventEtcdRegistry(t) | ||
| fakeClient.Data[path] = item.existing | ||
| err := registry.Create(api.NewContext(), key, item.toCreate) | ||
| if !item.errOK(err) { | ||
| t.Errorf("%v: unexpected error: %v", name, err) | ||
| } | ||
|
|
||
| if e, a := item.expect, fakeClient.Data[path]; !reflect.DeepEqual(e, a) { | ||
| t.Errorf("%v:\n%s", name, util.ObjectDiff(e, a)) | ||
| } | ||
| } | ||
| } |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My previous comment got lost. I am concerned about the etcd-ness leaking into each registry module. Shouldn't registries be defined in terms of backing store interfaces, so they can use etcd but not actually KNOW that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is phrased that way-- see generic.Registry, which makes no mention of etcd.
This function is specifically to make an etcd registry, so of course it has to mention etcd.
If we get a new FrooBobber store, then we'd have a file here with a NewFrooBobberRegistry() function.
Would you prefer that I make a pkg/registry/event/etcd directory for this to go in? I can do that if it makes you happy, it just seemed a little excessive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caveat, I am not super familiar with this area,s till. My point is that the word and concept of etcd should never appear here.
struct registry should be defined as something like
Where BackingStore is an interface with functions that map to Etcd. Creating a registry should be something like:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That confuses me. This file is providing the thing you've called "RegsitryBackingStore". How can I make that more clear?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This creates an Event-specific backing store. Presumably there would be a Pod-specific backing store and a Service-specific backing store and an Enpoints-specific backing store and ...
Etcd should be mentioned once in the whole init sequence. It is the medium. Then you can define the schemas that sit atop the medium - Event, Pod, Service..
If this isn't clear, maybe it is I who just isn't getting it - come by and we can discuss after lunch?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(offline convo: resolution here is that tools.EtcdHelper should be hidden behind some sort of StorageHelper interface. I'll do this in a follow-up PR at some point.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM
On Mon, Oct 13, 2014 at 2:31 PM, Daniel Smith notifications@github.com
wrote: