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

New Update Strategy only when workload changes in ManifestWork #133

Merged

Conversation

qiujian16
Copy link
Member

@qiujian16 qiujian16 commented Nov 15, 2024

fixes #132

@openshift-ci openshift-ci bot requested a review from deads2k November 15, 2024 07:21
Copy link

openshift-ci bot commented Nov 15, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qiujian16

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

type ServerSideApplyConfig struct {
...
// IgnoreFields defines a list of json paths in the resource that will not be updated on the spoke.
IgnoreFields []string `json:"ignoreFields,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Does the IgnoreFields take effect when OnSpokeChange is NoOverride?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, so when something changes in ignoreFields in the mw, the work-agent will not update resource all the time.

Copy link
Member

Choose a reason for hiding this comment

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

I still don't quite understand, will IgnoreFields ignore OnSpokeChange? No matter OnSpokeChange is Override or NoOverride, changes on the manifestworks will not updated to the spoke? why do we need the OnSpokeChange for ServerSideApply?

when apply the resource to the spoke cluster will add an annotation with the key `open-cluster-management.io/object-hash`.
The value of the annotation is the computed hash of the resource spec in the `ManifestWork`. Later when another actor
updates the resource, the work-agent will at first check if the object-hash mismatches with the current resource spec
in the `ManifestWork`. If it is the same, the resource will not be updated so the change from spoke is ignored. When
Copy link
Member

Choose a reason for hiding this comment

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

Should we explain the behavior of "Delete" when OnSpokeChange is set to NoOverride here? It seems when the resource is deleted, object-hash is removed and the resource will be applied again?

Copy link
Member Author

Choose a reason for hiding this comment

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

the resource itself will be deleted, so does the annotation. The annotation will be kept if the deleteOption is Orphan, and in this case, I do not think we need to remove the annotation.


In addition, in `ServerSideApplly` strategy, we would also introduce a new `ignoreDifferences` options similar as what
is defined in argoCD (https://argo-cd.readthedocs.io/en/stable/user-guide/diffing/). Such that user can choose to not
let work-agent patch certain resource fields.
Copy link
Member

Choose a reason for hiding this comment

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

This will also be able to resolve the zero creationTime in metadata problem, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it could, but it means we will not revert the change on the spoke by another actor.


```go
type UpdateConfig struct {
// OnSpokeChange defines whether the resource should be overriden by the manifestwork it is changed
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// OnSpokeChange defines whether the resource should be overriden by the manifestwork it is changed
// OnSpokeChange defines whether the resource should be overridden by the manifestwork once it is changed

type ServerSideApplyConfig struct {
...
// IgnoreFields defines a list of json paths in the resource that will not be updated on the spoke.
IgnoreFields []string `json:"ignoreFields,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

I still don't quite understand, will IgnoreFields ignore OnSpokeChange? No matter OnSpokeChange is Override or NoOverride, changes on the manifestworks will not updated to the spoke? why do we need the OnSpokeChange for ServerSideApply?

@qiujian16
Copy link
Member Author

@zhujian7

< I still don't quite understand, will IgnoreFields ignore OnSpokeChange? No matter OnSpokeChange is Override or NoOverride, changes on the manifestworks will not updated to the spoke? why do we need the OnSpokeChange for ServerSideApply?

with OnSpokeChange, the work-agent will still update manifests when the resource in the manifestwork is updated. IgnoreFields makes a finer granularity control on certain fields in the resource, so that even if these fields is changed in manifestwork resource or spoke, the work-agent will not update the resource.

Signed-off-by: Jian Qiu <[email protected]>
@elgnay
Copy link
Contributor

elgnay commented Nov 26, 2024

/lgtm
/hold Hold in case anyone else wants to take a look.

@qiujian16
Copy link
Member Author

/unhold

@openshift-merge-bot openshift-merge-bot bot merged commit cc9e453 into open-cluster-management-io:main Nov 26, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A new update strategy in manifestwork to only update resource when mw spec is changed.
6 participants