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

🌱 Add conditions to the KubeadmConfig object #3125

Merged
merged 2 commits into from
Jun 8, 2020
Merged

🌱 Add conditions to the KubeadmConfig object #3125

merged 2 commits into from
Jun 8, 2020

Conversation

fabriziopandini
Copy link
Member

What this PR does / why we need it:
This PR adds conditions to the KubeadmConfig object

Which issue(s) this PR fixes:
Fixes #3110

@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 Jun 1, 2020
@k8s-ci-robot k8s-ci-robot requested review from benmoss and vincepri June 1, 2020 20:10
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 1, 2020
@vincepri
Copy link
Member

vincepri commented Jun 2, 2020

/retitle 🌱 Add conditions to the KubeadmConfig object

@k8s-ci-robot k8s-ci-robot changed the title 🏃 Add conditions to the KubeadmConfig object 🌱 Add conditions to the KubeadmConfig object Jun 2, 2020
@fabriziopandini fabriziopandini mentioned this pull request Jun 3, 2020
Closed
@fabriziopandini
Copy link
Member Author

@vincepri @detiber I have drifted away from handling error conditions with defer, and now I'm setting error condition when the error is due to user misconfigurations
I also fixed capitalization for all log errors

vincepri
vincepri approved these changes Jun 4, 2020
View reviewed changes
Copy link
Member

@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.

/approve
/milestone v0.3.7
/assign @detiber

@k8s-ci-robot k8s-ci-robot added this to the v0.3.7 milestone Jun 4, 2020
detiber
detiber reviewed Jun 4, 2020
View reviewed changes
Copy link
Member

@detiber detiber left a comment

Choose a reason for hiding this comment

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

After reviewing the related KCP PR, I'm wondering if it would make sense to track whether the certificates are available separate from CertificatesGeneratedCondtion, since that would give an indication of progress, even after we remove certificate generation from this controller in the future.

bootstrap/kubeadm/controllers/kubeadmconfig_controller.go Show resolved Hide resolved
@fabriziopandini
Copy link
Member Author

fabriziopandini commented Jun 4, 2020 •
edited
Loading

I'm wondering if it would make sense to track whether the certificates are available separate from CertificatesGeneratedCondition, since that would give an indication of progress

My hot take is that I will be a little bit awkward having many KubeadmConfig objects each one reporting certificates are available, but it something that we can reconsider as soon as we get to a point where we can test conditions working on the core set of objects and evaluate possible improvements across the board.

vincepri
vincepri approved these changes Jun 4, 2020
View reviewed changes
Copy link
Member

@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
/hold
for @detiber's final review

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 4, 2020
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 4, 2020
sedefsavas
sedefsavas reviewed Jun 5, 2020
View reviewed changes
bootstrap/kubeadm/api/v1alpha3/condition_consts.go Outdated
//
// NOTE: Having the first control plane machine ready is a pre-condition for the additional control planes and
// for workers to join; the KubeadmConfig controller ensure this pre-condition is satisfied.
WaitingForControlPlaneInitializedReason = "WaitingForControlPlaneInitialized"
Copy link

Choose a reason for hiding this comment

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

Is this same as control plane being ready?

Copy link
Member Author

@fabriziopandini fabriziopandini Jun 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

In this case, we are waiting for the first control plane instance to be up and running.
For historical reason, we have two flags tracking this, ready and initialized (with slight differences), but given that for Ready for conditions has a well defined meaning, I'm using WaitingForControlPlaneInitialized here

bootstrap/kubeadm/api/v1alpha3/condition_consts.go Outdated Show resolved Hide resolved
bootstrap/kubeadm/api/v1alpha3/condition_consts.go Show resolved Hide resolved
util/secret/certificates.go Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 5, 2020
@vincepri
Copy link
Member

vincepri commented Jun 5, 2020

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2020
@fabriziopandini
Copy link
Member Author

@vincepri let me know when I can squash commits

@fabriziopandini
Copy link
Member Author

@vincepri switched to CertificateAvailable
With the new semantic I extended the conditions also to joining controlPlanes/workers and added code to report generation failures or certificate corrumption

@fabriziopandini
Copy link
Member Author

@vincepri PTAL

@fabriziopandini fabriziopandini force-pushed the add-conditions-to-kubeadmconfig branch from 1aff5d8 to 8cb28c4 Compare June 8, 2020 09:45
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, vincepri

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:
  • OWNERS [fabriziopandini,vincepri]

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

@fabriziopandini
Copy link
Member Author

@vincepri PTAL
rebased, squashed and added a utility for fixing tests using fake client

vincepri
vincepri reviewed Jun 8, 2020
View reviewed changes
util/patch/fake/client.go Outdated Show resolved Hide resolved
@fabriziopandini fabriziopandini mentioned this pull request Jun 8, 2020
Merged
@fabriziopandini fabriziopandini force-pushed the add-conditions-to-kubeadmconfig branch from 8cb28c4 to abd654b Compare June 8, 2020 15:20
@fabriziopandini fabriziopandini force-pushed the add-conditions-to-kubeadmconfig branch from abd654b to 28b1087 Compare June 8, 2020 15:53
vincepri
vincepri approved these changes Jun 8, 2020
View reviewed changes
Copy link
Member

@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 "Looks good to me", indicates that a PR is ready to be merged. label Jun 8, 2020
@k8s-ci-robot k8s-ci-robot merged commit 691f268 into kubernetes-sigs:master Jun 8, 2020
@fabriziopandini fabriziopandini deleted the add-conditions-to-kubeadmconfig branch June 9, 2020 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sedefsavas sedefsavas sedefsavas left review comments

@detiber detiber detiber left review comments

@vincepri vincepri vincepri approved these changes

@benmoss benmoss Awaiting requested review from benmoss

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 "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v0.3.7
Development

Successfully merging this pull request may close these issues.

Add conditions to the KubeadmConfig object
5 participants
@fabriziopandini @vincepri @k8s-ci-robot @detiber @sedefsavas