Skip to content
This repository has been archived by the owner on Jul 30, 2021. It is now read-only.

Port init lock improvements #166

Merged
merged 1 commit into from Aug 27, 2019
Merged

Port init lock improvements #166

merged 1 commit into from Aug 27, 2019

Conversation

chuckha
Copy link
Contributor

@chuckha chuckha commented Aug 22, 2019

Signed-off-by: Chuck Ha chuckh@vmware.com

What this PR does / why we need it:
This PR ports improvements over from CAPA.

It also updates the tests to use the fake client while still allowing for overriding errors.
Standardize imports.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #138

Special notes for your reviewer:
None

Release note:

Kubeadm Init lock will now contain the machine that created the lock

/assign @detiber

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 22, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha

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 approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 22, 2019
detiber
detiber reviewed Aug 22, 2019
View reviewed changes
controllers/control_plane_init_locker.go Outdated Show resolved Hide resolved
controllers/control_plane_init_locker.go Outdated Show resolved Hide resolved
controllers/kubeadmconfig_controller.go Show resolved Hide resolved
@chuckha chuckha force-pushed the lock-improvements branch 4 times, most recently from 3425cf3 to c453805 Compare August 22, 2019 16:54
@detiber
Copy link
Contributor

detiber commented Aug 22, 2019

lgtm, but would like to have @ncdc give it a look over as well

ncdc
ncdc reviewed Aug 23, 2019
View reviewed changes
controllers/control_plane_init_locker.go Outdated Show resolved Hide resolved
controllers/control_plane_init_locker.go Outdated Show resolved Hide resolved
controllers/control_plane_init_locker.go Outdated Show resolved Hide resolved
controllers/control_plane_init_locker.go Outdated Show resolved Hide resolved
controllers/control_plane_init_locker.go Outdated Show resolved Hide resolved
controllers/control_plane_init_locker.go Outdated Show resolved Hide resolved
controllers/control_plane_init_locker.go Outdated Show resolved Hide resolved
controllers/control_plane_init_locker.go Outdated Show resolved Hide resolved
vincepri
vincepri reviewed Aug 23, 2019
View reviewed changes
controllers/control_plane_init_locker.go Outdated Show resolved Hide resolved
controllers/control_plane_init_locker.go Outdated Show resolved Hide resolved
controllers/control_plane_init_locker.go Outdated Show resolved Hide resolved
controllers/control_plane_init_locker.go Outdated Show resolved Hide resolved
@chuckha
Copy link
Contributor Author

chuckha commented Aug 26, 2019

#166 (comment)

I don't know why I can't respond to this comment but I'd be curious to hear your thoughts about why we'd want to keep the struct private.

My argument for doing it this way is that this allows consumers to use the struct as they will and define their own interface instead of forcing our interface on them. We aren't writing their programs and don't know how they will consume the InitLocker.

@detiber
Copy link
Contributor

detiber commented Aug 26, 2019

I don't know why I can't respond to this comment but I'd be curious to hear your thoughts about why we'd want to keep the struct private.

I can't speak for Andy, but my take here is that this implementation is meant for use by CABPK only and not a general purpose locking mechanism.

@chuckha
Copy link
Contributor Author

chuckha commented Aug 26, 2019

@detiber in that case I still think it should still be exported but in an internal/ directory so it cannot be used outside this repo.

@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 Aug 26, 2019
@chuckha chuckha force-pushed the lock-improvements branch 3 times, most recently from 1c790dd to 9869d91 Compare August 26, 2019 17:44
ncdc
ncdc reviewed Aug 26, 2019
View reviewed changes
internal/locking/control_plane_init_mutex.go Outdated Show resolved Hide resolved
internal/locking/control_plane_init_mutex.go Outdated Show resolved Hide resolved
internal/locking/control_plane_init_mutex_test.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 26, 2019
@k8s-ci-robot k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 26, 2019
@chuckha chuckha force-pushed the lock-improvements branch 3 times, most recently from 7417307 to 99e86cc Compare August 26, 2019 18:58
@chuckha chuckha force-pushed the lock-improvements branch 2 times, most recently from 5d911e4 to 679565f Compare August 26, 2019 19:19
@chuckha
Port init lock improvements
9b7e096 
Signed-off-by: Chuck Ha <chuckh@vmware.com>
@chuckha
Copy link
Contributor Author

chuckha commented Aug 27, 2019

This has been tested with 3 control plane machines and works like a charm

vincepri
vincepri approved these changes Aug 27, 2019
View reviewed changes
Copy link
Contributor

@vincepri vincepri left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 27, 2019
@k8s-ci-robot k8s-ci-robot merged commit bc66887 into kubernetes-retired:master Aug 27, 2019
@chuckha chuckha deleted the lock-improvements branch August 27, 2019 19:13
@joonas joonas mentioned this pull request Aug 30, 2019
Closed
6 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ncdc ncdc ncdc left review comments

@joonas joonas joonas left review comments

@detiber detiber detiber left review comments

@vincepri vincepri vincepri approved these changes

@fabriziopandini fabriziopandini Awaiting requested review from fabriziopandini

Assignees

@detiber detiber

@vincepri vincepri

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 Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve control plane init lock handling
6 participants
@chuckha @k8s-ci-robot @detiber @ncdc @joonas @vincepri