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

RHIROS-1411 Update recommendations job #149

Conversation

saltgen
Copy link
Contributor

@saltgen saltgen commented Nov 3, 2023

Job Steps

  1. Checks if /updateRecommendations is up every 2 seconds with 10 attempts
  2. Goes through every workload with missing recommendation and submits the respective workload.ExperimentName and workload.MetricsUploadAt
  3. Saves recommendation and historical recommendation
  4. Updates a Prometheus counter when invalid recommendations arrive from job execution

@saltgen saltgen marked this pull request as draft November 3, 2023 09:41
@saltgen saltgen force-pushed the job/update-recommendations-kruize-v20 branch from bc83bde to 549f0e0 Compare November 3, 2023 09:54
@saltgen saltgen marked this pull request as ready for review November 3, 2023 09:54
@saltgen saltgen force-pushed the job/update-recommendations-kruize-v20 branch from 549f0e0 to e36037c Compare November 3, 2023 12:14
Copy link
Collaborator

@patilsuraj767 patilsuraj767 left a comment

Choose a reason for hiding this comment

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

Hello @saltgen code indentation in the PR is messed up can you please check.

func checkURLStatus() bool {

url := cfg.KruizeUrl + "/updateRecommendations"
resp, err := http.Get(url)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Have you tested this? I think kruize does not support GET method for /updateRecommendations API. So you will never get 200 response in return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't want to repeat my testing cycles, additionally most of the code here is re-used. Hence the Do Not Merge label.
Now that the new Kruize image is available, I'll get these minor aspects checked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change pushed

panic(err.Error())
}

if !reflect.ValueOf(recommendationSets).IsZero() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain why there is a need to this check over here? and anyway we will be running this job after truncating the recommendationSets table that means this will every time evaluate to False and else block will be executed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • The exclamation symbol is more of a typo, I will revert that change.
  • This check has been added to make the job idempotent, so that we can use this job in future if required.
  • The recommendationSets table will be empty yes, but this job will look for recommendations for existing workloads, and then update those specifically

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change pushed

@upadhyeammit
Copy link
Contributor

Instead of checking every 2 seconds for 10 attempts can we do: https://aws.amazon.com/blogs/architecture/exponential-backoff-and-jitter/

@saltgen
Copy link
Contributor Author

saltgen commented Nov 8, 2023

Hello @saltgen code indentation in the PR is messed up can you please check.

I have no clue why this happens, locally formatting seems fine, will check.

@saltgen saltgen force-pushed the job/update-recommendations-kruize-v20 branch from e36037c to fa8df81 Compare November 8, 2023 14:08
@saltgen saltgen force-pushed the job/update-recommendations-kruize-v20 branch from fa8df81 to 3fe8b15 Compare November 8, 2023 14:38
@saltgen saltgen requested a review from patilsuraj767 November 8, 2023 14:39
@anurag03
Copy link

/retest

@saltgen
Copy link
Contributor Author

saltgen commented Feb 4, 2025

Closing out this PR as stale

@saltgen saltgen closed this Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants