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

adds basic database access test to ensure connectivity #118

Merged
merged 1 commit into from
May 10, 2024

Conversation

tonytheleg
Copy link
Collaborator

What this PR does

With the velocity of changes happening to this repo and getting development processes updated and defined, it seemed adding a very basic db connectivity test to frontend was worth it. This way folks working on frontend updates, who need to test database CRUD transactions, can at least confirm connectivity is there and functioning by reviewing start up logs.

  • Adds database.go which in the future will likely get more use, but for now its just to house a very basic check
  • Adds DBConnectionTest on startup in main
  • Adds env vars to container for defining the DB Name and URL
  • Updates makefile to automatically set DB values in oc process command, even if no DB has been deployed
  • Also updates makefile to remove unnecessary calls to Azure to set values when deleting the frontend deployment

Jira:
Link to demo recording:

Special notes for your reviewer

# When a svc cluster is created with no Cosmos DB (Make target sets DB_NAME to "none"
$ kc logs aro-hcp-frontend-6c4cd6f968-k9gkp
{"time":"2024-05-09T15:22:01.8717461Z","level":"INFO","msg":"ARO HCP Frontend (unknown) started"}
{"time":"2024-05-09T15:22:01.872313306Z","level":"INFO","msg":"Testing DB Access"}
{"time":"2024-05-09T15:22:01.87264431Z","level":"INFO","msg":"Database check completed -- No database configured, skipping"}
{"time":"2024-05-09T15:22:01.873324517Z","level":"INFO","msg":"listening on 0.0.0.0:8443"}

# When a svc cluster is created with a Cosmos DB (Make target sets DB_NAME to actual DB name pulled from Azure
$ kc logs aro-hcp-frontend-7c85b9dd84-f2gkk
{"time":"2024-05-09T13:33:48.871441027Z","level":"INFO","msg":"ARO HCP Frontend (unknown) started"}
{"time":"2024-05-09T13:33:48.871912632Z","level":"INFO","msg":"Testing DB Access"}
{"time":"2024-05-09T13:33:49.666258443Z","level":"INFO","msg":"Database check completed -- {\"id\":\"aro-hcp-svc-cluster-anatale-rp-cosmos\",\"_rid\":\"a-A0AA==\",\"_etag\":\"\\\"00004f04-0000-0100-0000-663bce280000\\\"\",\"_self\":\"dbs/a-A0AA==/\",\"_ts\":1715195432}"}
{"time":"2024-05-09T13:33:49.666712648Z","level":"INFO","msg":"listening on 0.0.0.0:8443"}

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

  • PR: The PR description is expressive enough and will help future contributors
  • Code: Write code that humans can understand and Keep it simple
  • Refactor: You have left the code cleaner than you found it (Boy Scout Rule)
  • Upgrade: Impact of this change on upgrade flows was considered and addressed if required
  • Deployment: The deployment process was considered and addressed if required
  • Testing: New code requires new unit tests.
  • Documentation: Is the documentation updated? Either in the doc located in focus area, in the README or in the code itself.
  • Customers: Is this change affecting customers? Is the release plan considered?

frontend/main.go Outdated Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this functionality be in a /healthz or /readyz endpoint instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

are we talking about leveraging the existing healthz/ready endpoing for liveness/readiness? Or is this a different implementation? I only ask because it seems like hitting the DB on every probe seems a bit much but agree long term it should be checked. Is this maybe future state when we look into more definitive implementation (see thread about circuit breaker)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, whichever endpoint (readyz makes sense to me) that is configured as a pod readinessProbe eventually. We wouldn't want to send requests to the frontend if it isn't able to connect to the database yet and the period/failure threshold for the probe is tunable. I'm ok to implement it later

frontend/Makefile Outdated Show resolved Hide resolved
Copy link
Contributor

@s-amann s-amann left a comment

Choose a reason for hiding this comment

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

tested locally, which worked, but had to update the deploy make target to use the "resource group" as the "deployment name" when setting FRONTEND_MI_CLIENT_ID.

@tonytheleg tonytheleg force-pushed the ARO-6379-frontend-updates branch 2 times, most recently from 75d0a3d to cff6a37 Compare May 9, 2024 20:14
@tonytheleg tonytheleg force-pushed the ARO-6379-frontend-updates branch from cff6a37 to 3892904 Compare May 9, 2024 20:15
@s-amann s-amann merged commit 3e49dc2 into Azure:main May 10, 2024
5 checks passed
@tonytheleg tonytheleg deleted the ARO-6379-frontend-updates branch May 21, 2024 19:23
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.

4 participants