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

Allow using specific object ID on diff #11400

Merged
merged 3 commits into from
Nov 1, 2021
Merged

Conversation

lgfa29
Copy link
Contributor

@lgfa29 lgfa29 commented Oct 27, 2021

Currently Nomad will use a generic diff method when computing changes in a jobspec. This method uses a hash of the block content as a way to match values from the old jobspec to the new one. This approach generates less than ideal diffs since any change to block will result in a add/delete pair instead of a more precise edit output.

This PR introduces a new interface that can be implemented by structs that have some kind of stable and uniquely identifiable field that can be used to match blocks from the old jobspec to the new one.

Implementing this interface for the Template struct, for example, allows using the destination path as the ID. Nomad guarantees its uniqueness in a task and If the user modifies it it's fair to assume this is a new block.

Big thanks to @apollo13 for flagging this and proposing a solution. Builds upon #11378.

Sample outpus

Before:

$ nomad job plan example.nomad
+/- Job: "example"
+/- Task Group: "cache" (1 create/destroy update)
  +/- Task: "redis" (forces create/destroy update)
    + Template {
      + ChangeMode:   "restart"
        ChangeSignal: ""
      + DestPath:     "local/test"
      + EmbeddedTmpl: "new test"
      + Envvars:      "false"
      + LeftDelim:    "{{"
      + Perms:        "0644"
      + RightDelim:   "}}"
        SourcePath:   ""
      + Splay:        "5000000000"
      + VaultGrace:   "0"
      }
    - Template {
      - ChangeMode:   "restart"
        ChangeSignal: ""
      - DestPath:     "local/test"
      - EmbeddedTmpl: "test"
      - Envvars:      "false"
      - LeftDelim:    "{{"
      - Perms:        "0644"
      - RightDelim:   "}}"
        SourcePath:   ""
      - Splay:        "5000000000"
      - VaultGrace:   "0"
      }

After:

$ nomad job plan example.nomad
+/- Job: "example"
+/- Task Group: "cache" (1 create/destroy update)
  +/- Task: "redis" (forces create/destroy update)
    +/- Template {
          ChangeMode:   "restart"
          ChangeSignal: ""
          DestPath:     "local/test"
      +/- EmbeddedTmpl: "test" => "new test"
          Envvars:      "false"
          LeftDelim:    "{{"
          Perms:        "0644"
          RightDelim:   "}}"
          SourcePath:   ""
          Splay:        "5000000000"
          VaultGrace:   "0"
        }

@apollo13
Copy link
Contributor

apollo13 commented Oct 28, 2021 via email

Copy link
Contributor

@DerekStrickland DerekStrickland left a comment

Choose a reason for hiding this comment

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

Looks great.

@lgfa29
Copy link
Contributor Author

lgfa29 commented Oct 28, 2021

The interface usage makes sense and is nicer than your intial try with the function 👍

Oh yeah, that was really just throw-away code to validate the idea 🙂

to bad that only those few can be changed, I was hoping for even more stanzas

The other two fields that use the primitiveObjectSetDiff function are constraint and affinity, which suffer from the same problem we had with check before, in which there's no way to uniquely identify them. We could try to port the heuristic approach we did there, but I think it would take some work to make that generic and re-usable.

Maybe with another interface that provides a function that does the matching? But I'm not sure how that will look like and the benefit we would gain, since these 2 attributes are pretty small.

@apollo13
Copy link
Contributor

apollo13 commented Oct 29, 2021 via email

@lgfa29
Copy link
Contributor Author

lgfa29 commented Nov 1, 2021

I guess it boils down to how useful people think job planning is. I personally find it very useful and would love it to be as good as possible.
100% agree and the more accurate they are the better. I didn't mean to reduce the importance of good diffs in my last comment, just saying that improving constraint and affinity diffs would require more work than is being done in this PR. I will create follow-up issues for them.

I'll happily work on those but I'll need a bit of guidance
Thank you! I will take a look at them.

So maybe let's takle issues like the ones mentioned before we attack a correct but suboptimal check/constraint output? My thinking here is that the more time we spend with diff.go the better our final solution for generalizing check/constraint diffs might turn out simply because we have a better view of the whole picture. (That shouldn't stop us from merging this PR which I think is good to go).
👍

@lgfa29 lgfa29 merged commit ffa4d07 into main Nov 1, 2021
@lgfa29 lgfa29 deleted the primitive-obj-diff-with-id branch November 1, 2021 19:16
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants