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

Record cluster scoped handlers #277

Merged
merged 1 commit into from
Apr 23, 2019
Merged

Conversation

daxmc99
Copy link
Contributor

@daxmc99 daxmc99 commented Apr 19, 2019

Problem: Cluster scoped handlers were not being cleaned up without
manually writing garbage collection functions.

Solution: All cluster scoped handlers will be recorded in a global map
in a generic format so that they can be removed by a generic function.


// needs to be accessible by both project controllers and mgmt controllers
var (
CleanerFuncMap = struct {
Copy link

@cjellick cjellick Apr 23, 2019

Choose a reason for hiding this comment

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

change the name of this to be ClusterScopedFinalizerGroupVersionResources

Also, generally, putting the type (ie "Map") in the name of a variable is unnecessary

Choose a reason for hiding this comment

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

add a comment explaining what this variable is used for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is technically more than a list of cluster scoped finalizers. This is a set of all rancher generated types.

CleanerFuncMap = struct {
sync.RWMutex
M map[string]schema.GroupVersionResource
}{M: make(map[string]schema.GroupVersionResource)}

Choose a reason for hiding this comment

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

change the map to have a key of GroupVersionResource and bool value

sync.RWMutex
M map[string]schema.GroupVersionResource
}{M: make(map[string]schema.GroupVersionResource)}
)

Choose a reason for hiding this comment

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

We might as well go the extra step and properly protect this map from concurrency issues.

  • Make the M lowercase so that it is not publicly exposed.
  • add a put method that is wrapped in Lock()
  • add a Get function that returns a copy of the map

Choose a reason for hiding this comment

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

rather than a copy of the map, return a list of GVRs

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 think it would be more useful in the future to have the map to check for existence ( O(1) vs O(n) for a list)

Problem: Cluster scoped handlers were not being cleaned up without
manually writing garbage collection functions.

Solution: All cluster scoped handlers will be recorded in a global map
in a generic format so that they can be removed by a generic function.
@daxmc99 daxmc99 force-pushed the cluster_scoped_gc branch from ae75024 to bbf450e Compare April 23, 2019 19:15
@cjellick
Copy link

LGTM

@cjellick cjellick merged commit 7387aa5 into rancher:master Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants