You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by neykov <gi...@git.apache.org> on 2017/04/03 07:45:49 UTC
[GitHub] brooklyn-server pull request #617: Persist management plane ID
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 <sv...@cloudsoftcorp.com>
Date: 2017-03-30T14:19:43Z
Persist management plane ID
commit 701c716aa27dbc3999c9c2fdd728e09089448b1a
Author: Svetoslav Neykov <sv...@cloudsoftcorp.com>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #617: Persist management plane ID
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/617
Thanks @geomacy. Addressed comments.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by aledsage <gi...@git.apache.org>.
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<ManagementContext> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by neykov <gi...@git.apache.org>.
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()}).
* <p>
- * 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by neykov <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by neykov <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by geomacy <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by geomacy <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by geomacy <gi...@git.apache.org>.
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<ManagementContext> 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<Void>() {
+ @Override
+ public Void call() throws Exception {
+ String planeId = readFile(planeIdFile);
+ assertEquals(oldPlaneId, planeId);
+ return null;
+ }
+ });
+ }
+
+ @Test
+ public void testColdRebindInitialisesPlaneId() throws Exception {
+ final LocalManagementContext origMgmt = createManagementContext(PersistMode.AUTO, HighAvailabilityMode.DISABLED);
+ checkRebindIdPersisted(origMgmt);
+ Entities.destroyAll(origMgmt);
+
+ LocalManagementContext rebindMgmt = createManagementContext(PersistMode.AUTO, HighAvailabilityMode.DISABLED);
+
+ assertEquals(origMgmt.getManagementPlaneId(), rebindMgmt.getManagementPlaneId());
+ }
+
+
+ @DataProvider
+ public Object[][] haSlaveModes() {
+ return new Object[][] {
+ {HighAvailabilityMode.AUTO},
+ {HighAvailabilityMode.STANDBY},
+ {HighAvailabilityMode.HOT_STANDBY},
+ {HighAvailabilityMode.HOT_BACKUP},
+ };
+ }
+
+ @Test(dataProvider="haSlaveModes")
+ public void testHaRebindInitialisesPlaneId(HighAvailabilityMode slaveMode) throws Exception {
+ final LocalManagementContext origMgmt = createManagementContext(PersistMode.AUTO, HighAvailabilityMode.AUTO);
+ final LocalManagementContext rebindMgmt = createManagementContext(PersistMode.AUTO, slaveMode);
+
+ Asserts.succeedsEventually(new Runnable() {
+ @Override
+ public void run() {
+ assertEquals(origMgmt.getManagementPlaneId(), rebindMgmt.getManagementPlaneId());
+ }
+ });
+ }
+
+ @Test
+ public void testHaFailoverKeepsPlaneId() throws Exception {
+ final LocalManagementContext origMgmt = createManagementContext(PersistMode.AUTO, HighAvailabilityMode.MASTER);
+ final LocalManagementContext rebindMgmt = createManagementContext(PersistMode.AUTO, HighAvailabilityMode.STANDBY);
+
+ Asserts.succeedsEventually(new Runnable() {
+ @Override
+ public void run() {
+ assertEquals(origMgmt.getManagementPlaneId(), rebindMgmt.getManagementPlaneId());
+ }
+ });
+
+ Entities.destroyAll(origMgmt);
+
+ Asserts.succeedsEventually(new Runnable() {
+ @Override
+ public void run() {
+ assertEquals(rebindMgmt.getHighAvailabilityManager().getNodeState(), ManagementNodeState.MASTER);
+ if (rebindMgmt.getRebindManager().isAwaitingInitialRebind()) {
+ throw new IllegalStateException("still rebinding");
+ }
+ }
+ });
+
+ assertEquals(origMgmt.getManagementPlaneId(), rebindMgmt.getManagementPlaneId());
+ }
+
+ @Test
+ public void testPlaneIdBackedUp() throws Exception {
+ final LocalManagementContext origMgmt = createManagementContext(PersistMode.AUTO, HighAvailabilityMode.AUTO);
+ checkRebindIdPersisted(origMgmt);
+ Entities.destroyAll(origMgmt);
+
+ LocalManagementContext rebindMgmt = createManagementContextWithBackups(PersistMode.AUTO, HighAvailabilityMode.AUTO);
+
+ assertEquals(origMgmt.getManagementPlaneId(), rebindMgmt.getManagementPlaneId());
+
+ String backupContainer = BrooklynServerPaths.newBackupPersistencePathResolver(rebindMgmt).resolve();
+
+ File[] promotionFolders = new File(backupContainer).listFiles(new FilenameFilter() {
+ @Override
+ public boolean accept(File dir, String name) {
+ return name.contains("promotion");
+ }
+ });
+
+ assertEquals(promotionFolders.length, 1);
+
+ File planeIdFile = new File(promotionFolders[0], BrooklynMementoPersisterToObjectStore.PLANE_ID_FILE_NAME);
+ String planeId = readFile(planeIdFile);
+ assertEquals(origMgmt.getManagementPlaneId(), planeId);
+ }
+
+ @Test
+ public void testFullPersist() throws Exception {
+ final LocalManagementContext origMgmt = createManagementContext(PersistMode.DISABLED, HighAvailabilityMode.DISABLED);
+ origMgmt.getRebindManager().getPersister().enableWriteAccess();
+ origMgmt.getRebindManager().forcePersistNow(true, null);
+ checkRebindIdPersisted(origMgmt);
+ }
+
+ protected LocalManagementContext createManagementContext(PersistMode persistMode, HighAvailabilityMode haMode) {
+ return createManagementContext(persistMode, haMode, false);
+ }
+
+ protected LocalManagementContext createManagementContextWithBackups(PersistMode persistMode, HighAvailabilityMode haMode) {
+ return createManagementContext(persistMode, haMode, true);
+ }
+
+ protected LocalManagementContext createManagementContext(PersistMode persistMode, HighAvailabilityMode haMode, boolean backedUp) {
+ BrooklynProperties props = BrooklynProperties.Factory.newEmpty();
+ props.put(BrooklynServerConfig.PERSISTENCE_BACKUPS_DIR, mementoDir);
+ LocalManagementContext mgmt = RebindTestUtils.managementContextBuilder(mementoDir, classLoader)
+ .persistPeriodMillis(1)
+ .persistMode(persistMode)
+ .haMode(HighAvailabilityMode.DISABLED)
--- End diff --
`.haMode(haMode)` ? Currently `haMode` parameter is unused.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by neykov <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by geomacy <gi...@git.apache.org>.
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<ManagementContext> 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<Void>() {
+ @Override
+ public Void call() throws Exception {
+ String planeId = readFile(planeIdFile);
+ assertEquals(oldPlaneId, planeId);
+ return null;
+ }
+ });
+ }
+
+ @Test
+ public void testColdRebindInitialisesPlaneId() throws Exception {
+ final LocalManagementContext origMgmt = createManagementContext(PersistMode.AUTO, HighAvailabilityMode.DISABLED);
+ checkRebindIdPersisted(origMgmt);
+ Entities.destroyAll(origMgmt);
+
+ LocalManagementContext rebindMgmt = createManagementContext(PersistMode.AUTO, HighAvailabilityMode.DISABLED);
+
+ assertEquals(origMgmt.getManagementPlaneId(), rebindMgmt.getManagementPlaneId());
+ }
+
+
+ @DataProvider
+ public Object[][] haSlaveModes() {
+ return new Object[][] {
+ {HighAvailabilityMode.AUTO},
+ {HighAvailabilityMode.STANDBY},
+ {HighAvailabilityMode.HOT_STANDBY},
+ {HighAvailabilityMode.HOT_BACKUP},
+ };
+ }
+
+ @Test(dataProvider="haSlaveModes")
+ public void testHaRebindInitialisesPlaneId(HighAvailabilityMode slaveMode) throws Exception {
+ final LocalManagementContext origMgmt = createManagementContext(PersistMode.AUTO, HighAvailabilityMode.AUTO);
+ final LocalManagementContext rebindMgmt = createManagementContext(PersistMode.AUTO, slaveMode);
+
+ Asserts.succeedsEventually(new Runnable() {
+ @Override
+ public void run() {
+ assertEquals(origMgmt.getManagementPlaneId(), rebindMgmt.getManagementPlaneId());
+ }
+ });
+ }
+
+ @Test
+ public void testHaFailoverKeepsPlaneId() throws Exception {
+ final LocalManagementContext origMgmt = createManagementContext(PersistMode.AUTO, HighAvailabilityMode.MASTER);
+ final LocalManagementContext rebindMgmt = createManagementContext(PersistMode.AUTO, HighAvailabilityMode.STANDBY);
+
+ Asserts.succeedsEventually(new Runnable() {
+ @Override
+ public void run() {
+ assertEquals(origMgmt.getManagementPlaneId(), rebindMgmt.getManagementPlaneId());
+ }
+ });
+
+ Entities.destroyAll(origMgmt);
+
+ Asserts.succeedsEventually(new Runnable() {
+ @Override
+ public void run() {
+ assertEquals(rebindMgmt.getHighAvailabilityManager().getNodeState(), ManagementNodeState.MASTER);
+ if (rebindMgmt.getRebindManager().isAwaitingInitialRebind()) {
+ throw new IllegalStateException("still rebinding");
+ }
+ }
+ });
+
+ assertEquals(origMgmt.getManagementPlaneId(), rebindMgmt.getManagementPlaneId());
+ }
+
+ @Test
+ public void testPlaneIdBackedUp() throws Exception {
+ final LocalManagementContext origMgmt = createManagementContext(PersistMode.AUTO, HighAvailabilityMode.AUTO);
+ checkRebindIdPersisted(origMgmt);
+ Entities.destroyAll(origMgmt);
+
+ LocalManagementContext rebindMgmt = createManagementContextWithBackups(PersistMode.AUTO, HighAvailabilityMode.AUTO);
+
+ assertEquals(origMgmt.getManagementPlaneId(), rebindMgmt.getManagementPlaneId());
+
+ String backupContainer = BrooklynServerPaths.newBackupPersistencePathResolver(rebindMgmt).resolve();
+
+ File[] promotionFolders = new File(backupContainer).listFiles(new FilenameFilter() {
+ @Override
+ public boolean accept(File dir, String name) {
+ return name.contains("promotion");
+ }
+ });
+
+ assertEquals(promotionFolders.length, 1);
+
+ File planeIdFile = new File(promotionFolders[0], BrooklynMementoPersisterToObjectStore.PLANE_ID_FILE_NAME);
+ String planeId = readFile(planeIdFile);
+ assertEquals(origMgmt.getManagementPlaneId(), planeId);
+ }
+
+ @Test
+ public void testFullPersist() throws Exception {
+ final LocalManagementContext origMgmt = createManagementContext(PersistMode.DISABLED, HighAvailabilityMode.DISABLED);
+ origMgmt.getRebindManager().getPersister().enableWriteAccess();
+ origMgmt.getRebindManager().forcePersistNow(true, null);
+ checkRebindIdPersisted(origMgmt);
+ }
+
+ protected LocalManagementContext createManagementContext(PersistMode persistMode, HighAvailabilityMode haMode) {
+ return createManagementContext(persistMode, haMode, false);
+ }
+
+ protected LocalManagementContext createManagementContextWithBackups(PersistMode persistMode, HighAvailabilityMode haMode) {
+ return createManagementContext(persistMode, haMode, true);
+ }
+
+ protected LocalManagementContext createManagementContext(PersistMode persistMode, HighAvailabilityMode haMode, boolean backedUp) {
+ BrooklynProperties props = BrooklynProperties.Factory.newEmpty();
+ props.put(BrooklynServerConfig.PERSISTENCE_BACKUPS_DIR, mementoDir);
+ LocalManagementContext mgmt = RebindTestUtils.managementContextBuilder(mementoDir, classLoader)
+ .persistPeriodMillis(1)
+ .persistMode(persistMode)
+ .haMode(HighAvailabilityMode.DISABLED)
+ .enablePersistenceBackups(backedUp)
+ .emptyCatalog(true)
+ .properties(props)
+ .enableOsgi(false)
+ .buildStarted();
+ markForTermination(mgmt);
+ return mgmt;
+ }
+
+ private void markForTermination(ManagementContext mgmt) {
+ managementContextForTermination.add(mgmt);
+ }
+
+ protected static String readFile(File planeIdFile) throws IOException {
+ return new String(Files.readAllBytes(planeIdFile.toPath()), StandardCharsets.UTF_8);
+ }
+
+ protected void checkRebindIdPersisted(final ManagementContext mgmt) {
--- End diff --
`checkPlaneIdPersisted` surely?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by neykov <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by geomacy <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on a diff in the pull request:
https://github.com/apache/brooklyn-server/pull/617#discussion_r111908706
--- 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 --
I think this actually gives you `debug(String, Object, Object)` rather than an overload that knows about `Throwable`, so you'll not get the right parameter matching here.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by asfgit <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by geomacy <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #617: Persist management plane ID
Posted by neykov <gi...@git.apache.org>.
Github user neykov commented on the issue:
https://github.com/apache/brooklyn-server/pull/617
Thanks @aledsage addressed the comments.
> I'd be interested if this has highlighted any areas of Brooklyn that we should improve,
There's definitely room for improvement around HA + persistence. One example is we could clean up the code around that's responsible for starting persistence/HA in the various cases (ha, noha * persistence, no persistence matrix). It's something I had to replicate in the [test - see TODO](https://github.com/apache/brooklyn-server/pull/617/files#diff-37574fbba472cb8c35ca723a3118ed93R268). All interactions should go through `HighAvailabilityManager` enabled or not.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by aledsage <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by neykov <gi...@git.apache.org>.
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<ManagementContext> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #617: Persist management plane ID
Posted by geomacy <gi...@git.apache.org>.
Github user geomacy commented on the issue:
https://github.com/apache/brooklyn-server/pull/617
LGTM; Jenkins build break was unrelated, I rebuilt it (build number 2007) and it succeeded. Will merge.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by neykov <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by neykov <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by aledsage <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server issue #617: Persist management plane ID
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the issue:
https://github.com/apache/brooklyn-server/pull/617
This fixes https://issues.apache.org/jira/browse/BROOKLYN-202
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by geomacy <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by aledsage <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by neykov <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by geomacy <gi...@git.apache.org>.
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()}).
* <p>
- * 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] brooklyn-server pull request #617: Persist management plane ID
Posted by neykov <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---