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

Modify NEG libraries to use composite types. #926

Merged
merged 3 commits into from
Nov 9, 2019
Merged

Modify NEG libraries to use composite types. #926

merged 3 commits into from
Nov 9, 2019

Conversation

prameshj
Copy link
Contributor

@prameshj prameshj commented Nov 4, 2019

Verified that the existing NEG creation/endpoint add works.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 4, 2019
@prameshj
Copy link
Contributor Author

prameshj commented Nov 4, 2019

@spencerhance
/assign @freehan

spencerhance
spencerhance reviewed Nov 4, 2019
View reviewed changes
Copy link
Contributor

@spencerhance spencerhance left a comment

Choose a reason for hiding this comment

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

Just one comment

pkg/utils/serviceport.go
@@ -53,6 +54,14 @@ type ServicePort struct {
BackendNamer namer.BackendNamer
}

func GetAPIVersionFromServicePort(sp *ServicePort) meta.Version {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can use VersionFromServicePort() in pkg/backends/features/features.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Synced offline to discuss this. I will add a TODO to unify this with the other version lookups we are doing. Current logic for api lookup is not on a per-resource basis.

Copy link
Contributor

Choose a reason for hiding this comment

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

add comment?

@freehan
Copy link
Contributor

freehan commented Nov 5, 2019

The unit tests are still broken. Let me know if you need help to convert the unit tests to use composite types.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 5, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 6, 2019
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 6, 2019
freehan
freehan reviewed Nov 7, 2019
View reviewed changes
pkg/composite/gen.go
}

return compositeMap, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

I am thinking returning a map[meta.Key]*NetworkEndpoinGroup.

This is probably more natural than the current output.

Need a small change on the consumer side. I think only NEG GC uses AggregatedList.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

pkg/utils/serviceport.go
@@ -53,6 +54,14 @@ type ServicePort struct {
BackendNamer namer.BackendNamer
}

func GetAPIVersionFromServicePort(sp *ServicePort) meta.Version {
Copy link
Contributor

Choose a reason for hiding this comment

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

add comment?

pkg/neg/readiness/poller_test.go
@@ -383,7 +384,11 @@ func TestPoll(t *testing.T) {

// add NE with healthy status
negtypes.GetNetworkEndpointStore(negCloud).AddNetworkEndpointHealthStatus(*meta.ZonalKey(negName, zone), negtypes.NetworkEndpointEntry{
NetworkEndpoint: ne,
NetworkEndpoint: &compute.NetworkEndpoint{
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: is there a conversion helper function can change composite.NetworkEndpoint to compute.NetworkEndpoint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we do not generate it for derived resources - i.e There is one for NetworkEndpointGroup, NetworkEndpoint is created because it is part of the NetworkEndpointGroup proto. Given that we want to move the mock NetworkEndpointStore to also use composite types, I think it is ok to leave it for now. Is that ok?

pkg/neg/types/types.go
@@ -252,5 +255,14 @@ func (key NegSyncerKey) String() string {
return fmt.Sprintf("%s/%s-%s-%s", key.Namespace, key.Name, key.Subset, key.PortTuple.String())
}

func (key NegSyncerKey) GetAPIVersion() meta.Version {
Copy link
Contributor

Choose a reason for hiding this comment

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

add comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@prameshj
Modify NEG libraries to use composite types
93022bb 
Modified syncer_test to use only one v1 import.
@prameshj
Composite change to AggregatedList
e01a626 
There is some code duplication since map[string]Struct cannot be cast
to map[string]interface{}
Fixed a bug in ListNetworkEndpointGroups, where the incorrect err
variable was being returned.
@prameshj
Autogenerated changes.
3849ae4
freehan
freehan reviewed Nov 9, 2019
View reviewed changes
Copy link
Contributor

@freehan freehan left a comment

Choose a reason for hiding this comment

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

Only one nit. Probably better to fix in a follow up PR.

/lgtm
/approve

pkg/neg/types/cloudprovideradapter.go
ctx, cancel := cloud.ContextWithCallTimeout()
defer cancel()

func (a *cloudProviderAdapter) AggregatedListNetworkEndpointGroup(version meta.Version) (map[string][]*composite.NetworkEndpointGroup, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

change the adapter to return map[meta.Key]*composite.NetworkEndpointGroup as well?

Or add a TODO? Should not be too complex. The GC logic may need to change a little

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 added a TODO to my current branch, which has changes to create VM_IP NEGs. I will include this change along with that PR.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, prameshj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2019
@k8s-ci-robot k8s-ci-robot merged commit 011cb28 into kubernetes:master Nov 9, 2019
@prameshj prameshj mentioned this pull request Nov 13, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@freehan freehan freehan left review comments

@spencerhance spencerhance spencerhance left review comments

@MrHohn MrHohn Awaiting requested review from MrHohn

@thockin thockin Awaiting requested review from thockin

Assignees

@freehan freehan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@prameshj @freehan @k8s-ci-robot @spencerhance