-
Notifications
You must be signed in to change notification settings - Fork 26
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
interface: drift and configuration difference related status API changes #922
interface: drift and configuration difference related status API changes #922
Conversation
A few notes on this PR: This PR will report resources that have drifts and those that have configuration differences in separate status fields; it will also report the drift/diff details in the structured form of JSON patches to allow better readability. There has been discussions regarding: a) report diffs as a CRP condition, with the diff details being explained as a condition message; i.e., drifts will have per-resource details, diffs will only have a cluster-scoped summary. This PR: status:
placementStatuses:
- clusterName: bravelion
driftedPlacements:
- resourceIdentifier:
kind: Deployment
name: app
observedDrifts: ...
diffedPlacements:
- resourceIdentifier:
kind: ConfigMap
name: app
observedDiffs: ...
...
... The condition form: status:
placementStatuses:
- clusterName: bravelion
driftedPlacements:
- resourceIdentifier:
kind: Deployment
name: app
conditions:
- type: ResourceDiffed
message: ConfigMap resources `work/app` and 3 more are observed to have configuration differences
...
... b) instead of using structured drift/diff details in the JSON patch format, we produces a summary string of JSON patches as the drift diff details Structured form (this PR): observedDrifts:
- op: replace
field: /spec/replicas
oldValue: 1
value: 2
- op: add
field: /spec/revisionHistoryLimit
value: 5
... Summary form:
|
The concern is mostly that we would like to strike a balance between simplicity (CRP status is already an overburdened struct) and usefulness/being informative (users can find out exactly where goes wrong). It is true that K8s objects are not the best place to put drift/diff details; but we do not have an CLI/UI available yet. Of course none of these are function blockers. |
// | ||
// To control the object size, only the first 100 diffed resources will be included. | ||
// This field is only meaningful if the `ClusterName` is not empty. | ||
// +kubebuilder:validation:Optional | ||
// +kubebuilder:validation:MaxItems=100 | ||
DiffedPlacements []DiffedResourcePlacement `json:"diffedPlacements,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this object exist if the apply type is not "reportDiff"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Ryan! If the applyStrategy
is not reportDiff
, and there are no resources pending takeover actions, this field will be omitted.
// Note that this field is kept for informational purposes only; it is not a required part of the | ||
// JSON patch; see RFC 6902 for more information. | ||
// +kubebuilder:validation:Optional | ||
CurrentValue string `json:"value,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am wondering whether you still need the "op" if you show both values. BTW, CurrentValue vs Value is confusing. Can we use hub value and member value?
Path: "/name" ValueInHub: "foo" ValueInMember: "bar"
Path: "/age" ValueInHub: "10" ValueInMember: nil
Path: "/occupation" ValueInHub: nil ValueInMember: "engineer"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Liqian! I have made the requested changes. 🙏
…-detection-takeover-status
Due to timeline complications, we will merge this PR first to unblock progress. Will open future PR to address any further concerns; many apologies for the inconvenience. |
Description of your changes
This PR includes drafted changes to the status API regarding drift and configuration difference reporting.
I have:
make reviewable
to ensure this PR is ready for review.How has this code been tested
N/A
Special notes for your reviewer
Please see the design doc for more information regarding this PR.