Page MenuHomePhabricator

Remove concept of --no-touch / --beta-only-change
ClosedPublic

Authored by demon on May 23 2017, 12:04 AM.

Details

Reviewers
mmodell
thcipriani
Group Reviewers
Release-Engineering-Team
Commits
rMSCAcbf17817c720: Remove concept of --no-touch / --beta-only-change
Patch without arc
git checkout -b D654 && curl -L https://phabricator.wikimedia.org/D654?download=true | git apply
Summary

Touching InitialiseSettings isn't scary. In fact, we should touch it more

Test Plan

Untested

Diff Detail

Repository
rMSCA Scap
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

demon created this revision.May 23 2017, 12:04 AM
Restricted Application added a reviewer: mmodell. · View Herald TranscriptMay 23 2017, 12:04 AM
Restricted Application added a reviewer: Release-Engineering-Team. · View Herald Transcript
Restricted Application added a project: Release-Engineering-Team. · View Herald Transcript
demon added a subscriber: thcipriani.

@thcipriani Like we talked about last week. Really, this should do it on any scap pull now, regardless of what files are sync'd.

We might also want to touch the master at the very end of AbstractSync.main() after _after_cluster_sync() so future syncs don't think the targets are ahead of the masters (which seems like it could be an issue currently, regardless of this patch)

thcipriani edited edge metadata.Jun 2 2017, 5:25 PM

I agree with the idea that we should be touching IS after all the syncs that are meant to change things on the cluster; however, I think the --beta-only-change came out of a problem with how HHVM handled reclaiming TC cache space (i.e., exploding) T149872 and T103886#1401890 both talk about this.

The problem used to be HHVM could explode after innocuous syncs for files that didn't affect production, but were just being sync'd out because they were merged into mediawiki/config and no one wants to surprise the next deployer who pulls down changes they didn't make.

I haven't seen HHVM explode in a while, but T103886 is still open so I'm unclear the status of this failure mode. As of right now, I definitely use this to sync out beta-only changes fairly regularly, so it's at least useful to keeping the small amount of peace-of-mind I am able to cling to in place for whatever that's worth.

demon added a comment.EditedJun 2 2017, 5:50 PM

I would be curious if we had info on how often the flag is used vs. how often beta changes go out without the flag attached. I know I do it all the time if for no other reason than I forget the flag exists. scap sync-file foo 'No op beta-only' is pretty common. If those situations don't cause a cache rush, I'm not sure what we're still worried about (although you're right, T103886 remains open).

demon added a comment.Jul 3 2017, 8:13 PM

Per pretty extensive IRC discussions, we're reasonably confident that T103886 is no longer an issue. That, coupled with the fact that this option is routinely underused means we're probably safe in dropping this option. Suggest accepting :)

thcipriani requested changes to this revision.Jul 3 2017, 8:17 PM
In D654#13833, @demon wrote:

Per pretty extensive IRC discussions, we're reasonably confident that T103886 is no longer an issue. That, coupled with the fact that this option is routinely underused means we're probably safe in dropping this option. Suggest accepting :)

fair enough. I have a feeling I'm the only one who uses this flag anyway :)

One inline thing though.

scap/main.py
587

We probably need to get rid of this. I think this would prevent us from syncing php files from /srv/mediawiki-staging/wmf-config

This revision now requires changes to proceed.Jul 3 2017, 8:17 PM
demon updated this revision to Diff 1839.Jul 3 2017, 8:22 PM
demon edited edge metadata.
  • Remove check for PHP files in beta-only changes
demon marked an inline comment as done.Jul 3 2017, 8:22 PM
thcipriani accepted this revision.Jul 3 2017, 8:24 PM
This revision is now accepted and ready to land.Jul 3 2017, 8:24 PM
This revision was automatically updated to reflect the committed changes.