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

Decoupling Daemon External Port from healthcheck script #16649

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

SanabriaRusso
Copy link
Collaborator

@SanabriaRusso SanabriaRusso commented Feb 20, 2025

Explain your changes:
This PR avoids using a static port for reaching the daemon's graphql endpoint. It defines an global variable for all the Functions that inherits the value from the environment, or sets it to the previously used password otherwise (i.e., 3085).

Explain how you tested your changes:
I expect to produce an artifact and check the corresponding readinessProbe in a Kubernetes deployment completes successfully.

UPDATE: Deployed a pod for devnet and confirmed two important things:

  1. Node fails the ReadinessProbe at startup
  2. Node passes ReadinessProbe ONLY when state returned by daemon's graphql endpoint is SYNCED.

Checklist:

  • [*] Dependency versions are unchanged
    • Notify Velocity team if dependencies must change in CI
  • [*] Document code purpose, how to use it
    • Mention expected invariants, implicit constraints
  • All tests pass (CI will check this if you didn't)

@SanabriaRusso SanabriaRusso self-assigned this Feb 20, 2025
@SanabriaRusso SanabriaRusso requested a review from a team as a code owner February 20, 2025 16:31
@SanabriaRusso
Copy link
Collaborator Author

!ci-build-me

@SanabriaRusso
Copy link
Collaborator Author

SanabriaRusso commented Feb 21, 2025

Copy link
Member

@dkijania dkijania left a comment

Choose a reason for hiding this comment

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

Approved with small nit


function updateSyncStatusLabel() {
status=$(
curl --silent --show-error --header "Content-Type:application/json" -d'{ "query": "query { syncStatus } " }' localhost:3085/graphql | \
curl --silent --show-error \
Copy link
Member

Choose a reason for hiding this comment

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

NIT: looks like there is a potential to introduce single function and inject query:

function sendQraphqlQuery() {
  local __query="$1"

  echo
        curl --silent --show-error \
            --header "Content-Type:application/json" \
            -d'{ "query": "$__query" }' \
            "localhost:${DAEMON_REST_PORT}/graphql"
}

and then

function updateSyncStatusLabel() {
    sendQraphqlQuery "query { syncStatus} "  | jq '.data.syncStatus' | sed 's/"//g''
}

etc

@coveralls
Copy link

coveralls commented Feb 21, 2025

Coverage Status

coverage: 60.754% (+0.002%) from 60.752%
when pulling fca34f6 on pod-probes
into 575904a on develop.

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.

3 participants