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

interface: drift and configuration difference related status API changes #922

Merged

Conversation

michaelawyu
Copy link
Contributor

Description of your changes

This PR includes drafted changes to the status API regarding drift and configuration difference reporting.

I have:

  • Run 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.

@michaelawyu
Copy link
Contributor Author

michaelawyu commented Oct 9, 2024

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:

drifts have been found in the resource: if applied, in the live state "/spec/replicas" (value: "2", oldValue: "1") and 4 more path(s) will be replaced; "/spec/revisionHistoryLImit" (value: "5") and 1 more path(s) will be added.

@michaelawyu
Copy link
Contributor Author

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.

@michaelawyu michaelawyu changed the title interface: (drafted) drift and configuration difference related status API changes interface: drift and configuration difference related status API changes Oct 10, 2024
Comment on lines +885 to +890
//
// 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"`
Copy link
Contributor

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"

Copy link
Contributor Author

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"`
Copy link
Contributor

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"

Copy link
Contributor Author

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. 🙏

@michaelawyu
Copy link
Contributor Author

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.

@zhiying-lin zhiying-lin merged commit 6b82c64 into Azure:main Oct 23, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants