"scap deploy"'s config-deploy should check for broken symlinks
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	RKemper
	Jul 18 2023, 7:33 PM

Description

We've been provisioning new bullseye wdqs hosts and ran into some issues with scap; namely "incomplete" deploys that leave the host in a non-functional state.

Scap works as we expect when we run with the --force flag:

scap deploy --force -l 'wdqs2016.codfw.wmnet' ${VERSION}

However if we run it without the --force flag, and the directory /srv/deployment/wdqs/wdqs-cache/revs/$CURRENT_DEPLOY_COMMIT_HASH/.git/config-files/etc/query_service is not present, scap will silently "fail" the deploy but not raise an error:

Here's a log from wdqs2016 where its .git/config-files/etc/query_service directory has been manually deleted by the operator:
https://phabricator.wikimedia.org/P49572

And here's a log from the same host, same directory missing, but with the --force flag:
https://phabricator.wikimedia.org/P49573

I think based off this log message:
{"name": "target.wdqs2016.codfw.wmnet.deploy-local", "msg": "Revision directory already exists (use --force to override)", "args": [], "levelno": 20, "filename": "deploy.py", "exc_text": null, "lineno": 336, "funcName": "fetch", "created": 1689706482.8972232, "msecs": 897.2232341766357, "relativeCreated": 743.9661026000977, "host": "wdqs2016.codfw.wmnet"}

The issue is possibly that scap is happy as long as /srv/deployment/wdqs/wdqs-cache/revs/$CURRENT_DEPLOY_COMMIT_HASH/ exists, but doesn't check that all relevant subdirectories are as they should.

When necessary subdirectories of the main git directory are not present and/or have different contents than expected, scap will actually raise an error when the --force flag is not present

Related Objects
Search...

Status	Assigned	Task
Open	None	T291916 Tracking task for Bullseye migrations in production
Resolved	Gehel	T323921 [Epic] Migrate all Search Platform servers to Debian Bullseye
Resolved	bking	T332314 Service implementation for wdqs20[13-22]
Resolved	bking	T331300 Ensure WDQS stack works on Bullseye
Resolved	Gehel	T342060 Investigate WDQS categories update failures on Bullseye hosts
Resolved	dancy	T342162 "scap deploy"'s config-deploy should check for broken symlinks

Event Timeline

RKemper created this task.Jul 18 2023, 7:33 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 18 2023, 7:33 PM

First draft of this ticket up. There's a couple things that aren't perfect:

(1) I think the current description is a bit confusing, I'm not sure if another team has enough context in the ticket's current state.

(2) I haven't tagged the team who would work this ticket. Probably rel-eng?

Scap would be releng ye

RKemper updated the task description. (Show Details)Jul 18 2023, 7:38 PM

EBernhardson subscribed.Jul 18 2023, 7:41 PM

bking added a parent task: T342060: Investigate WDQS categories update failures on Bullseye hosts.Jul 18 2023, 9:00 PM

I think I have the context to understand this.

It looks like /srv/deployment/wdqs/wdqs-cache/revs/$CURRENT_DEPLOY_COMMIT_HASH/.git/config-files/etc/query_service is symlinked at /etc/query_service/ldf-config.json is that true?

I see this line in log output:

/etc/query_service/ldf-config.json is already linked to current rev(use --force to override)

Which is exactly what you're describing. It comes from the check in scap deploy here: https://gitlab.wikimedia.org/repos/releng/scap/-/blob/master/scap/deploy.py#L212

The assumption is that if a file is already symlinked, there's no need to regenerate the file. But it sounds like it's a bad assumption in this case, is that true?

@thcipriani that's correct. For context, we are manually deleting the directory because puppet runs scap before git-fat is installed, and it results in an incomplete deploy (more details here ) . The deletion results in a broken symlink, maybe scap could check for that?

Also, if we try and run scap before deleting these directories, scap does not detect the missing files (large files provided by git-fat) and reports a successful deployment.

I'm not trying to cross the streams with what might be 2 different bugs (or even expected behavior), just trying to explain why we're manually deleting that directory.

In T342162#9025774, @thcipriani wrote:
I think I have the context to understand this.

It looks like /srv/deployment/wdqs/wdqs-cache/revs/$CURRENT_DEPLOY_COMMIT_HASH/.git/config-files/etc/query_service is symlinked at /etc/query_service/ldf-config.json is that true?

I see this line in log output:
/etc/query_service/ldf-config.json is already linked to current rev(use --force to override)
Which is exactly what you're describing. It comes from the check in scap deploy here: https://gitlab.wikimedia.org/repos/releng/scap/-/blob/master/scap/deploy.py#L212

The assumption is that if a file is already symlinked, there's no need to regenerate the file. But it sounds like it's a bad assumption in this case, is that true?

Right, I think we want to check that the file associated with the symlink actually exists, in addition to checking that the symlink itself is present.

Config-deploy/symlink checking

In T342162#9025804, @bking wrote:

@thcipriani that's correct. For context, we are manually deleting the directory because puppet runs scap before git-fat is installed, and it results in an incomplete deploy (more details here ) . The deletion results in a broken symlink, maybe scap could check for that?

Yeah, I think that makes sense for this task

In T342162#9025921, @RKemper wrote:

Right, I think we want to check that the file associated with the symlink actually exists, in addition to checking that the symlink itself is present.

Agreed.

I'll re-title this task to reflect this symlink-checking bug in scap deploy.

Other parts of what you're seeing could use a different ticket. There are a few issues there. Summary below.

Other issues

In T342162#9025804, @bking wrote:

Also, if we try and run scap before deleting these directories, scap does not detect the missing files (large files provided by git-fat) and reports a successful deployment.

I'm not trying to cross the streams with what might be 2 different bugs (or even expected behavior), just trying to explain why we're manually deleting that directory.

That makes sense. Scap tries to avoid doing work where possible, so exiting early is "expected" But(!) it sounds like some of the checks are bad/confusing—which is a bug. Possibly one of several bugs here 😅

Additional thoughts:

Initial run (as I understand it from T331300#8983005)

Puppet runs scap
scap's git checkout succeeds
git checkout triggers the smudge filter, which calls git-fat
git-fat fails (since the executable doesn't exist)

Is that right?

Subsequent scap deploy runs

scap deploy scap doesn't re-try the git checkout (unless you use --force) due to this check (which is different than the symlink check above. The logic of this check is: git rev-parse --verify HEAD == <rev you're trying to deploy> ? nothing to do && exit )
Smudge is never triggered
git-fat is never called to do it's thing
git-fat never pulls the actual files

Bugs in this process

I suppose there are a few problems:
0. scap is trying to be too smart/helpful and exiting in confusing ways

git-fat should exist before the scap run, so there's some missing dependency/race condition/erroneous graph in the initial puppet run
scap should probably check if git-fat was successful, otherwise the bad initial puppet run is unrecoverable without manual intervention
maybe we caused 1 because we're phasing out git-fat in favor of git-lfs.

Anyway, this one is more gnarly than adding a symlink check—let's make a separate task. :)

thcipriani renamed this task from Scap deploy does not raise error on missing directory to "scap deploy"'s config-deploy should check for broken symlinks.Jul 18 2023, 10:20 PM

Gehel edited projects, added Data-Platform-SRE; removed Discovery-Search.Jul 24 2023, 3:31 PM

Gehel moved this task from Incoming to Blocked / Waiting on the Data-Platform-SRE board.Aug 29 2023, 1:33 PM

bking added a parent task: T340793: Implement depool (source only) and keep-downtime options on data-transfer cookbook.Sep 12 2023, 4:53 PM

RKemper removed a parent task: T340793: Implement depool (source only) and keep-downtime options on data-transfer cookbook.Sep 14 2023, 9:55 PM

Gehel edited projects, added Data-Platform-SRE (2023.12.01 - 2023.12.31); removed Data-Platform-SRE.Dec 6 2023, 9:23 AM

Gehel moved this task from Backlog to Blocked / Waiting on the Data-Platform-SRE (2023.12.01 - 2023.12.31) board.Dec 6 2023, 9:25 AM

Gehel edited projects, added Data-Platform-SRE; removed Data-Platform-SRE (2023.12.01 - 2023.12.31).Dec 15 2023, 10:07 AM

Gehel moved this task from Blocked / Waiting to Watching on the Data-Platform-SRE board.

• Sandeeps subscribed.Mar 25 2024, 7:24 PM

• Sandeeps claimed this task.Mar 26 2024, 2:42 PM

• Sandeeps changed the task status from Open to In Progress.Mar 26 2024, 5:41 PM

sandeeps opened https://gitlab.wikimedia.org/repos/releng/scap/-/merge_requests/322