OpenShift SDN: Changed ovs.Transaction from pseudo to real atomic transaction #19393

pravisankar · 2018-04-17T20:37:19Z

Leverages ovs.bundle() to perform atomic transactions
Now ovs.Transaction interface has AddFlow(), DeleteFlows() and Commit() methods
General usage:
otx := ovs.NewTransaction()
otx.AddFlow(flow1) // No execution, only caches flow context
...
otx.DeleteFlows(flowN) // No execution, only caches flow context
...
err := otx.Commit() // Executes all cached flows as single atomic transaction
With this change, most of the operations in ovs controller like setup,
addHostSubnet, addService, etc. has only one ovs bundle call. So there
won't be partial commited changes and it reduces the downtime during
watch resync events.

pravisankar · 2018-04-17T20:37:56Z

@openshift/sig-networking PTAL

danwinship

I think exposing Bundle in the API adds a lot of complexity without much benefit. In particular, if you drop it from the public API, then you can also drop basically all of your changes to fake_ovs, and also remove BundleAddFlowRepr and BundleDeleteFlowRepr as APIs (and just have them be internal details of ovsExec's AddFlow and DeleteFlows implementations).

danwinship · 2018-04-17T21:18:49Z

pkg/util/ovs/ovs.go

+		stdin := bytes.NewBufferString(stdinString)
+		kcmd.SetStdin(stdin)
+
+		glog.V(4).Infof("Executing: %s %s |\n%s", cmd, strings.Join(args, " "), stdinString)


The use of | there is weird: it makes it look like you're piping the output of ovs-ofctl to the string.
Actually, just changing | to << should make it clear what's going on, shell-syntax-wise, I think?

danwinship · 2018-04-17T21:23:25Z

pkg/util/ovs/fake_ovs.go

+	addPrefix := "flow add"
+	delPrefix := "flow delete"
+
+	if strings.Contains(bundleFlow, addPrefix) {


strings.HasPrefix

danwinship · 2018-04-17T21:30:06Z

pkg/util/ovs/fake_ovs.go

@@ -283,16 +284,12 @@ func (fake *ovsFake) Bundle(bundleFlows []string) error {
 		}

 		if isAdd {
-			tx.AddFlow(flow)
+			fake.addFlowHelper(flow)


you need to check the error return here

openshift-ci-robot · 2018-04-18T00:05:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pravisankar

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:

~~pkg/network/OWNERS~~ [pravisankar]
~~pkg/util/ovs/OWNERS~~ [pravisankar]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pravisankar · 2018-04-18T00:11:50Z

@danwinship Updated, PTAL

pravisankar · 2018-04-18T06:59:32Z

/retest

danwinship · 2018-04-18T14:00:12Z

pkg/util/ovs/ovs.go

@@ -106,7 +107,7 @@ type ovsExec struct {
 }

 // New returns a new ovs.Interface
-func New(execer exec.Interface, bridge string, minVersion string) (Interface, error) {
+func New(execer exec.Interface, bridge string, minVersion string) (*ovsExec, error) {


Hm... isn't that bad style? Maybe you could just have TestBundle() cast it from Interface to *ovsExec.

Alternatively, after the next commit, TestBundle() doesn't really do anything beyond what TestTransactionSuccess() and TestTransactionFailure() do, so you could just drop TestBundle() (and maybe add a TestEmptyTransaction() in the next commit if you want to explicitly test that case too.)

danwinship · 2018-04-18T14:03:45Z

pkg/network/node/ovscontroller.go

@@ -496,7 +491,7 @@ func (oc *ovsController) UpdateEgressNetworkPolicyRules(policies []networkapi.Eg
 		otx.DeleteFlows("table=101, reg0=%d, cookie=1/1", vnid)


UpdateEgressNetworkPolicyRules no longer needs to do the "temporarily drop all outgoing traffic to avoid race conditions" thing.

You can also revert the changes from #19346 since they're no longer needed

danwinship · 2018-04-18T14:10:14Z

pkg/util/ovs/ovs_test.go

@@ -53,8 +53,7 @@ func ensureTestResults(t *testing.T, fexec *fakeexec.FakeExec) {

 func TestTransactionSuccess(t *testing.T) {
 	fexec := normalSetup()
-	addTestResult(t, fexec, "ovs-ofctl -O OpenFlow13 add-flow br0 flow1", "", nil)
-	addTestResult(t, fexec, "ovs-ofctl -O OpenFlow13 add-flow br0 flow2", "", nil)
+	addTestResult(t, fexec, "ovs-ofctl -O OpenFlow13 bundle br0 -", "", nil)


(It would be nice if we could test the stdin value too...)

danwinship · 2018-04-18T14:18:35Z

pkg/util/ovs/fake_ovs.go

 }

 func (fake *ovsFake) NewTransaction() Transaction {
-	return &ovsFakeTx{fake: fake, err: fake.ensureExists()}
+	return &ovsFakeTx{fake: fake, flows: []string{}}


Note that there's a mild behavior change here: it used to check that you had called ovsif.AddBridge() on the ovsFake, but now it doesn't. (We apparently don't test this case in fake_ovs_test.go.) Anyway, you could just move the fake.ensureExists() call to Commit

danwinship · 2018-04-18T14:25:50Z

pkg/util/ovs/fake_ovs.go

-	err := tx.err
-	tx.err = nil
-	return err
+func (tx *ovsFakeTx) Commit() error {


This doesn't actually implement transactional behavior: if you do

tx.AddFlow("table=10, actions=drop") tx.AddFlow("@!#$!@#$!@#$") tx.Commit()

then it will return an error saying that the second flow couldn't be parsed, but the "table=10, actions=drop" flow will still have been added.

There should probably be a test of transactionality in fake_ovs_test.go

pravisankar · 2018-04-24T22:53:57Z

@danwinship Addressed all your comments, PTAL

knobunc · 2018-05-02T15:13:43Z

Looks good to me.

@dcbw can you take a look please?

- bundle() enables execution of given add and/or delete ovs flows in a single atomic transaction

- Leverages ovs.bundle() to perform atomic transactions - Now ovs.Transaction interface has AddFlow(), DeleteFlows() and Commit() methods General usage: otx := ovs.NewTransaction() otx.AddFlow(flow1) // No execution, only caches flow context ... otx.DeleteFlows(flowN) // No execution, only caches flow context ... err := otx.Commit() // Executes all cached flows as single atomic transaction - With this change, most of the operations in ovs controller like setup, addHostSubnet, addService, etc. has only one ovs bundle call. So there won't be partial commited changes and it reduces the downtime during watch resync events.

- With ovs atomic transaction, flows are actually executed when Commit() is called so we no longer need the earlier workaround.

danwinship · 2018-05-03T07:03:59Z

/lgtm

pravisankar · 2018-05-03T17:09:05Z

/retest

pravisankar · 2018-05-03T18:16:40Z

/retest

pravisankar · 2018-05-03T22:30:12Z

/retest

openshift-bot · 2018-05-04T01:03:28Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 17, 2018

openshift-ci-robot requested review from dcbw and smarterclayton April 17, 2018 20:37

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 17, 2018

pravisankar added the component/networking label Apr 17, 2018

openshift-ci-robot added the sig/networking label Apr 17, 2018

pravisankar requested review from danwinship, rajatchopra and knobunc April 17, 2018 20:41

danwinship suggested changes Apr 17, 2018

View reviewed changes

pravisankar force-pushed the ovs-atomic-transactions branch 2 times, most recently from 37d263e to 8207b0b Compare April 17, 2018 23:49

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 17, 2018

pravisankar force-pushed the ovs-atomic-transactions branch from 8207b0b to 6c4fda1 Compare April 18, 2018 00:05

openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Apr 18, 2018

pravisankar force-pushed the ovs-atomic-transactions branch from 6c4fda1 to ffc8186 Compare April 18, 2018 06:13

danwinship suggested changes Apr 18, 2018

View reviewed changes

pravisankar force-pushed the ovs-atomic-transactions branch 2 times, most recently from 7176cdf to 68f3e6d Compare April 24, 2018 22:51

pravisankar force-pushed the ovs-atomic-transactions branch from 68f3e6d to 06ac661 Compare April 30, 2018 16:55

Ravi Sankar Penta added 5 commits May 2, 2018 08:15

Added internal bundle() method to ovsExec interface

c6b4383

- bundle() enables execution of given add and/or delete ovs flows in a single atomic transaction

Fix fake ovs transaction to support ovs controller testing

cf8cd58

Revert openshift#19346

e752ca6

- With ovs atomic transaction, flows are actually executed when Commit() is called so we no longer need the earlier workaround.

OVS test: Validate stdin values to bundle() call

105395d

pravisankar force-pushed the ovs-atomic-transactions branch from 06ac661 to 105395d Compare May 2, 2018 15:18

openshift-ci-robot assigned danwinship May 3, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 3, 2018

openshift-merge-robot merged commit 7a3aaab into openshift:master May 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OpenShift SDN: Changed ovs.Transaction from pseudo to real atomic transaction #19393

OpenShift SDN: Changed ovs.Transaction from pseudo to real atomic transaction #19393

pravisankar commented Apr 17, 2018 •

edited

Loading

pravisankar commented Apr 17, 2018

danwinship left a comment

danwinship Apr 17, 2018

danwinship Apr 17, 2018

danwinship Apr 17, 2018

openshift-ci-robot commented Apr 18, 2018

pravisankar commented Apr 18, 2018

pravisankar commented Apr 18, 2018

danwinship Apr 18, 2018

danwinship Apr 18, 2018

danwinship Apr 18, 2018

danwinship Apr 18, 2018

danwinship Apr 18, 2018

pravisankar commented Apr 24, 2018

knobunc commented May 2, 2018

danwinship commented May 3, 2018

pravisankar commented May 3, 2018

pravisankar commented May 3, 2018

pravisankar commented May 3, 2018

openshift-bot commented May 4, 2018

		@@ -496,7 +491,7 @@ func (oc *ovsController) UpdateEgressNetworkPolicyRules(policies []networkapi.Eg
		otx.DeleteFlows("table=101, reg0=%d, cookie=1/1", vnid)

OpenShift SDN: Changed ovs.Transaction from pseudo to real atomic transaction #19393

OpenShift SDN: Changed ovs.Transaction from pseudo to real atomic transaction #19393

Conversation

pravisankar commented Apr 17, 2018 • edited Loading

pravisankar commented Apr 17, 2018

danwinship left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Apr 18, 2018

pravisankar commented Apr 18, 2018

pravisankar commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pravisankar commented Apr 24, 2018

knobunc commented May 2, 2018

danwinship commented May 3, 2018

pravisankar commented May 3, 2018

pravisankar commented May 3, 2018

pravisankar commented May 3, 2018

openshift-bot commented May 4, 2018

pravisankar commented Apr 17, 2018 •

edited

Loading