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

ENT-11988: Atomic copy_from in files promise #5611

Merged
merged 1 commit into from
Oct 10, 2024

Conversation

larsewi
Copy link
Contributor

@larsewi larsewi commented Oct 8, 2024

Changes to files promise in copy_from attribute:

  • The new file (i.e., <FILENAME>.cfnew) is now created with correct
    permission during remote copy. Previously it would be created with
    default permissions.
  • The destination file (i.e., <FILENAME>) is no longer deleted on
    backup during file copy. Previously it would be renamed to
    <FILENAME>.cfsaved, causing the original file to dissappear. Now an
    actual copy of the original file with the same permissions is created
    instead.

As a result, there will no longer be a brief moment where the original
file is inaccessible.

Back-ported to:

@larsewi
Copy link
Contributor Author

larsewi commented Oct 9, 2024

@cf-bottom Jenkins please :)

@cf-bottom
Copy link

cf-bottom commented Oct 9, 2024

Copy link
Contributor

@vpodzime vpodzime left a comment

Choose a reason for hiding this comment

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

Looks good to me except that we should use (the code from) CopyRegularFileDisk(), I believe.

Changes to `files` promise in `copy_from` attribute:

- The new file (i.e., `<FILENAME>.cfnew`) is now created with correct
  permission during remote copy. Previously it would be created with
  default permissions.
- The destination file (i.e., `<FILENAME>`) is no longer deleted on
  backup during file copy.  Previously it would be renamed to
  `<FILENAME>.cfsaved`, causing the original file to dissappear. Now an
  actual copy of the original file with the same permissions is created
  instead.

As a result, there will no longer be a brief moment where the original
file is inaccessible.

Ticket: ENT-11988
Changelog: Commit
Signed-off-by: Lars Erik Wik <[email protected]>
@larsewi
Copy link
Contributor Author

larsewi commented Oct 9, 2024

Looks good to me except that we should use (the code from) CopyRegularFileDisk(), I believe.

Thanks 🍻

@larsewi larsewi requested a review from vpodzime October 9, 2024 11:12
Copy link
Contributor

@craigcomstock craigcomstock left a comment

Choose a reason for hiding this comment

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

Yeah, makes sense. I had to go looking for the "move cfnew to dest" code and found it at

        if (rename(changes_new, changes_dest) == 0)
        {
            RecordChange(ctx, pp, attr, "Moved '%s' to '%s'", new, dest);
            *result = PromiseResultUpdate(*result, PROMISE_RESULT_CHANGE);
        }

Interesting that the RecordChange() call uses different variables than the rename() call but they "should" be the same. ;)

Copy link
Contributor

@vpodzime vpodzime left a comment

Choose a reason for hiding this comment

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

ACK

@larsewi larsewi added the cherry-pick? Fixes which may need to be cherry-picked to LTS branches label Oct 10, 2024
@larsewi larsewi merged commit a0b3725 into cfengine:master Oct 10, 2024
44 checks passed
@larsewi larsewi removed the cherry-pick? Fixes which may need to be cherry-picked to LTS branches label Oct 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants