[OCPCLOUD-1349] Add Machine Deletion Hook consts to machine API #1050
Conversation
JoelSpeed
force-pushed
the
mdh-consts
branch
from
de49856
to
9304543
Compare
| // This could be the name of a controller (e.g. clusteroperator/etcd) | ||
| // or an administrator managing the hook. | ||
| // +required | ||
| Owner string `json:"owner"` |
There was a problem hiding this comment.
can this value be empty? if not can we add validation for it?
There was a problem hiding this comment.
Value cannot be empty, it is expected to always be populated in each entry, though with what we don't specify. I think this can be free form text. Not sure what validation you had in mind?
There was a problem hiding this comment.
maybe some minimal length if it makes sense here
There was a problem hiding this comment.
I think a minimum length is hard to quantify, we could put 5 characters, but then what if the owner is etcd, or something equivalently short. I think we should trust operators to put something useful within this field
There was a problem hiding this comment.
how about min==3 max==512? otherwise it becomes a paragraph.
There was a problem hiding this comment.
Do we typically have a maximum length for Condition Types? Since the name becomes part of the condition type, perhaps it's sensible to even limit at 256
There was a problem hiding this comment.
Do we typically have a maximum length for Condition Types? Since the name becomes part of the condition type, perhaps it's sensible to even limit at 256
We do, see comment below.
machine/v1beta1/types_machine.go
Outdated
| // LifecycleHook represents a single instance of a lifecycle hook | ||
| type LifecycleHook struct { | ||
| // Name defines a unique name for the lifcycle hook. | ||
| // The name should be unique and descriptive, ideally 1-3 words, in camelCase with no spaces. |
There was a problem hiding this comment.
is "camelCase" specified by the upstream? If not, we normally use CamelCase.
There was a problem hiding this comment.
That would be a typo, I had intended (from the examples I've created) that this be CamelCase, good catch, will update
machine/v1beta1/types_machine.go
Outdated
| // Name defines a unique name for the lifcycle hook. | ||
| // The name should be unique and descriptive, ideally 1-3 words, in camelCase with no spaces. | ||
| // Names must be unique and should only be managed by a single entity. | ||
| // +kubebuilder:validation:Pattern:="[A-Za-z]+" |
There was a problem hiding this comment.
no numbers, no slashes, no dots is intentional? I'm ok with it if that is your preference. Finalizers were envisioned as structured as, "foo.domain.name/item".
There was a problem hiding this comment.
Since the name becomes part of the Condtion Type, I had originally limited it so that it would just be standard alpha CamelCase as this is typically what I've seen in Condition Types. I'm not against extending it to be more but then it might conflict with the Name forming part of the Condition Type and having those fit to conventions
There was a problem hiding this comment.
From Condition in meta/v1
// type of condition in CamelCase or in foo.example.com/CamelCase.
// ---
// Many .condition.type values are consistent across resources like Available, but because arbitrary conditions can be
// useful (see .node.status.conditions), the ability to deconflict is important.
// The regex it matches is (dns1123SubdomainFmt/)?(qualifiedNameFmt)
// +required
// +kubebuilder:validation:Required
// +kubebuilder:validation:Pattern=`^([a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*/)?(([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9])$`
// +kubebuilder:validation:MaxLength=316
Type string `json:"type" protobuf:"bytes,1,opt,name=type"`
if you want to line it up to match.
JoelSpeed
force-pushed
the
mdh-consts
branch
from
9304543
to
de5bac2
Compare
JoelSpeed
force-pushed
the
mdh-consts
branch
from
de5bac2
to
e7f5dde
Compare
|
/approve |
openshift-ci
bot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alexander-demichev, deads2k, JoelSpeed 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 |
This is to implement openshift/enhancements#862 as it currently stands.
This adds the concept of lifecycle hooks to the machine to allow consumers to pause the machine removal process at either the draining or termination phases.
The condition constants will be used to add reporting on the Machines themselves as to the state of the hooks, see the enhancement for further details on this.