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 KubeadmControlPlane object #3139

Merged
merged 1 commit into from
Jun 18, 2020
Merged

🌱 Add conditions to the KubeadmControlPlane object #3139

merged 1 commit into from
Jun 18, 2020

Conversation

fabriziopandini
Copy link
Member

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

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

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 4, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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 Jun 4, 2020
@fabriziopandini
Copy link
Member Author

fabriziopandini commented Jun 4, 2020 •
edited
Loading

@vincepri @detiber this PR is complete and it can be reviewed, but it requires #3136 to merge in order to compile

@fabriziopandini fabriziopandini force-pushed the add-conditions-to-kcp branch from 99892c6 to e0a8e4e Compare June 4, 2020 13:34
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.

It might be good to also add a condition for ControlPlaneAvailable or APIServerAvailable that could be determined in a couple of different ways:

  • Is there a control plane endpoint defined, if so, does /healthz/ready look good
  • Do we have enough Machines that are stating they are Ready to provide quorum for etcd

I'm also wondering if it would make sense to track other subsets of health as conditions:

  • external etcd (future)
    • is it available?
    • possibly some type of naive health check (is it writeable, maybe)
  • stacked etcd:
    • is it available
    • does it have quorum
    • is it degraded in any way (missing members, individual members belong to the same cluster, etc)
  • api server health
  • controllers health
  • other resources we manage
    • coredns deployed at correct version
    • kubeproxy deployed at correct version
    • ???

controlplane/kubeadm/controllers/controller.go Outdated Show resolved Hide resolved
controlplane/kubeadm/controllers/controller.go Outdated Show resolved Hide resolved
controlplane/kubeadm/controllers/controller.go Outdated Show resolved Hide resolved
controlplane/kubeadm/controllers/controller.go Outdated Show resolved Hide resolved
controlplane/kubeadm/controllers/controller.go Outdated Show resolved Hide resolved
controlplane/kubeadm/internal/etcd/util/set.go Outdated Show resolved Hide resolved
controlplane/kubeadm/internal/machine_collection.go Show resolved Hide resolved
@benmoss benmoss mentioned this pull request Jun 4, 2020
Merged
@fabriziopandini fabriziopandini mentioned this pull request Jun 5, 2020
Closed
@gab-satchi gab-satchi mentioned this pull request Jun 5, 2020
Closed
@fabriziopandini fabriziopandini force-pushed the add-conditions-to-kcp branch 2 times, most recently from ecd7ac3 to 910f46d Compare June 11, 2020 10:21
@fabriziopandini
Copy link
Member Author

Removed WIP and rebased
As per discussion, only an initial set of conditions in left in this PR: MachinesReady, CertificatesAvailable, (control plane)Available
@vincepri @detiber PTAL

@fabriziopandini fabriziopandini changed the title [WIP] 🌱 Add conditions to the KubeadmControlPlane object 🌱 Add conditions to the KubeadmControlPlane object Jun 11, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 11, 2020
@vincepri
Copy link
Member

/lgtm

/hold
for @detiber

@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 11, 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 11, 2020
@vincepri
Copy link
Member

/milestone v0.3.7

@k8s-ci-robot k8s-ci-robot added this to the v0.3.7 milestone Jun 11, 2020
detiber
detiber reviewed Jun 11, 2020
View reviewed changes
controlplane/kubeadm/controllers/status.go
@@ -69,6 +70,7 @@ func (r *KubeadmControlPlaneReconciler) updateStatus(ctx context.Context, kcp *c
// This only gets initialized once and does not change if the kubeadm config map goes away.
if status.HasKubeadmConfig {
kcp.Status.Initialized = true
conditions.MarkTrue(kcp, controlplanev1.AvailableCondition)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we should tie AvailableCondition to whether there is a kubeadmconfig. Should this check the /healthz/ready endpoint for the apiserver instead?

Copy link
Member

Choose a reason for hiding this comment

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

Both? :)

Copy link
Member

Choose a reason for hiding this comment

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

Both definitely doesn't hurt :)

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 17, 2020
@fabriziopandini
Copy link
Member Author

@vince, @detiber I have added more conditions documenting scaling up/down and rolling update.
Thank you very much for all the support and all the valuable tips that helped this PR to shape out

@vince
Copy link

vince commented Jun 17, 2020

Wrong vince, @fabriziopandini . I think I get keep getting summoned here and I'd really prefer you stop it.

@detiber
Copy link
Member

detiber commented Jun 17, 2020

@fabriziopandini linter doesn't appear happy, otherwise lgtm, especially since I can't think of better naming for the Resized condition.

@detiber
Copy link
Member

detiber commented Jun 17, 2020

/test pull-cluster-api-test

@detiber
Copy link
Member

detiber commented Jun 17, 2020

/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 17, 2020
@fabriziopandini
Copy link
Member Author

fabriziopandini commented Jun 18, 2020 •
edited
Loading

shashed
@vincepri is it ok to unhold now?

@fabriziopandini fabriziopandini force-pushed the add-conditions-to-kcp branch from 3733aeb to 74f2b4f Compare June 18, 2020 09:47
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 18, 2020
@detiber
Copy link
Member

detiber commented Jun 18, 2020

/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 18, 2020
@vincepri
Copy link
Member

Unholding in a bit, wanted to get #3210 merged first, which should reduce the overall test running time

@vincepri
Copy link
Member

/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 18, 2020
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Jun 18, 2020 •
edited
Loading

@fabriziopandini: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
pull-cluster-api-integration ecd7ac3 link /test pull-cluster-api-integration
pull-cluster-api-apidiff 74f2b4f link /test pull-cluster-api-apidiff

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@vincepri
Copy link
Member

/test pull-cluster-api-test

@k8s-ci-robot k8s-ci-robot merged commit 0225dbf into kubernetes-sigs:master Jun 18, 2020
@fabriziopandini fabriziopandini deleted the add-conditions-to-kcp branch June 19, 2020 13:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vincepri vincepri vincepri left review comments

@detiber detiber detiber left review comments

@JoelSpeed JoelSpeed Awaiting requested review from JoelSpeed

@justinsb justinsb Awaiting requested review from justinsb

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 KubeadmControlPlane object
5 participants
@fabriziopandini @k8s-ci-robot @vincepri @vince @detiber