chore: (cstor-volume-mgmt) Add support to update cstorvolume status with known replicas #1447

mittachaitu · 2019-09-19T06:45:07Z

What this PR does / why we need it:
This PR does the following changes:

Add support to update trusty replica information in cstorvolume status to make replica information persist in etcd.
Add support to trigger istgtcontrol drf from volume-mgmt on success updates istgt.conf file(target configuration file).
Ex: command istgtcontrol drf <vol_name> <value>
To implement this cstor-volume-mgmt starts the new UnixDomainServer and cstor-istgt(main car) connects to it and update the information related to known replicas and configurations.

Example YAML:

spec:
  capacity: 5G
  consistencyFactor: 1
  desiredReplicationFactor: 1
  iqn: iqn.2016-09.com.openebs.cstor:pvc-a2a53a14-1cc4-4bdd-ad4b-ab88e07d7823
  nodeBase: iqn.2016-09.com.openebs.cstor
  replicationFactor: 1
  status: Init
  targetIP: 10.0.0.28
  targetPort: "3260"
  targetPortal: 10.0.0.28:3260
status:
  capacity: 5G
  lastTransitionTime: "2019-09-27T08:31:54Z"
  lastUpdateTime: "2019-09-27T08:33:53Z"
  phase: Healthy
  replicaDetails: {}
  replicaStatuses:
  - checkpointedIOSeq: "0"
    inflightRead: "0"
    inflightSync: "0"
    inflightWrite: "0"
    mode: Healthy
    quorum: "1"
    replicaId: "16362917944657237302"
    upTime: 147

Below two fields are introduced in the schema as part of replica scaleup support.
replicaDetails -- replicas present in this list are trusty replicas from where the target can perform read operations.
desiredReplicationFactor -- It is useful to scale up replicas from n to m (where m <= 5) by default it will be equal to replicationFactor.

For more information, please follow the doc: openebs/openebs#2739

Special notes for your reviewer:
TODO:

Need to test this code after fixing istgt travis issue.
UnitTest cases for this PR changes.

Checklist:

Fixes #
Labelled this PR & related issue with documentation tag
PR messages has document related information
Labelled this PR & related issue with breaking-changes tag
PR messages has breaking changes related information
Labelled this PR & related issue with requires-upgrade tag
PR messages has upgrade related information
Commit has unit tests
Commit has integration tests

pkg/cstor/volume/targetserver/server.go

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

pkg/apis/openebs.io/v1alpha1/cstor_volume.go

vishnuitta · 2019-09-19T17:59:29Z

pkg/cstor/volume/targetserver/server.go

+func StartTargetServer(kubeConfig string) {
+
+	klog.Info("Starting unix domain server")
+	if err := os.RemoveAll(string(volumeMgmtUnixSock)); err != nil {


why to remove?

Before starting server on that sock file clearing the sock path.

not required

pkg/cstor/volume/targetserver/server.go

vishnuitta · 2019-09-19T18:39:36Z

pkg/cstor/volume/targetserver/server.go

+		if n > 0 {
+			completeBytes = append(completeBytes, buf[0:n]...)
+			if strings.HasSuffix(string(completeBytes), endOfLine) {
+				lines := strings.Split(string(completeBytes), endOfLine)


why to Split and why to append?

This piece of code needs fix.. please put the communication protocol in comments

pkg/cstor/volume/v1alpha1/cstorvolume.go

pkg/cstor/volume/targetserver/server.go

mynktl · 2019-09-25T07:33:32Z

pkg/cstor/volume/targetserver/server.go

+		}
+		if n > 0 {
+			completeBytes = append(completeBytes, buf[0:n]...)
+			if strings.HasSuffix(string(completeBytes), endOfLine) {


What if completedBytes contains multiple lines?

As we discussed, add comment here about communication protocol.

pkg/cstor/volume/targetserver/server.go

mynktl · 2019-09-25T07:39:23Z

pkg/cstor/volume/targetserver/server.go

+}
+
+// GetRequiredData returns error if doesn't have json format
+func GetRequiredData(data string) (string, error) {


Though comment is there, Let's change this function name something like GetJsonData..

mynktl · 2019-09-25T09:55:49Z

pkg/cstor/volume/targetserver/server.go

+				lines := strings.Split(string(completeBytes), endOfLine)
+				for _, line := range lines {
+					if len(line) != 0 {
+						req = append(req, line+endOfLine)


remove endOfLine

made into return lines[0]

mynktl · 2019-09-25T09:56:59Z

pkg/cstor/volume/targetserver/server.go

+		klog.Errorf("failed to read data: {%v}", err)
+		return
+	}
+	filteredData, err = GetRequiredData(readData)


No need to do this. You can execute unmarshal on readData.

mynktl · 2019-09-25T10:11:47Z

pkg/cstor/volume/targetserver/server.go

+	for {
+		sockFd, err := listen.Accept()
+		if err != nil {
+			klog.Fatalf("failed to accept error: {%v}", err)


No need of Fatalf here.

if not fatal, we need to handle it then by starting server again

It was for old code.

pkg/cstor/volume/targetserver/server.go

1. Included executing istgtcontrol drf command 2. Updating istgt conf file by taking lock 3. Handiling to do part as part of server to start Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

ghost

Found some fixes!

P.S. share your ideas, feedbacks or issues with us at https://github.com/fixmie/feedback (this message will be removed after the beta stage).

cmd/cstor-volume-mgmt/volume/volume.go

pkg/apis/openebs.io/v1alpha1/cstor_volume.go

pkg/cstor/volume/targetserver/server.go

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

pkg/util/fileoperator.go

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

ghost

Found some fixes!

P.S. share your ideas, feedbacks or issues with us at https://github.com/fixmie/feedback (this message will be removed after the beta stage).

pkg/cstor/volume/targetserver/server.go

vishnuitta · 2019-09-28T11:04:05Z

cmd/cstor-volume-mgmt/controller/volume-controller/handler.go

@@ -214,7 +233,9 @@ func (c *CStorVolumeController) cStorVolumeEventHandler(
 			cStorVolumeGot.Status.LastTransitionTime = cStorVolumeGot.Status.LastUpdateTime
 		}

-		cStorVolumeGot.Status.ReplicaStatuses = volStatus.ReplicaStatuses
+		if volStatus != nil {


no need for this check

It may be required if errror occurred for istgtcontrol replica status we are not returning(I don't know the reason) so instead of returning I made this check.

volStatus should be validated after volume.GetVolumeStatus.

volStatus, err := volume.GetVolumeStatus(cStorVolumeGot) if err != nil || volStatus == nil {

Let's add an issue in hacktoberfest to fix this cmd/cstor-volume-mgmt/volume.GetVolumeStatus.

vishnuitta · 2019-09-28T20:23:48Z

pkg/cstor/volume/targetserver/server.go

+	}
+
+	sigc := make(chan os.Signal, 1)
+	signal.Notify(sigc, os.Interrupt, syscall.SIGTERM)


why for Interrupt?

should we consider SIGKILL?

or you better remove all the code related to signals

If remove the code related to signals then we are not closing the listener connection which will be the problem for the next listener(i.e when the container starts again) it might throw an error address already in use.

Removed signal code os.RemoveAll kept as is

vishnuitta · 2019-09-28T20:28:51Z

pkg/cstor/volume/targetserver/server.go

+	for {
+		sockFd, err := listen.Accept()
+		if err != nil {
+			klog.Errorf("failed to accept error: {%v}", err)


do a break from here

for anything other than EINTR, EAGAIN, EWOULDBLOCK, do a break

I think I need to exit the process, isn't it?

pkg/cstor/volume/v1alpha1/cstorvolume.go

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

mynktl · 2019-09-30T07:11:24Z

cmd/cstor-volume-mgmt/controller/volume-controller/handler.go

+			c.recorder.Event(
+				cStorVolumeGot, corev1.EventTypeWarning,
+				string(common.FailureCreate),
+				fmt.Sprintf("failed to create cstorvolume validation "+


let's change it to failed to validate cstorvolume.

We are not failed to validate we failed during validation I think current looks good

mynktl · 2019-09-30T07:18:35Z

cmd/cstor-volume-mgmt/controller/volume-controller/handler.go

@@ -214,7 +233,9 @@ func (c *CStorVolumeController) cStorVolumeEventHandler(
 			cStorVolumeGot.Status.LastTransitionTime = cStorVolumeGot.Status.LastUpdateTime
 		}

-		cStorVolumeGot.Status.ReplicaStatuses = volStatus.ReplicaStatuses
+		if volStatus != nil {


volStatus should be validated after volume.GetVolumeStatus.

volStatus, err := volume.GetVolumeStatus(cStorVolumeGot) if err != nil || volStatus == nil {

Let's add an issue in hacktoberfest to fix this cmd/cstor-volume-mgmt/volume.GetVolumeStatus.

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

vishnuitta · 2019-09-30T10:25:37Z

pkg/cstor/volume/targetserver/server.go

+		n, err := r.Read(buf[:])
+		if err != nil {
+			if err == io.EOF {
+				klog.Info("Reached End Of file")


'break' here makes to return error.. do we return error in such case?
And, is this not the regular case? if so, log may not be good to have here

EOF will occur only when client closes the connection and here we are expecting \r\n as end of data as mentioned in comments. If data doesn't have \r\n better to error out it. I will remove this log.

vishnuitta · 2019-09-30T10:42:31Z

pkg/cstor/volume/targetserver/server.go

+		klog.Errorf("failed to read data: {%v}", err)
+		return
+	}
+	replicationData := &cstorv1alpha1.CVReplicationDetails{}


as it doesn't contain all the details, instead of CVReplicationDetails, how about CVReplicationInfo..

In discussion I heard that we need to accept entire trusty replica list and perform update in etcd so I had structure name as CVReplicationDetails

vishnuitta · 2019-09-30T10:47:02Z

pkg/cstor/volume/targetserver/server.go

+		klog.Errorf("failed to unmarshal replication data {%v}", err)
+		return
+	}
+	csc := &cstorv1alpha1.CStorVolumeConfig{


lets try to avoid creating one more struct just to pass kubeclient

I will take it as a good practice. I will pass as a functional arguments

vishnuitta · 2019-09-30T12:42:28Z

pkg/cstor/volume/v1alpha1/cstorvolume.go

+}
+
+// Validate verifies whether CStorReplication data read on wire is valid or not
+func (csr *CVReplicationDetails) Validate() error {


variable names are changing everywhere.. here why ~~cvr~~ csr?

I used csr everywhere --> cstorreplication

vishnuitta · 2019-09-30T13:00:59Z

pkg/cstor/volume/v1alpha1/cstorvolume.go

+	if cv.Status.ReplicaDetails.KnownReplicas == nil {
+		cv.Status.ReplicaDetails.KnownReplicas = map[string]uint64{}
+	}
+	cv.Status.ReplicaDetails.KnownReplicas[csc.ReplicaID] = csc.ReplicaGUID


log the info that you are updating

vishnuitta · 2019-09-30T13:31:22Z

pkg/cstor/volume/v1alpha1/cstorvolume.go

+	if err != nil {
+		return errors.Wrapf(err, "failed to get cstorvolume")
+	}
+	if len(cv.Status.ReplicaDetails.KnownReplicas) >= cv.Spec.DesiredReplicationFactor {


replica replacement case will get error here
Also, duplicate updates also will get error here, like, if update to CR succeeded but not returned success to istgt.

pkg/util/fileoperator.go

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

mynktl · 2019-10-01T05:22:16Z

pkg/cstor/volume/v1alpha1/cstorvolume.go

 	if csr.VolumeName == "" {
 		return errors.Errorf("volume name can not be empty")
 	}
-	if csr.ReplicaKey == "" {
+	if csr.ReplicaID == "" {


let's use length to check if string is empty.

I am slowly adapting some standard practice in go by looking into core files in standard package for the string to verify whether it is empty or the will do if str == "" {//operation}. If it is map or array or slice they are using len() so I made use of it.

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

vishnuitta

changes are good

golangcibot reviewed Sep 19, 2019

View reviewed changes

pkg/cstor/volume/targetserver/server.go Outdated Show resolved Hide resolved

mittachaitu added 4 commits September 19, 2019 16:43

Add support to update cstorvolume status with known replicas

c008cf5

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

This commit adds the auto generated code

4dad0ff

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

This commit fixes linting issues

c9c160e

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

This commit fixes the golangci liniting issue

51bb4dd

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

mittachaitu force-pushed the add_suport_scale_up branch from 71dde43 to b176c2f Compare September 19, 2019 12:00

vishnuitta requested review from prateekpandey14 and AmitKumarDas September 19, 2019 12:58

This commit updates the glog with klog

1b693de

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

mittachaitu force-pushed the add_suport_scale_up branch from b176c2f to 1b693de Compare September 19, 2019 14:25

mittachaitu added the pr/release-note PR should be included in release notes label Sep 19, 2019

mittachaitu self-assigned this Sep 19, 2019

vishnuitta reviewed Sep 19, 2019

View reviewed changes

pkg/apis/openebs.io/v1alpha1/cstor_volume.go Show resolved Hide resolved

vishnuitta reviewed Sep 19, 2019

View reviewed changes

pkg/cstor/volume/targetserver/server.go Outdated Show resolved Hide resolved

vishnuitta reviewed Sep 19, 2019

View reviewed changes

pkg/cstor/volume/targetserver/server.go Show resolved Hide resolved

vishnuitta reviewed Sep 19, 2019

View reviewed changes

mittachaitu commented Sep 20, 2019

View reviewed changes

pkg/cstor/volume/v1alpha1/cstorvolume.go Outdated Show resolved Hide resolved

AmitKumarDas approved these changes Sep 23, 2019

View reviewed changes

mynktl suggested changes Sep 25, 2019

View reviewed changes

mittachaitu changed the title ~~chore: (cstor-volume-mgmt) Add support to update cstorvolume status with known replicas~~ [WIP]chore: (cstor-volume-mgmt) Add support to update cstorvolume status with known replicas Sep 25, 2019

This commit does mutliple changes

d228703

1. Included executing istgtcontrol drf command 2. Updating istgt conf file by taking lock 3. Handiling to do part as part of server to start Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

ghost reviewed Sep 26, 2019

View reviewed changes

cmd/cstor-volume-mgmt/volume/volume.go Show resolved Hide resolved

pkg/apis/openebs.io/v1alpha1/cstor_volume.go Show resolved Hide resolved

pkg/cstor/volume/targetserver/server.go Outdated Show resolved Hide resolved

mittachaitu changed the title ~~[WIP]chore: (cstor-volume-mgmt) Add support to update cstorvolume status with known replicas~~ chore: (cstor-volume-mgmt) Add support to update cstorvolume status with known replicas Sep 26, 2019

This commit fixes the linting issues by fixmie

74d9b09

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

AmitKumarDas approved these changes Sep 27, 2019

View reviewed changes

This commit fixes the travis issue and fix one bug in earlier code

683f8fb

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

prateekpandey14 reviewed Sep 27, 2019

View reviewed changes

pkg/util/fileoperator.go Outdated Show resolved Hide resolved

prateekpandey14 reviewed Sep 27, 2019

View reviewed changes

pkg/util/fileoperator.go Show resolved Hide resolved

mittachaitu added 2 commits September 27, 2019 17:14

This commit updates the target conf file with Replcia details

8e854a3

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

This commit fixes connection closing connection

d83160d

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

ghost reviewed Sep 27, 2019

View reviewed changes

pkg/cstor/volume/targetserver/server.go Outdated Show resolved Hide resolved

vishnuitta reviewed Sep 28, 2019

View reviewed changes

pkg/cstor/volume/v1alpha1/cstorvolume.go Outdated Show resolved Hide resolved

vishnuitta reviewed Sep 28, 2019

View reviewed changes

pkg/cstor/volume/v1alpha1/cstorvolume.go Outdated Show resolved Hide resolved

vishnuitta reviewed Sep 28, 2019

View reviewed changes

pkg/cstor/volume/v1alpha1/cstorvolume.go Outdated Show resolved Hide resolved

mittachaitu added 2 commits September 30, 2019 13:46

This commit incorporates the review comments

6d27229

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

This commit fixes typo in comments

1cf9d3a

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

mynktl approved these changes Sep 30, 2019

View reviewed changes

This commit fixes old bug in GetVolumeStatus function

61c48a7

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

vishnuitta reviewed Sep 30, 2019

View reviewed changes

pkg/util/fileoperator.go Outdated Show resolved Hide resolved

vishnuitta reviewed Sep 30, 2019

View reviewed changes

pkg/util/fileoperator.go Outdated Show resolved Hide resolved

mittachaitu added 2 commits September 30, 2019 21:13

This commit updates bug fixes and incorporate review comments

228c03a

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

This commit updates the file handeling

576e4b4

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

mynktl reviewed Oct 1, 2019

View reviewed changes

This commit fixes the IsDRFPending check

e0f91dc

Signed-off-by: mittachaitu <sai.chaithanya@mayadata.io>

vishnuitta approved these changes Oct 1, 2019

View reviewed changes

vishnuitta merged commit 96c044c into openebs-archive:master Oct 1, 2019

mittachaitu mentioned this pull request Feb 25, 2020

feat(cvc-operator): add automatic scaling of volumereplicas for CSI volumes #1613

Merged

9 tasks

chore: (cstor-volume-mgmt) Add support to update cstorvolume status with known replicas #1447

chore: (cstor-volume-mgmt) Add support to update cstorvolume status with known replicas #1447

Conversation

mittachaitu commented Sep 19, 2019 • edited Loading

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

ghost left a comment

Choose a reason for hiding this comment

ghost 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mittachaitu Sep 30, 2019 • edited Loading

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

vishnuitta Sep 30, 2019 • edited Loading

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

vishnuitta left a comment

Choose a reason for hiding this comment

mittachaitu commented Sep 19, 2019 •

edited

Loading

mittachaitu Sep 30, 2019 •

edited

Loading

vishnuitta Sep 30, 2019 •

edited

Loading