Pb-4844: Changes in stork for it to enable debug level log messages for kopia operations #1577

kgarg-px · 2023-11-27T11:15:19Z

What type of PR is this?

improvement

What this PR does / why we need it:
This PR introduces a change where we can access debug level logs for kopia operations. We need the this, so that in future whenever required, we could use it to view logs in debug level.

Does this PR change a user-facing CRD or CLI?:

Is a release note needed?:

Issue:
User Impact:
Resolution

Does this change need to be cherry-picked to a release branch?:

Link to PX-Backup PR related to this particular PR is: https://github.com/portworx/px-backup/pull/1407
Link to KDMP PR related to this particular PR is: portworx/kdmp#324

Attaching some screenshots for the reference:

cnbu-jenkins · 2023-11-27T11:19:06Z

Can one of the admins verify this patch?

vsundarraj-px · 2023-11-28T06:12:27Z

drivers/volume/kdmp/kdmp.go

+	kdmpData, err := core.Instance().GetConfigMap(stork_driver.KdmpConfigmapName, stork_driver.KdmpConfigmapNamespace)
+	if err != nil {
+		logrus.Info("enableKopiaExecutorDebugMode unable to read the cm")
+		logrus.Tracef("error readig kdmp config map: %v", err)


nit: Typo on readig

Going to change it to, logrus.warnf and thinking of adding cm name as well, would that be better? @vsundarraj-px

vsundarraj-px · 2023-11-28T06:14:56Z

drivers/volume/kdmp/kdmp.go

@@ -371,6 +375,24 @@ func (k *kdmp) GetBackupStatus(backup *storkapi.ApplicationBackup) ([]*storkapi.
 	}
 	return volumeInfos, nil
 }
+
+// Check whether to run kopia executor in debug mode


If its check function, then the function name is missleading. enableKopiaExecutorDebugMode() means enabling it not checking.
Suggestion: IsKopiaExecutorDebugModeEnabled()

vsundarraj-px · 2023-11-28T06:15:33Z

drivers/volume/kdmp/kdmp.go

+
+// Check whether to run kopia executor in debug mode
+func enableKopiaExecutorDebugMode() bool {
+	logrus.Info("enableKopiaExecutorDebugMode entering")


This is ok while developing but not for production, suggestion to remove this info message.

vsundarraj-px · 2023-11-28T06:22:51Z

drivers/volume/kdmp/kdmp.go

@@ -765,6 +787,12 @@ func (k *kdmp) StartRestore(
 		dataExport.Annotations[utils.SkipResourceAnnotation] = "true"
 		dataExport.Annotations[utils.BackupObjectUIDKey] = backupUID
 		dataExport.Annotations[pvcUIDKey] = bkpvInfo.PersistentVolumeClaimUID
+		logrus.Info("enableKopiaExecutorDebugMode calling")
+		if enableKopiaExecutorDebugMode() {
+			logrus.Info("enableKopiaExecutorDebugMode able to find the debug mode present in kdmp-config cm")


Redundant Info log. Its already getting printed in check function.. hence this should be removed to reduce logfile payload.

Yup, sorry about this, I forgot to remove log messages while raising the PR

vsundarraj-px · 2023-11-28T06:23:58Z

drivers/volume/kdmp/kdmp.go

+		if enableKopiaExecutorDebugMode() {
+			logrus.Info("enableKopiaExecutorDebugMode able to find the debug mode present in kdmp-config cm")
+			dataExport.Annotations[utils.KopiaDebugModeEnabled] = "true"
+			logrus.Info("enableKopiaDebugMode annotation added in the CR")


Same as above, if you are already checking checkFunction you can avoid this. Or you can remove the log.info in checkfunction() if you are printing here.. Too much of log messages will create high logfile payload.

vsundarraj-px · 2023-11-28T06:24:41Z

pkg/applicationmanager/controllers/applicationrestore.go

@@ -361,6 +361,7 @@ func (a *ApplicationRestoreController) handle(ctx context.Context, restore *stor
 		}
 	}

+	logrus.Infof("StorageClassMapping restore spec storage class mapping is: %v", restore.Spec.StorageClassMapping)


Why is this log added in this pR? Merge conflicts?

There was a case when while restoring, storage class was not being taken in destination cluster by default, had used this log to debug and find out the issue, I forgot to remove the all the log messages that I had used while developing. Will be removing these. Thanks for pointing them out!

vsundarraj-px

LGTM

vsundarraj-px · 2023-12-07T10:22:38Z

You need resolve PR checks.

prashanthpx · 2023-12-12T04:45:53Z

@kgarg-px Indeed we need not do this way as reconciler code in kdmp does read kdmp-config value (for eg check KopiaResourceRequirements) where we can set kdmp job resoruces on the fly.
We should be following the same with which, we won't need any stork change except vendoring the code to stork

codecov · 2023-12-12T08:45:57Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0e0a41c) 67.05% compared to head (741a94b) 67.03%.

❗ Current head 741a94b differs from pull request most recent head c3c469c. Consider uploading reports for the commit c3c469c to get more accurate results

Additional details and impacted files

@@               Coverage Diff                @@
##           epic-pb-4519    #1577      +/-   ##
================================================
- Coverage         67.05%   67.03%   -0.02%     
================================================
  Files                43       43              
  Lines              5585     5585              
================================================
- Hits               3745     3744       -1     
- Misses             1501     1502       +1     
  Partials            339      339

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: kgarg-px <[email protected]>

kgarg-px changed the title ~~Pb 4844~~ Pb-4844: Changes in stork for it to enable debug level log messages for kopia operations Nov 27, 2023

vsundarraj-px requested changes Nov 28, 2023

View reviewed changes

kgarg-px force-pushed the pb-4844 branch 2 times, most recently from 98f87f1 to 5d7bccd Compare December 1, 2023 08:42

kgarg-px force-pushed the pb-4844 branch 2 times, most recently from c0c241a to 45d245e Compare December 6, 2023 07:39

vsundarraj-px approved these changes Dec 7, 2023

View reviewed changes

kgarg-px force-pushed the pb-4844 branch 3 times, most recently from 9b5ffbb to 0cdf4a8 Compare December 10, 2023 17:32

kgarg-px force-pushed the pb-4844 branch 5 times, most recently from 741a94b to c3c469c Compare December 18, 2023 14:18

PB-4844: Vendoring changes from kdmp

ebcfc45

Signed-off-by: kgarg-px <[email protected]>

kgarg-px force-pushed the pb-4844 branch from c3c469c to ebcfc45 Compare December 18, 2023 16:59

kgarg-px merged commit e7fb9bc into epic-pb-4519 Dec 18, 2023
1 of 5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pb-4844: Changes in stork for it to enable debug level log messages for kopia operations #1577

Pb-4844: Changes in stork for it to enable debug level log messages for kopia operations #1577

kgarg-px commented Nov 27, 2023 •

edited

Loading

cnbu-jenkins commented Nov 27, 2023

vsundarraj-px Nov 28, 2023

kgarg-px Nov 28, 2023

vsundarraj-px Nov 28, 2023

vsundarraj-px Nov 28, 2023

vsundarraj-px Nov 28, 2023

kgarg-px Nov 28, 2023

vsundarraj-px Nov 28, 2023

vsundarraj-px Nov 28, 2023

kgarg-px Nov 28, 2023

vsundarraj-px left a comment

vsundarraj-px commented Dec 7, 2023

prashanthpx commented Dec 12, 2023

codecov bot commented Dec 12, 2023 •

edited

Loading

Pb-4844: Changes in stork for it to enable debug level log messages for kopia operations #1577

Pb-4844: Changes in stork for it to enable debug level log messages for kopia operations #1577

Conversation

kgarg-px commented Nov 27, 2023 • edited Loading

cnbu-jenkins commented Nov 27, 2023

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

vsundarraj-px left a comment

Choose a reason for hiding this comment

vsundarraj-px commented Dec 7, 2023

prashanthpx commented Dec 12, 2023

codecov bot commented Dec 12, 2023 • edited Loading

Codecov Report

kgarg-px commented Nov 27, 2023 •

edited

Loading

codecov bot commented Dec 12, 2023 •

edited

Loading