[GitHub] brooklyn-server pull request #617: Persist management plane ID

[asfgit]

Github user asfgit closed the pull request at:

https://github.com/apache/brooklyn-server/pull/617


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[geomacy]

Github user geomacy commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r112392507
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/mgmt/internal/LocalManagementContext.java
 ---
@@ -224,6 +224,7 @@ public String getManagementPlaneId() {
 public void setManagementPlaneId(String newPlaneId) {
 if (managementPlaneId != null && 
!managementPlaneId.equals(newPlaneId)) {
 log.warn("Management plane ID changed from {} to {}", 
managementPlaneId, newPlaneId);
+log.debug("Management plane ID changed from {} to {}", new 
Object[] {managementPlaneId, newPlaneId}, new RuntimeException("Stack trace for 
setManagementPlaneId"));
--- End diff --

Hm, interesting!  That's new to me - a good one to know!


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r112185296
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/mgmt/internal/LocalManagementContext.java
 ---
@@ -224,6 +224,7 @@ public String getManagementPlaneId() {
 public void setManagementPlaneId(String newPlaneId) {
 if (managementPlaneId != null && 
!managementPlaneId.equals(newPlaneId)) {
 log.warn("Management plane ID changed from {} to {}", 
managementPlaneId, newPlaneId);
+log.debug("Management plane ID changed from {} to {}", new 
Object[] {managementPlaneId, newPlaneId}, new RuntimeException("Stack trace for 
setManagementPlaneId"));
--- End diff --

Good spot @geomacy. The fix might surprise you though (it surely did 
surprise me):
```
log.debug("Management plane ID changed from {} to {}", new Object[] 
{managementPlaneId, newPlaneId, new RuntimeException("Stack trace for 
setManagementPlaneId")});
```

Turns out slf4j will [do the right 
thing](https://www.slf4j.org/faq.html#paramException) in this case and log the 
last argument as an exception. Verified the debug log contains:

```
2017-04-19 15:11:34,011 DEBUG o.a.b.c.m.i.LocalManagementContext 
[brooklyn-execmanager-KWNkyI3R-3]: Management plane ID changed from X7dpWzuF to 
wJl3j3K1
java.lang.RuntimeException: Stack trace for setManagementPlaneId
at 
org.apache.brooklyn.core.mgmt.internal.LocalManagementContext.setManagementPlaneId(LocalManagementContext.java:227)
 [classes/:na]
at 
org.apache.brooklyn.core.mgmt.ha.HighAvailabilityManagerImpl.updateLocalPlaneId(HighAvailabilityManagerImpl.java:465)
 [classes/:na]
at 
org.apache.brooklyn.core.mgmt.ha.HighAvailabilityManagerImpl.checkMaster(HighAvailabilityManagerImpl.java:725)
 [classes/:na]
at 
org.apache.brooklyn.core.mgmt.ha.HighAvailabilityManagerImpl.publishAndCheck(HighAvailabilityManagerImpl.java:602)
 [classes/:na]
at 
org.apache.brooklyn.core.mgmt.ha.HighAvailabilityManagerImpl$2.run(HighAvailabilityManagerImpl.java:557)
 [classes/:na]
at 
java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:471) 
[na:1.7.0_80]
at 
org.apache.brooklyn.util.core.task.BasicExecutionManager$ScheduledTaskCallable$1.call(BasicExecutionManager.java:451)
 [classes/:na]
at 
org.apache.brooklyn.util.core.task.BasicExecutionManager$SubmissionCallable.call(BasicExecutionManager.java:529)
 [classes/:na]
at java.util.concurrent.FutureTask.run(FutureTask.java:262) 
[na:1.7.0_80]
at 
java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1145) 
[na:1.7.0_80]
at 
java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:615) 
[na:1.7.0_80]
at java.lang.Thread.run(Thread.java:745) [na:1.7.0_80]

```


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r110929027
  
--- Diff: 
core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/ManagementPlaneIdTest.java
 ---
@@ -0,0 +1,247 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.core.mgmt.rebind;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertNotEquals;
+
+import java.io.File;
+import java.io.FilenameFilter;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.concurrent.Callable;
+
+import org.apache.brooklyn.api.mgmt.ManagementContext;
+import org.apache.brooklyn.api.mgmt.ha.HighAvailabilityMode;
+import org.apache.brooklyn.api.mgmt.ha.ManagementNodeState;
+import org.apache.brooklyn.core.entity.Entities;
+import org.apache.brooklyn.core.internal.BrooklynProperties;
+import org.apache.brooklyn.core.mgmt.internal.LocalManagementContext;
+import 
org.apache.brooklyn.core.mgmt.persist.BrooklynMementoPersisterToObjectStore;
+import org.apache.brooklyn.core.mgmt.persist.FileBasedObjectStore;
+import org.apache.brooklyn.core.mgmt.persist.PersistMode;
+import org.apache.brooklyn.core.server.BrooklynServerConfig;
+import org.apache.brooklyn.core.server.BrooklynServerPaths;
+import org.apache.brooklyn.test.Asserts;
+import org.apache.brooklyn.util.os.Os;
+import org.apache.brooklyn.util.text.Strings;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+public class ManagementPlaneIdTest {
+private File mementoDir;
+
+protected ClassLoader classLoader = getClass().getClassLoader();
+
+private Collection managementContextForTermination;
+
+@BeforeMethod
+public void setUp() {
+mementoDir = Os.newTempDir(getClass());
+managementContextForTermination = new ArrayList<>();
+}
+
+@AfterMethod(alwaysRun=true)
+public void tearDown() throws Exception {
+if (managementContextForTermination != null) {
+for (ManagementContext mgmt : managementContextForTermination) 
{
+Entities.destroyAll(mgmt);
+}
+}
+if (mementoDir != null) 
FileBasedObjectStore.deleteCompletely(mementoDir);
+}
+
+@Test
+public void testUninitializedThrows() {
+ManagementContext mgmt = new 
LocalManagementContext(BrooklynProperties.Factory.newEmpty());
+assertFalse(mgmt.getOptionalManagementPlaneId().isPresent(), 
"expected managementPlaneId to be absent");
+}
+
+@Test
+public void testPlaneIdPersists() throws Exception {
+final ManagementContext mgmt = 
createManagementContext(PersistMode.AUTO, HighAvailabilityMode.DISABLED);
+checkPlaneIdPersisted(mgmt);
+}
+
+@Test(enabled=false) //Need to wait for 1hr to verify the update will 
not overwrite
--- End diff --

Reversed the test to verify that the local planeId is "fixed" by the 
persisted state so it's quick now.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r110928904
  
--- Diff: 
core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/ManagementPlaneIdTest.java
 ---
@@ -0,0 +1,247 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.core.mgmt.rebind;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertNotEquals;
+
+import java.io.File;
+import java.io.FilenameFilter;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.concurrent.Callable;
+
+import org.apache.brooklyn.api.mgmt.ManagementContext;
+import org.apache.brooklyn.api.mgmt.ha.HighAvailabilityMode;
+import org.apache.brooklyn.api.mgmt.ha.ManagementNodeState;
+import org.apache.brooklyn.core.entity.Entities;
+import org.apache.brooklyn.core.internal.BrooklynProperties;
+import org.apache.brooklyn.core.mgmt.internal.LocalManagementContext;
+import 
org.apache.brooklyn.core.mgmt.persist.BrooklynMementoPersisterToObjectStore;
+import org.apache.brooklyn.core.mgmt.persist.FileBasedObjectStore;
+import org.apache.brooklyn.core.mgmt.persist.PersistMode;
+import org.apache.brooklyn.core.server.BrooklynServerConfig;
+import org.apache.brooklyn.core.server.BrooklynServerPaths;
+import org.apache.brooklyn.test.Asserts;
+import org.apache.brooklyn.util.os.Os;
+import org.apache.brooklyn.util.text.Strings;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+public class ManagementPlaneIdTest {
--- End diff --

That's how the test started, but I needed some functionality which wasn't 
available in `RebindTestFixture` (can't remember the details now)


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r110928608
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java ---
@@ -467,6 +469,15 @@ protected void instantiateMementos() throws 
IOException {
 memento = persistenceStoreAccess.loadMemento(mementoRawData, 
rebindContext.lookup(), exceptionHandler);
 }
 
+protected void initPlaneId() {
+String persistedPlaneId = mementoRawData.getPlaneId();
+if (persistedPlaneId == null) {
+
((LocalManagementContext)managementContext).generateManagementPlaneId();
--- End diff --

Added a warning here if there's already existing state, but no planeId.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r110928407
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/mgmt/persist/BrooklynMementoPersisterToObjectStore.java
 ---
@@ -674,6 +685,22 @@ private void delete(String subPath, String id, 
PersistenceExceptionHandler excep
 }
 }
 
+private void updatePlaneId(String planeId, PersistenceExceptionHandler 
exceptionHandler) {
+try {
+if (planeId==null) {
+LOG.warn("Null content for planeId");
+}
+
+String persistedPlaneId = read(PLANE_ID_FILE_NAME);
+if (persistedPlaneId != null && 
!persistedPlaneId.equals(planeId)) {
+throw new IllegalStateException("Persisted planeId found 
(" + persistedPlaneId + ") but instance planeId is different (" + planeId + 
")");
+}
+getWriter(PLANE_ID_FILE_NAME).put(planeId);
--- End diff --

+1


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r110708530
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/mgmt/persist/BrooklynMementoPersisterToObjectStore.java
 ---
@@ -674,6 +685,22 @@ private void delete(String subPath, String id, 
PersistenceExceptionHandler excep
 }
 }
 
+private void updatePlaneId(String planeId, PersistenceExceptionHandler 
exceptionHandler) {
+try {
+if (planeId==null) {
+LOG.warn("Null content for planeId");
+}
+
+String persistedPlaneId = read(PLANE_ID_FILE_NAME);
+if (persistedPlaneId != null && 
!persistedPlaneId.equals(planeId)) {
+throw new IllegalStateException("Persisted planeId found 
(" + persistedPlaneId + ") but instance planeId is different (" + planeId + 
")");
+}
+getWriter(PLANE_ID_FILE_NAME).put(planeId);
--- End diff --

Can we skip the `write` if the `read` returned the expected plane id?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r110710330
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/RebindIteration.java ---
@@ -467,6 +469,15 @@ protected void instantiateMementos() throws 
IOException {
 memento = persistenceStoreAccess.loadMemento(mementoRawData, 
rebindContext.lookup(), exceptionHandler);
 }
 
+protected void initPlaneId() {
+String persistedPlaneId = mementoRawData.getPlaneId();
+if (persistedPlaneId == null) {
+
((LocalManagementContext)managementContext).generateManagementPlaneId();
--- End diff --

On balance, I agree with this code. But it feels like we're relying on null 
too much. We don't distinguish between persisted state that has no entities 
etc, and an unpopulated persisted state directory - that makes it hard to write 
code that asserts/warns it's in the state we expect.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r110711348
  
--- Diff: 
core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/ManagementPlaneIdTest.java
 ---
@@ -0,0 +1,247 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.core.mgmt.rebind;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertNotEquals;
+
+import java.io.File;
+import java.io.FilenameFilter;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.concurrent.Callable;
+
+import org.apache.brooklyn.api.mgmt.ManagementContext;
+import org.apache.brooklyn.api.mgmt.ha.HighAvailabilityMode;
+import org.apache.brooklyn.api.mgmt.ha.ManagementNodeState;
+import org.apache.brooklyn.core.entity.Entities;
+import org.apache.brooklyn.core.internal.BrooklynProperties;
+import org.apache.brooklyn.core.mgmt.internal.LocalManagementContext;
+import 
org.apache.brooklyn.core.mgmt.persist.BrooklynMementoPersisterToObjectStore;
+import org.apache.brooklyn.core.mgmt.persist.FileBasedObjectStore;
+import org.apache.brooklyn.core.mgmt.persist.PersistMode;
+import org.apache.brooklyn.core.server.BrooklynServerConfig;
+import org.apache.brooklyn.core.server.BrooklynServerPaths;
+import org.apache.brooklyn.test.Asserts;
+import org.apache.brooklyn.util.os.Os;
+import org.apache.brooklyn.util.text.Strings;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+public class ManagementPlaneIdTest {
--- End diff --

Can we extend `RebindTestFixture`? Would that save us much code, e.g. 
simplifying the `createManagementContext`? I'm guessing not, because it will 
have already created the managementContext in setUp. No strong feelings from me.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[aledsage]

Github user aledsage commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r110710806
  
--- Diff: 
core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/ManagementPlaneIdTest.java
 ---
@@ -0,0 +1,247 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.core.mgmt.rebind;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertFalse;
+import static org.testng.Assert.assertNotEquals;
+
+import java.io.File;
+import java.io.FilenameFilter;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.concurrent.Callable;
+
+import org.apache.brooklyn.api.mgmt.ManagementContext;
+import org.apache.brooklyn.api.mgmt.ha.HighAvailabilityMode;
+import org.apache.brooklyn.api.mgmt.ha.ManagementNodeState;
+import org.apache.brooklyn.core.entity.Entities;
+import org.apache.brooklyn.core.internal.BrooklynProperties;
+import org.apache.brooklyn.core.mgmt.internal.LocalManagementContext;
+import 
org.apache.brooklyn.core.mgmt.persist.BrooklynMementoPersisterToObjectStore;
+import org.apache.brooklyn.core.mgmt.persist.FileBasedObjectStore;
+import org.apache.brooklyn.core.mgmt.persist.PersistMode;
+import org.apache.brooklyn.core.server.BrooklynServerConfig;
+import org.apache.brooklyn.core.server.BrooklynServerPaths;
+import org.apache.brooklyn.test.Asserts;
+import org.apache.brooklyn.util.os.Os;
+import org.apache.brooklyn.util.text.Strings;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+public class ManagementPlaneIdTest {
+private File mementoDir;
+
+protected ClassLoader classLoader = getClass().getClassLoader();
+
+private Collection managementContextForTermination;
+
+@BeforeMethod
+public void setUp() {
+mementoDir = Os.newTempDir(getClass());
+managementContextForTermination = new ArrayList<>();
+}
+
+@AfterMethod(alwaysRun=true)
+public void tearDown() throws Exception {
+if (managementContextForTermination != null) {
+for (ManagementContext mgmt : managementContextForTermination) 
{
+Entities.destroyAll(mgmt);
+}
+}
+if (mementoDir != null) 
FileBasedObjectStore.deleteCompletely(mementoDir);
+}
+
+@Test
+public void testUninitializedThrows() {
+ManagementContext mgmt = new 
LocalManagementContext(BrooklynProperties.Factory.newEmpty());
+assertFalse(mgmt.getOptionalManagementPlaneId().isPresent(), 
"expected managementPlaneId to be absent");
+}
+
+@Test
+public void testPlaneIdPersists() throws Exception {
+final ManagementContext mgmt = 
createManagementContext(PersistMode.AUTO, HighAvailabilityMode.DISABLED);
+checkPlaneIdPersisted(mgmt);
+}
+
+@Test(enabled=false) //Need to wait for 1hr to verify the update will 
not overwrite
--- End diff --

Perhaps a sign that we should make that period configurable.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r110387016
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/PeriodicDeltaChangeListener.java
 ---
@@ -382,6 +401,8 @@ protected void persistNowInternal(boolean 
alreadyHasMutex) {
 if (!alreadyHasMutex) persistingMutex.acquire();
 if (!isActive() && state != ListenerState.STOPPING) return;
 
+updatePlaneIdIfTimedOut();
--- End diff --

I agree it makes sense but would do it for all writes, not just the 
planeId. Not comfortable with doing it just for planeId - would break the 
existing assumption about writes.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[geomacy]

Github user geomacy commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r110366800
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/PeriodicDeltaChangeListener.java
 ---
@@ -382,6 +401,8 @@ protected void persistNowInternal(boolean 
alreadyHasMutex) {
 if (!alreadyHasMutex) persistingMutex.acquire();
 if (!isActive() && state != ListenerState.STOPPING) return;
 
+updatePlaneIdIfTimedOut();
--- End diff --

Hm; if they are lossy then for planeId would we need to write it using a 
"keep trying and double-check until successful" approach?  I.e. don't progress 
until write is confirmed?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r110364489
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/PeriodicDeltaChangeListener.java
 ---
@@ -382,6 +401,8 @@ protected void persistNowInternal(boolean 
alreadyHasMutex) {
 if (!alreadyHasMutex) persistingMutex.acquire();
 if (!isActive() && state != ListenerState.STOPPING) return;
 
+updatePlaneIdIfTimedOut();
--- End diff --

Writes to the datastore are lossy. We'll just log failures and move on. 
(Most) entities will get updated multiple times in their lifecycle so not a 
huge deal. planeId does not get updated so if the first write fails it's not 
available to the HA cluster at all. That's why I decided to periodically write 
it to the datastore. Will add a comment.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r110364101
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/mgmt/persist/BrooklynMementoPersisterToObjectStore.java
 ---
@@ -674,6 +685,22 @@ private void delete(String subPath, String id, 
PersistenceExceptionHandler excep
 }
 }
 
+private void updatePlaneId(String planeId, PersistenceExceptionHandler 
exceptionHandler) {
+try {
+if (planeId==null) {
+LOG.warn("Null content for planeId");
+}
+
+String persistedPlaneId = read(PLANE_ID_FILE_NAME);
+if (persistedPlaneId != null && 
!persistedPlaneId.equals(planeId)) {
+throw new IllegalStateException("Persisted planeId found 
(" + persistedPlaneId + ") but instance planeId is different (" + planeId + "). 
Overwriting!");
--- End diff --

Good catch - it used to overwrite but then I changed my mind. The persisted 
state is considered the "ground truth" now, so won't overwrite the planeId if 
different.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r110363846
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/mgmt/ha/HighAvailabilityManagerImpl.java
 ---
@@ -456,6 +460,12 @@ else if (startMode==HighAvailabilityMode.HOT_STANDBY 
&& getNodeState()==Manageme
 registerPollTask();
 }
 
+protected void updatePlaneId(ManagementPlaneSyncRecord existingMaster) 
{
--- End diff --

Renaming to `updateLocalPlaneId`. It would be surprising to update with a 
different planeId, but I decided to support the use case where someone goes and 
edits the persisted state planeId. In case the planeId is different it will log 
a warning, but will accept the change.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[geomacy]

Github user geomacy commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r110160727
  
--- Diff: 
core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/ManagementPlaneIdTest.java
 ---
@@ -0,0 +1,251 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.core.mgmt.rebind;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotEquals;
+
+import java.io.File;
+import java.io.FilenameFilter;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.concurrent.Callable;
+
+import org.apache.brooklyn.api.mgmt.ManagementContext;
+import org.apache.brooklyn.api.mgmt.ha.HighAvailabilityMode;
+import org.apache.brooklyn.api.mgmt.ha.ManagementNodeState;
+import org.apache.brooklyn.core.entity.Entities;
+import org.apache.brooklyn.core.internal.BrooklynProperties;
+import org.apache.brooklyn.core.mgmt.internal.LocalManagementContext;
+import 
org.apache.brooklyn.core.mgmt.persist.BrooklynMementoPersisterToObjectStore;
+import org.apache.brooklyn.core.mgmt.persist.FileBasedObjectStore;
+import org.apache.brooklyn.core.mgmt.persist.PersistMode;
+import org.apache.brooklyn.core.server.BrooklynServerConfig;
+import org.apache.brooklyn.core.server.BrooklynServerPaths;
+import org.apache.brooklyn.test.Asserts;
+import org.apache.brooklyn.util.os.Os;
+import org.apache.brooklyn.util.text.Strings;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+public class ManagementPlaneIdTest {
+private File mementoDir;
+
+protected ClassLoader classLoader = getClass().getClassLoader();
+
+private Collection managementContextForTermination;
+
+@BeforeMethod
+public void setUp() {
+mementoDir = Os.newTempDir(getClass());
+managementContextForTermination = new ArrayList<>();
+}
+
+@AfterMethod(alwaysRun=true)
+public void tearDown() throws Exception {
+if (managementContextForTermination != null) {
+for (ManagementContext mgmt : managementContextForTermination) 
{
+Entities.destroyAll(mgmt);
+}
+}
+if (mementoDir != null) 
FileBasedObjectStore.deleteCompletely(mementoDir);
+}
+
+@Test
+public void testUninitializedThrows() {
+ManagementContext mgmt = new 
LocalManagementContext(BrooklynProperties.Factory.newEmpty());
+try {
+mgmt.getManagementPlaneId();
+Asserts.shouldHaveFailedPreviously("managementPlaneId not 
initialized");
+} catch (NullPointerException e) {
+Asserts.expectedFailureContains(e, "not initialized");
+}
+}
+
+@Test
+public void testPlaneIdPersists() throws Exception {
+final ManagementContext mgmt = 
createManagementContext(PersistMode.AUTO, HighAvailabilityMode.DISABLED);
+checkRebindIdPersisted(mgmt);
+}
+
+@Test(enabled=false) //Need to wait for 1hr to verify the update will 
not overwrite
+public void testPlaneIdNotOverwritten() throws Exception {
+final LocalManagementContext mgmt = 
createManagementContext(PersistMode.AUTO, HighAvailabilityMode.DISABLED);
+
+final File planeIdFile = new File(mementoDir, 
BrooklynMementoPersisterToObjectStore.PLANE_ID_FILE_NAME);
+checkRebindIdPersisted(mgmt);
+final String oldPlaneId = mgmt.getManagementPlaneId();
+mgmt.setManagementPlaneId(Strings.makeRandomId(8));
+assertNotEquals(oldPlaneId, mgmt.getManagementPlaneId());
+Asserts.succeedsContinually(new Callable() {

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[geomacy]

Github user geomacy commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r110162824
  
--- Diff: 
core/src/test/java/org/apache/brooklyn/core/mgmt/rebind/ManagementPlaneIdTest.java
 ---
@@ -0,0 +1,251 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.brooklyn.core.mgmt.rebind;
+
+import static org.testng.Assert.assertEquals;
+import static org.testng.Assert.assertNotEquals;
+
+import java.io.File;
+import java.io.FilenameFilter;
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.concurrent.Callable;
+
+import org.apache.brooklyn.api.mgmt.ManagementContext;
+import org.apache.brooklyn.api.mgmt.ha.HighAvailabilityMode;
+import org.apache.brooklyn.api.mgmt.ha.ManagementNodeState;
+import org.apache.brooklyn.core.entity.Entities;
+import org.apache.brooklyn.core.internal.BrooklynProperties;
+import org.apache.brooklyn.core.mgmt.internal.LocalManagementContext;
+import 
org.apache.brooklyn.core.mgmt.persist.BrooklynMementoPersisterToObjectStore;
+import org.apache.brooklyn.core.mgmt.persist.FileBasedObjectStore;
+import org.apache.brooklyn.core.mgmt.persist.PersistMode;
+import org.apache.brooklyn.core.server.BrooklynServerConfig;
+import org.apache.brooklyn.core.server.BrooklynServerPaths;
+import org.apache.brooklyn.test.Asserts;
+import org.apache.brooklyn.util.os.Os;
+import org.apache.brooklyn.util.text.Strings;
+import org.testng.annotations.AfterMethod;
+import org.testng.annotations.BeforeMethod;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+
+public class ManagementPlaneIdTest {
+private File mementoDir;
+
+protected ClassLoader classLoader = getClass().getClassLoader();
+
+private Collection managementContextForTermination;
+
+@BeforeMethod
+public void setUp() {
+mementoDir = Os.newTempDir(getClass());
+managementContextForTermination = new ArrayList<>();
+}
+
+@AfterMethod(alwaysRun=true)
+public void tearDown() throws Exception {
+if (managementContextForTermination != null) {
+for (ManagementContext mgmt : managementContextForTermination) 
{
+Entities.destroyAll(mgmt);
+}
+}
+if (mementoDir != null) 
FileBasedObjectStore.deleteCompletely(mementoDir);
+}
+
+@Test
+public void testUninitializedThrows() {
+ManagementContext mgmt = new 
LocalManagementContext(BrooklynProperties.Factory.newEmpty());
+try {
+mgmt.getManagementPlaneId();
+Asserts.shouldHaveFailedPreviously("managementPlaneId not 
initialized");
+} catch (NullPointerException e) {
+Asserts.expectedFailureContains(e, "not initialized");
+}
+}
+
+@Test
+public void testPlaneIdPersists() throws Exception {
+final ManagementContext mgmt = 
createManagementContext(PersistMode.AUTO, HighAvailabilityMode.DISABLED);
+checkRebindIdPersisted(mgmt);
+}
+
+@Test(enabled=false) //Need to wait for 1hr to verify the update will 
not overwrite
+public void testPlaneIdNotOverwritten() throws Exception {
+final LocalManagementContext mgmt = 
createManagementContext(PersistMode.AUTO, HighAvailabilityMode.DISABLED);
+
+final File planeIdFile = new File(mementoDir, 
BrooklynMementoPersisterToObjectStore.PLANE_ID_FILE_NAME);
+checkRebindIdPersisted(mgmt);
+final String oldPlaneId = mgmt.getManagementPlaneId();
+mgmt.setManagementPlaneId(Strings.makeRandomId(8));
+assertNotEquals(oldPlaneId, mgmt.getManagementPlaneId());
+Asserts.succeedsContinually(new Callable() {

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[geomacy]

Github user geomacy commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r110156077
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/mgmt/rebind/PeriodicDeltaChangeListener.java
 ---
@@ -382,6 +401,8 @@ protected void persistNowInternal(boolean 
alreadyHasMutex) {
 if (!alreadyHasMutex) persistingMutex.acquire();
 if (!isActive() && state != ListenerState.STOPPING) return;
 
+updatePlaneIdIfTimedOut();
--- End diff --

What's the motivation for updating the plane id like this?  When would you 
expect this to be required?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[geomacy]

Github user geomacy commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r109957781
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/mgmt/persist/BrooklynMementoPersisterToObjectStore.java
 ---
@@ -674,6 +685,22 @@ private void delete(String subPath, String id, 
PersistenceExceptionHandler excep
 }
 }
 
+private void updatePlaneId(String planeId, PersistenceExceptionHandler 
exceptionHandler) {
+try {
+if (planeId==null) {
+LOG.warn("Null content for planeId");
+}
+
+String persistedPlaneId = read(PLANE_ID_FILE_NAME);
+if (persistedPlaneId != null && 
!persistedPlaneId.equals(planeId)) {
+throw new IllegalStateException("Persisted planeId found 
(" + persistedPlaneId + ") but instance planeId is different (" + planeId + "). 
Overwriting!");
--- End diff --

"Overwriting!"?  does throwing the exception here not in fact skip the 
overwrite?  As per your comment on the description of the PR, "don't overwrite 
persisted store planeId if different"?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[geomacy]

Github user geomacy commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r109941412
  
--- Diff: 
core/src/main/java/org/apache/brooklyn/core/mgmt/ha/HighAvailabilityManagerImpl.java
 ---
@@ -456,6 +460,12 @@ else if (startMode==HighAvailabilityMode.HOT_STANDBY 
&& getNodeState()==Manageme
 registerPollTask();
 }
 
+protected void updatePlaneId(ManagementPlaneSyncRecord existingMaster) 
{
--- End diff --

The parameter name sounds wrong here.  Also the method name suggested 
initially to me that it was going to update something in persistence, whereas 
all that it does is set the value in the local management context object; so 
maybe call it `updateManagementContextWithPlaneIdIfNotNull`?

Is there a need in here to check whether the management context already has 
a plane id?  What would be the right behaviour if it did?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[geomacy]

Github user geomacy commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r109937046
  
--- Diff: 
api/src/main/java/org/apache/brooklyn/api/mgmt/ManagementContext.java ---
@@ -66,9 +67,20 @@
  * In other words the value of {@link 
Application#getManagementContext()#getManagementPlaneId()} 
  * will generally be constant (in contrast to {@link 
#getManagementNodeId()}).
  * 
- * This value should not be null unless the management context is a 
non-functional
- * (non-deployment) instance. */
+ * Throws an {@link NullPointerException} if the value hasn't been 
initialized yet. The value is set:
--- End diff --

Maybe leave as-was but deprecate in favour of 
`getOptionalManagementPlaneId`?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[neykov]

Github user neykov commented on a diff in the pull request:

https://github.com/apache/brooklyn-server/pull/617#discussion_r109357619
  
--- Diff: 
api/src/main/java/org/apache/brooklyn/api/mgmt/ManagementContext.java ---
@@ -66,9 +67,20 @@
  * In other words the value of {@link 
Application#getManagementContext()#getManagementPlaneId()} 
  * will generally be constant (in contrast to {@link 
#getManagementNodeId()}).
  * 
- * This value should not be null unless the management context is a 
non-functional
- * (non-deployment) instance. */
+ * Throws an {@link NullPointerException} if the value hasn't been 
initialized yet. The value is set:
--- End diff --

Wonder whether we should leave this to return `null` when not initialised. 
It's been useful to find instances of wrong (too early) usage but on the other 
hand changes the method behaviour.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

[GitHub] brooklyn-server pull request #617: Persist management plane ID

[neykov]

GitHub user neykov opened a pull request:

https://github.com/apache/brooklyn-server/pull/617

Persist management plane ID

Some considerations on where to place the planeId initialisation:
 * BrooklynMemento (RebindManager). `planeId` will be available only after 
rebind, so it will
   be available to MASTER nodes only (and potentially HOT_STANDBY)
 * ManagementPlaneSyncRecord (HighAvailabilityManager). Used only when HA 
is enabled. When HA is disabled
   HighAvailabilityManager is not initialised, doesn't access the store at 
all, no
   records are getting written.
 * Separate init step in BrooklynLauncher. Early in the lifecycle so users 
need not to care
   too much when they are allowed to read it. On the other hand it's not 
encapsulated so
   need to touch all places that use persistence (i.e. launcher, 
copy-state, backup). Can
   only read from the store at this point, write access is not enabled yet. 
If HA is configured
   store writes are enabled only after becoming master. Writing to the 
store in this case would
   be risky, even if we know that it's a clean state (no planeId file). 
Better leave only the
   master update the store.

Because of the above I decided to go for a mixed approach:
  * persist the `planeId` immediately after starting persistence (i.e. 
MASTER when HA) and repeat hourly
  * don't overwrite persisted store `planeId` if different
  * init `planeId` as part of the rebind sequence; if no `planeId` exists 
in store generate one
  * init `planeId` when reading the management records so we have it 
initialized for non-`MASTER` nodes as well; don't generate one if not in 
persisted store

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/neykov/brooklyn-server plane-id

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/brooklyn-server/pull/617.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

This closes #617


commit 7bcc566bec9e4875207036b1943c4528aae72bea
Author: Svetoslav Neykov 
Date:   2017-03-30T14:19:43Z

Persist management plane ID

commit 701c716aa27dbc3999c9c2fdd728e09089448b1a
Author: Svetoslav Neykov 
Date:   2017-04-03T07:32:54Z

Persisting management plane ID test coverage




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---