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

Add replication factor as env to jiva controller #388

Merged
merged 2 commits into from
Jul 12, 2018
Merged

Add replication factor as env to jiva controller #388

merged 2 commits into from
Jul 12, 2018

Conversation

kmova
Copy link
Contributor

@kmova kmova commented Jul 12, 2018

Signed-off-by: kmova [email protected]

What this PR does / why we need it:
The jiva controller needs to determine whether to have the volume as read-only/read-write based on the quorum from the replicas. When the count is not available (in 0.5.x), controller was depending on the registration process to determine the number of replicas that are connected. This auto-registration process leads to several race conditions for determining - whether the volume will be launched with a single replica or multiple replicas. By passing this replication factor, controller will handle the cases for multiple replica's better.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:
Related to : openebs-archive/jiva#83

@kmova
Copy link
Contributor Author

kmova commented Jul 12, 2018

The controller spec generated with the fix is as follows:

spec:
  containers:
  - args:
    - controller
    - --frontend
    - gotgt
    - --clusterIP
    - 10.47.255.236
    - pvc-9e6c7860-85b9-11e8-9ef7-42010a800016
    command:
    - launch
    env:
    - name: REPLICATION_FACTOR
      value: "1"
    image: openebs/jiva:0.6.0-RC3
    imagePullPolicy: IfNotPresent
    name: pvc-9e6c7860-85b9-11e8-9ef7-42010a800016-ctrl-con
    ports:
    - containerPort: 3260
      protocol: TCP
    - containerPort: 9501
      protocol: TCP
    resources:
      requests:
        cpu: 100m
    terminationMessagePath: /dev/termination-log
    terminationMessagePolicy: File
    volumeMounts:
    - mountPath: /var/run/secrets/kubernetes.io/serviceaccount
      name: default-token-nr792
      readOnly: true

// - if replication factor is 1, volume will be marked as RW when one replica connects
// - if replication factor is 3, volume will be marked as RW when when atleast 2 replica connect
// Similar logic will be applied to turn the volume into RO, when qorum is lost.
//Note: When kubectl scale up/down is done, this ENV needs to be patched.

Choose a reason for hiding this comment

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

When kubectl scale up/down is done to the Replica deployment, this ENV i.e. Controller deployment needs to be patched.

@codecov
Copy link

codecov bot commented Jul 12, 2018

Codecov Report

Merging #388 into master will decrease coverage by 0.04%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #388      +/-   ##
==========================================
- Coverage   29.37%   29.33%   -0.05%     
==========================================
  Files          65       65              
  Lines        5514     5523       +9     
==========================================
  Hits         1620     1620              
- Misses       3720     3729       +9     
  Partials      174      174
Impacted Files Coverage Δ
orchprovider/k8s/v1/k8s.go 34.4% <0%> (-0.36%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8acd08b...659c6c0. Read the comment docs.

Copy link
Contributor

@prateekpandey14 prateekpandey14 left a comment

Choose a reason for hiding this comment

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

lgtm

@prateekpandey14 prateekpandey14 merged commit 58254da into openebs-archive:master Jul 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants