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

aws health checks #78

Merged
merged 1 commit into from
Apr 17, 2024
Merged

aws health checks #78

merged 1 commit into from
Apr 17, 2024

Conversation

philbrookes
Copy link
Collaborator

First pass at AWS health checks in DNS Operator.

Some uses-cases not fully covered, and no smart backoff, or e2e tests yet.

Related PR for kuadrant-operator:
Kuadrant/kuadrant-operator#543


address, _ := provider.GetEndpointAddress(endpoint)

if exists {
Copy link
Collaborator

Choose a reason for hiding this comment

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

when we update / create the health check we should add this call to a metric so we can track the number of calls to the provider. I believe @makslion is adding a metric like that

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess the client might be the place to do that?

Copy link
Collaborator

@maleck13 maleck13 left a comment

Choose a reason for hiding this comment

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

few questions and comments. I realise there is some pressure to get this merged. But lets see what tests we can get in over the next day or so WDYT?

@philbrookes philbrookes force-pushed the gh-44 branch 2 times, most recently from e06adf4 to 9b702a8 Compare April 16, 2024 07:58
@philbrookes philbrookes force-pushed the gh-44 branch 2 times, most recently from ed0f1d1 to d749d02 Compare April 16, 2024 08:04
@philbrookes philbrookes force-pushed the gh-44 branch 7 times, most recently from 2112bb9 to adc6274 Compare April 17, 2024 07:50
@philbrookes philbrookes force-pushed the gh-44 branch 2 times, most recently from 3bdbb30 to 314fb25 Compare April 17, 2024 08:42
Copy link
Collaborator

@maleck13 maleck13 left a comment

Choose a reason for hiding this comment

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

There being not tests here is really not ideal. We need to make sure we get these in place @philbrookes can you link to an issue

@mikenairn In order to unblock moving kuadarant operator back to main of dns operator, I think we can make an exception for this knowing the focus will be on adding tests before we do the next release of DNS operator?

@philbrookes
Copy link
Collaborator Author

There being not tests here is really not ideal. We need to make sure we get these in place @philbrookes can you link to an issue

@mikenairn In order to unblock moving kuadarant operator back to main of dns operator, I think we can make an exception for this knowing the focus will be on adding tests before we do the next release of DNS operator?

My intention was to keep this issue open until the tests are added and the logic is firmed up more. #44

@maleck13 maleck13 added this pull request to the merge queue Apr 17, 2024
@maleck13 maleck13 removed this pull request from the merge queue due to a manual request Apr 17, 2024
@maleck13 maleck13 added this pull request to the merge queue Apr 17, 2024
Merged via the queue into Kuadrant:main with commit bc470c4 Apr 17, 2024
9 checks passed
mikenairn added a commit to mikenairn/dns-operator that referenced this pull request Apr 22, 2024
When we are deleting a DNSRecord we just ignore all errors relating to
the ManagedZone being missing since there is nothing we can do about it,
and we don't want the record staying around forever.

Check was missing for health checks
Kuadrant#78
philbrookes pushed a commit to philbrookes/dns-operator that referenced this pull request May 9, 2024
When we are deleting a DNSRecord we just ignore all errors relating to
the ManagedZone being missing since there is nothing we can do about it,
and we don't want the record staying around forever.

Check was missing for health checks
Kuadrant#78
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