Make clusterwide config upload a separate stage #1266

Steap2448 · 2021-02-15T20:07:17Z

For historical reasons, there were two issues in the patch_clusterwide logics:

The 5-second timeout for all communications was hardcoded.
No optimizations were undertaken there, it was was suboptimal and wasteful.

It was OK when the config was small and there were only a few instances in the cluster, but it's a problem for big installations nowadays.

This patch introduces a new patch_clusterwide stage - upload (consolidated in a separate module). It spreads the data across instances in a network-efficient manner.

Also in this patch:

Change issues_test.lua and remove unnecessary checks from cypress sigstop.spec.js because the SIGStOP doesn't result in config lock and mismatch anymore.

I didn't forget about

Tests
Changelog
Documentation

Close #1145

cartridge/upload.lua

Recently we've encountered aweird test behavior: a test (unit.upload from #1266) succeded when run alone, but failed in conjunction with unit.cfg_errors test. The reason is in monkeypatching and reload of lua modules used there. With this patch all test functions are moved to a simple empty server and run over net.box (remote-control)

Recently we've encountered a weird test behavior: a test (`unit.upload` from #1266) succeded when run alone, but failed in conjunction with `unit.cfg_errors` test. The reason is in monkeypatching and reloading of lua modules used there. With this patch, all test functions are moved to a simple empty server and run over netbox (remote-control).

olegrok · 2021-03-04T07:35:51Z

cartridge/upload.lua

+
+    local upload_path = get_upload_path(upload_id)
+    local ok, err = utils.mktree(fio.dirname(upload_path))
+    fiber.testcancel()


testcancel raises. You won't perform vars.upload_fibers[upload_id] = nil if this function raises an error.

It's ok. In this case the upload_fibers will be cleaned by the cleanup stage which cancelled the fiber.

Ok, but it's not so obvious, please add a comment.

However, in fact we can't guarantee that cleanup stage will be executed someday in case of temporary problems with network.

Ok, but it's not so obvious, please add a comment.

Done

However, in fact we can't guarantee that cleanup stage will be executed someday in case of temporary problems with network.

The necessity of persisting upload_fibers is to avoid a race between begin and cleanup. The necessity of cleaning up the upload_fibers is to avoid canceling an innocent fiber that belongs to the iproto fiber pool and has already been reused by that time.

To sum up, if the cleanup stage wasn't called for some reason, the garbage will remain in /tmp and in lua context. And it's ok for now.

olegrok · 2021-03-04T07:36:07Z

cartridge/upload.lua

+    local upload_path = get_upload_path(upload_id)
+    local payload_path = fio.pathjoin(upload_path, 'payload')
+    local ok, err = utils.file_write(payload_path, payload)
+    fiber.testcancel()


testcancel raises. You won't perform vars.upload_fibers[upload_id] = nil if this function raises an error.

olegrok · 2021-03-04T07:38:19Z

cartridge/upload.lua

+    local random_path = utils.randomize_path(upload_path)
+
+    local ok = fio.rename(upload_path, random_path)
+    local _errno = errno()


It's redundant action to call errno here. It's not needed if ok is true.

The main danger of errno is that it may be overridden by some other consequent fio operation. I've changed this place, but in another way.

local _errno local ok, err = fio.some_operation() if not ok then _errno = errno() end

No redefinition, to excess actions.

olegrok · 2021-03-04T07:38:28Z

cartridge/upload.lua

+
+    local ok = fio.rmtree(random_path)
+    local _errno = errno()
+    if not ok then


It's redundant action to call errno here. It's not needed if ok is true.

olegrok · 2021-03-04T07:43:08Z

cartridge/upload.lua

+_G.__cartridge_upload_begin = function(...) return errors.pcall('E', upload_begin, ...) end
+_G.__cartridge_upload_transmit = function(...) return errors.pcall('E', upload_transmit, ...) end
+_G.__cartridge_upload_finish = function(...) return errors.pcall('E', upload_finish, ...) end
+_G.__cartridge_upload_cleanup = function(...) return errors.pcall('E', upload_cleanup, ...) end


You don't actually need to pass ... here because you exactly know a number of arguments - one upload_id.
varargs are bit slower than usual function call with determined number of arguments.

I'm ok to sacrifice that performance for the sake of code brevity. Hope it's not too much.

test/integration/upload_test.lua

olegrok · 2021-03-04T07:51:59Z

test/integration/upload_test.lua

+                {env = {TARANTOOL_UPLOAD_PREFIX = g.upload_path_1}},
+                {env = {TARANTOOL_UPLOAD_PREFIX = g.upload_path_1}},


Maybe it's reasonable to increase number of servers up to 5 or even more. In case of problems with concurrency and shared resource problems we have more chances to catch a problem in CI.

Steap2448 changed the title ~~Clusterwide-upload module for config distribution~~ Clusterwide-upload module for a config distribution Feb 15, 2021

Steap2448 force-pushed the 2pc-prepare-stage branch 5 times, most recently from d1edaea to 037e24b Compare February 19, 2021 19:39

rosik force-pushed the 2pc-prepare-stage branch 2 times, most recently from bef2018 to 94f3001 Compare February 24, 2021 14:56

rosik changed the title ~~Clusterwide-upload module for a config distribution~~ Make clusterwide config upload a separate stage Feb 24, 2021

rosik force-pushed the 2pc-prepare-stage branch 2 times, most recently from 43d6481 to 9321d8e Compare February 24, 2021 16:50

rosik marked this pull request as ready for review February 24, 2021 16:58

rosik force-pushed the 2pc-prepare-stage branch 11 times, most recently from 16bdca7 to d03611e Compare February 25, 2021 22:25

HustonMmmavr approved these changes Feb 26, 2021

View reviewed changes

rosik requested a review from olegrok February 26, 2021 13:36

rosik reviewed Feb 26, 2021

View reviewed changes

cartridge/upload.lua Show resolved Hide resolved

cartridge/upload.lua Outdated Show resolved Hide resolved

olegrok reviewed Mar 1, 2021

View reviewed changes

cartridge/upload.lua Outdated Show resolved Hide resolved

olegrok reviewed Mar 1, 2021

View reviewed changes

cartridge/upload.lua Show resolved Hide resolved

rosik force-pushed the 2pc-prepare-stage branch from d03611e to 63fb0fc Compare March 1, 2021 11:01

rosik mentioned this pull request Mar 3, 2021

Sandbox unit.cfg_errors test #1285

Merged

3 tasks

Steap2448 and others added 3 commits March 3, 2021 17:58

Make clusterwide config upload a separate stage

e66869a

Minor edits

e643a3b

Enhance upload configuration

9c71a65

rosik force-pushed the 2pc-prepare-stage branch from 63fb0fc to 9c71a65 Compare March 3, 2021 16:24

Fix other tests

2c7f8e1

olegrok reviewed Mar 4, 2021

View reviewed changes

rosik added 2 commits March 4, 2021 12:05

Fine-tune errno handling

88e4f39

Add a comment

42a92c6

rosik mentioned this pull request Mar 4, 2021

[4pt] Cartridge skips two-phase commit if didn't change and says "OK" when it's not #1046

Closed

olegrok approved these changes Mar 4, 2021

View reviewed changes

rosik approved these changes Mar 4, 2021

View reviewed changes

rosik merged commit dfd75b9 into master Mar 4, 2021

rosik deleted the 2pc-prepare-stage branch March 4, 2021 12:09

This was referenced Mar 21, 2021

Incorrect netbox_call_timeout in twophase.lua #1326

Closed

Too long instance bootstrap on a big cluster #1197

Closed

		{env = {TARANTOOL_UPLOAD_PREFIX = g.upload_path_1}},
		{env = {TARANTOOL_UPLOAD_PREFIX = g.upload_path_1}},

Make clusterwide config upload a separate stage #1266

Make clusterwide config upload a separate stage #1266

Uh oh!

Conversation

Steap2448 commented Feb 15, 2021 • edited by rosik Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Steap2448 commented Feb 15, 2021 •

edited by rosik

Loading