Fix to disable worker identity reuse for registration unless in k8s #18454

lucyge2022 · 2023-12-01T22:19:40Z

What changes are proposed in this pull request?

Put back the restriction of reuse worker identity for non-k8s env deployment.

Why are the changes needed?

Worker identity gets reused by wrong deployment behaviors such as copy conf/ over for new worker setup, as opposed to k8s deployment is thru operator / automation, bare metal deployment has no way of prevention, thus putting back the restriction for non-k8s deployment.

Does this PR introduce any user facing changes?

No.

ssz1997

Shall we make this property a global one, i.e. K8S_ENV? So that other components, like master, may use it in the future without bringing confusion.

dbw9580 · 2023-12-05T02:32:07Z

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

+          throw new AlreadyExistsException(
+              "Some other member with same id registered on the ring, bail."
+              + "Different workers can't assume same worker identity in non-k8s env."
+                  + "Clean local worker identity settings to continue.");


include in the exception message the conflicting worker's addresses and the ID

dbw9580 · 2023-12-05T02:34:00Z

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

+        if (mConf.isSet(PropertyKey.WORKER_IN_K8S_ENV)
+            && mConf.getBoolean(PropertyKey.WORKER_IN_K8S_ENV)) {
+          // 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.


how is worker ID generated in a Kubernetes deployment? Is there a chance there are actually two live workers with the same ID but different hostnames?

There's no special mechanism to generate IDs in k8s, so it's the random string, so theoretically that shouldn't happen.

no special mechanism to generate IDs in k8s, so it's the random string

Do you mean that the ID is not explicitly set via any Alluxio configuration properties by the operator or the helm chart, but gets generated at the first time the worker instance is created?

If so, sounds good to me as people rely on the k8s operator or the helm chart to configure new worker instances, rathar than copying Alluxio configs between workers.

If so, sounds good to me as people rely on the k8s operator or the helm chart to configure new worker instances, rathar than copying Alluxio configs between workers.

exactly, after ur worker id changes cluster deployment needs to rely on coordinated workflow. since theres no such thing for bare-metal, we have to safe-guard everything from code point of view.

dbw9580 · 2023-12-05T02:45:28Z

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

@@ -93,16 +94,25 @@ public void join(WorkerInfo workerInfo) throws IOException {
    if (existingEntityBytes != null) {


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

Lines 94 to 95 in 87702fd

if (existingEntityBytes != null) {

// It's not me, or not the same me.

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

Lines 118 to 120 in 87702fd

} else {

// If haven't created myself onto the ring before, create now.

mAlluxioEtcdClient.createForPath(pathOnRing, Optional.of(serializedEntity));

is there a time-of-check to time-of-use race between these lines? Two workers with conflicting IDs may be registering at the same time, and both can happen to see no conflicting records exist on etcd so proceed to register with the ID, accidentally overwriting each other. I assume etcd transaction is not used for registering a worker.

yeah u r right, wasn't using txn coz before ur workerid change, no two workers will have same workernetaddr, which was used as the unique identifier then, to begin with. Will change to txn semantics here, thanks

@dbw9580 can u double check this logics to handle race for me plz?

lucyge2022 · 2023-12-05T18:37:23Z

Shall we make this property a global one, i.e. K8S_ENV? So that other components, like master, may use it in the future without bringing confusion.

done

alluxio-bot · 2023-12-05T18:59:36Z

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 must be capitalized
  - First word of title ("only") 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.

lucyge2022 · 2023-12-05T23:53:13Z

alluxio-bot, check this please

alluxio-bot · 2023-12-05T23:53:16Z

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:
  - Title is too long (73 characters). Must be at most 72 characters.
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.

dbw9580

LGTM, only have a few small questions. Thanks for the work!

dbw9580 · 2023-12-07T06:03:37Z

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

+      ByteSequence keyToPut = ByteSequence.from(pathOnRing, StandardCharsets.UTF_8);
+      ByteSequence valToPut = ByteSequence.from(serializedEntity);
+      CompletableFuture<TxnResponse> txnResponseFut = txn
+          .If(new Cmp(keyToPut, Cmp.Op.EQUAL, CmpTarget.version(0L)))


would appreciate a comment for etcd outsiders:

Suggested change

.If(new Cmp(keyToPut, Cmp.Op.EQUAL, CmpTarget.version(0L)))

// version of a key starts from 1; so version being 0 means

// the key does not exist (never existed or was deleted).

.If(new Cmp(keyToPut, Cmp.Op.EQUAL, CmpTarget.version(0L)))

what's the difference between version == 0 and createRevision == 0?

'revision' refers to a global number put onto each op for all keys(create/delete), createRevision = revision of the last creation on the key, version = the version of this particular key, if deleted, the version is reset to 0
create /k1 -> /k1 version=1 global revision = 1
create /k2 -> /k2 ... global revision=2
delete /k2 -> /k2 ... global revision=3
modify /k1 -> /k1 version=2 global revision = 4 => /k1 version = 2 createRevision=1
delete /k1 -> /k1 version=0 global revision = 5
create /k1 -> /k1 version=1 global revision = 6 => /k1 version = 1 createRevision=6

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

jiacheliu3 · 2023-12-07T08:25:00Z

dora/core/common/src/main/java/alluxio/conf/PropertyKey.java

@@ -1048,6 +1048,15 @@ public String toString() {
          .setConsistencyCheckLevel(ConsistencyCheckLevel.ENFORCE)
          .setScope(Scope.MASTER)
          .build();
+  public static final PropertyKey K8S_ENV_DEPLOYMENT =


i have the feeling that whether we allow worker to reg with the same identity is relevant to whether it's k8s, but k8s is not 100% the decider. In other words, I feel we can have

PropertyKey ON_K8S and PropertyKey WORKER_SAME_IDENTITY_ACTION = ON_K8S ? OVERWRITE : REJECT

actually identity should never be allowed to overwrite, from code side shouldnt even open this can of worm, it should be a deployment effort. k8s property can also be extended for other usage as suggested by @ssz1997

alluxio-bot · 2023-12-07T20:26:20Z

Automated checks report:

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

All checks passed!

lucyge2022 · 2023-12-07T20:26:28Z

alluxio-bot, check this please

alluxio-bot · 2023-12-07T20:26:31Z

Automated checks report:

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

All checks passed!

…ipManager.java Co-authored-by: Bowen Ding <[email protected]>

lucyge2022 · 2023-12-09T06:16:53Z

alluxio-bot, merge this please.

### What changes are proposed in this pull request? Put back the restriction of reuse worker identity for non-k8s env deployment. ### Why are the changes needed? Worker identity gets reused by wrong deployment behaviors such as copy conf/ over for new worker setup, as opposed to k8s deployment is thru operator / automation, bare metal deployment has no way of prevention, thus putting back the restriction for non-k8s deployment. ### Does this PR introduce any user facing changes? No. pr-link: Alluxio#18454 change-id: cid-295ea352895b16c2a5f0a23fa790c9f42a5e3881

lucyge2022 requested review from ssz1997 and dbw9580 December 1, 2023 22:19

ssz1997 reviewed Dec 1, 2023

View reviewed changes

dbw9580 suggested changes Dec 5, 2023

View reviewed changes

lucyge2022 changed the title ~~only allow multiple workers assume same worker identity in k8s env~~ Fix to only allow multiple workers assume same worker identity in k8s env Dec 5, 2023

dbw9580 approved these changes Dec 7, 2023

View reviewed changes

jiacheliu3 reviewed Dec 7, 2023

View reviewed changes

lucyge2022 changed the title ~~Fix to only allow multiple workers assume same worker identity in k8s env~~ Fix to disable worker identity reuse for registration unless in k8s Dec 7, 2023

ssz1997 mentioned this pull request Dec 8, 2023

Give k8s env property a default true value Alluxio/k8s-operator#96

Merged

lucyge2022 and others added 8 commits December 8, 2023 21:42

only allow multiple workers assume same worker identity in k8s env

6099ebd

address review comments -1

ce53e63

compile error

b776240

address review comments - 2

6c3e75f

correct CmpTarget

a76c7d8

log exception out in register phase

c8544a0

Update dora/core/common/src/main/java/alluxio/membership/EtcdMembersh…

eabee1b

…ipManager.java Co-authored-by: Bowen Ding <[email protected]>

review comments

33fd329

lucyge2022 force-pushed the disable-workerid-reuse branch from 83c847b to 33fd329 Compare December 9, 2023 05:42

lucyge2022 added the type-bug This issue is about a bug label Dec 9, 2023

alluxio-bot merged commit 22af1b1 into Alluxio:main Dec 9, 2023
12 checks passed

dbw9580 mentioned this pull request Dec 11, 2023

Use cluster view to replace list of workers #18451

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to disable worker identity reuse for registration unless in k8s #18454

Fix to disable worker identity reuse for registration unless in k8s #18454

lucyge2022 commented Dec 1, 2023

ssz1997 left a comment

dbw9580 Dec 5, 2023

lucyge2022 Dec 5, 2023

dbw9580 Dec 5, 2023

ssz1997 Dec 5, 2023

dbw9580 Dec 7, 2023

lucyge2022 Dec 7, 2023

dbw9580 Dec 5, 2023

lucyge2022 Dec 5, 2023

lucyge2022 Dec 5, 2023

lucyge2022 commented Dec 5, 2023

alluxio-bot commented Dec 5, 2023

lucyge2022 commented Dec 5, 2023

alluxio-bot commented Dec 5, 2023

dbw9580 left a comment

dbw9580 Dec 7, 2023

dbw9580 Dec 7, 2023

lucyge2022 Dec 7, 2023

lucyge2022 Dec 7, 2023

jiacheliu3 Dec 7, 2023

lucyge2022 Dec 7, 2023

alluxio-bot commented Dec 7, 2023

lucyge2022 commented Dec 7, 2023

alluxio-bot commented Dec 7, 2023

lucyge2022 commented Dec 9, 2023

		@@ -93,16 +94,25 @@ public void join(WorkerInfo workerInfo) throws IOException {
		if (existingEntityBytes != null) {

	if (existingEntityBytes != null) {
	// It's not me, or not the same me.

	} else {
	// If haven't created myself onto the ring before, create now.
	mAlluxioEtcdClient.createForPath(pathOnRing, Optional.of(serializedEntity));

Fix to disable worker identity reuse for registration unless in k8s #18454

Fix to disable worker identity reuse for registration unless in k8s #18454

Conversation

lucyge2022 commented Dec 1, 2023

What changes are proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user facing changes?

ssz1997 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucyge2022 commented Dec 5, 2023

alluxio-bot commented Dec 5, 2023

lucyge2022 commented Dec 5, 2023

alluxio-bot commented Dec 5, 2023

dbw9580 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

alluxio-bot commented Dec 7, 2023

lucyge2022 commented Dec 7, 2023

alluxio-bot commented Dec 7, 2023

lucyge2022 commented Dec 9, 2023