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

📖 Add MHCClass spec and details to proposal #5511

Conversation

killianmuldoon
Copy link
Contributor

What this PR does / why we need it:
This PR adds information to the ClusterClass proposal describing how MachineHealthChecks can be added to the API so that clusters stamped from a particular ClusterClass have MachineHealthChecks generated for them from a common spec.

This proposal also introduces full defaulting for a MachineHealthChecks generated from a ClusterClass meaning that the check itself needs only be added as an empty struct to the ClusterClass in order to get a standard MHC configuration generated.

C/F #5125

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 27, 2021
@killianmuldoon killianmuldoon force-pushed the proposal/mhc-for-clusterclass branch from a104275 to e4fdb47 Compare October 27, 2021 13:20
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 27, 2021
Copy link
Contributor

@alexander-demicev alexander-demicev left a comment

Choose a reason for hiding this comment

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

a good description of MHCClass, thanks

@enxebre
Copy link
Member

enxebre commented Oct 27, 2021

thanks @killianmuldoon. This seems reasonable to me, dropped some comments.

Copy link
Contributor

@ykakarap ykakarap left a comment

Choose a reason for hiding this comment

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

Can you update the example ClusterClass defined in l756-l799 to include machine health checks and may be add a comment above them saying the value will be defaulted (if you are going with the empty object example)?

Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

this generally makes sense to me, i do think we should be crafting the API in a way that makes adding future integrations in consistent manner. added a thought inline

// MachineHealthCheckClass defines a MachineHealthCheck for each
// ControlPlaneClass and MachineDeploymentClass in this ClusterClass.
// +optional
MachineHealthCheck *MachineHealthCheckClass `json:"machineHealthCheck,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of adding this as a top level entry, i wonder about having a field for "lifecycle integrations" or similar, to accommodate a future where we include more components?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a good idea, but I guess we'd want to be clear that we're definitely looking to add more integrations (and that they'd be limited to lifecycle integrations instead of something else).

I don't think we have an answer on this yet. I'll open an issue to cover whether and which integrations we should be thinking about for clusterClass.

Copy link
Member

@sbueringer sbueringer Nov 22, 2021

Choose a reason for hiding this comment

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

I think we should be specific here and keep it as is.

I think we can always add other features with additional individual fields and if we want to group them at some point we can deprecate + merge.

For now it would be very speculative as we don't know how/if other features are added.

  • ClusterResourceSet seems to be out-of-scope of ClusterClass at the moment (as it's orthogonal to ClusterClass and applies across multiple clusters)
  • Autoscaler:

Overall ClusterClass itself is all about the cluster lifecycle, so I'm not sure an additional field/level like integrations or lifecycleIntegrations would add a lot of benefit and depending on which and how we add additional features the field name might not fit.

So I would suggest, let's choose the best field name / structure we can right now, and potentially improve it over time, once we know how/if other features are added.

@elmiko WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

@sbueringer i think your reasoning makes good sense. i would be happy to see the integration defined better in an enhancement that comes out of #5491.

Copy link
Member

Choose a reason for hiding this comment

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

Could we move this definition under WorkersClass? And specify that this one will be applied to each one of the MachineDeployment classes unless they supply their own configuration/overrides?

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 the most straightforward implementation is to define this on the lowest levels. If we want overriding behaviour later on we can introduce the higher level workerClass MHC.

Copy link
Member

Choose a reason for hiding this comment

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

Having the MHC Class top level can be confusing to users, because it's one level above ControlPlane so folks might think that there is some layering in place.

Copy link
Member

Choose a reason for hiding this comment

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

+1 to have MHC only inside CP and MD for the first iteration (and thus to remove it from here)

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Dec 1, 2021

CLA Signed

The committers are authorized under a signed CLA.

@killianmuldoon killianmuldoon force-pushed the proposal/mhc-for-clusterclass branch 2 times, most recently from 8cc4fce to b1e509e Compare December 1, 2021 14:22
@killianmuldoon killianmuldoon force-pushed the proposal/mhc-for-clusterclass branch 3 times, most recently from 4d54a51 to 92e0441 Compare December 1, 2021 15:28
@sbueringer
Copy link
Member

lgtm pending squash from my side

@killianmuldoon killianmuldoon force-pushed the proposal/mhc-for-clusterclass branch from 92e0441 to 1949797 Compare December 3, 2021 16:17
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 3, 2021
@killianmuldoon killianmuldoon force-pushed the proposal/mhc-for-clusterclass branch from 1949797 to 3f4e9e4 Compare December 3, 2021 16:21
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 20, 2021
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

a couple of nits

@killianmuldoon killianmuldoon force-pushed the proposal/mhc-for-clusterclass branch from 6de3d3b to ffcda0b Compare December 22, 2021 11:14
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 22, 2021
@sbueringer
Copy link
Member

/test pull-cluster-api-verify-main

Signed-off-by: killianmuldoon <kmuldoon@vmware.com>
@killianmuldoon killianmuldoon force-pushed the proposal/mhc-for-clusterclass branch from 4bebddb to 1eb6d00 Compare January 10, 2022 18:34
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 10, 2022
@vincepri
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 10, 2022
@fabriziopandini
Copy link
Member

/lgtm
thanks @killianmuldoon for the patience and the perseverance in pushing this forward.
This provides the foundation for MHC in ClusterClass and it allows future iteration for improving UX, e.g reviving the idea of defaults

Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

+1 from me, thanks for all the hard work @killianmuldoon

@enxebre
Copy link
Member

enxebre commented Jan 10, 2022

Thanks!
/lgtm

@@ -291,6 +305,42 @@ type LocalObjectTemplate struct {
// offered by a provider.
Ref *corev1.ObjectReference `json:"ref"`
}

// MachineHealthCheckClass defines a MachineHealthCheck for a group of Machines.
type MachineHealthCheckClass struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

The fields below seem to be mostly duplicates of the fields in MachineHealthCheckSpec. Are we concerned this might cause some trouble long-term for maintenance? How do we ensure the two don't diverge? What if a new feature is added to one place but not the other? Have we considered sharing a common struct so they can share the fields that are in both places or making one a subset of the other?

Please let me know if this comment feels out of scope. I realize this is a pattern that exists in other places in ClusterClasses, which is why this may be a bigger discussion.

Copy link
Member

Choose a reason for hiding this comment

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

+1 to start working on some guidelines for common structs, given that this was discussed also in last week office hours

@CecileRobertMichon
Copy link
Contributor

I don't want to hold this proposal on the discussion of sharing fields vs. duplicating but I feel it's a conversation we should have. Overall the proposal is sound and nothing jumps at me.

/lgtm

@fabriziopandini
Copy link
Member

/approve

@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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 11, 2022
@k8s-ci-robot k8s-ci-robot merged commit c655a59 into kubernetes-sigs:main Jan 11, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.1 milestone Jan 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

None yet

10 participants