issue-464:Added support for JvmOption customization #462

Prabhaker24 · 2020-10-12T13:37:35Z

Signed-off-by: prabhaker24 [email protected]

Change log description

This pr will enable the user to modify both controller and sgmentstore JVMOptions as well as options during runtime

Purpose of the change

Fixes #464

What the code does

The code looks for any change in the jvm option or option in either of controller or segmentstore and if there is a change i.e user has changed any of the jvmOptions or options or has added a new value, in that case, we update the configmap and restart each pod one by one.
I am doing validation as well and is done by the method validateConfigMap() which ensures user doesn't change any of the following values:-

controller.containerCount
controller.container.count
pravegaservice.containerCount
pravegaservice.container.count
bookkeeper.bkLedgerPath
bookkeeper.ledger.path
controller.retention.bucketCount
controller.retention.bucket.count
controller.watermarking.bucketCount
controller.watermarking.bucket.count
pravegaservice.dataLogImplementation
pravegaservice.dataLog.impl.name
pravegaservice.storageImplementation
pravegaservice.storage.impl.name
storageextra.storageNoOpMode
storageextra.noOp.mode.enable

How to verify it

Deploy Pravega cluster and update segmentstore JVM Option we should see that segmentstore configmap should get updated and each pod should restart sequentially.
Deploy Pravega cluster and update controller JVM Option we should see that controller configmap should get updated and each pod should restart sequentially.
Deploy Pravega cluster and update Option we should see that controller and segmentstore configmap should get updated and each of the controller and ss pod should restart sequentially.
Try changing any of the above 16 values that the user is not supposed to change and the operator should error saying that the parameter is not supposed to be changed.

Signed-off-by: prabhaker24 <[email protected]>

codecov-io · 2020-10-12T20:14:24Z

Codecov Report

Merging #462 into master will decrease coverage by 2.69%.
The diff coverage is 17.87%.

@@            Coverage Diff             @@
##           master     #462      +/-   ##
==========================================
- Coverage   75.29%   72.60%   -2.70%     
==========================================
  Files          15       15              
  Lines        3068     3285     +217     
==========================================
+ Hits         2310     2385      +75     
- Misses        669      803     +134     
- Partials       89       97       +8

Impacted Files	Coverage Δ
pkg/apis/pravega/v1beta1/pravegacluster_types.go	`26.20% <5.71%> (+3.46%)`	⬆️
...roller/pravegacluster/pravegacluster_controller.go	`50.45% <22.95%> (-10.46%)`	⬇️
pkg/controller/pravega/pravega_controller.go	`100.00% <100.00%> (ø)`
pkg/controller/pravega/pravega_segmentstore.go	`100.00% <100.00%> (ø)`
pkg/util/pravegacluster.go	`97.03% <100.00%> (+0.09%)`	⬆️

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 569dfb3...4a2632c. Read the comment docs.

pbelgundi · 2020-10-13T05:44:25Z

pkg/controller/pravegacluster/pravegacluster_controller.go

-	err = r.client.Create(context.TODO(), configMap)
-	if err != nil && !errors.IsAlreadyExists(err) {
-		return err
+	err = r.client.Get(context.TODO(), types.NamespacedName{Name: configMap.Name, Namespace: p.Namespace}, currentConfigMap)


This does not seem like a good place and a good way to check for updates to config map.
Please follow the logic used in zookeeper -operator:
https://github.com/pravega/zookeeper-operator/blob/master/pkg/controller/zookeepercluster/zookeepercluster_controller.go#L150

In p-operator we're checking very specific things in the reconcile loop, instead of checking for updates to each component and that is complicating the flow of code way too much...
Lets refactor and change it to the way it is done in ZK operator...

pbelgundi · 2020-10-13T05:45:06Z

pkg/controller/pravegacluster/pravegacluster_controller.go

@@ -329,6 +351,29 @@ func (r *ReconcilePravegaCluster) deployController(p *pravegav1beta1.PravegaClus
 	if err != nil && !errors.IsAlreadyExists(err) {
 		return err
 	}
+	if !eq {


Same comment adding too many ifs... complicates the flow of code.. I think its time to refactor the reconcile loop...

Signed-off-by: prabhaker24 <[email protected]>

pbelgundi

LGTM. Just make sure we add enough tests to cover this.

Signed-off-by: prabhaker24 <[email protected]>

anishakj · 2020-10-20T06:31:08Z

test/e2e/cmchanges_test.go

+	err = pravega_e2eutil.UpdatePravegaCluster(t, f, ctx, pravega)
+	g.Expect(err).NotTo(HaveOccurred())
+
+	//checking if the upgrade of options was successfull


Fix the typo successful

Signed-off-by: prabhaker24 <[email protected]>

anishakj

LGTM

added support for jvmOption customization

eda074a

Signed-off-by: prabhaker24 <[email protected]>

Prabhaker24 marked this pull request as ready for review October 12, 2020 13:39

Prabhaker24 requested review from anishakj, SrishT and pbelgundi October 12, 2020 13:40

prabhaker24 added 3 commits October 12, 2020 21:26

removed sleep

f7a1df1

Signed-off-by: prabhaker24 <[email protected]>

added timeout and made restart pod function modular as well

3d1e53c

Signed-off-by: prabhaker24 <[email protected]>

changed restart logic for controller pods

7ac6948

Signed-off-by: prabhaker24 <[email protected]>

pbelgundi requested changes Oct 13, 2020

View reviewed changes

added reconcileconfigmap and validation

92df785

Signed-off-by: prabhaker24 <[email protected]>

Prabhaker24 requested a review from Ranganaths8 October 16, 2020 10:06

Prabhaker24 changed the title ~~issue-316:Added support for JvmOption customization~~ issue-464:Added support for JvmOption customization Oct 16, 2020

ut for compareConfigMap added

a469512

Signed-off-by: prabhaker24 <[email protected]>

pbelgundi approved these changes Oct 16, 2020

View reviewed changes

prabhaker24 added 2 commits October 19, 2020 15:58

adding e2e for cm changes added

d29cd94

Signed-off-by: prabhaker24 <[email protected]>

more validation added

4a2632c

Signed-off-by: prabhaker24 <[email protected]>

anishakj reviewed Oct 20, 2020

View reviewed changes

fixed typo

991a6fe

Signed-off-by: prabhaker24 <[email protected]>

anishakj approved these changes Oct 20, 2020

View reviewed changes

anishakj merged commit 1c5f0ce into master Oct 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue-464:Added support for JvmOption customization #462

issue-464:Added support for JvmOption customization #462

Prabhaker24 commented Oct 12, 2020 •

edited

Loading

codecov-io commented Oct 12, 2020 •

edited

Loading

pbelgundi Oct 13, 2020

Prabhaker24 Oct 16, 2020

pbelgundi Oct 13, 2020

Prabhaker24 Oct 16, 2020

pbelgundi left a comment

anishakj Oct 20, 2020

Prabhaker24 Oct 20, 2020

anishakj left a comment

issue-464:Added support for JvmOption customization #462

issue-464:Added support for JvmOption customization #462

Conversation

Prabhaker24 commented Oct 12, 2020 • edited Loading

Change log description

Purpose of the change

What the code does

How to verify it

codecov-io commented Oct 12, 2020 • edited Loading

Codecov Report

pbelgundi Oct 13, 2020

Choose a reason for hiding this comment

Prabhaker24 Oct 16, 2020

Choose a reason for hiding this comment

pbelgundi Oct 13, 2020

Choose a reason for hiding this comment

Prabhaker24 Oct 16, 2020

Choose a reason for hiding this comment

pbelgundi left a comment

Choose a reason for hiding this comment

anishakj Oct 20, 2020

Choose a reason for hiding this comment

Prabhaker24 Oct 20, 2020

Choose a reason for hiding this comment

anishakj left a comment

Choose a reason for hiding this comment

Prabhaker24 commented Oct 12, 2020 •

edited

Loading

codecov-io commented Oct 12, 2020 •

edited

Loading