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

Extract BackendPool interface into three 3 separate interfaces #424

Merged
merged 1 commit into from
Aug 13, 2018

Conversation

rramkumar1
Copy link
Contributor

@rramkumar1 rramkumar1 commented Aug 7, 2018

This PR extracts the BackendPool interface into 3 separate interfaces:

1. BackendPool - Simple CRUD for backends
2. Syncer - Sync and GC backends
3. Linker - Link backends to their associated groups. 

This allows us to break the current BackendPool implementation up into more manageable chunks and each chunk is much more testable. Furthermore, some implementations, namely BackendPool and Syncer, can be easily reused.

/assign @bowei

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 7, 2018
@rramkumar1 rramkumar1 force-pushed the backend-pool-changes branch 2 times, most recently from 3f800c2 to be5ea3e Compare August 7, 2018 22:34
@rramkumar1
Copy link
Contributor Author

/assign @freehan

@@ -0,0 +1,180 @@
package backends
Copy link
Member

Choose a reason for hiding this comment

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

copyright

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed everywhere.

@rramkumar1 rramkumar1 force-pushed the backend-pool-changes branch 3 times, most recently from 0291d72 to 18d5abc Compare August 10, 2018 00:29
@rramkumar1
Copy link
Contributor Author

All tests are now migrated over to new interfaces.

Added a new test file called integration_test.go which contains some tests carried over from the original. Those are failing so need to figure out why.

@rramkumar1
Copy link
Contributor Author

Update: All tests pass.

Copy link
Member

@bowei bowei left a comment

Choose a reason for hiding this comment

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

minor stuff

// will dictate whether we search for an existing legacy health check.
hcLink, err := b.ensureHealthCheck(sp, hasLegacyHC)
if err != nil {
// Implements Pool.
Copy link
Member

Choose a reason for hiding this comment

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

Update implements Pool.

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

}

// Delete deletes the Backend for the given port.
// Implements Pool.
func (b *Backends) Delete(name string) (err error) {
Copy link
Member

Choose a reason for hiding this comment

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

Delete implements ...

Copy link
Member

Choose a reason for hiding this comment

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

Can we use non-implicit err here?

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 wasn't able to get rid of it because of the defer.

if !oldBackends.Equal(newBackends) {
backendService.Backends = targetBackends
return b.cloud.UpdateBetaGlobalBackendService(backendService)
// Implements Pool.
Copy link
Member

Choose a reason for hiding this comment

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

GetLocalSnapshot implements Pool.

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

func (b *Backends) edgeHop(be *composite.BackendService, igLinks []string) error {
addIGs, err := getInstanceGroupsToAdd(be, igLinks)
// Implements Pool.
func (b *Backends) Get(name string, version meta.Version) (*composite.BackendService, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Get implements ...

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

func (b *Backends) Status(name string) string {
backend, err := b.cloud.GetGlobalBackendService(name)
if err != nil || len(backend.Backends) == 0 {
// Implements Pool.
Copy link
Member

Choose a reason for hiding this comment

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

// Health implements ..

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

const maxRPS = 1

// InstanceGroupLinker handles linking backends to InstanceGroup's.
type InstanceGroupLinker struct {
Copy link
Member

Choose a reason for hiding this comment

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

does this have to be exported?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it for this and others as well.

}
}

// Implements Link.
Copy link
Member

Choose a reason for hiding this comment

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

fix all comments

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

}
}

// We first try to create the backend with balancingMode=RATE. If this + return addIGs
Copy link
Member

Choose a reason for hiding this comment

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

weird formatting on this line

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

@@ -0,0 +1,210 @@
/*
Copyright 2015 The Kubernetes Authors.
Copy link
Member

Choose a reason for hiding this comment

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

date

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

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 13, 2018
Copy link
Member

@bowei bowei left a comment

Choose a reason for hiding this comment

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

/lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants