3. Adding Recommendation Updater Abstraction and VPA Updater [KRUIZE-VPA Integration] #1412

shekhar316 · 2024-12-05T20:52:14Z

Description

This PR introduces a basic abstraction for the RecommendationUpdater.

The RecommendationUpdater interface is created, with a generic implementation provided by RecommendationUpdaterImpl.
The VpaUpdaterImpl extends RecommendationUpdaterImpl to offer a VPA-specific implementation for updating resource recommendations.
Additionally, the necessary constants and exceptions are included to support the functionality.
This abstraction lays the foundation for adding more specific updater implementations in the future.

Type of change

Bug fix
New feature
Docs update
Breaking change (What changes might users need to make in their application due to this PR?)
Requires DB changes

How has this been tested?

Tested on both the ResourceHub cluster and the Kind cluster to verify any issues.

Test Configuration

Kubernetes clusters tested on: ResourceHub (OpenShift), Kind

Checklist 🎯

Additional information

This PR is based on PR#1411. Please merge PR#1411 before this one.

Signed-off-by: Shekhar Saxena <[email protected]>

dinogun · 2024-12-10T13:35:57Z

@bharathappali @msvinaykumar Can you please review this PR, thanks

src/main/java/com/autotune/analyzer/recommendations/updater/vpa/VpaUpdaterImpl.java

Signed-off-by: Shekhar Saxena <[email protected]>

shekhar316 · 2024-12-11T13:48:43Z

Hi @bharathappali ,

I’ve pushed the changes incorporating the modifications and suggestions you provided. Thank you for your review and valuable feedback. Could you please review it again?

Thank you!

Signed-off-by: Shekhar Saxena <[email protected]>

bharathappali

LGTM!

dinogun · 2024-12-13T10:54:43Z

src/main/java/com/autotune/analyzer/recommendations/updater/vpa/VpaUpdaterImpl.java

+        try {
+            // checks if updater is installed or not
+            if (isUpdaterInstalled()) {
+                LOGGER.info(String.format(AnalyzerConstants.RecommendationUpdaterConstants.InfoMsgs.CREATEING_VPA, kruizeObject.getExperimentName()));


This might create too much logging if we are apply VPA for every object in the cluster. I'd suggest make it LOGGER.debug. Only error's should be logged by default

Agree. I've updated the info messages to debug messages which are experiment specific. Thanks for pointing out.

dinogun · 2024-12-13T10:55:16Z

src/main/java/com/autotune/analyzer/recommendations/updater/vpa/VpaUpdaterImpl.java

+                    LOGGER.info(String.format(AnalyzerConstants.RecommendationUpdaterConstants.InfoMsgs.RECOMMENDATION_VALUE,
+                            AnalyzerConstants.RecommendationItem.CPU, containerName, cpuRecommendationValue));
+                    LOGGER.info(String.format(AnalyzerConstants.RecommendationUpdaterConstants.InfoMsgs.RECOMMENDATION_VALUE,
+                            AnalyzerConstants.RecommendationItem.MEMORY, containerName, memoryRecommendationValue));


LOGGER.debug

Updated to use debug messages.

dinogun · 2024-12-13T10:56:34Z

src/main/java/com/autotune/analyzer/recommendations/updater/vpa/VpaUpdaterImpl.java

+                LOGGER.info(String.format(AnalyzerConstants.RecommendationUpdaterConstants.InfoMsgs.CHECKING_IF_VPA_PRESENT, vpaName));
+                NamespacedVerticalPodAutoscalerClient client = new DefaultVerticalPodAutoscalerClient();
+                VerticalPodAutoscalerList vpas = client.v1().verticalpodautoscalers().inAnyNamespace().list();
+
+                if (null != vpas && null != vpas.getItems() && !vpas.getItems().isEmpty()) {
+                    for (VerticalPodAutoscaler vpa : vpas.getItems()) {
+                        if (vpaName.equals(vpa.getMetadata().getName())) {
+                            LOGGER.info(String.format(AnalyzerConstants.RecommendationUpdaterConstants.InfoMsgs.VPA_WITH_NAME_FOUND, vpaName));
+                            return vpa;
+                        }
+                    }
+                }
+                LOGGER.info(String.format(AnalyzerConstants.RecommendationUpdaterConstants.InfoMsgs.VPA_WITH_NAME_NOT_FOUND, vpaName));


This is called while applying recommendations right? Please use LOGGER.debug everywhere

Yes, as we discussed, it will verify before applying the recommendations or before creating the vpa objects.

dinogun · 2024-12-13T10:58:24Z

src/main/java/com/autotune/analyzer/recommendations/updater/vpa/VpaUpdaterImpl.java

+                        }
+                    }
+                }
+                LOGGER.info(String.format(AnalyzerConstants.RecommendationUpdaterConstants.InfoMsgs.VPA_WITH_NAME_NOT_FOUND, vpaName));


This should be LOGGER.error right? Others should be debug

Updated this one to error message.

dinogun · 2024-12-13T11:00:31Z

src/main/java/com/autotune/analyzer/recommendations/updater/vpa/VpaUpdaterImpl.java

+    @Override
+    public boolean isUpdaterInstalled() {
+        try {
+            LOGGER.info(AnalyzerConstants.RecommendationUpdaterConstants.InfoMsgs.CHECKING_IF_UPDATER_INSTALLED,


General rule to follow, only log it as info if it is a rare occurence, eg once per kruize start. All other items which are on a per experiment basis should only be logged as debug unless there is a justification

Thanks for pointing it out @dinogun! I’ve updated the info logs to debug messages and ensured that only error messages are logged by default. I’ll ensure this in future implementations also.

Signed-off-by: Shekhar Saxena <[email protected]>

dinogun

LGTM

shekhar316 added the remote_monitoring label Dec 5, 2024

shekhar316 added this to the Kruize 0.3 Release milestone Dec 5, 2024

shekhar316 requested a review from dinogun December 5, 2024 20:52

shekhar316 self-assigned this Dec 5, 2024

adding basic abstraction for updaters

9e1d5e0

Signed-off-by: Shekhar Saxena <[email protected]>

shekhar316 force-pushed the adding-updater-abstraction branch from a46287c to 9e1d5e0 Compare December 6, 2024 08:49

shekhar316 added 3 commits December 9, 2024 23:34

Merge branch 'mvp_demo' into adding-updater-abstraction

9bdc3fb

adding VPA specific implementations

d96fe64

Signed-off-by: Shekhar Saxena <[email protected]>

updating vpa recommender key

e037b3b

Signed-off-by: Shekhar Saxena <[email protected]>

shekhar316 changed the title ~~3. Adding Recommendation Updater Abstraction and VPA Updater [KRUIZE-VPA Integration]~~ 3. [Testing - In Progress] Adding Recommendation Updater Abstraction and VPA Updater [KRUIZE-VPA Integration] Dec 10, 2024

updating logs

c64090f

Signed-off-by: Shekhar Saxena <[email protected]>

shekhar316 changed the title ~~3. [Testing - In Progress] Adding Recommendation Updater Abstraction and VPA Updater [KRUIZE-VPA Integration]~~ 3. Adding Recommendation Updater Abstraction and VPA Updater [KRUIZE-VPA Integration] Dec 10, 2024

dinogun requested review from bharathappali and msvinaykumar December 10, 2024 13:35

bharathappali requested changes Dec 11, 2024

View reviewed changes

addressing review comments

0c1d6bc

Signed-off-by: Shekhar Saxena <[email protected]>

shekhar316 requested a review from bharathappali December 11, 2024 13:46

shekhar316 added 2 commits December 12, 2024 12:35

replacing sync func to sync block

f837746

Signed-off-by: Shekhar Saxena <[email protected]>

removing sync from func

5bb1b8a

Signed-off-by: Shekhar Saxena <[email protected]>

bharathappali approved these changes Dec 12, 2024

View reviewed changes

dinogun reviewed Dec 13, 2024

View reviewed changes

reducing logging events

223b155

Signed-off-by: Shekhar Saxena <[email protected]>

shekhar316 requested a review from dinogun December 13, 2024 13:06

dinogun approved these changes Dec 13, 2024

View reviewed changes

dinogun merged commit da8ae97 into kruize:mvp_demo Dec 13, 2024
1 of 2 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3. Adding Recommendation Updater Abstraction and VPA Updater [KRUIZE-VPA Integration] #1412

3. Adding Recommendation Updater Abstraction and VPA Updater [KRUIZE-VPA Integration] #1412

shekhar316 commented Dec 5, 2024

dinogun commented Dec 10, 2024

shekhar316 commented Dec 11, 2024

bharathappali left a comment

dinogun Dec 13, 2024

shekhar316 Dec 13, 2024

dinogun Dec 13, 2024

shekhar316 Dec 13, 2024

dinogun Dec 13, 2024

shekhar316 Dec 13, 2024

dinogun Dec 13, 2024

shekhar316 Dec 13, 2024

dinogun Dec 13, 2024

shekhar316 Dec 13, 2024

dinogun left a comment

3. Adding Recommendation Updater Abstraction and VPA Updater [KRUIZE-VPA Integration] #1412

3. Adding Recommendation Updater Abstraction and VPA Updater [KRUIZE-VPA Integration] #1412

Conversation

shekhar316 commented Dec 5, 2024

Description

Type of change

How has this been tested?

Checklist 🎯

Additional information

dinogun commented Dec 10, 2024

shekhar316 commented Dec 11, 2024

bharathappali 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

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

dinogun left a comment

Choose a reason for hiding this comment