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.

Sign up for GitHub

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

Jump to bottom

Fix RemoveOfferOperation for server-side transactions #12494

Merged
merged 1 commit into from
Jan 10, 2021
Merged

Fix RemoveOfferOperation for server-side transactions #12494

merged 1 commit into from
Jan 10, 2021

Conversation

manadart
Copy link
Member

@manadart manadart commented Jan 8, 2021

The model operation for removing application offers currently accrues transaction operations for removing the remote application before those for pushing relation units out of scope and removing relations.

It means that the following operations against the remoteApplications collection are accumulated (in listed order):

(txn.Op) {
  C: (string) (len=18) "remoteApplications",
  Id: (string) (len=46) "2e156330-ca09-406e-8668-e9e6bfc32290:wordpress",
  Assert: (bson.D) (len=2 cap=2) {
   (bson.DocElem) {
    Name: (string) (len=4) "life",
    Value: (state.Life) alive
   },
   (bson.DocElem) {
    Name: (string) (len=13) "relationcount",
    Value: (int) 1
   }
  },
  Insert: (interface {}) <nil>,
  Update: (interface {}) <nil>,
  Remove: (bool) true
 },

(txn.Op) {
  C: (string) (len=18) "remoteApplications",
  Id: (string) (len=46) "2e156330-ca09-406e-8668-e9e6bfc32290:wordpress",
  Assert: (bson.D) (len=2 cap=2) {
   (bson.DocElem) {
    Name: (string) (len=13) "relationcount",
    Value: (bson.D) (len=1 cap=1) {
     (bson.DocElem) {
      Name: (string) (len=3) "$gt",
      Value: (int) 0
     }
    }
   },
   (bson.DocElem) {
    Name: (string) (len=4) "life",
    Value: (state.Life) alive
   }
  },
  Insert: (interface {}) <nil>,
  Update: (bson.D) (len=1 cap=1) {
   (bson.DocElem) {
    Name: (string) (len=4) "$inc",
    Value: (bson.D) (len=1 cap=1) {
     (bson.DocElem) {
      Name: (string) (len=13) "relationcount",
      Value: (int) -1
     }
    }
   }
  },
  Remove: (bool) false
 },
}

This is a deletion and subsequent update of the same document, which is forgiven by the client-side transaction implementation, but not by the server-side one.

This patch rearranges the composition so that the update occurs before the deletion.

QA steps

The exact same operations are generated as before, just in a different order.

  • Apply this patch:
diff --git a/state/open.go b/state/open.go
index e1e6284012..930a7da75e 100644
--- a/state/open.go
+++ b/state/open.go
@@ -136,7 +136,7 @@ func newState(
        // plus txn.Op slices that are wrong, eg insert dup or missing asserts.
        // Related test failures here:
        // https://jenkins.juju.canonical.com/job/github-make-check-juju/9317/testReport/
-       sstxn = false
+       sstxn = true

        if sstxn {
                logger.Infof("using server-side transactions")
  • Run the applicationOffersSuite.TestRemoveOffersWithConnectionsForce unit test in the state package.

Documentation changes

None.

Bug reference

N/A

@manadart
Rearanges transaction operations composition for application offer
700f3d8 
removals so that removal of remote application occurs after units have
exited scope and the relation is removed.

Required for server-side transactions, which are less forgiving than the
old client-side implementation.
@manadart
Copy link
Member Author

manadart commented Jan 8, 2021

Fix model operation for removing application offers

@wallyworld
Copy link
Member

$$merge$$

@jujubot jujubot merged commit 389da95 into juju:develop Jan 10, 2021
@manadart manadart deleted the 2.9-app-offers-remove-ops branch January 11, 2021 11:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wallyworld wallyworld wallyworld approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@manadart @wallyworld @jujubot