Skip to content

Smart Comparing Specs To Check If Dependent Resource Needs to Applied Again #920

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

Closed
Tracked by #923
csviri opened this issue Feb 9, 2022 · 2 comments · Fixed by #945
Closed
Tracked by #923

Smart Comparing Specs To Check If Dependent Resource Needs to Applied Again #920

csviri opened this issue Feb 9, 2022 · 2 comments · Fixed by #945
Assignees
Labels
dependent-resources-epic kind/feature Categorizes issue or PR as related to a new feature.

Comments

@csviri
Copy link
Collaborator

csviri commented Feb 9, 2022

Problem Statement

For dependent resources we want to have a default behavior to compare k8s resource specs in order to check if the desired resource needs to be reapplied or not. For example if somebody provides a desired definition of a target resource, like a Deployment object , after the apply K8S fills some attributes with default values. So the actual resource on the server will be always different specified in desired. So we cannot simply compare specs.

A sample jsonpatch after deployment is applied (desired vs applied):

{"op":"add","path":"/template/spec/containers/0/ports/0/protocol","value":"TCP"}
{"op":"add","path":"/template/spec/containers/0/imagePullPolicy","value":"IfNotPresent"}
{"op":"add","path":"/template/spec/containers/0/resources","value":{}}
{"op":"add","path":"/template/spec/containers/0/terminationMessagePath","value":"/dev/termination-log"}
{"op":"add","path":"/template/spec/containers/0/terminationMessagePolicy","value":"File"}
{"op":"add","path":"/template/spec/volumes/0/configMap/defaultMode","value":420}
{"op":"add","path":"/template/spec/dnsPolicy","value":"ClusterFirst"}
{"op":"add","path":"/template/spec/restartPolicy","value":"Always"}
{"op":"add","path":"/template/spec/schedulerName","value":"default-scheduler"}
{"op":"add","path":"/template/spec/securityContext","value":{}}
{"op":"add","path":"/template/spec/terminationGracePeriodSeconds","value":30}
{"op":"add","path":"/progressDeadlineSeconds","value":600}
{"op":"add","path":"/revisionHistoryLimit","value":10}
{"op":"add","path":"/strategy","value":{"rollingUpdate":{"maxSurge":"25%","maxUnavailable":"25%"},"type":"RollingUpdate"}}

In addition to that, for resources like Service the ClusterIP is changes for different instances of Service. (If this diff would be the same, we could just cache the diff and apply before comparison).

@csviri csviri added dependent-resources-epic kind/feature Categorizes issue or PR as related to a new feature. labels Feb 9, 2022
@csviri
Copy link
Collaborator Author

csviri commented Feb 13, 2022

Solution

It's quite normal that resources are altered during create and/or update, adding default values but also think of mutation hooks (adding side cart containers or similar is quite common for example). One solution is to just compare if that desired state and actual state, if there are only additions accept that. So, this makes sure no value from desired spec is different from the actual spec (otherwise would be applied again), if a mutation hook changes a value of course this would fail, but probably that would be an exception where user can write it's own matcher. So:

  public boolean match(R actualResource, R desiredResource, Context context) {
    var desiredSpecNode = mapper.valueToTree(ReconcilerUtils.getSpec(desiredResource));
    var actualSpecNode = mapper.valueToTree(ReconcilerUtils.getSpec(actualResource));
    var diffJsonPatch = JsonDiff.asJson(desiredSpecNode, actualSpecNode);
    for (int i = 0; i < diffJsonPatch.size(); i++) {
      String operation = diffJsonPatch.get(i).get("op").asText();
      if (!operation.equals("add")) {
        return false;
      }
    }
    return true;
  }

This impl uses jackson, to create a jsonpatch between actual and desired state, if it contains only adds it means the desired values are not changed.

Of course for resources which don't have specs (like Secret, ConfigMap) will have different approach, like comparing data.

@csviri csviri changed the title Smart Comparing Specs of Resources for Automatic Handling Smart Comparing Specs To Check If Dependent Resource Needs to Applied Again Feb 13, 2022
@csviri csviri self-assigned this Feb 16, 2022
@csviri csviri linked a pull request Feb 16, 2022 that will close this issue
@csviri csviri closed this as completed Feb 16, 2022
@csviri
Copy link
Collaborator Author

csviri commented Feb 16, 2022

Just small additional to note: after created/update and possibly default values and mutation hooks are applied. Detecting further changes on spec can be done by recording the generation after create/update in memory. This might or might be not desirable though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependent-resources-epic kind/feature Categorizes issue or PR as related to a new feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant