Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Describe the semi-automated procedure that migrates Discovery Service V1 to V2. #1861

Merged
merged 23 commits into from
Oct 31, 2024

Conversation

akarasavov
Copy link
Contributor

@akarasavov akarasavov commented Oct 15, 2024

Before publishing the docs the internal prefix from the procedure path needs to be removed

@NataliaIvakina NataliaIvakina self-assigned this Oct 15, 2024
modules/ROOT/pages/clustering/setup/discovery.adoc Outdated Show resolved Hide resolved
modules/ROOT/pages/clustering/setup/discovery.adoc Outdated Show resolved Hide resolved
modules/ROOT/pages/clustering/setup/discovery.adoc Outdated Show resolved Hide resolved
modules/ROOT/pages/clustering/setup/discovery.adoc Outdated Show resolved Hide resolved
modules/ROOT/pages/clustering/setup/discovery.adoc Outdated Show resolved Hide resolved
modules/ROOT/pages/clustering/setup/discovery.adoc Outdated Show resolved Hide resolved
Copy link
Contributor

@nick-giles-neo nick-giles-neo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small tweak as a consequence of the re-ordering, then I'd like @NataliaIvakina to give the OK on the section names, then I'm happy.

modules/ROOT/pages/clustering/setup/discovery.adoc Outdated Show resolved Hide resolved
@NataliaIvakina
Copy link
Contributor

NataliaIvakina commented Oct 21, 2024

We also need to add this new procedure to the procedures' list on the https://neo4j.com/docs/operations-manual/current/reference/procedures/

@NataliaIvakina
Copy link
Contributor

NataliaIvakina commented Oct 21, 2024

However, I don't see this new procedure in the procedure.json file in the code.

BTW, do you know that arguments' descriptions have to be written in a user-friendly way now? See https://github.com/neo-technology/neo4j/pull/26625

modules/ROOT/pages/clustering/setup/discovery.adoc Outdated Show resolved Hide resolved
modules/ROOT/pages/clustering/setup/discovery.adoc Outdated Show resolved Hide resolved
modules/ROOT/pages/clustering/setup/discovery.adoc Outdated Show resolved Hide resolved
modules/ROOT/pages/clustering/setup/discovery.adoc Outdated Show resolved Hide resolved
modules/ROOT/pages/clustering/setup/discovery.adoc Outdated Show resolved Hide resolved
modules/ROOT/pages/clustering/setup/discovery.adoc Outdated Show resolved Hide resolved
+
[source,cypher]
----
CALL dbms.cluster.showParallelDiscoveryState();
----
+
They should display "Matching" in the `stateComparison` column.
The output indicates mode `V1_ONLY`, i.e., only `V1` is running on this server.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'on this server' or 'on this cluster'?
You say 'on this server' here but the procedure is for moving the entire cluster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This procedure CALL dbms.cluster.showParallelDiscoveryState(); is used to check the state of a single member. So we need to use the term server. This procedure CALL internal.dbms.cluster.moveToNextDiscoveryVersion() moves several members to the next discovery version

Copy link
Contributor Author

@akarasavov akarasavov Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think your question is: why do we query the discovery state (CALL dbms.cluster.showParallelDiscoveryState()) of a single server after calling CALL internal.dbms.cluster.moveToNextDiscoveryVersion() instead of checking the state of other servers?

Our goal is to minimize the number of steps the DB admin needs to take to migrate from V1_ONLY to V2_ONLY. We are quite confident that if the state of a single server matches (i.e., showParallelDiscoveryState returns 'Matching'), the states of the other servers are also likely to match. This is the reason why we don't ask db-admins to check the state of other servers.

We can add the following explanation to clarify why we query only a single server after executing moveToNextDiscoveryVersion

@akarasavov
Copy link
Contributor Author

However, I don't see this new procedure in the procedure.json file in the code.

BTW, do you know that arguments' descriptions have to be written in a user-friendly way now? See neo-technology/neo4j#26625

@akarasavov akarasavov closed this Oct 21, 2024
@akarasavov akarasavov reopened this Oct 21, 2024
@akarasavov akarasavov force-pushed the update_discovery_migration_guide branch from dbf9413 to 43874e3 Compare October 22, 2024 10:59
@akarasavov akarasavov force-pushed the update_discovery_migration_guide branch from 43874e3 to a187ddf Compare October 22, 2024 11:07
@neo-technology-commit-status-publisher
Copy link
Collaborator

Thanks for the documentation updates.

The preview documentation has now been torn down - reopening this PR will republish it.

@neo-technology-commit-status-publisher
Copy link
Collaborator

This PR includes documentation updates
View the updated docs at https://neo4j-docs-operations-1861.surge.sh

Updated pages:

@akarasavov
Copy link
Contributor Author

Prodecures testing fails mismatch for quarantineDatabase.

AssertionError: Mismatch in signature for procedure dbms.quarantineDatabase.
== Docs ==
dbms.quarantineDatabase(databaseName :: STRING, setStatus :: BOOLEAN, reason = No reason given :: STRING) :: (databaseName :: STRING, quarantined :: BOOLEAN, result :: STRING)
== Neo4j ==
dbms.quarantineDatabase(databaseName :: STRING, setStatus :: BOOLEAN, reason = No reason given :: STRING, wipeMode = DO_NOT_WIPE :: STRING) :: (databaseName :: STRING, quarantined :: BOOLEAN, result :: STRING)
operations/test-procedures.py:75: in test_settings
    assert docs_procedures[name]['signature'] == procedure['signature'], \
E   AssertionError: Mismatch in signature for procedure dbms.quarantineDatabase.
E   == Docs ==
E   dbms.quarantineDatabase(databaseName :: STRING, setStatus :: BOOLEAN, reason = No reason given :: STRING) :: (databaseName :: STRING, quarantined :: BOOLEAN, result :: STRING)
E   == Neo4j ==
E   dbms.quarantineDatabase(databaseName :: STRING, setStatus :: BOOLEAN, reason = No reason given :: STRING, wipeMode = DO_NOT_WIPE :: STRING) :: (databaseName :: STRING, quarantined :: BOOLEAN, result :: STRING)

I didn't change anything in the area of quarantineDatabase so it is interesting why it fails :)

@NataliaIvakina
Copy link
Contributor

Prodecures testing fails mismatch for quarantineDatabase.

AssertionError: Mismatch in signature for procedure dbms.quarantineDatabase.
== Docs ==
dbms.quarantineDatabase(databaseName :: STRING, setStatus :: BOOLEAN, reason = No reason given :: STRING) :: (databaseName :: STRING, quarantined :: BOOLEAN, result :: STRING)
== Neo4j ==
dbms.quarantineDatabase(databaseName :: STRING, setStatus :: BOOLEAN, reason = No reason given :: STRING, wipeMode = DO_NOT_WIPE :: STRING) :: (databaseName :: STRING, quarantined :: BOOLEAN, result :: STRING)
operations/test-procedures.py:75: in test_settings
    assert docs_procedures[name]['signature'] == procedure['signature'], \
E   AssertionError: Mismatch in signature for procedure dbms.quarantineDatabase.
E   == Docs ==
E   dbms.quarantineDatabase(databaseName :: STRING, setStatus :: BOOLEAN, reason = No reason given :: STRING) :: (databaseName :: STRING, quarantined :: BOOLEAN, result :: STRING)
E   == Neo4j ==
E   dbms.quarantineDatabase(databaseName :: STRING, setStatus :: BOOLEAN, reason = No reason given :: STRING, wipeMode = DO_NOT_WIPE :: STRING) :: (databaseName :: STRING, quarantined :: BOOLEAN, result :: STRING)

I didn't change anything in the area of quarantineDatabase so it is interesting why it fails :)

It failed because a new option was added to the quarantine procedure. We can ignore this failure. We have another PR to cover changes made to the quarantine procedure.

@NataliaIvakina
Copy link
Contributor

@akarasavov, since this PR is for the 5.26 release, please do not merge it now. We'll publish the 5.25 docs first and then merge everything related to 5.26.

@NataliaIvakina NataliaIvakina merged commit f8f5e89 into dev Oct 31, 2024
7 of 8 checks passed
@NataliaIvakina NataliaIvakina deleted the update_discovery_migration_guide branch October 31, 2024 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants