praefect: Check of the service readiness with RPC call

For the praefect binary we have a sub-command to verify if praefect service can operate without issues. The verification process checks if migrations were applied, if gitaly nodes are reachable, if the clock synced, etc. This check can be done only when you have direct access to the binary. With introducing of ReadinessCheck RPC we now can run the same verification process mentioned above by issuing an RPC call. The new RPC will be part of the gitlab:gitaly:check task. It is noop for the gitaly service as of now.

Part of: gitlab#348174 (closed)

Once we release this change the next step would be to include invocation of it in the gitlab:gitaly:check task.

added Category:Gitaly backend devopscreate groupgitaly labels

assigned to @8bitlife

@8bitlife - please add typebug typefeature, typemaintenance or a subtype label to this merge request.

• typebug: Defects in shipped code and fixes for those defects. This includes all the bug types (availability, performance, security vulnerability, mobile, etc.)
• typefeature: Effort to deliver new features, feature changes & improvements. This includes all changes as part of new product requirements like application limits.
• typemaintenance: Up-keeping efforts & catch-up corrective improvements that are not Features nor Bugs. This includes restructuring for long-term maintainability, stability, reducing technical debt, improving the contributor experience, or upgrading dependencies.

See the handbook for more guidance on classifying.

---

This message was created with automation and Engineering Productivity is looking for feedback in this issue: https://gitlab.com/gitlab-org/quality/engineering-productivity/team/-/issues/43

added typemaintenance label

resolved all threads

added 1 commit

• f14df758 - praefect: Check of the service readiness with RPC call

Compare with previous version

changed the description

requested review from @jcaigitlab

@8bitlife nice addition. Had a few suggestions/comments

removed review request for @jcaigitlab

requested review from @jcaigitlab

added 213 commits

• f14df758...a454dd8c - 212 commits from branch master
• dc89f97c - praefect: Check of the service readiness with RPC call

Compare with previous version

How the response with failed checks looks like:

{
  "failure_response": {
    "checks": [
      {
        "name": "gitaly node connectivity & disk access",
        "cause": "can't connect to node"
      },
      {
        "name": "clock synchronization",
        "cause": "praefect: clock is out"
      }
    ]
  },
  "result": "failure_response"
}

@8bitlife overall it looks good but could you break it up into a couple of commits so that it's a little easier to review?

• the proto change
• refactoring checks into the service package
• the actual code change

added 3 commits

• 037123c5 - proto: RPC to check service readiness
• 29ae6928 - praefect: Move of the service checks
• 3b408ee6 - praefect: Check of the service readiness with RPC call

Compare with previous version

requested review from @samihiltunen

@samihiltunen could you please give a review here? Assigned you because you already know the context here.

@8bitlife looking good--not much more to add from @samihiltunen's feedback.

removed review request for @jcaigitlab

added 3 commits

• 6e3d7c4f - proto: RPC to check service readiness
• 0ee6bb60 - praefect: Move of the service checks
• bf3d7b63 - praefect: Check of the service readiness with RPC call

Compare with previous version

mentioned in issue #4386

added 1 commit

• 18ce5906 - praefect: Check of the service readiness with RPC call

Compare with previous version

added 1 commit

• 74236fa2 - praefect: Check of the service readiness with RPC call

Compare with previous version

added 64 commits

• 74236fa2...17a3fc6c - 61 commits from branch master
• 04bc67b6 - proto: RPC to check service readiness
• e1d2b44a - praefect: Move of the service checks
• d4b062d9 - praefect: Check of the service readiness with RPC call

Compare with previous version

added 1 commit

• ef5d3389 - praefect: Check of the service readiness with RPC call

Compare with previous version

added 1 commit

• ee753856 - praefect: Check of the service readiness with RPC call

Compare with previous version

removed review request for @samihiltunen

added 1 commit

• 13d942c1 - praefect: Check of the service readiness with RPC call

Compare with previous version

added 1 commit

• 03549c62 - praefect: Check of the service readiness with RPC call

Compare with previous version

added 55 commits

• 03549c62...333f0808 - 52 commits from branch master
• 7730fa65 - proto: RPC to check service readiness
• a572cbee - praefect: Move of the service checks
• 6195cb5c - praefect: Check of the service readiness with RPC call

Compare with previous version

requested review from @samihiltunen

requested review from @jcaigitlab

OK, the issue is that we run tests as unprivileged user and it has no rights to use ports lower that 1024.
I am trying to find a solution for that atm. But overall the code change is pretty much done so can be reviewed.

added 1 commit

• fc2343cc - .gitlab-ci: Run test as super user

Compare with previous version

added 1 commit

• 0e627279 - .gitlab-ci: Run test as super-user

Compare with previous version

approved this merge request

@8bitlife a couple of nits. It's unfortunately the NTP client hardcoded the port...

removed review request for @jcaigitlab

added 5 commits

• c848b6b3 - proto: RPC to check service readiness
• 0a2f5550 - praefect: Move of the service checks
• 3ef0b170 - praefect: Make runPraefectServer exportable
• 6a8cb11d - praefect: Make readiness checks injectable
• d03c651d - praefect: Check of the service readiness with RPC call

Compare with previous version

added 38 commits

• d03c651d...31ea786f - 33 commits from branch master
• d1d63496 - proto: RPC to check service readiness
• 1b019586 - praefect: Move of the service checks
• a0a0525f - praefect: Make runPraefectServer exportable
• 94287b74 - praefect: Make readiness checks injectable
• 6f55f8ae - praefect: Check of the service readiness with RPC call

Compare with previous version

added 1 commit

• aa0ffccf - praefect: Check of the service readiness with RPC call

Compare with previous version

added 1 commit

• 4012dfef - praefect: Check of the service readiness with RPC call

Compare with previous version

added 3 commits

• 155539fe - praefect: Make runPraefectServer exportable
• 9ecf0828 - praefect: Make readiness checks injectable
• cfad4a6e - praefect: Check of the service readiness with RPC call

Compare with previous version

removed review request for @samihiltunen

Is this needed? If not, we should remove it so we don't add more work for the sha256 migration accidentally. The tests seems to run fine with the build tag set and this tag removed.

I think it is as the comment where the tag was added to all tests says that no tests should run.

@8bitlife The problem was that not all of the existing tests worked with SHA256 so the build tag was added to fix the tests package by package to support SHA256. We've since been updating the tests to support SHA256 and removing the build tag once that's. Since these tests work fine without the build tag, we shouldn't have it here. It adds extra work to the migration as we'll have to go through the package again to remove it and it allows for new tests to be a added in the files without SHA256 support.

Maybe @toon can correct me here if I am mistaken.

changed this line in version 23 of the diff

@samihiltunen @8bitlife I'm sorry, but I should have added some documentation.

Anyhow let me give you a brief summary. Normally we set up repos in test with SHA1 hashes. But over time we want to have Gitaly support SHA256 as well. Most tests won't work with that, so I've added build tag !gitaly_test_sha256 to those. If a test is able to run with SHA256 hashes, it should not have this tag. Over time we plan to have more and more tests that also work with SHA256.

To run tests with SHA256 hashes, pass GITALY_TESTING_ENABLE_SHA256=1 to make test-go. There's also a CI job that will do that. This method should at least help us to not add more tests files that do not support SHA256 hashes.

There's some context in #4307 (closed) and it's parent's epic.

added 5 commits

• 6c066031 - proto: RPC to check service readiness
• a19da4b1 - praefect: Move of the service checks
• ef5adcc0 - praefect: Make runPraefectServer exportable
• 468e12bc - praefect: Make readiness checks injectable
• 714c0310 - praefect: Check of the service readiness with RPC call

Compare with previous version

requested review from @samihiltunen

added 5 commits

• be875cbf - proto: RPC to check service readiness
• cba676d9 - praefect: Move of the service checks
• aa93a2bb - praefect: Make runPraefectServer exportable
• bac3afd1 - praefect: Make readiness checks injectable
• b34db136 - praefect: Check of the service readiness with RPC call

Compare with previous version

approved this merge request

removed review request for @samihiltunen

added 1 commit

• 10dc304c - praefect: Check of the service readiness with RPC call

Compare with previous version

added 121 commits

• 10dc304c...6bb5f696 - 116 commits from branch master
• d0919136 - proto: RPC to check service readiness
• f6459afe - praefect: Move of the service checks
• 8d4ab55c - praefect: Make runPraefectServer exportable
• 07c48fac - praefect: Make readiness checks injectable
• 96af8f1c - praefect: Check of the service readiness with RPC call

Compare with previous version

resolved all threads

added typefeature label and removed typemaintenance label

started a merge train

merged

mentioned in commit 9765a973

added workflowstaging-canary label

added workflowcanary label and removed workflowstaging-canary label

added workflowstaging label and removed workflowcanary label

added workflowproduction label and removed workflowstaging label

mentioned in commit gitlab@bd53f01a

mentioned in commit gitlab@5c13c713

mentioned in merge request gitlab!95243 (merged)

mentioned in commit gitlab@de07bb1f

mentioned in commit gitlab@aaeca080

mentioned in commit gitlab@d4b5e11a

mentioned in commit gitlab@e74c60a7

mentioned in commit gitlab@fce4c200

mentioned in commit gitlab@00ece26d

mentioned in commit gitlab@a93b4ffb

added releasedcandidate label

mentioned in commit gitlab@919c57f4

mentioned in commit gitlab@5923d4d2

added releasedpublished label and removed releasedcandidate label

mentioned in merge request gitlab!100629 (merged)

mentioned in issue #4598 (closed)