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

Add a endpoint based sync point after scaling up the pods in import step #14

Merged

Conversation

sergenyalcin
Copy link
Member

@sergenyalcin sergenyalcin commented Apr 25, 2024

Description of your changes

Follow-up of #13. We observed some issues on pod-based sync point for import step in provider-gcp tests. This PR adds an endpoint-based sync point after scaling up the pods in import step.

  • Checks the port field of Endpoints object are not empty.
  • Retry kubectl patch steps.

I have:

  • Run make reviewable test to ensure this PR is ready for review.

How has this code been tested

Tested locally.

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
@sergenyalcin sergenyalcin force-pushed the change-sync-point-to-svc-endpoints branch from 9d3d078 to 3e1eca3 Compare April 25, 2024 12:38
fi
done

if ${KUBECTL} get managed ; then
Copy link
Collaborator

@ulucinar ulucinar Apr 26, 2024

Choose a reason for hiding this comment

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

The API conversion functions (invoked by the conversion webhooks) may not kick in here, if the CRD API storage version is the highest priority version, e.g., when the available versions are v1beta1 and v1beta2 and the storage version is v1beta2.

How about parameterizing this function with the arguments to be passed to kubectl and retry the actual command for a finite count till it succeeds?

Looks like we should also be able to utilize a DeploymentRuntimeConfig to introduce a readiness probe for the webhook service. This together with the check on the endpoints can be an alternative.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added another script to retry the patch step.

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
@sergenyalcin sergenyalcin force-pushed the change-sync-point-to-svc-endpoints branch from b67322a to 840f4e7 Compare May 6, 2024 10:47
@sergenyalcin sergenyalcin requested a review from ulucinar May 6, 2024 13:46
Copy link
Collaborator

@ulucinar ulucinar left a comment

Choose a reason for hiding this comment

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

Thanks @sergenyalcin, lgtm.

- script: |
#!/bin/bash

function check_endpoints {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't it possible to define a generic kubectl_retry function that will retry the passed kubectl command together with its arguments and call it for kubectl get endpoints & kubectl patch?

Signed-off-by: Sergen Yalçın <yalcinsergen97@gmail.com>
@sergenyalcin sergenyalcin force-pushed the change-sync-point-to-svc-endpoints branch from 3b4b1b0 to 701c15b Compare May 7, 2024 18:42
@sergenyalcin sergenyalcin merged commit ac371c9 into crossplane:main May 7, 2024
6 checks passed
@sergenyalcin sergenyalcin deleted the change-sync-point-to-svc-endpoints branch May 7, 2024 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants