Page MenuHomePhabricator

Preparing VisualEditor for Parsoid-PHP switch
Closed, ResolvedPublic

Description

This is a tracker task to figure out testing / qa needs and any work / changes needed to VisualEditor to switchover from Parsoid/JS to Parsoid/PHP.

T229015: Tracking: Direct live production traffic at Parsoid/PHP is the tracker task for deployment. But, we don't anticipate a deploy before mid-September 2019 at this time. But, let us handle this as if we are going to be deploying around mid to end September.

Since VE talks to RESTBase, there is nothing that will change on that end. However, T229025: Add ability to RESTBase to partition client traffic to Parsoid/PHP and T229018: RESTBase should be able to store Parsoid/PHP contents in Cassandra alongwith Parsoid/JS contents implies that VE might have to handle cookies tracking which version of Parsoid generated its HTML unless we come with a mechanism for handling it in RESTBase / Parsoid that is transparent to clients. But, for now, it is probably safe to assume that VE might need to handle a cookie related to this.

We will also do an early deployment to the beta cluster so that clients can do early testing there. But, please comment on the ticket / edit the description adding any other requirements to ensure we cover all our bases.

Using Parsoid/PHP with VE

In order to test Parsoid/PHP output and transformations, the client (VE) needs to send the X-Parsoid-Variant: php header with the request. This needs to be done consistently for the same domain, i.e. all VE -> RB requests (both GETs and POSTs) need to contain the header for the domain in question (one can go on a per-session basis as well but this is prone to errors). An important thing to note here: this means that even initial GET requests need to be made explicitly with the header. This means that VE cannot use preloaded content for such edits, as it will result in a 404 from RESTBase during transforms.

Related Objects

Event Timeline

ssastry renamed this task from Preparing VisualEditor fo Parsoid-PHP switch to Preparing VisualEditor for Parsoid-PHP switch.Jul 26 2019, 12:45 AM
JTannerWMF added subscribers: marcella, DLynch, dchan and 2 others.

The Editing-team engineers will discuss this task to determine if there is an action for us. @Esanders @matmarex @DLynch @dchan @marcella

Hey @ssastry when should we expect to start paying attention to this?

Hey @ssastry when should we expect to start paying attention to this?

Once we deploy the code to beta cluster ( T231569 ), I'll ping you.

Hey @ssastry when should we expect to start paying attention to this?

Once we deploy the code to beta cluster ( T231569 ), I'll ping you.

But, I hope it is by end of the week.

Here's my draft list of scenarios to test, from our hangout the other day:

  1. Creating a new page (no pre-existing revision)
  2. Switching back and forth from stash (ie the sort of thing which triggered T227216: Adding or editing citations using VisualEditor causes major formatting issues involving pipes, equals signs and nowiki tags)
  3. Pasting chunks of wikitext into VE (which get wt2html conversion on the fly)
  4. Pasting chunks of HTML into new wikitext editor (beta feature; html2wt on the fly)
  5. Template editing (wt2html to re-render)
  6. Using gallery tool or link inspector in new wikitext editor
  7. Doing trivial edits on complex pages, and ensuring no nuisance diffs (checking selser is functional)
  8. Check the variant conversion in mobile (html2html endpoint)

@JTannerWMF, @Esanders, see the note @mobrovac added about testing VE in the beta cluster. See @cscott's note about testing plan.
But, you can start testing starting Monday, and we recommend starting your testing next week so we can catch any issues and fix them promptly - we want to enable Parsoid/PHP for all traffic before Thanskgiving break.

But, you can start testing starting Monday, and we recommend starting your testing next week so we can catch any issues and fix them promptly - we want to enable Parsoid/PHP for all traffic before Thanskgiving break.

RESTBase support for Parsoid/PHP is now live in both Beta and production, so you can start using the X-Parsoid-Variant: php header to have pages parsed and transformed using Parsoid/PHP.

ssastry triaged this task as High priority.Nov 4 2019, 4:04 AM

@JTannerWMF, @Esanders, we would ideally like to switch over all services to Parsoid-PHP the week before Thanksgiving and appreciate you tacking this sooner than later.

@JTannerWMF, @Esanders, we would ideally like to switch over all services to Parsoid-PHP the week before Thanksgiving and appreciate you tacking this sooner than later.

Thank you for pinging @ssastry. Two questions for you (I think):

  • 1. What – if any – changes/additions do you think need to be made to the testing plan below?
  • 2. In the context of testing, what should be done with the information Marko shared in T229074#5610921 RE using the X-Parsoid-Variant: php header?

Testing plan

Where to test: Beta cluster

What to test: The scenarios listed in this comment: T229074#5587900

@JTannerWMF, @Esanders, we would ideally like to switch over all services to Parsoid-PHP the week before Thanksgiving and appreciate you tacking this sooner than later.

Thank you for pinging @ssastry. Two questions for you (I think):

  • 1. What – if any – changes/additions do you think need to be made to the testing plan below?

I think Scott's outline is good. But, broadly, the goal is making sure all of the VE's functionality continues to work with a new Parsoid backend.

  • 2. In the context of testing, what should be done with the information Marko shared in T229074#5610921 RE using the X-Parsoid-Variant: php header?

You need a way to tell RESTBase that you want to talk with Parsoid/PHP and that header is the mechanism to accomplish that. Ed / Bartosz / .. can answer that a lot more specifically about the code changes necessary. I imagine it is a minor change.

I'm thinking that we can't just push it everywhere, because that'd break any non-foundation user of VE who hasn't configured restbase to know about that variant. (E.g. my updated-yesterday vagrant install falls over and breaks as soon as I start sending that header.)

So: new config value VisualEditorRestbaseParsoidVariant, false by default, triggers sending of the header if it contains a string value? Then we can deploy that, and push a config change to beta/production which will let us enable it.

Change 548784 had a related patch set uploaded (by DLynch; owner: DLynch):
[mediawiki/extensions/VisualEditor@master] Config value for X-Parsoid-Variant

https://gerrit.wikimedia.org/r/548784

That patch implements the strategy I described. Adding $wgVisualEditorRestbaseParsoidVariant = 'php'; to the config file triggers the appropriate header-sending.

Because of the aforementioned "vagrant's restbase isn't configured for this" I have minimal knowledge of whether this works, technically. I'd also want a review to see whether I missed anywhere that the header needs to be sent.

+1 for the logic and the patch. I added @matmarex to the patch as he has been doing extensive VE <-> RB communication debugging lately so he'll be able to tell you for sure if there are other places where the header should be added.

The patch looks good, but I also don't have the environment set up to test locally. I'm going to SWAT a patch to enable this on Beta Cluster, and see if everything keeps working.

This means that VE cannot use preloaded content for such edits, as it will result in a 404 from RESTBase during transforms.

This looks important, but I don't understand what you mean by "preloaded content" here. I can only think of the [[https://www.mediawiki.org/wiki/Manual:Parameters_to_index.php#Edit_and_submit|preload query parameter]], but surely this isn't about that?

Change 549227 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[operations/mediawiki-config@master] Set wgVisualEditorRestbaseParsoidVariant='php' on Beta enwiki

https://gerrit.wikimedia.org/r/549227

Change 548784 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Config value for X-Parsoid-Variant

https://gerrit.wikimedia.org/r/548784

Change 549227 merged by jenkins-bot:
[operations/mediawiki-config@master] Set wgVisualEditorRestbaseParsoidVariant='php' on Beta enwiki

https://gerrit.wikimedia.org/r/549227

Okay, it's live on https://en.wikipedia.beta.wmflabs.org/ (only – I left other Beta sites unchanged, so that if anything acts weird, you can compare to e.g. https://commons.wikimedia.beta.wmflabs.org/).

First quick test:

Other notes:

Correction: new pages containing a slash in their title can't be created. I filed T237666: Parsoid/PHP fails for transforms for new pages with slashes in the title for this. I tried VE-creating pages without slashes in their names and confirmed they are created using Parsoid/PHP.

Change 549629 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[mediawiki/extensions/VisualEditor@master] Ensure all requests in one edit session use the same X-Parsoid-Variant

https://gerrit.wikimedia.org/r/549629

Change 549633 had a related patch set uploaded (by Bartosz Dziewoński; owner: Bartosz Dziewoński):
[operations/mediawiki-config@master] Explicitly set wgVisualEditorRestbaseParsoidVariant='js' everywhere else

https://gerrit.wikimedia.org/r/549633

I would strongly advise against the above patches (PS 549629 and PS 549633). There is special logic in RESTBase for handling the transition period; it allows us to (i) test Parsoid/PHP; and (ii) selectively switch projects over (for a detailed explanation of the modus operandi cf. the description of PR #1207). However, this logic is bypassed if the client supplies the X-Parsoid-Variant header. This possibility was given to clients exclusively for the purposes of them being able to test and play with Parsoid/PHP at their own pace, but was never meant to be used everywhere. Therefore, VE setting X-Parsoid-Variant everywhere would interfere with our transition plans and would leave little room for quick reverts.

Change 549629 abandoned by Bartosz Dziewoński:
Ensure all requests in one edit session use the same X-Parsoid-Variant

https://gerrit.wikimedia.org/r/549629

Change 549633 abandoned by Bartosz Dziewoński:
Explicitly set wgVisualEditorRestbaseParsoidVariant='js' everywhere else

https://gerrit.wikimedia.org/r/549633

@mobrovac Thank you for the explanation.

To clarify, the patches were supposed to avoid this problem:

  • On further thought, it seems to me that changing the config option could cause lost edits or page corruption for edits which are in progress – that is, if someone opens the editor with JS Parsoid, then we switch to PHP Parsoid, then they try saving.

But it looks like you already handle the scenario that I was worried about:

Because of this gradual switch, there is also a built-in fall-back for transform end points in this mode: if a request for the non-default variant 404s, then we retry the request using the default variant in order to cover the clients that are in the middle of an editing session during the switch.

So the other patches should not be needed.

Editing team: We want to catch problems sooner than later, so checking in about updates on how testing is going here. Anything that you need from us? Or, is everything looking good except for that one bug that @matmarax found ( T237666: Parsoid/PHP fails for transforms for new pages with slashes in the title ) ?

Editing team: We want to catch problems sooner than later, so checking in about updates on how testing is going here.

We started testing today, with plans to finish on Thursday.

Anything that you need from us? Or, is everything looking good except for that one bug that @matmarax found ( T237666: Parsoid/PHP fails for transforms for new pages with slashes in the title ) ?

Today, we found T238161 which, per @matmarex's comment in T238161#5658627, might be related to T237666.


Meta: @ssastry, what is the best way to surface bugs to y'all? Adding tickets and children to T229015? cc @Ryasmeen

Editing team: We want to catch problems sooner than later, so checking in about updates on how testing is going here.

We started testing today, with plans to finish on Thursday.

Awesome! Thanks!

Meta: @ssastry, what is the best way to surface bugs to y'all? Adding tickets and children to T229015? cc @Ryasmeen

Ya, that would be ideal.

@Ryasmeen: the 'new page creation' bug is now resolved and new code is on beta cluster as of y'day. Let us know if you find any other bugs. Or, a confirmation that you could run all your QA tests successfully! Thanks!

@Ryasmeen: the 'new page creation' bug is now resolved and new code is on beta cluster as of y'day. Let us know if you find any other bugs. Or, a confirmation that you could run all your QA tests successfully! Thanks!

@ssastry: Yes, I have checked all the scenarios mentioned above and it seems they are all passing. There were two other bugs found related to reverting edits but those are probably not relevant to this change.

ssastry claimed this task.

@ppelberg @Ryasmeen I am going to resolve this ticket then.

@Ryasmeen: the 'new page creation' bug is now resolved and new code is on beta cluster as of y'day. Let us know if you find any other bugs. Or, a confirmation that you could run all your QA tests successfully! Thanks!

@ssastry: Yes, I have checked all the scenarios mentioned above and it seems they are all passing. There were two other bugs found related to reverting edits but those are probably not relevant to this change.

And, thanks for confirming all is good. That is the kind of news I wanted to hear. :-)

test.wikipedia.org and test2.wikipedia.org have been switched to serve Parsoid/PHP HTML only. Please test and report any issues you spot.

Change 552088 had a related patch set uploaded (by Mobrovac; owner: Mobrovac):
[mediawiki/core@master] Parsoid VRS: Add the Host header

https://gerrit.wikimedia.org/r/552088

Change 552088 merged by jenkins-bot:
[mediawiki/core@master] Parsoid VRS: Add the Host header

https://gerrit.wikimedia.org/r/552088

Change 552097 had a related patch set uploaded (by Mobrovac; owner: Mobrovac):
[mediawiki/core@wmf/1.35.0-wmf.5] Parsoid VRS: Add the Host header

https://gerrit.wikimedia.org/r/552097

Change 552097 merged by jenkins-bot:
[mediawiki/core@wmf/1.35.0-wmf.5] Parsoid VRS: Add the Host header

https://gerrit.wikimedia.org/r/552097

Mentioned in SAL (#wikimedia-operations) [2019-11-20T18:17:25Z] <mobrovac@deploy1001> Synchronized php-1.35.0-wmf.5/includes/libs/virtualrest/ParsoidVirtualRESTService.php: Parsoid VRS: Add the Host header - T229015 T229078 T229074 (duration: 00m 52s)

Change 579002 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[mediawiki/extensions/VisualEditor@master] Remove X-Parsoid-Variant configuration value, which is no longer needed

https://gerrit.wikimedia.org/r/579002

Change 579004 had a related patch set uploaded (by C. Scott Ananian; owner: C. Scott Ananian):
[operations/mediawiki-config@master] Remove $wgVisualEditorRestbaseParsoidVariant, no longer needed

https://gerrit.wikimedia.org/r/579004

Change 579004 merged by jenkins-bot:
[operations/mediawiki-config@master] Remove $wgVisualEditorRestbaseParsoidVariant, no longer needed

https://gerrit.wikimedia.org/r/579004

Change 579002 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Remove X-Parsoid-Variant configuration value, which is no longer needed

https://gerrit.wikimedia.org/r/579002