From c4c415e2b3cceb47d4d1858083f9b550971518f2 Mon Sep 17 00:00:00 2001 From: Rahul Desirazu Date: Wed, 6 May 2020 00:39:35 -0700 Subject: [PATCH] [#4341] Remove deleted flags from server.conf Summary: Currently we persist flag removal in the YW DB, but we don't delete the entry from the server conf file. This means that on restart, we pick up this entry that should have been deleted. Fix this by adding an ansible task that takes in parameter `gflags_to_remove` list and removes any entries in the server conf that match an entry in this list. This list is generated by YW taking a difference between the UserIntent flags and taskParams() flags. Test Plan: UpgradeUniverseTest and NodeManagerTest unit tests as well as manual testing of just flag deletion as well as flag deletion + addition together. Reviewers: bogdan, ram Reviewed By: ram Subscribers: jenkins-bot, yugaware Differential Revision: https://phabricator.dev.yugabyte.com/D8432 --- .../opscli/ybops/cloud/common/method.py | 4 + .../configure-cluster-server/tasks/main.yml | 8 ++ .../commissioner/tasks/UpgradeUniverse.java | 10 ++- .../subtasks/AnsibleConfigureServers.java | 3 + .../com/yugabyte/yw/common/NodeManager.java | 13 +++- .../yw/controllers/UniverseController.java | 3 +- .../tasks/UpgradeUniverseTest.java | 74 +++++++++++++++++-- .../yugabyte/yw/common/NodeManagerTest.java | 44 +++++++++-- .../controllers/UniverseControllerTest.java | 8 +- 9 files changed, 138 insertions(+), 29 deletions(-) diff --git a/managed/devops/opscli/ybops/cloud/common/method.py b/managed/devops/opscli/ybops/cloud/common/method.py index acc0a55b3d81..b4b98fdf7a8d 100644 --- a/managed/devops/opscli/ybops/cloud/common/method.py +++ b/managed/devops/opscli/ybops/cloud/common/method.py @@ -435,6 +435,7 @@ def prepare(self): self.parser.add_argument('--extra_gflags', default=None) self.parser.add_argument('--gflags', default=None) self.parser.add_argument('--replace_gflags', action="store_true") + self.parser.add_argument('--gflags_to_remove', default=None) self.parser.add_argument('--master_addresses_for_tserver') self.parser.add_argument('--master_addresses_for_master') self.parser.add_argument('--rootCA_cert') @@ -507,6 +508,9 @@ def callback(self, args): if args.extra_gflags is not None: self.extra_vars["extra_gflags"] = json.loads(args.extra_gflags) + if args.gflags_to_remove is not None: + self.extra_vars["gflags_to_remove"] = json.loads(args.gflags_to_remove) + if args.rootCA_cert is not None: self.extra_vars["rootCA_cert"] = args.rootCA_cert diff --git a/managed/devops/roles/configure-cluster-server/tasks/main.yml b/managed/devops/roles/configure-cluster-server/tasks/main.yml index 82a3d68a44ea..97648cdd2614 100644 --- a/managed/devops/roles/configure-cluster-server/tasks/main.yml +++ b/managed/devops/roles/configure-cluster-server/tasks/main.yml @@ -77,6 +77,14 @@ create: yes with_dict: "{{ gflags | default(_gflags) }}" +- name: Configure | Delete {{ yb_process_type }} gflags + lineinfile: + dest: "{{ yb_conf_file }}" + regexp: "^--{{ item }}=(.*)" + state: absent + with_items: "{{ gflags_to_remove }}" + when: gflags_to_remove is defined + - name: Configure | Add yb server ctl script vars: mount_paths: "{{ _mount_points | join(' ') }}" diff --git a/managed/src/main/java/com/yugabyte/yw/commissioner/tasks/UpgradeUniverse.java b/managed/src/main/java/com/yugabyte/yw/commissioner/tasks/UpgradeUniverse.java index 37c65f1331e8..75791bc52c9f 100644 --- a/managed/src/main/java/com/yugabyte/yw/commissioner/tasks/UpgradeUniverse.java +++ b/managed/src/main/java/com/yugabyte/yw/commissioner/tasks/UpgradeUniverse.java @@ -133,8 +133,7 @@ public void run() { didUpgradeUniverse = true; break; case GFlags: - if (!taskParams().masterGFlags.isEmpty() && - !taskParams().masterGFlags.equals(primIntent.masterGFlags)) { + if (!taskParams().masterGFlags.equals(primIntent.masterGFlags)) { LOG.info("Updating Master gflags: {} for {} nodes in universe {}", taskParams().masterGFlags, masterNodes.size(), universe.name); if (!taskParams().rollingUpgrade) { @@ -157,8 +156,7 @@ public void run() { } didUpgradeUniverse = true; } - if (!taskParams().tserverGFlags.isEmpty() && - !taskParams().tserverGFlags.equals(primIntent.tserverGFlags)) { + if (!taskParams().tserverGFlags.equals(primIntent.tserverGFlags)) { LOG.info("Updating T-Server gflags: {} for {} nodes in universe {}", taskParams().tserverGFlags, tServerNodes.size(), universe.name); if (taskParams().rollingUpgrade) { @@ -357,8 +355,12 @@ private AnsibleConfigureServers getConfigureTask(NodeDetails node, } else if (type == UpgradeTaskType.GFlags) { if (processType.equals(ServerType.MASTER)) { params.gflags = taskParams().masterGFlags; + params.gflagsToRemove = userIntent.masterGFlags.keySet().stream().filter( + flag -> !taskParams().masterGFlags.containsKey(flag)).collect(Collectors.toSet()); } else { params.gflags = taskParams().tserverGFlags; + params.gflagsToRemove = userIntent.tserverGFlags.keySet().stream().filter( + flag -> !taskParams().tserverGFlags.containsKey(flag)).collect(Collectors.toSet()); } } diff --git a/managed/src/main/java/com/yugabyte/yw/commissioner/tasks/subtasks/AnsibleConfigureServers.java b/managed/src/main/java/com/yugabyte/yw/commissioner/tasks/subtasks/AnsibleConfigureServers.java index dfcc103b2874..e23bff32520f 100644 --- a/managed/src/main/java/com/yugabyte/yw/commissioner/tasks/subtasks/AnsibleConfigureServers.java +++ b/managed/src/main/java/com/yugabyte/yw/commissioner/tasks/subtasks/AnsibleConfigureServers.java @@ -21,6 +21,8 @@ import java.util.HashMap; import java.util.Map; +import java.util.HashSet; +import java.util.Set; public class AnsibleConfigureServers extends NodeTaskBase { public static final Logger LOG = LoggerFactory.getLogger(AnsibleConfigureServers.class); @@ -37,6 +39,7 @@ public static class Params extends NodeTaskParams { public boolean enableClientToNodeEncrypt = false; public boolean allowInsecure = true; public Map gflags = new HashMap<>(); + public Set gflagsToRemove = new HashSet<>(); public boolean updateMasterAddrsOnly = false; public CollectionLevel callhomeLevel; // Development params. diff --git a/managed/src/main/java/com/yugabyte/yw/common/NodeManager.java b/managed/src/main/java/com/yugabyte/yw/common/NodeManager.java index d1064d17ea11..5263f5ef6bfd 100644 --- a/managed/src/main/java/com/yugabyte/yw/common/NodeManager.java +++ b/managed/src/main/java/com/yugabyte/yw/common/NodeManager.java @@ -305,10 +305,12 @@ private List getConfigureSubCommand(AnsibleConfigureServers.Params taskP case GFlags: { if (!taskParam.updateMasterAddrsOnly && - (taskParam.gflags == null || taskParam.gflags.isEmpty())) { - throw new RuntimeException(taskParam.gflags + " GFlags data provided for " + + (taskParam.gflags == null || taskParam.gflags.isEmpty()) && + (taskParam.gflagsToRemove == null || taskParam.gflagsToRemove.isEmpty())) { + throw new RuntimeException("GFlags data provided for " + taskParam.nodeName + "'s " + - taskParam.getProperty("processType") + " process."); + taskParam.getProperty("processType") + " process" + + " have no changes from existing flags."); } String processType = taskParam.getProperty("processType"); @@ -336,6 +338,11 @@ private List getConfigureSubCommand(AnsibleConfigureServers.Params taskP } subcommand.add("--gflags"); subcommand.add(Json.stringify(Json.toJson(gflags))); + + if (taskParam.gflagsToRemove != null && !taskParam.gflagsToRemove.isEmpty()) { + subcommand.add("--gflags_to_remove"); + subcommand.add(Json.stringify(Json.toJson(taskParam.gflagsToRemove))); + } } break; } diff --git a/managed/src/main/java/com/yugabyte/yw/controllers/UniverseController.java b/managed/src/main/java/com/yugabyte/yw/controllers/UniverseController.java index f9985a6e2825..8aa71e9e7f51 100644 --- a/managed/src/main/java/com/yugabyte/yw/controllers/UniverseController.java +++ b/managed/src/main/java/com/yugabyte/yw/controllers/UniverseController.java @@ -1258,8 +1258,7 @@ public Result upgrade(UUID customerUUID, UUID universeUUID) { break; case GFlags: customerTaskType = CustomerTask.TaskType.UpgradeGflags; - if ((taskParams.masterGFlags == null || taskParams.masterGFlags.isEmpty()) && - (taskParams.tserverGFlags == null || taskParams.tserverGFlags.isEmpty())) { + if (taskParams.masterGFlags == null && taskParams.tserverGFlags == null) { return ApiResponse.error( BAD_REQUEST, "gflags param is required for taskType: " + taskParams.taskType); diff --git a/managed/src/test/java/com/yugabyte/yw/commissioner/tasks/UpgradeUniverseTest.java b/managed/src/test/java/com/yugabyte/yw/commissioner/tasks/UpgradeUniverseTest.java index 46c12133bff8..1fe6049da772 100644 --- a/managed/src/test/java/com/yugabyte/yw/commissioner/tasks/UpgradeUniverseTest.java +++ b/managed/src/test/java/com/yugabyte/yw/commissioner/tasks/UpgradeUniverseTest.java @@ -228,13 +228,21 @@ private int assertSoftwareUpgradeSequence(Map> subTasksB private int assertGFlagsUpgradeSequence(Map> subTasksByPosition, ServerType serverType, int startPosition, boolean isRollingUpgrade) { return assertGFlagsUpgradeSequence(subTasksByPosition, serverType, startPosition, - isRollingUpgrade, false); + isRollingUpgrade, false, false); + } + + private int assertGFlagsUpgradeSequence(Map> subTasksByPosition, + ServerType serverType, int startPosition, + boolean isRollingUpgrade, boolean isEdit) { + return assertGFlagsUpgradeSequence(subTasksByPosition, serverType, startPosition, + isRollingUpgrade, isEdit, false); } private int assertGFlagsUpgradeSequence(Map> subTasksByPosition, ServerType serverType, int startPosition, boolean isRollingUpgrade, - boolean isEdit) { + boolean isEdit, + boolean isDelete) { int position = startPosition; if (isRollingUpgrade) { List taskSequence = GFLAGS_ROLLING_UPGRADE_TASK_SEQUENCE; @@ -252,12 +260,13 @@ private int assertGFlagsUpgradeSequence(Map> subTasksByP )); if (taskType.equals(TaskType.AnsibleConfigureServers)) { - JsonNode gflagValue = serverType.equals(MASTER) ? - Json.parse("{\"master-flag\":" + (isEdit ? "\"m2\"}" : "\"m1\"}")) : - Json.parse("{\"tserver-flag\":" + (isEdit ? "\"t2\"}" : "\"t1\"}")); - assertValues.putAll(ImmutableMap.of( - "gflags", gflagValue, "processType", serverType.toString() - )); + if (!isDelete) { + JsonNode gflagValue = serverType.equals(MASTER) ? + Json.parse("{\"master-flag\":" + (isEdit ? "\"m2\"}" : "\"m1\"}")) : + Json.parse("{\"tserver-flag\":" + (isEdit ? "\"t2\"}" : "\"t1\"}")); + assertValues.putAll(ImmutableMap.of("gflags", gflagValue)); + } + assertValues.put("processType", serverType.toString()); } assertNodeSubTask(tasks, assertValues); } @@ -661,4 +670,53 @@ public void run(Universe universe) { UpgradeType.ROLLING_UPGRADE_MASTER_ONLY, true); assertEquals(26, position); } + + @Test + public void testRemoveFlags() { + for (ServerType serverType : ImmutableList.of(MASTER, TSERVER)) { + // Simulate universe created with master flags and tserver flags. + final Map tserverFlags = ImmutableMap.of("tserver-flag", "m123"); + final Map masterGFlags = ImmutableMap.of("master-flag", "m123"); + Universe.UniverseUpdater updater = new Universe.UniverseUpdater() { + public void run(Universe universe) { + UniverseDefinitionTaskParams universeDetails = universe.getUniverseDetails(); + UserIntent userIntent = universeDetails.getPrimaryCluster().userIntent; + userIntent.masterGFlags = masterGFlags; + userIntent.tserverGFlags = tserverFlags; + universe.setUniverseDetails(universeDetails); + } + }; + Universe.saveDetails(defaultUniverse.universeUUID, updater); + + UpgradeUniverse.Params taskParams = new UpgradeUniverse.Params(); + // This is a delete operation on the master flags. + if (serverType == MASTER) { + taskParams.masterGFlags = new HashMap<>(); + taskParams.tserverGFlags = tserverFlags; + } else { + taskParams.masterGFlags = masterGFlags; + taskParams.tserverGFlags = new HashMap<>(); + } + + int expectedVersion = serverType == MASTER ? 3 : 14; + TaskInfo taskInfo = submitTask(taskParams, + UpgradeUniverse.UpgradeTaskType.GFlags, + expectedVersion); + + int numInvocations = serverType == MASTER ? 9 : 18; + verify(mockNodeManager, times(numInvocations)).nodeCommand(any(), any()); + + List subTasks = new ArrayList<>(taskInfo.getSubTasks()); + Map> subTasksByPosition = + subTasks.stream().collect(Collectors.groupingBy(w -> w.getPosition())); + int position = serverType == MASTER ? 0 : 1; + position = assertGFlagsUpgradeSequence(subTasksByPosition, serverType, position, + true, true, true); + position = assertGFlagsCommonTasks(subTasksByPosition, position, serverType == MASTER ? + UpgradeType.ROLLING_UPGRADE_MASTER_ONLY : UpgradeType.ROLLING_UPGRADE_TSERVER_ONLY, + true); + assertEquals(serverType == MASTER ? 26 : 28, position); + } + + } } diff --git a/managed/src/test/java/com/yugabyte/yw/common/NodeManagerTest.java b/managed/src/test/java/com/yugabyte/yw/common/NodeManagerTest.java index ad6e39f816c0..1db05e827525 100644 --- a/managed/src/test/java/com/yugabyte/yw/common/NodeManagerTest.java +++ b/managed/src/test/java/com/yugabyte/yw/common/NodeManagerTest.java @@ -38,6 +38,8 @@ import java.util.Map; import java.util.UUID; import java.util.function.Predicate; +import java.util.Set; +import java.util.HashSet; import static com.yugabyte.yw.commissioner.tasks.UniverseDefinitionTaskBase.ServerType.MASTER; import static com.yugabyte.yw.commissioner.tasks.UniverseDefinitionTaskBase.ServerType.TSERVER; @@ -356,14 +358,16 @@ private List nodeCommand( expectedCommand.add("--extra_gflags"); expectedCommand.add(Json.stringify(Json.toJson(gflags))); } else if (configureParams.type == GFlags) { - if (!configureParams.gflags.isEmpty()) { - String processType = configureParams.getProperty("processType"); - expectedCommand.add("--yb_process_type"); - expectedCommand.add(processType.toLowerCase()); - String gflagsJson = Json.stringify(Json.toJson(gflags)); - expectedCommand.add("--replace_gflags"); - expectedCommand.add("--gflags"); - expectedCommand.add(gflagsJson); + String processType = configureParams.getProperty("processType"); + expectedCommand.add("--yb_process_type"); + expectedCommand.add(processType.toLowerCase()); + String gflagsJson = Json.stringify(Json.toJson(gflags)); + expectedCommand.add("--replace_gflags"); + expectedCommand.add("--gflags"); + expectedCommand.add(gflagsJson); + if (configureParams.gflagsToRemove != null && !configureParams.gflagsToRemove.isEmpty()) { + expectedCommand.add("--gflags_to_remove"); + expectedCommand.add(Json.stringify(Json.toJson(configureParams.gflagsToRemove))); } } else { expectedCommand.add("--extra_gflags"); @@ -881,6 +885,30 @@ public void testSoftwareUpgradeWithoutReleasePackage() { } } + @Test + public void testGFlagRemove() { + for (TestData t : testData) { + for (String serverType : ImmutableList.of(MASTER.toString(), TSERVER.toString())) { + AnsibleConfigureServers.Params params = new AnsibleConfigureServers.Params(); + buildValidParams(t, params, Universe.saveDetails(createUniverse().universeUUID, + ApiUtils.mockUniverseUpdater(t.cloudType))); + Set gflagsToRemove = new HashSet<>(); + gflagsToRemove.add("flag1"); + gflagsToRemove.add("flag2"); + params.type = GFlags; + params.isMasterInShellMode = true; + params.gflagsToRemove = gflagsToRemove; + params.setProperty("processType", serverType); + + List expectedCommand = new ArrayList<>(t.baseCommand); + expectedCommand.addAll(nodeCommand(NodeManager.NodeCommandType.Configure, params, t)); + nodeManager.nodeCommand(NodeManager.NodeCommandType.Configure, params); + verify(shellProcessHandler, times(1)).run(expectedCommand, + t.region.provider.getConfig()); + } + } + } + @Test public void testGFlagsUpgradeNullProcessType() { for (TestData t : testData) { diff --git a/managed/src/test/java/com/yugabyte/yw/controllers/UniverseControllerTest.java b/managed/src/test/java/com/yugabyte/yw/controllers/UniverseControllerTest.java index c7f478da37b2..1ebf69294abd 100644 --- a/managed/src/test/java/com/yugabyte/yw/controllers/UniverseControllerTest.java +++ b/managed/src/test/java/com/yugabyte/yw/controllers/UniverseControllerTest.java @@ -1021,7 +1021,7 @@ public void testUniverseGFlagsUpgradeWithInvalidParams() { String url = "/api/customers/" + customer.uuid + "/universes/" + u.universeUUID + "/upgrade"; Result result = doRequestWithAuthTokenAndBody("POST", url, authToken, bodyJson); - assertBadRequest(result, "gflags param is required for taskType: GFlags"); + assertBadRequest(result, "Neither master nor tserver gflags changed."); assertAuditEntry(0, customer.uuid); } @@ -1081,7 +1081,7 @@ public void testUniverseGFlagsUpgradeWithMissingGflags() { String url = "/api/customers/" + customer.uuid + "/universes/" + u.universeUUID + "/upgrade"; Result result = doRequestWithAuthTokenAndBody("POST", url, authToken, bodyJsonMissingGFlags); - assertBadRequest(result, "gflags param is required for taskType: GFlags"); + assertBadRequest(result, "Neither master nor tserver gflags changed."); assertAuditEntry(0, customer.uuid); } @@ -1104,7 +1104,7 @@ public void testUniverseGFlagsUpgradeWithMalformedTServerFlags() { String url = "/api/customers/" + customer.uuid + "/universes/" + u.universeUUID + "/upgrade"; Result result = doRequestWithAuthTokenAndBody("POST", url, authToken, bodyJson); - assertBadRequest(result, "gflags param is required for taskType: GFlags"); + assertBadRequest(result, "Neither master nor tserver gflags changed."); assertAuditEntry(0, customer.uuid); } @@ -1127,7 +1127,7 @@ public void testUniverseGFlagsUpgradeWithMalformedMasterGFlags() { String url = "/api/customers/" + customer.uuid + "/universes/" + u.universeUUID + "/upgrade"; Result result = doRequestWithAuthTokenAndBody("POST", url, authToken, bodyJson); - assertBadRequest(result, "gflags param is required for taskType: GFlags"); + assertBadRequest(result, "Neither master nor tserver gflags changed."); assertAuditEntry(0, customer.uuid); }