Cluster read only #100

MerceaOtniel · 2018-08-16T11:40:06Z

No description provided.

AMecea · 2018-08-17T16:27:42Z

pkg/mysqlcluster/orc_reconciliation.go

+		}
+
+		if node.ReadOnly == true {
+			isReadOnly = isReadOnly && true


Write it just once and above if statement. And use logical OR because using AND it will not work.

isReadOnly = isReadOnly && !node.ReadOnly

AMecea · 2018-08-17T16:29:09Z

pkg/mysqlcluster/orc_reconciliation.go

+		}
+
+	}
+	glog.Infof("\n---------- displaying the array/checking if every value is the same---------------\n")


No need of debugging logging anymore.

AMecea · 2018-08-20T08:45:13Z

pkg/mysqlcluster/orc_reconciliation.go

 			f.updateNodeCondition(host, api.NodeConditionMaster, core.ConditionFalse)
 		}
+		if err == nil {
+			root = append(root, master)


rename it to masterForNode

AMecea · 2018-08-20T12:29:16Z

pkg/mysqlcluster/orc_reconciliation.go

+	}
+
+	if len(getMasterForNode(insts)) != 0 {
+		masterHostName, check := verifyMasterArray(getMasterForNode(insts))


you can merge those two functions, you will always use them like this verifyMasterArray(getMasterForNode(insts)) so you can make just one function:

func determinMasterFor(inst []orc.Instace) (string, err)

AMecea · 2018-08-20T12:29:30Z

pkg/mysqlcluster/orc_reconciliation.go

+
+	}
+
+	if len(getMasterForNode(insts)) != 0 {


no need for this check

AMecea · 2018-08-20T12:37:29Z

pkg/mysqlcluster/orc_reconciliation.go

+func (f *cFactory) setMasterWritable(masterHostName string, insts []orc.Instance) {
+	master, err := getInstance(masterHostName, insts)
+
+	if err == nil {


if err != nil { glog.Errorf("Get instance error:%s", err) return }

AMecea · 2018-08-20T12:39:58Z

pkg/util/orchestrator/orchestrator.go

@@ -30,6 +30,9 @@ type Interface interface {

 	AuditRecovery(cluster string) ([]TopologyRecovery, error)
 	AckRecovery(id int64, commnet string) error
+
+	SetHostWritable(host string, port int) error


You can use as parameter InstanceKey because instance key has both host and port

AMecea · 2018-08-20T12:42:32Z

pkg/mysqlcluster/orc_reconciliation.go

+			f.updateNodeCondition(masterHostName, api.NodeConditionMaster, core.ConditionFalse)
+		}
+
+		f.setMasterWritable(masterHostName, insts)


move this function call at line 48 after updateStatusFromOrc call

AMecea · 2018-08-20T12:44:30Z

pkg/mysqlcluster/orc_reconciliation.go

+}
+
+func (f *cFactory) setMasterWritable(masterHostName string, insts []orc.Instance) {
+	master, err := getInstance(masterHostName, insts)


you can compute masterHostName using determinMasterFor and don't need to pass it as parameter. See see the next comments.

AMecea · 2018-08-21T11:57:34Z

pkg/mysqlcluster/orc_reconciliation.go

+	if err == nil {
+		f.updateNodeCondition(masterHostName, api.NodeConditionMaster, core.ConditionTrue)
+	} else {
+		f.updateNodeCondition(masterHostName, api.NodeConditionMaster, core.ConditionFalse)


if there is an error then what value masterHostName will have?

else statement can be deleted or better log the error

AMecea · 2018-08-21T11:59:41Z

pkg/mysqlcluster/orc_reconciliation.go

+		f.updateNodeCondition(masterHostName, api.NodeConditionMaster, core.ConditionFalse)
+	}
+
+	err = f.setMasterWritable(masterHostName, insts)


This code should be moved in SyncOrchestratorStatus function near to updateStatusFromOrc call.

AMecea · 2018-08-21T12:02:27Z

pkg/mysqlcluster/orc_reconciliation.go

+}
+
+func (f *cFactory) setMasterWritable(masterHostName string, insts []orc.Instance) error {
+	master, err := getInstance(masterHostName, insts)


Get master here with determineMasterFor.

Also in this function you have to make sure that only one node is writable!

Make sure that failover works as expected, by manually testing.

Test case:

create a cluster 2 nodes

delete pod 0

check node:0 -> read only and node:1 -> writable (cluster status)

Test case 2:

create cluster 3 nodes

make node:1 comaster

check status

AMecea · 2018-08-24T14:27:58Z

cmd/mysql-helper/apphelper/apphelper.go

-		query = "SET GLOBAL SUPER_READ_ONLY = 1"
-	}
+
+	query = "SET GLOBAL SUPER_READ_ONLY = 1"


query := "SET GLOBAL SUPER_READ_ONLY = 1"

AMecea · 2018-08-24T14:28:17Z

pkg/apis/mysql/v1alpha1/types.go

@@ -107,6 +107,8 @@ type ClusterSpec struct {
 	// QueryLimits represents limits for a query
 	// +optional
 	QueryLimits *QueryLimits `json:"queryLimits,omitempty"`
+
+	ReadOnly bool `json:"readOnly,omitempty"`


Add a description here

AMecea · 2018-08-24T14:45:32Z

pkg/mysqlcluster/orc_reconciliation.go

+
+func (f *cFactory) updateNodesReadOnlyFlagInOrc(insts []orc.Instance) error {
+	master, err := determineMasterFor(insts)
+


Let's treat the error here, the code will be much more clearly.

master, err := determineMasterFor(insts) if err != nil { // if no master determined then set the cluster in read only. for _, inst := range insts { f.putNodeInMaintenance(inst) f.setInstReadOnly(inst) } }

AMecea · 2018-08-24T14:46:03Z

pkg/mysqlcluster/orc_reconciliation.go

+		} else {
+			f.updateNodeCondition(host, api.NodeConditionReadOnly, core.ConditionFalse)
+		}
+


to many new lines.

AMecea · 2018-08-24T14:55:03Z

pkg/mysqlcluster/orc_reconciliation.go

+	}
+
+	master, err := determineMasterFor(insts)
+	if err == nil {


if err != nil { glog.Errorf("Error acquiring master name %s", err) } f.updateNodeCondition(master.Key.Hostname, api.NodeConditionMaster, core.ConditionTrue) ....

AMecea · 2018-08-24T14:56:40Z

pkg/mysqlcluster/orc_reconciliation_test.go

+	"k8s.io/client-go/kubernetes/fake"
+	"k8s.io/client-go/tools/record"
+	"testing"
+	"time"


the time and testing should be in the first group of imports, then in the second group external imports and finally the third group should be local imports.

AMecea · 2018-08-24T14:57:17Z

pkg/mysqlcluster/orc_reconciliation_test.go

@@ -99,6 +98,8 @@ var _ = Describe("Mysql cluster reconcilation", func() {
 				Ω(rec.Events).Should(Receive(&event))
 				Expect(event).To(ContainSubstring("ReplicationStopped"))
 				Ω(rec.Events).Should(Receive(&event))
+				Expect(event).To(ContainSubstring("DemoteMaster"))
+				Ω(rec.Events).Should(Receive(&event))


Let's replace all Ω with Expect. Ω is hard to type.

AMecea · 2018-08-24T15:04:48Z

pkg/util/orchestrator/fake/client.go

@@ -65,6 +65,38 @@ func (o *FakeOrc) AddInstance(cluster, host string, master bool, sls int64, slav
 	o.Clusters[cluster] = []Instance{inst}
 }

+func (o *FakeOrc) AddInstanceInTopology(cluster, host string, port int, master bool, sls int64, slaveR, upToDate bool, masterKey string, masterPort int, coMaster bool) {


I propose that, lets return a pointer to newly created Instance so you can easily modify instance so you don't need to pass that much parameters to this function.

And also you can use AddInstance function from above.

func (o *FakeOrc) AddInstance(cluster, host string, master bool, sls int64, slaveR, upToDate bool) *Instance

By keeping the old parameters you don't have to modify old tests. The new test will lock like:

inst := orcClient.AddInstanceInTopology("asd.default", "foo122-mysql-0", 3306, false, -1, false, true) inst.MasterKey = InstanceKey{Hostname: "foo122-mysql-5", Port: 3305} inst.IsCoMaster = true

AMecea · 2018-08-24T15:05:41Z

pkg/util/orchestrator/fake/client.go

+
+func (o *FakeOrc) SetHostWritable(key InstanceKey) error {
+
+	var check bool = false


check := false

AMecea · 2018-08-24T15:15:15Z

pkg/util/orchestrator/orchestrator.go

+
+}
+
+func (o *orchestrator) EndMaintenance(key InstanceKey, owner, reason string) error {


I don't use owner and reason params.

AMecea · 2018-08-28T10:48:24Z

test/e2e/cluster/cluster.go

+}
+
+// tests if the cluster is in orchestrator and is properly configured
+func testClusterRegistrationInOrchestrator(f *framework.Framework, cluster *api.MysqlCluster, ClusterReadOnly bool) {


use lower letter for ClusterReadOnly param

AMecea · 2018-08-28T10:48:45Z

pkg/util/orchestrator/orchestrator.go

+	}
+
+	return nil
+


AMecea · 2018-08-28T10:53:56Z

pkg/util/orchestrator/fake/client.go

 	}
-	o.Clusters[cluster] = []Instance{inst}
+
+	for _, inst1 := range o.Clusters[cluster] {


You don't need to iterate over any more.
Also you can create inst in the first place as pointer and to return the address:

inst := &Instance{ ... return inst

AMecea · 2018-08-28T10:54:24Z

pkg/mysqlcluster/orc_reconciliation_test.go

+
+				_, err := determineMasterFor(insts)
+				Expect(err).ToNot(BeNil())
+


AMecea · 2018-08-28T10:54:31Z

pkg/mysqlcluster/orc_reconciliation_test.go

+
+				_, err := determineMasterFor(insts)
+				Expect(err).ToNot(BeNil())
+


AMecea · 2018-08-28T10:54:48Z

pkg/mysqlcluster/orc_reconciliation_test.go

+
+				_, err := determineMasterFor(insts)
+				Expect(err).To(BeNil())
+


AMecea · 2018-08-28T10:55:56Z

pkg/mysqlcluster/orc_reconciliation_test.go


 				Expect(cluster.Status.Nodes[0].GetCondition(api.NodeConditionMaster).Status).To(
 					Equal(core.ConditionTrue))

 				orcClient.RemoveInstance("asd.default", cluster.GetPodHostname(0))
-				Ω(factory.SyncOrchestratorStatus(ctx)).Should(Succeed())
+				Expect(factory.SyncOrchestratorStatus(ctx)).Should(Succeed())

 				Expect(cluster.Status.Nodes[0].GetCondition(api.NodeConditionMaster).Status).To(
 					Equal(core.ConditionUnknown))



AMecea · 2018-08-28T10:57:13Z

pkg/mysqlcluster/orc_reconciliation_test.go

 	fakeOrc "github.com/presslabs/mysql-operator/pkg/util/orchestrator/fake"
 	tutil "github.com/presslabs/mysql-operator/pkg/util/test"
+	core "k8s.io/api/core/v1"


external imports should be in the second group of imports.
First is go imports, second is external imports and third are the local imports(imports from project)

…use this is probably the default behavior that users of vtop want. (bitpoke#100) Signed-off-by: Peter Farr <Peter@PrismaPhonic.com>

MerceaOtniel requested a review from AMecea August 16, 2018 11:56

MerceaOtniel force-pushed the clusterReadOnly branch 2 times, most recently from c494ae2 to 5dc3a76 Compare August 17, 2018 09:02

AMecea suggested changes Aug 20, 2018

View reviewed changes

MerceaOtniel force-pushed the clusterReadOnly branch from 7e9144d to ded1f6c Compare August 20, 2018 11:36

AMecea suggested changes Aug 20, 2018

View reviewed changes

MerceaOtniel force-pushed the clusterReadOnly branch 3 times, most recently from 6e78f63 to eebc592 Compare August 21, 2018 10:59

AMecea suggested changes Aug 21, 2018

View reviewed changes

MerceaOtniel force-pushed the clusterReadOnly branch 3 times, most recently from a4b9f6d to cc95119 Compare August 24, 2018 07:15

AMecea suggested changes Aug 24, 2018

View reviewed changes

MerceaOtniel force-pushed the clusterReadOnly branch 6 times, most recently from 13dd266 to 0b714ea Compare August 28, 2018 10:00

AMecea suggested changes Aug 28, 2018

View reviewed changes

Add the logic required for spec.readOnly

197e025

MerceaOtniel force-pushed the clusterReadOnly branch from 397e7e4 to 197e025 Compare August 28, 2018 11:25

AMecea approved these changes Aug 28, 2018

View reviewed changes

MerceaOtniel merged commit 7524d2d into master Aug 28, 2018

MerceaOtniel mentioned this pull request Aug 28, 2018

Allow setting the cluster readOnly #58

Closed

3 tasks

calind deleted the clusterReadOnly branch September 12, 2018 11:25

AMecea mentioned this pull request Nov 3, 2020

Let the orchestrator handle the read-only/writable of nodes #627

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cluster read only #100

Cluster read only #100

MerceaOtniel commented Aug 16, 2018

AMecea Aug 17, 2018

AMecea Aug 17, 2018

AMecea Aug 20, 2018

AMecea Aug 20, 2018

AMecea Aug 20, 2018

AMecea Aug 20, 2018

AMecea Aug 20, 2018

AMecea Aug 20, 2018

AMecea Aug 20, 2018

AMecea Aug 21, 2018

AMecea Aug 21, 2018

AMecea Aug 21, 2018

AMecea Aug 21, 2018

AMecea Aug 21, 2018

AMecea Aug 21, 2018

AMecea Aug 24, 2018

AMecea Aug 24, 2018

AMecea Aug 24, 2018

AMecea Aug 24, 2018

AMecea Aug 24, 2018

AMecea Aug 24, 2018

AMecea Aug 24, 2018

AMecea Aug 24, 2018

AMecea Aug 24, 2018

AMecea Aug 24, 2018

AMecea Aug 28, 2018

AMecea Aug 28, 2018

AMecea Aug 28, 2018

AMecea Aug 28, 2018

AMecea Aug 28, 2018

AMecea Aug 28, 2018

AMecea Aug 28, 2018

AMecea Aug 28, 2018


		func (f *cFactory) updateNodesReadOnlyFlagInOrc(insts []orc.Instance) error {
		master, err := determineMasterFor(insts)


		func (o *FakeOrc) SetHostWritable(key InstanceKey) error {

		var check bool = false


		}

		func (o *orchestrator) EndMaintenance(key InstanceKey, owner, reason string) error {


		_, err := determineMasterFor(insts)
		Expect(err).ToNot(BeNil())

Cluster read only #100

Cluster read only #100

Conversation

MerceaOtniel commented Aug 16, 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

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

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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment