Remove restriction in overwriting worker info within join logic #18275

lucyge2022 · 2023-10-16T20:30:36Z

What changes are proposed in this pull request?

Now with worker id can be assumed from a different worker instance whether on a different pod in k8s or a different host machine for baremetal. The creation onto the persisted ring path : /DHT/DefaultAlluxioCluster/AUTHORIZED/ should not bail if a different value is seen.

Why are the changes needed?

to enable rejoin of a worker bearing same worker id but with different host or other WorkerInfo fileds.

Does this PR introduce any user facing changes?

No

jiacheliu3

Thanks for the work! I left a comment on a corner case that might take a bit of discussion. I feel we may want to add a bit more in-line comments on that situation?

jiacheliu3 · 2023-10-17T01:12:01Z

dora/core/common/src/main/java/alluxio/membership/EtcdMembershipManager.java

+        LOG.warn("Same worker entity found bearing same workerid,"
+            + "maybe benign if pod restart in k8s env or same worker"
+            + " scheduled to restart on another machine in baremetal env.");
+        mAlluxioEtcdClient.createForPath(pathOnRing, Optional.of(serializedEntity));


if you now observe a new worker with the same identity, would there be an old worker with the same id on the ring? If so then that obsolete one needs to be removed? Hmmmm but what about the corner case that the admin incorrectly used the same ID on multiple workers, like copying the config blindly?

yes it will override, for enterprise customer normally we should always do automated scripts such as ansible to run deployment instead of letting customer do stuff like copy properties. typical adding-node or new-deployment should always do a wipe-out clean to clean system info files such as worker_identity file

nit: log the old one so we could tell from the logs if something is unexpected

dbw9580 · 2023-10-17T14:24:07Z

dora/core/common/src/main/java/alluxio/membership/EtcdMembershipManager.java

@@ -93,11 +92,14 @@ public void join(WorkerInfo workerInfo) throws IOException {
    byte[] serializedEntity = entity.serialize();
    // If there's existing entry, check if it's me.
    if (existingEntityBytes != null) {
-      // It's not me, something is wrong.
+      // It's not me, or not the same me.
      if (!Arrays.equals(existingEntityBytes, serializedEntity)) {


why compare by the serialized form? The WorkerServiceEntity implements equals and ignores comparing WorkerNetAddress deliberately. If compared by the serialized form, the netaddress would be taken into account, which I find confusing.

in bare metal cases, if a worker restart none of the fields in workerserviceentity will get changed, and the content should be the same, so we don't need to insert the kv pair again to etcd. if not , then thats a case of the worker changing hostname like in k8s, hence comparing the serialized form, just to say is it the same me on etcd? or its a different me, and im about to overwriting the content

dbw9580 · 2023-10-17T14:26:18Z

dora/core/common/src/main/java/alluxio/membership/EtcdMembershipManager.java

-            "Some other member with same id registered on the ring, bail.");
+        // In k8s this might be bcos worker pod restarting with the same worker identity
+        // but certain fields such as hostname has been changed. Register to ring path anyway.
+        LOG.warn("Same worker entity found bearing same workerid,"


If the serialized forms don't compare equal, it doesn't mean the two have different worker IDs. Could be different network address, or anything ephemeral in WorkerServiceEntity.

yeah exactly, we found the key already exists, so now we have to overwrite it with a different WorkerServiceEntity content, before it doesnt allow doing so.

Ok I see what you are trying to achieve. If the serialized forms are equal, then you don't have to overwrite anything. Makes sense.

dbw9580

LGTM

jiacheliu3

LGTM, thanks for the work!

jiacheliu3 · 2023-10-23T01:09:37Z

dora/core/common/src/main/java/alluxio/membership/EtcdMembershipManager.java

+        LOG.warn("Same worker entity found bearing same workerid,"
+            + "maybe benign if pod restart in k8s env or same worker"
+            + " scheduled to restart on another machine in baremetal env.");
+        mAlluxioEtcdClient.createForPath(pathOnRing, Optional.of(serializedEntity));


nit: log the old one so we could tell from the logs if something is unexpected

alluxio-bot · 2023-10-23T17:18:32Z

Automated checks report:

PR title follows the conventions: FAIL
- The title of the PR does not pass all the checks. Please fix the following issues:
  - First word of title ("Restart") is not an imperative verb. Please use one of the valid words
Commits associated with Github account: PASS

Some checks failed. Please fix the reported issues and reply 'alluxio-bot, check this please' to re-run checks.

alluxio-bot · 2023-10-23T17:27:29Z

Automated checks report:

PR title follows the conventions: PASS
Commits associated with Github account: PASS

All checks passed!

lucyge2022 · 2023-10-23T17:52:42Z

alluxio-bot, merge this please.

### What changes are proposed in this pull request? Now with worker id can be assumed from a different worker instance whether on a different pod in k8s or a different host machine for baremetal. The creation onto the persisted ring path : /DHT/DefaultAlluxioCluster/AUTHORIZED/ should not bail if a different value is seen. ### Why are the changes needed? to enable rejoin of a worker bearing same worker id but with different host or other WorkerInfo fileds. ### Does this PR introduce any user facing changes? No pr-link: Alluxio#18275 change-id: cid-51322e010e0d51ae4f81268c2bb607b568f08c46

lucyge2022 requested review from jiacheliu3 and dbw9580 October 16, 2023 20:30

jiacheliu3 reviewed Oct 17, 2023

View reviewed changes

dbw9580 reviewed Oct 17, 2023

View reviewed changes

elega force-pushed the main branch from b597c61 to 4271e79 Compare October 17, 2023 15:08

apc999 force-pushed the main branch 2 times, most recently from 2fec0ec to b597c61 Compare October 17, 2023 19:11

dbw9580 approved these changes Oct 20, 2023

View reviewed changes

jiacheliu3 approved these changes Oct 23, 2023

View reviewed changes

lucyge2022 added 3 commits October 23, 2023 10:19

remove checks to register on the ring in case k8s pod restart

313618b

fix checkstyle

48e4d17

review comments for give specific info in warn log

c5d1162

lucyge2022 force-pushed the restart-join-logic branch from d7bf480 to c5d1162 Compare October 23, 2023 17:19

lucyge2022 changed the title ~~Restart join logic~~ Remove restriction in overwriting worker info within join logic Oct 23, 2023

lucyge2022 added the type-feature This issue is a feature request label Oct 23, 2023

alluxio-bot merged commit 0884fc3 into Alluxio:main Oct 23, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove restriction in overwriting worker info within join logic #18275

Remove restriction in overwriting worker info within join logic #18275

lucyge2022 commented Oct 16, 2023

jiacheliu3 left a comment

jiacheliu3 Oct 17, 2023

lucyge2022 Oct 17, 2023

jiacheliu3 Oct 23, 2023

lucyge2022 Oct 23, 2023

dbw9580 Oct 17, 2023

lucyge2022 Oct 17, 2023

dbw9580 Oct 17, 2023 •

edited

Loading

lucyge2022 Oct 17, 2023

dbw9580 Oct 20, 2023

lucyge2022 Oct 23, 2023

dbw9580 left a comment

jiacheliu3 left a comment

jiacheliu3 Oct 23, 2023

alluxio-bot commented Oct 23, 2023

alluxio-bot commented Oct 23, 2023

lucyge2022 commented Oct 23, 2023

Remove restriction in overwriting worker info within join logic #18275

Remove restriction in overwriting worker info within join logic #18275

Conversation

lucyge2022 commented Oct 16, 2023

What changes are proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user facing changes?

jiacheliu3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbw9580 Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbw9580 left a comment

Choose a reason for hiding this comment

jiacheliu3 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alluxio-bot commented Oct 23, 2023

alluxio-bot commented Oct 23, 2023

lucyge2022 commented Oct 23, 2023

dbw9580 Oct 17, 2023 •

edited

Loading