OTA-1339: UpdateStatus: Initial working draft
#2012
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Hello @petr-muller! Some important instructions when contributing to openshift/api: |
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
size/XXL
petr-muller
force-pushed
the
update-status-api
branch
2 times, most recently
from
7c1d5de to
5d0db69
Compare
petr-muller
force-pushed
the
update-status-api
branch
2 times, most recently
from
ddca9e1 to
41b0a0c
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
petr-muller
changed the title
UpdateStatus: Initial working draftUpdateStatus: Initial working draft
openshift-ci-robot
added
the
jira/valid-reference
|
@petr-muller: This pull request references OTA-1339 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.18.0" version, but no target version was set. In response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
| type ControlPlaneUpdateVersions struct { | ||
| // previous is the version of the control plane before the update | ||
| // +optional | ||
| Previous Version `json:"previous,omitempty"` |
There was a problem hiding this comment.
nit: omitempty doesn't do anything for structs (golang/go#11939, golang/go#51261). You should either drop omitempty here, or use *Version.
There was a problem hiding this comment.
good point, thanks! I audited the rest of the file for this, and either made the field required/not-omitempty, or made it a pointer.
I think there was a guideline about not using pointers in one of the conventions, but kube allows it and can't find anything for openshift.
There was a problem hiding this comment.
I think there was a guideline about not using pointers in one of the conventions...
The OpenShift guideline for that is tucked away in this policy doc. There's a subsequent section allowing pointers to structs when an API validation that requires the field to be unset, which doesn't seem to apply here. I'm just not sold on "inline zero values to help discoverability" vs. "use omitempty to hide things the admin doesn't care about; future admins can learn about new knobs via oc explain ...". Feel free to drop the pointers and the omitempty if the API approvers press you, and feel free to mark this thread resolved either way.
There was a problem hiding this comment.
Yeah, I eventually found this guideline (the API conventions are linked from the top level conventions). The OpenShift guideline about avoiding pointers is about configuration APIs ("In configuration APIs specifically,...") which Status API is not. The point about pointers being harder to work with still stands.
I ended up using the pointer at a single place in the whole API, the PoolResource in ControlPlane. I reworked the ControlPlaneUpdateVersions (https://github.com/openshift/api/pull/2012/files#diff-382a9e9c3b33f1db2b3e759e6d6019ab4db0f3ea29c8448a281bcee9bc0f4c81R71-R81) part (also simplified the metadata, the type discriminated union felt like overkill etc) and now it is no longer a pointer.
|
|
||
| const ( | ||
| // installation denotes a boolean that indicates the update was initiated as an installation | ||
| InstallationMetadata VersionMetadataKey = "installation" |
There was a problem hiding this comment.
ControlPlaneUpdateVersions has separate previous and target. Do we need an explicit "I'm the install!" marker in version metadata? Or can that be inferred from "previous is empty"? I can't think of a situation where you'd be in the middle of a real A->B update but still care about whether A was an install version or not.
There was a problem hiding this comment.
this basically mimics what we do in oc prototype:
if this was the only use case for version metadata then maybe the implicit 'previous empty implies installation' would be good enough but we have a good use case for metadata (partial) so I think it's worth being explicit, we have the mechanism for it
There was a problem hiding this comment.
I have no problem with openshift/oc#4978e7d226e8cb2 deciding to use an explicit property internally to read more clearly. And I'm fine with you following that pattern here in the initial v1alpha1 approach. But the public-API form increases my personal threshold for whether the structure should allow inconsistent data, and "claims to be an installation but has a different previous" and "claims to be an update, but has no previous" are two possible inconsistent states for this structure. But either way, feel free to mark this thread resolved, to get v1alpha1 landed :)
There was a problem hiding this comment.
I like that point. Not having more metadata is also simpler which is always a better start. I'll change this.
petr-muller
force-pushed
the
update-status-api
branch
2 times, most recently
from
e438453 to
40182b1
Compare
Adding a resource reference to this level improves the cohesion with workerPools part of the API, and can be useful in standalone/hcp scenarios where controlplane is represented by different CRDs.
We are expecting use cases for more flexible metadata about the versions involved in the update, such as 'architecture' for pseudo-updates when clusters are migrated from `x86_64` to `multi`. The original two boolean flags were replaced with a generic list of name+type+value triplets.
Generated with: ``` ./tests/hack/gen-minimal-test.sh update/v1alpha1 v1alpha1 ```
- Use `+kubebuilder:validation:Required` consistently (not `+required`) - Do not mention (assume) underlying controller name - Integers use `int32` types with enforced bounds - Constants are `PascalNames` by convention - GoDoc start by JSON name by convention - Other GoDoc description tweaks
Make the `ResourceRef` type follow [OpenShift API conventions](https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#use-resource-name-rather-than-kind-in-object-references). I also considered to follow the ["Use Specific Types for Object Reference"](https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md#use-specific-types-for-object-references-and-omit-ref-suffix) more strictly and introduce speficic types instead using `ResourceRef` everywhere but consistency felt much worse (control plane would need ClusterVersion/HostedCluster reference, status insights would need just names, pool references would need MCP/NodePool but not in control plane...). Consistency is also a value, and because we use resource references at different places, it felt better to stay consistent everywhere.
CI enforces the presence of listType annotations on slices. List of resources is not a set, because resources are not scalars. It is not simple map because resource does not have a suitable key field. So it must be atomic I guess?
OpenShift API convention forbids bool usage and enforces this with CI. While I could argue for an exception (reasoning behind the convention does not apply in this case, as it would be easy to migrate from bools to strings if necessary), we can also simplify the API and make the metadata a simple key->value map, similar to `labels`. Value is optional and with empty value the presence of the key has a boolean=true semantics. It simplifies the API and possibly even improves the usability for clients.
petr-muller
force-pushed
the
update-status-api
branch
from
59f0b9e to
c864947
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: petr-muller The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
petr-muller
force-pushed
the
update-status-api
branch
from
c864947 to
332b1f4
Compare
|
Looking at the results of the |
Initially `UpdateStatus` was made namespaces to accomodate HCP more easily, but recently it seems the org has established a practice where HCP resources have specialized variants. We have also identified some differences in how the API will need to behave in HCP, so it makes sense to make the API cluster-scoped in standalone, and in the future we will have a HCP-specific namespaced variant.
petr-muller
force-pushed
the
update-status-api
branch
from
332b1f4 to
0ec7f29
Compare
|
@petr-muller: The following test failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
|
Ahaa! This is something I just merged which is showing that there is a diff between |
|
@JoelSpeed I'm still changing the content a bit, I planned to address the linter issues once I'm done |
Initial draft of the API, not expecting API reviewers to review yet, just OTAStill kinda WIP but it starts being partially reviewable. I still need to incorporate some feedback from Update Health API Draft and also revise this paying more attention to OpenShift API Conventions.
I'm keeping iterations as commits but will squash before (eventual) merge.