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

refactor(keel): use SpinnakerRetrofitErrorHandler with KeelService #4636

Merged
merged 4 commits into from
Feb 27, 2024

Conversation

Pranav-b-7
Copy link
Contributor

@Pranav-b-7 Pranav-b-7 commented Jan 22, 2024

This PR lays the foundational work for upgrading the retrofit version to 2.x, specifically focusing on refactoring the exception handling for KeelService.

Note, there's a behaviour change on the Task Results error message format when KeelService API throws any 4xx/5xx http errors with empty error body.

  • On any 4xx http errors with empty error body:

    before:

    11:56:19.324 [Test worker] ERROR com.netflix.spinnaker.orca.keel.task.ImportDeliveryConfigTask - {message=Non-retryable HTTP response 400 received from downstream service: HTTP 400 http://localhost:62130/delivery-configs/: 400 Bad Request}

    after:

    12:00:02.018 [Test worker] ERROR com.netflix.spinnaker.orca.keel.task.ImportDeliveryConfigTask - {message=Non-retryable HTTP response 400 received from downstream service: HTTP 400 http://localhost:62275/delivery-configs/: Status: 400, URL: http://localhost:62275/delivery-configs/, Message: Bad Request}

  • On any 5xx http errors with empty error body:

    before:

    TaskResult(status=RUNNING, context={repoType=stash, projectKey=SPKR, repositorySlug=keeldemo, directory=., manifest=spinnaker.yml, ref=refs/heads/master, attempt=2, maxRetries=5, errorFromLastAttempt=Retryable HTTP response 500 received from downstream service: HTTP 500 http://localhost:65311/delivery-configs/: 500 Server Error}, outputs={})

    after:

    TaskResult(status=RUNNING, context={repoType=stash, projectKey=SPKR, repositorySlug=keeldemo, directory=., manifest=spinnaker.yml, ref=refs/heads/master, attempt=1, maxRetries=5, errorFromLastAttempt=Retryable HTTP response 500 received from downstream service: HTTP 500 http://localhost:49862/delivery-configs/: Status: 500, URL: http://localhost:49862/delivery-configs/, Message: Server Error}, outputs={})

Note : The RetrofitError catch block will still be relevant even after the code changes in the below classes, due to the reasons mentioned inline:

  • ImportDeliveryConfigTask.kt - (IgorService retrofit client is not configured with SpinnakerRetrofitErrorHandler , so the APIs still throw RetrofitError)
  • DeleteApplicationTask.groovy - (Front50Service retrofit client is not configured with SpinnakerRetrofitErrorHandler, so the APIs still throw RetrofitError)

@Pranav-b-7 Pranav-b-7 marked this pull request as ready for review January 29, 2024 10:34
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-KeelService branch from 5599618 to 2c86134 Compare January 29, 2024 16:46
@Pranav-b-7 Pranav-b-7 marked this pull request as draft January 29, 2024 16:57
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-KeelService branch from 2c86134 to f6cef42 Compare January 30, 2024 13:33
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-KeelService branch 3 times, most recently from ad23108 to b0bde12 Compare February 1, 2024 11:34
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-KeelService branch 17 times, most recently from 5cff4f7 to 8f3521d Compare February 6, 2024 16:44
@Pranav-b-7 Pranav-b-7 marked this pull request as ready for review February 6, 2024 16:45
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-KeelService branch 9 times, most recently from 15b8fbc to 0fca8b2 Compare February 26, 2024 14:21
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-KeelService branch 6 times, most recently from 76ab1a0 to b85fb76 Compare February 27, 2024 11:21
across different time units when any 4xx http error has occured. These tests ensures the upcoming changes on KeelService with SpinnakerRetrofitErrorHandler, will not modify/break the existing functionality.

Tests covers positive and negative cases with different units of timestamps. In addition, another test added to verify when the error response body doesn't have timestamp field.
…errors and network errors

Test cases added to verify the upcoming changes when KeelService is configured with SpinnakerRetrofitErrorHandler.

These tests verifies the behaviour of the method handleRetryableFailures() when 4xx with empty error body and 5xx http erros are thrown, and also during network errors.

NOTE : These tests only verifies the scenario when the error response body is empty/null, and network errors.
@Pranav-b-7 Pranav-b-7 force-pushed the remove-retrofitError-KeelService branch from 6d9cea3 to 6320f01 Compare February 27, 2024 11:44
The deleted tests are the cases/scenarios which are already covered as part of the commit: 5d711ab.

These tests covers up the http 4xx error cases of the API : keelService.publishDeliveryConfig.

Tests deleted are : 1. 'keel access denied error', 2. 'delivery config parsing error'.
This PR lays the foundational work for upgrading the retrofit version to 2.x, specifically focusing on refactoring the exception handling for KeelService

The tests modified as part of this PR will verify the new changes with the scenarios:- Reading the Http error response body and building the TaskResult by instantiating SpringHttpError.

Note, there's a behaviour change on the Task Results error message format when KeelService API throws any 4xx/5xx http errors with empty error body.

- On any 4xx http errors with empty error body:

  before:

  11:56:19.324 [Test worker] ERROR com.netflix.spinnaker.orca.keel.task.ImportDeliveryConfigTask - {message=Non-retryable HTTP response 400 received from downstream service: HTTP 400 http://localhost:62130/delivery-configs/: 400 Bad Request}

  after:

  12:00:02.018 [Test worker] ERROR com.netflix.spinnaker.orca.keel.task.ImportDeliveryConfigTask - {message=Non-retryable HTTP response 400 received from downstream service: HTTP 400 http://localhost:62275/delivery-configs/: Status: 400, URL: http://localhost:62275/delivery-configs/, Message: Bad Request}

- On any 5xx http errors with empty error body:

  before:

  TaskResult(status=RUNNING, context={repoType=stash, projectKey=SPKR, repositorySlug=keeldemo, directory=., manifest=spinnaker.yml, ref=refs/heads/master, attempt=2, maxRetries=5, errorFromLastAttempt=Retryable HTTP response 500 received from downstream  service: HTTP 500 http://localhost:65311/delivery-configs/: 500 Server Error}, outputs={})

  after:

  TaskResult(status=RUNNING, context={repoType=stash, projectKey=SPKR, repositorySlug=keeldemo, directory=., manifest=spinnaker.yml, ref=refs/heads/master, attempt=1, maxRetries=5, errorFromLastAttempt=Retryable HTTP response 500 received from downstream  service: HTTP 500 http://localhost:49862/delivery-configs/: Status: 500, URL: http://localhost:49862/delivery-configs/, Message: Server Error}, outputs={})
@dbyron-sf dbyron-sf added the ready to merge Approved and ready for merge label Feb 27, 2024
@mergify mergify bot added the auto merged Merged automatically by a bot label Feb 27, 2024
@mergify mergify bot merged commit ca3db12 into spinnaker:master Feb 27, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto merged Merged automatically by a bot ready to merge Approved and ready for merge target-release/1.34
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants